Bug 182269 - Use CompletionHandlers for DelayedReplies
Summary: Use CompletionHandlers for DelayedReplies
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-29 16:37 PST by Alex Christensen
Modified: 2018-05-17 16:58 PDT (History)
5 users (show)

See Also:


Attachments
Patch (68.53 KB, patch)
2018-01-29 16:39 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (68.53 KB, patch)
2018-01-29 16:58 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (68.51 KB, patch)
2018-01-29 17:15 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (68.52 KB, patch)
2018-04-04 15:39 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (70.23 KB, patch)
2018-04-04 15:53 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (70.31 KB, patch)
2018-04-04 15:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (70.23 KB, patch)
2018-04-04 16:10 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.93 KB, patch)
2018-05-17 14:03 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (81.88 KB, patch)
2018-05-17 14:13 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (82.51 KB, patch)
2018-05-17 14:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.