Bug 182269 – Use CompletionHandlers for DelayedReplies

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Use CompletionHandlers for DelayedReplies

[Alex Christensen]

Created attachment 332600 [details]
Patch

[Alex Christensen]

Created attachment 332604 [details]
Patch

[Alex Christensen]

Created attachment 332606 [details]
Patch

[Alex Christensen]

Created attachment 337233 [details]
Patch

[youenn fablet]

Comment on attachment 337233 [details]
Patch

Great patch!

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

> Source/WebKit/ChangeLog:9
> +        called once.  This is what CompletionHandlers are for.

called once and always once.
Debug WK2 bots might tell us whether we always call these in any layout/api test.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:90
> +NetworkResourceLoader::NetworkResourceLoader(const NetworkResourceLoadParameters& parameters, NetworkConnectionToWebProcess& connection, Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::DelayedReply&& synchronousReply)

Since we are modifying NetworkResourceLoader constructor, maybe we could make it take a NetworkResourceLoadParameters&&?

> Source/WebKit/Platform/IPC/Connection.h:57
> +template<typename> struct CodingType;

Why do we need this one?

> Source/WebKit/PluginProcess/PluginControllerProxy.cpp:102
> +    return WTFMove(m_initializationReply);

Maybe we should do ASSERT(m_initializationReply)?

[Alex Christensen]

Created attachment 337235 [details]
Patch

[Alex Christensen]

(In reply to youenn fablet from comment #5)
> > Source/WebKit/Platform/IPC/Connection.h:57
> > +template<typename> struct CodingType;
> 
> Why do we need this one?
This was needed to get it to compile.  Something's different about the way this symbol is used and the way templates are instantiated.  It's just a forward declaration, though.

[Alex Christensen]

Created attachment 337236 [details]
Patch

[Alex Christensen]

Created attachment 337237 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r230283

[Radar WebKit Bug Importer]

<rdar://problem/39192707>

[Ryan Haddad]

This change introduced webkitpy test failures:

[6/1711] webkit.messages_unittest.HeaderTest.test_receiver_headers failed:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 334, in test_receiver_headers
      _expected_receiver_header_file_name)
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 324, in assertHeaderEqual
      self.assertGeneratedFileContentsEqual(actual_file_contents, expected_file_name)
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 315, in assertGeneratedFileContentsEqual
      self.assertEquals(actual_line, expected_line_list[index])
  AssertionError: '    using DelayedReply = CompletionHandler<void(const IPC::Connection::Handle& connectionHandle)>;    static void send(std::unique_ptr<IPC::Encoder>&&, IPC::Connection&, const IPC::Connection::Handle& connectionHandle);' != '    struct DelayedReply : public ThreadSafeRefCounted<DelayedReply> {'
  
[9/1711] webkit.messages_unittest.ReceiverImplementationTest.test_receiver_implementations failed:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 344, in test_receiver_implementations
      _expected_receiver_implementation_file_name)
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 328, in assertImplementationEqual
      self.assertGeneratedFileContentsEqual(actual_file_contents, expected_file_name)
    File "/Volumes/Data/slave/sierra-release-tests-wk1/build/Source/WebKit/Scripts/webkit/messages_unittest.py", line 315, in assertGeneratedFileContentsEqual
      self.assertEquals(actual_line, expected_line_list[index])
  AssertionError: 'void GetPluginProcessConnection::send(std::unique_ptr<IPC::Encoder>&& encoder, IPC::Connection& connection, const IPC::Connection::Handle& connectionHandle)' != 'GetPluginProcessConnection::DelayedReply::DelayedReply(Ref<IPC::Connection>&& connection, std::unique_ptr<IPC::Encoder> encoder)'
  
https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20(Tests)/builds/9255

[Ryan Haddad]

Reverted r230283 for reason:

Caused webkitpy test failures.

Committed r230293: <https://trac.webkit.org/changeset/230293>

[Ryan Haddad]

It looks like this change was also responsible for the com.apple.WebKit.Plugin.64.Development assertion failures seen here:
https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r230284%20(2730)/results.html

[Alex Christensen]

Created attachment 340635 [details]
Patch

[Alex Christensen]

Created attachment 340640 [details]
Patch

[Alex Christensen]

Created attachment 340650 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 340650 [details]
Patch

Clearing flags on attachment: 340650

Committed r231931: <https://trac.webkit.org/changeset/231931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.