WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Preliminary
(119.28 KB, patch)
2018-01-17 16:56 PST
,
Brady Eidson
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Preliminary
(119.72 KB, patch)
2018-01-17 17:04 PST
,
Brady Eidson
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Preliminary
(119.74 KB, patch)
2018-01-17 19:08 PST
,
Brady Eidson
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Preliminary
(119.26 KB, patch)
2018-01-17 20:42 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Preliminary
(119.32 KB, patch)
2018-01-17 22:17 PST
,
Brady Eidson
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
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
Details
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
Details
Preliminary
(118.45 KB, patch)
2018-01-18 09:53 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(126.59 KB, patch)
2018-01-18 13:16 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(126.66 KB, patch)
2018-01-18 20:47 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(126.69 KB, patch)
2018-01-18 20:53 PST
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(17)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 331653
[details]
Patch
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
Created
attachment 331704
[details]
Patch
Brady Eidson
Comment 40
2018-01-18 20:53:05 PST
Created
attachment 331706
[details]
Patch
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
<
rdar://problem/36647079
>
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
Committed
r227420
: <
https://trac.webkit.org/changeset/227420
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug