Bug 129670 - Move cross-port WebKit2 code to std::unique_ptr
Summary: Move cross-port WebKit2 code to std::unique_ptr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks: 128007
  Show dependency treegraph
 
Reported: 2014-03-04 01:25 PST by Zan Dobersek
Modified: 2014-04-27 06:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (51.13 KB, patch)
2014-03-04 01:25 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (46.73 KB, patch)
2014-03-25 13:40 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (46.94 KB, patch)
2014-04-26 06:29 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-03-04 01:25:15 PST
Move cross-port WebKit2 code to std::unique_ptr
Comment 1 Zan Dobersek 2014-03-04 01:25:55 PST
Created attachment 225754 [details]
Patch
Comment 2 Zan Dobersek 2014-03-25 13:40:44 PDT
Created attachment 227790 [details]
Patch
Comment 3 Anders Carlsson 2014-04-01 06:55:26 PDT
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 4 Zan Dobersek 2014-04-03 13:51:17 PDT
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 5 Darin Adler 2014-04-07 14:47:23 PDT
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 6 Zan Dobersek 2014-04-08 00:16:32 PDT
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.
Comment 7 Zan Dobersek 2014-04-26 06:29:38 PDT
Created attachment 230243 [details]
Patch for landing

Rechecking through EWS, landing after that.
Comment 8 Zan Dobersek 2014-04-27 06:40:42 PDT
Comment on attachment 230243 [details]
Patch for landing

Clearing flags on attachment: 230243

Committed r167854: <http://trac.webkit.org/changeset/167854>
Comment 9 Zan Dobersek 2014-04-27 06:40:55 PDT
All reviewed patches have been landed.  Closing bug.