Bug 129670 – Move cross-port WebKit2 code to std::unique_ptr

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Move cross-port WebKit2 code to std::unique_ptr

[Zan Dobersek]

Created attachment 225754 [details]
Patch

[Zan Dobersek]

Created attachment 227790 [details]
Patch

[Anders Carlsson]

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?

[Zan Dobersek]

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.

[Darin Adler]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 230243 [details]
Patch for landing

Rechecking through EWS, landing after that.

[Zan Dobersek]

Comment on attachment 230243 [details]
Patch for landing

Clearing flags on attachment: 230243

Committed r167854: <http://trac.webkit.org/changeset/167854>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.