Bug 182269

Summary: Use CompletionHandlers for DelayedReplies
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, ryanhaddad, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2018-01-29 16:37:10 PST
Use CompletionHandlers for DelayedReplies
Comment 1 Alex Christensen 2018-01-29 16:39:28 PST
Created attachment 332600 [details]
Patch
Comment 2 Alex Christensen 2018-01-29 16:58:04 PST
Created attachment 332604 [details]
Patch
Comment 3 Alex Christensen 2018-01-29 17:15:13 PST
Created attachment 332606 [details]
Patch
Comment 4 Alex Christensen 2018-04-04 15:39:21 PDT
Created attachment 337233 [details]
Patch
Comment 5 youenn fablet 2018-04-04 15:45:28 PDT
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)?
Comment 6 Alex Christensen 2018-04-04 15:53:34 PDT
Created attachment 337235 [details]
Patch
Comment 7 Alex Christensen 2018-04-04 15:55:33 PDT
(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.
Comment 8 Alex Christensen 2018-04-04 15:59:29 PDT
Created attachment 337236 [details]
Patch
Comment 9 Alex Christensen 2018-04-04 16:10:05 PDT
Created attachment 337237 [details]
Patch
Comment 10 Alex Christensen 2018-04-04 16:21:06 PDT
http://trac.webkit.org/r230283
Comment 11 Radar WebKit Bug Importer 2018-04-04 16:23:22 PDT
<rdar://problem/39192707>
Comment 12 Ryan Haddad 2018-04-04 17:12:49 PDT
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
Comment 13 Ryan Haddad 2018-04-04 20:16:26 PDT
Reverted r230283 for reason:

Caused webkitpy test failures.

Committed r230293: <https://trac.webkit.org/changeset/230293>
Comment 14 Ryan Haddad 2018-04-04 20:53:21 PDT
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
Comment 15 Alex Christensen 2018-05-17 14:03:23 PDT
Created attachment 340635 [details]
Patch
Comment 16 Alex Christensen 2018-05-17 14:13:40 PDT
Created attachment 340640 [details]
Patch
Comment 17 Alex Christensen 2018-05-17 14:47:28 PDT
Created attachment 340650 [details]
Patch
Comment 18 WebKit Commit Bot 2018-05-17 16:58:06 PDT
Comment on attachment 340650 [details]
Patch

Clearing flags on attachment: 340650

Committed r231931: <https://trac.webkit.org/changeset/231931>
Comment 19 WebKit Commit Bot 2018-05-17 16:58:08 PDT
All reviewed patches have been landed.  Closing bug.