Summary: | Use UniqueRef<> instead of std::unique_ptr<> when passing IPC::Encoder to sendMessage() | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, darin, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, kkinnunen, philipj, sam, sergio, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=224377 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2021-03-09 14:25:40 PST
Created attachment 422761 [details]
Patch
Created attachment 422771 [details]
Patch
Created attachment 422774 [details]
Patch
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. 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? 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. Created attachment 422780 [details]
Patch
Created attachment 422781 [details]
Patch
Committed r274189: <https://commits.webkit.org/r274189> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422781 [details]. |