Bug 222992 – Use UniqueRef<> instead of std::unique_ptr<> when passing IPC::Encoder to sendMessage()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Use UniqueRef<> instead of std::unique_ptr<> when passing IPC::Encoder to sendMessage(), to make it clear it cannot be null.

[Chris Dumez]

Created attachment 422761 [details]
Patch

[Chris Dumez]

Created attachment 422771 [details]
Patch

[Chris Dumez]

Created attachment 422774 [details]
Patch

[Alex Christensen]

Comment on attachment 422774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422774&action=review

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:-637
> -        replyEncoder.reset();

There are two of these.  Why don't you remove both of them?

> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h:75
> +    bool didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, UniqueRef<IPC::Encoder>& replyEncoder);

We should probably use UniqueRef<>&& and move the ownership.
It would be worth putting in the change log that we need to return a boolean because we are not checking the null status of the unique_ptr any more.

[Chris Dumez]

Comment on attachment 422774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422774&action=review

>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:-637
>> -        replyEncoder.reset();
> 
> There are two of these.  Why don't you remove both of them?

Those of these? I don't see any other replyEncoder.reset() in this function. If there was, I don't think it would build because UniqueRef::reset() is not a think AFAIK.
I must be misunderstanding your comment?

>> Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h:75
>> +    bool didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, UniqueRef<IPC::Encoder>& replyEncoder);
> 
> We should probably use UniqueRef<>&& and move the ownership.
> It would be worth putting in the change log that we need to return a boolean because we are not checking the null status of the unique_ptr any more.

I will clarify the ChangeLog.

The result I used a reference instead of a rvalue-reference is that the call site would look really scary with a r-value reference:
if (didReceiveSyncMessage(connection, decoder, WTFMove(encoder))
    return;
// Use encoder here.

I get that it would work since WTFMove() is just a cast. But it looks scary so I am not sure I want to make that change. WDYT?

[Alex Christensen]

Comment on attachment 422774 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=422774&action=review

>>> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:-637
>>> -        replyEncoder.reset();
>> 
>> There are two of these.  Why don't you remove both of them?
> 
> Those of these? I don't see any other replyEncoder.reset() in this function. If there was, I don't think it would build because UniqueRef::reset() is not a think AFAIK.
> I must be misunderstanding your comment?

The Messages::RemoteGraphicsContextGL::messageReceiverName code for decoders does something like this too.  Never mind.

[Chris Dumez]

Created attachment 422780 [details]
Patch

[Chris Dumez]

Created attachment 422781 [details]
Patch

[EWS]

Committed r274189: <https://commits.webkit.org/r274189>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422781 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75244991>