Bug 129670

Created attachment 225754 [details] Patch

Created attachment 227790 [details] Patch

Comment on attachment 227790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227790&action=review > Source/WebKit2/Shared/Network/CustomProtocols/CustomProtocolManager.h:44 > +#else > +#include "CustomProtocolManagerImpl.h" Why is this needed?

Comment on attachment 227790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227790&action=review >> Source/WebKit2/Shared/Network/CustomProtocols/CustomProtocolManager.h:44 >> +#include "CustomProtocolManagerImpl.h" > > Why is this needed? For the std::unique_ptr<CustomProtocolManagerImpl> member. The delete operator in std::default_delete<>() requires a complete type to operate on.

Comment on attachment 227790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227790&action=review The new #include "CustomProtocolManagerImpl.h" does not make sense. I don’t see how it’s related to the rest of this patch. It should only be done if there is a good reason. r=me on the rest of this patch > Source/WebKit2/Platform/IPC/Connection.cpp:281 > void Connection::dispatchWorkQueueMessageReceiverMessage(WorkQueueMessageReceiver* workQueueMessageReceiver, MessageDecoder* incomingMessageDecoder) > { > - OwnPtr<MessageDecoder> decoder = adoptPtr(incomingMessageDecoder); > + std::unique_ptr<MessageDecoder> decoder(incomingMessageDecoder); The argument type of this function should be changed to std::unique_ptr<MessageDecoder>; this “function that takes a raw pointer but adopts it” is not a good idiom. >>> Source/WebKit2/Shared/Network/CustomProtocols/CustomProtocolManager.h:44 >>> +#include "CustomProtocolManagerImpl.h" >> >> Why is this needed? > > For the std::unique_ptr<CustomProtocolManagerImpl> member. The delete operator in std::default_delete<>() requires a complete type to operate on. A member of what class? I don’t see an additional std::unique_ptr<CustomProtocolManagerImpl> in this patch; I still have no idea why this change is here in this patch. > Source/WebKit2/UIProcess/WebPageProxy.cpp:211 > + return std::move(record); Normally you would not need std::move here. Did you try without it? > Source/WebKit2/UIProcess/InspectorServer/WebSocketServer.cpp:92 > - Deque<OwnPtr<WebSocketServerConnection> >::iterator end = m_connections.end(); > - for (Deque<OwnPtr<WebSocketServerConnection> >::iterator it = m_connections.begin(); it != end; ++it) { > + for (auto it = m_connections.begin(), end = m_connections.end(); it != end; ++it) { > if (it->get() == connection) { > m_connections.remove(it); > return; > } > } Seeing this makes me think that a Deque is the wrong data structure to use. If this kind of operation is needed then it should be ListHashSet, not Deque. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:642 > + dispatcher->dispatch(bind(callCallbackFunction, context, callbackFunction, securityOrigins.release())); This release() idiom is really error-prone. We need to change this all around so it uses lambdas instead of bind to get rid of that.

Comment on attachment 227790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227790&action=review >> Source/WebKit2/Platform/IPC/Connection.cpp:281 >> + std::unique_ptr<MessageDecoder> decoder(incomingMessageDecoder); > > The argument type of this function should be changed to std::unique_ptr<MessageDecoder>; this “function that takes a raw pointer but adopts it” is not a good idiom. This isn't possible for the moment because this method is invoked through WTF::bind(). >>>> Source/WebKit2/Shared/Network/CustomProtocols/CustomProtocolManager.h:44 >>>> +#include "CustomProtocolManagerImpl.h" >>> >>> Why is this needed? >> >> For the std::unique_ptr<CustomProtocolManagerImpl> member. The delete operator in std::default_delete<>() requires a complete type to operate on. > > A member of what class? I don’t see an additional std::unique_ptr<CustomProtocolManagerImpl> in this patch; I still have no idea why this change is here in this patch. Of the CustomProtocolManager class, on non-Cocoa platforms. The error originates from the addSupplement<CustomProtocolManager>() call in the NetworkProcess constructor where the std::unique_ptr<CustomProtocolManager> destructor is generated. >> Source/WebKit2/UIProcess/WebPageProxy.cpp:211 >> + return std::move(record); > > Normally you would not need std::move here. Did you try without it? Indeed, std::move() is not needed here. >> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:642 >> + dispatcher->dispatch(bind(callCallbackFunction, context, callbackFunction, securityOrigins.release())); > > This release() idiom is really error-prone. We need to change this all around so it uses lambdas instead of bind to get rid of that. Unfortunately lambdas aren't able to consume move-only objects in C++11, but we should be able to fix this once we adopt C++14.

Created attachment 230243 [details] Patch for landing Rechecking through EWS, landing after that.

Comment on attachment 230243 [details] Patch for landing Clearing flags on attachment: 230243 Committed r167854: <http://trac.webkit.org/changeset/167854>

All reviewed patches have been landed. Closing bug.