RESOLVED FIXED Bug 181454
Make in-process MessagePorts be (mostly) asynchronous
https://bugs.webkit.org/show_bug.cgi?id=181454
Summary Make in-process MessagePorts be (mostly) asynchronous
Brady Eidson
Reported 2018-01-09 15:13:22 PST
Make in-process MessagePorts be fully asynchronous While working on https://bugs.webkit.org/show_bug.cgi?id=181178 the approach of using "MesasgePortChannels" became too unwieldly. It works, but is fragile. Instead, even in-process MessagePorts should be fully asynchronous (and this means that even a local MessagePort should never directly know about a remote one even if they are in the same process... even if they are in the same ScriptExecutionContext)
Attachments
Preliminary (119.25 KB, patch)
2018-01-17 16:23 PST, Brady Eidson
no flags
Preliminary (119.28 KB, patch)
2018-01-17 16:56 PST, Brady Eidson
ews-watchlist: commit-queue-
Preliminary (119.72 KB, patch)
2018-01-17 17:04 PST, Brady Eidson
ews-watchlist: commit-queue-
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.16 MB, application/zip)
2018-01-17 18:32 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.46 MB, application/zip)
2018-01-17 18:40 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews100 for mac-sierra (2.60 MB, application/zip)
2018-01-17 18:59 PST, EWS Watchlist
no flags
Preliminary (119.74 KB, patch)
2018-01-17 19:08 PST, Brady Eidson
ews-watchlist: commit-queue-
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.08 MB, application/zip)
2018-01-17 20:02 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews101 for mac-sierra (2.20 MB, application/zip)
2018-01-17 20:19 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.34 MB, application/zip)
2018-01-17 20:41 PST, EWS Watchlist
no flags
Preliminary (119.26 KB, patch)
2018-01-17 20:42 PST, Brady Eidson
no flags
Preliminary (119.32 KB, patch)
2018-01-17 22:17 PST, Brady Eidson
ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.59 MB, application/zip)
2018-01-17 23:23 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.28 MB, application/zip)
2018-01-17 23:41 PST, EWS Watchlist
no flags
Preliminary (118.45 KB, patch)
2018-01-18 09:53 PST, Brady Eidson
no flags
Patch (126.59 KB, patch)
2018-01-18 13:16 PST, Brady Eidson
no flags
Patch (126.66 KB, patch)
2018-01-18 20:47 PST, Brady Eidson
no flags
Patch (126.69 KB, patch)
2018-01-18 20:53 PST, Brady Eidson
no flags
Brady Eidson
Comment 1 2018-01-17 16:22:37 PST
"Fully asynchronous except for a sync function to support GC", really
Brady Eidson
Comment 2 2018-01-17 16:23:48 PST
Created attachment 331557 [details] Preliminary
EWS Watchlist
Comment 3 2018-01-17 16:27:31 PST
Attachment 331557 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/context/SWContextManager.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:155: Missing space before { [whitespace/braces] [5] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannel.cpp:167: Missing space before { [whitespace/braces] [5] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/dom/MessagePort.h:48: The parameter name "scriptExecutionContext" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/MessageChannel.h:39: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/workers/WorkerObjectProxy.h:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 13 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brady Eidson
Comment 4 2018-01-17 16:56:05 PST
Created attachment 331562 [details] Preliminary
EWS Watchlist
Comment 5 2018-01-17 16:59:28 PST
Attachment 331562 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brady Eidson
Comment 6 2018-01-17 17:04:25 PST
Created attachment 331564 [details] Preliminary
EWS Watchlist
Comment 7 2018-01-17 17:06:20 PST
Attachment 331564 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:163: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 8 2018-01-17 18:32:55 PST
Comment on attachment 331562 [details] Preliminary Attachment 331562 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6114152 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 9 2018-01-17 18:32:57 PST
Created attachment 331573 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 10 2018-01-17 18:40:51 PST
Comment on attachment 331564 [details] Preliminary Attachment 331564 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6114263 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 11 2018-01-17 18:40:52 PST
Created attachment 331574 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 12 2018-01-17 18:58:58 PST
Comment on attachment 331564 [details] Preliminary Attachment 331564 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6114770 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 13 2018-01-17 18:59:00 PST
Created attachment 331577 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Brady Eidson
Comment 14 2018-01-17 19:08:22 PST
Created attachment 331578 [details] Preliminary Accidentally removed a necessary unlockEarly() in the previous patches
EWS Watchlist
Comment 15 2018-01-17 19:10:03 PST
Attachment 331578 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 35 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 16 2018-01-17 20:02:18 PST
Comment on attachment 331578 [details] Preliminary Attachment 331578 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6115461 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 17 2018-01-17 20:02:19 PST
Created attachment 331581 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 18 2018-01-17 20:19:52 PST
Comment on attachment 331578 [details] Preliminary Attachment 331578 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6115556 New failing tests: security/cannot-read-self-from-file.html fast/xmlhttprequest/xmlhttprequest-no-file-access.html
EWS Watchlist
Comment 19 2018-01-17 20:19:54 PST
Created attachment 331582 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 20 2018-01-17 20:41:55 PST
Comment on attachment 331578 [details] Preliminary Attachment 331578 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6115628 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 21 2018-01-17 20:41:56 PST
Created attachment 331583 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Brady Eidson
Comment 22 2018-01-17 20:42:37 PST
Created attachment 331584 [details] Preliminary
EWS Watchlist
Comment 23 2018-01-17 20:45:18 PST
Attachment 331584 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brady Eidson
Comment 24 2018-01-17 22:17:12 PST
Created attachment 331597 [details] Preliminary Wow. Turns out RunLoop::main().dispatch() and callOnMainThread() have completely different semantics even though they seem like synonyms for one-another.
EWS Watchlist
Comment 25 2018-01-17 22:21:13 PST
Attachment 331597 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
EWS Watchlist
Comment 26 2018-01-17 23:23:19 PST
Comment on attachment 331597 [details] Preliminary Attachment 331597 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6117336 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 27 2018-01-17 23:23:20 PST
Created attachment 331601 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
EWS Watchlist
Comment 28 2018-01-17 23:41:20 PST
Comment on attachment 331597 [details] Preliminary Attachment 331597 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/6117329 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 29 2018-01-17 23:41:21 PST
Created attachment 331602 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Brady Eidson
Comment 30 2018-01-18 09:53:34 PST
Created attachment 331635 [details] Preliminary
EWS Watchlist
Comment 31 2018-01-18 09:57:38 PST
Attachment 331635 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 33 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brady Eidson
Comment 32 2018-01-18 13:16:17 PST
EWS Watchlist
Comment 33 2018-01-18 13:18:16 PST
Attachment 331653 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 6 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Alex Christensen
Comment 34 2018-01-18 16:04:54 PST
Comment on attachment 331653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331653&action=review > Source/WebCore/dom/MessagePort.cpp:229 > + auto innerHandler = [this, protectedThis = makeRef(*this)](Vector<MessageWithMessagePorts>&& messages) { MSVC doesn't like this line. I think it would be fixed if you did protectedThis = WTFMove(protectedThis)
Alex Christensen
Comment 35 2018-01-18 16:21:02 PST
Comment on attachment 331653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331653&action=review > Source/WebCore/dom/messageports/MessagePortChannel.h:55 > + void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&); This could use CompletionHandler > Source/WebCore/dom/messageports/MessagePortChannel.h:65 > + unsigned m_refCount { 1 }; What's this for? > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:47 > +void MessagePortChannelProvider::setSharedProvider(MessagePortChannelProvider& provider) This doesn't seem to be called. Will it be? > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:40 > +void MessagePortChannelProviderImpl::performActionOnAppropriateThread(Function<void()>&& action) performActionOnMainThread? > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:102 > + // FIXME: Remove this sync function call when GC logic is made asynchronous. Is this really ever going to happen? > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45 > + bool hasMessagesForPorts_temporarySync(const MessagePortIdentifier&, const MessagePortIdentifier&) final; We typically don't like _ in function names. Why is this temporary sync?
Brady Eidson
Comment 36 2018-01-18 17:13:57 PST
(In reply to Alex Christensen from comment #35) > Comment on attachment 331653 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331653&action=review > > > Source/WebCore/dom/messageports/MessagePortChannel.h:55 > > + void takeAllMessagesForPort(const MessagePortIdentifier&, Function<void(Vector<MessageWithMessagePorts>&&, Function<void()>&&)>&&); > > This could use CompletionHandler I might try this, but every time I do I run into some nullness requirement. > > > Source/WebCore/dom/messageports/MessagePortChannel.h:65 > > + unsigned m_refCount { 1 }; > > What's this for? Leftover from testing/debugging with my own ref/deref() :( Removed. > > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:47 > > +void MessagePortChannelProvider::setSharedProvider(MessagePortChannelProvider& provider) > > This doesn't seem to be called. Will it be? Yes! But I can remove it from here. > > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:40 > > +void MessagePortChannelProviderImpl::performActionOnAppropriateThread(Function<void()>&& action) > > performActionOnMainThread? Yah, sure. > > > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:102 > > + // FIXME: Remove this sync function call when GC logic is made asynchronous. > > Is this really ever going to happen? Absolutely. > > Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45 > > + bool hasMessagesForPorts_temporarySync(const MessagePortIdentifier&, const MessagePortIdentifier&) final; > > We typically don't like _ in function names. > Why is this temporary sync? I called this out in the ChangeLog - I want this to look ugly and wrong and discourage use. It's going away in the async GC followup.
Alex Christensen
Comment 37 2018-01-18 17:14:52 PST
Comment on attachment 331653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331653&action=review r=me once windows bots are green. > Source/WebCore/dom/messageports/MessagePortChannel.h:74 > + MessagePortIdentifier m_ports[2]; These could be pairs. Or not. > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:41 > + globalProvider = new MessagePortChannelProviderImpl; I feel like this is what NeverDestroyed is for. > Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37 > + __attribute__ ((noreturn)) ~MessagePortChannelRegistry(); This probably doesn't work on MSVC. It looks like we need __declspec(noreturn) in a macro.
Brady Eidson
Comment 38 2018-01-18 20:37:39 PST
(In reply to Alex Christensen from comment #37) > Comment on attachment 331653 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331653&action=review > > r=me once windows bots are green. > > > Source/WebCore/dom/messageports/MessagePortChannelProvider.cpp:41 > > + globalProvider = new MessagePortChannelProviderImpl; > > I feel like this is what NeverDestroyed is for. Kindof! But with the additional semantic of the outside client being able to override that makes NeverDestroyed a little less applicable. > > > Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:37 > > + __attribute__ ((noreturn)) ~MessagePortChannelRegistry(); > > This probably doesn't work on MSVC. It looks like we need > __declspec(noreturn) in a macro. Ugh.
Brady Eidson
Comment 39 2018-01-18 20:47:53 PST
Brady Eidson
Comment 40 2018-01-18 20:53:05 PST
EWS Watchlist
Comment 41 2018-01-18 20:55:07 PST
Attachment 331706 [details] did not pass style-queue: ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.cpp:164: MessagePortChannelRegistry::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelRegistry.h:47: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProvider.h:50: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.h:45: hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/dom/messageports/MessagePortChannelProviderImpl.cpp:100: MessagePortChannelProviderImpl::hasMessagesForPorts_temporarySync is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 5 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 42 2018-01-18 22:33:17 PST
Comment on attachment 331706 [details] Patch Clearing flags on attachment: 331706 Committed r227190: <https://trac.webkit.org/changeset/227190>
WebKit Commit Bot
Comment 43 2018-01-18 22:33:19 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 44 2018-01-18 22:34:51 PST
Jonathan Bedard
Comment 45 2018-01-19 10:13:45 PST
Follow-up build fix required to remove unused lambda captures <https://trac.webkit.org/changeset/227213>.
Michael Catanzaro
Comment 46 2018-01-23 09:26:47 PST
Note You need to log in before you can comment on or make changes to this bug.