Closed
Bug 1308956
Opened 6 years ago
Closed 6 years ago
Get rid of MessagePortList
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: baku, Mentored)
Details
Attachments
(2 files, 4 obsolete files)
3.19 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
27.92 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
There is no such thing in the spec now. It uses a FrozenArray instead. We presumably want to use an nsTArray for internal storage, have an internal accessor for it, and then do the usual [Constant, Cached, Frozen] sequence thing (or Pure if the list can change, but I expect it can't for a MessageEvent). Note also https://github.com/whatwg/html/issues/1882 about whether .ports should ever be null.
![]() |
Reporter | |
Updated•6 years ago
|
Mentor: bzbarsky
Assignee | ||
Comment 1•6 years ago
|
||
I don't remove MessagePortList yet because it's used by nsFrameMessageManager::ReceiveMessage. I'll remove it in a separate patch.
Assignee: nobody → amarchesini
Attachment #8799469 -
Flags: review?(bugs)
![]() |
Reporter | |
Comment 2•6 years ago
|
||
> if (aParam.mPorts.WasPassed() && !aParam.mPorts.Value().IsNull()) {
You shouldn't need the WasPassed() thing. Just make the default value null in the dictionary, no?
But past that, treating empty list as an alias for "null" isn't right; in terms of the current spec definition they should lead to different observable behavior...
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8799472 -
Flags: review?(bugs)
Comment 4•6 years ago
|
||
Comment on attachment 8799469 [details] [diff] [review] part 1 - MessagePortList removed from events >@@ -135,22 +134,17 @@ MessageEvent::Constructor(EventTarget* a > } else { > event->mPortSource = aParam.mSource.Value().GetAsMessagePort(); > } > > MOZ_ASSERT(event->mWindowSource || event->mPortSource); > } > > if (aParam.mPorts.WasPassed() && !aParam.mPorts.Value().IsNull()) { Yeah, as bz said, we should fix this stuff to follow the spec. Looks like our MessageEventInit isn't right atm. Could you upload another patch here to fix MessageEventInit and fix this ctor handling, or perhaps just fix it in a new version of this patch. +MessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts) { - MOZ_ASSERT(!mPorts && aPorts); - mPorts = aPorts; + if (mPorts.IsEmpty()) { + aPorts.SetNull(); + return; + } This is not what the spec says should happen. Null should be return only if the event was initialized with null ports. empty != null >+++ b/dom/webidl/MessageEvent.webidl >@@ -35,17 +35,18 @@ interface MessageEvent : Event { > */ > readonly attribute (WindowProxy or MessagePort)? source; > > /** > * Initializes this event with the given data, in a manner analogous to > * the similarly-named method on the nsIDOMEvent interface, also setting the > * data, origin, source, and lastEventId attributes of this appropriately. > */ >- readonly attribute MessagePortList? ports; >+ [Constant, Cached, Frozen] >+ readonly attribute sequence<MessagePort>? ports; The annotations look wrong, given the C++ implementation. One may call initMessageEvent and that should re-set the .ports value. I'm not seeing ClearCachedPortsValue anywhere in C++ >+++ b/dom/webidl/ServiceWorkerMessageEvent.webidl >@@ -30,17 +30,18 @@ interface ServiceWorkerMessageEvent : Ev > > /** > * The service worker or port which originated this event. > * FIXME: Use SameOject after IDL spec is updated > * https://bugzilla.mozilla.org/show_bug.cgi?id=1196097 > */ > [Constant] readonly attribute (ServiceWorker or MessagePort)? source; > >- [SameObject] readonly attribute MessagePortList? ports; >+ [Constant, Cached, Frozen] >+ readonly attribute sequence<MessagePort>? ports; Same here > #include "mozilla/dom/WorkerScope.h" >@@ -1241,39 +1240,38 @@ ExtendableMessageEvent::Constructor(mozi > event->mServiceWorker = aOptions.mSource.Value().Value().GetAsServiceWorker(); > } else if (aOptions.mSource.Value().Value().IsMessagePort()){ > event->mMessagePort = aOptions.mSource.Value().Value().GetAsMessagePort(); > } > MOZ_ASSERT(event->mClient || event->mServiceWorker || event->mMessagePort); > } > > if (aOptions.mPorts.WasPassed() && !aOptions.mPorts.Value().IsNull()) { This is similar to MessageEvent case, expect that the spec is also buggy here. https://github.com/w3c/ServiceWorker/issues/988 >+ExtendableMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts) > { >- return mPorts; >+ if (mPorts.IsEmpty()) { >+ aPorts.SetNull(); >+ return; >+ } >+ >+ aPorts.SetValue(mPorts); Same issue as that the GetPorts in MessageEvent >+ServiceWorkerMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts) > { >- MOZ_ASSERT(!mPorts && aPorts); >- mPorts = aPorts; >+ if (mPorts.IsEmpty()) { >+ aPorts.SetNull(); >+ return; >+ } >+ >+ aPorts.SetValue(mPorts); Here too
Attachment #8799469 -
Flags: review?(bugs) → review-
Updated•6 years ago
|
Attachment #8799472 -
Flags: review?(bugs) → review+
![]() |
Reporter | |
Comment 5•6 years ago
|
||
Spec just got updated to make .ports not nullable anymore.
Assignee | ||
Comment 6•6 years ago
|
||
WebIDL updated
Attachment #8799469 -
Attachment is obsolete: true
Attachment #8799628 -
Flags: review?(bugs)
Assignee | ||
Comment 7•6 years ago
|
||
I had to touch the codegen because it doesn't include XrayWrapper.h for ExtendedMessageEvent. This seems wrong to me and it hides another bug. I need feedback for that.
Attachment #8799661 -
Flags: review?(bugs)
Comment 8•6 years ago
|
||
Filed https://github.com/w3c/ServiceWorker/issues/989
Comment 9•6 years ago
|
||
Comment on attachment 8799628 [details] [diff] [review] part 1 - MessagePortList removed from events I wonder if we should wait a day or two, given that the specs (HTML and ServiceWorker) are very much in flux related to this stuff. > > dictionary MessageEventInit : EventInit { > any data; > DOMString origin; > DOMString lastEventId; > (Window or MessagePort)? source = null; >- sequence<MessagePort>? ports; >+ sequence<MessagePort> ports = []; > }; So the dictionary isn't still following the spec. Want to fix that in this bug or file a followup? All the properties have default values in the spec. >+void >+ExtendableMessageEvent::GetPorts(nsTArray<RefPtr<MessagePort>>& aPorts) > { >- return mPorts; >+ aPorts.Clear(); >+ aPorts.AppendElements(mPorts); > } So this is against the current spec... Ok, I think given the current mess in the specs, we shouldn't do anything here quite yet https://github.com/whatwg/html/issues/1882#issuecomment-252900662
Attachment #8799628 -
Flags: review?(bugs)
Comment 10•6 years ago
|
||
Comment on attachment 8799661 [details] [diff] [review] part 1 - MessagePortList removed from events Hmm, what is this? wrong patch or another version of the previous one?
Attachment #8799661 -
Flags: review?(bugs)
Assignee | ||
Comment 11•6 years ago
|
||
What about if we just do what this bug is about? Then in a follow up we can update interfaces.
Flags: needinfo?(bugs)
![]() |
Reporter | |
Comment 12•6 years ago
|
||
Comment on attachment 8799628 [details] [diff] [review] part 1 - MessagePortList removed from events >+ aPorts.Clear(); >+ aPorts.AppendElements(mPorts); aPorts = mPorts; should do the trick. And in the other similar places too. > dictionary ExtendableMessageEventInit : ExtendableEventInit { Shouldn't this change to have a non-nullable ports if the property on ExtendableMessageEvent is non-nullable? > dictionary ServiceWorkerMessageEventInit : EventInit And here.
Comment 13•6 years ago
|
||
Well, ExtendableMessageEventInit and ServiceWorkerMessageEventInit have still nullable ports in the spec. Which is why I'd like to see the specs sorted out. Maybe we'll end up having nullable ports everywhere. Or non-nullable. Who knows.
Flags: needinfo?(bugs)
Comment 14•6 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #11) > What about if we just do what this bug is about? > Then in a follow up we can update interfaces. That is fine. Then don't change the behavior otherwise, just replace use of MessagePortList with a sequence.
Assignee | ||
Comment 15•6 years ago
|
||
Attachment #8799628 -
Attachment is obsolete: true
Attachment #8799661 -
Attachment is obsolete: true
Attachment #8800378 -
Flags: review?(bugs)
Comment 16•6 years ago
|
||
Comment on attachment 8800378 [details] [diff] [review] part 1 - MessagePortList removed from events >+++ b/dom/webidl/MessageEvent.webidl >@@ -35,17 +35,18 @@ interface MessageEvent : Event { > */ > readonly attribute (WindowProxy or MessagePort)? source; > > /** > * Initializes this event with the given data, in a manner analogous to > * the similarly-named method on the nsIDOMEvent interface, also setting the > * data, origin, source, and lastEventId attributes of this appropriately. > */ >- readonly attribute MessagePortList? ports; >+ [Constant, Cached, Frozen] >+ readonly attribute sequence<MessagePort>? ports; The missing ClearCachedPortsValue crept in again to this patch. We really need a test for it, some very simple case where one first creates event with some ports and then re-initializes with different ports, and .ports should give the new ports. >-MessagePortList* >-ExtendableMessageEvent::GetPorts() const >+void >+ExtendableMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts) > { >- return mPorts; >+ if (mPorts.IsEmpty()) { >+ aPorts.SetNull(); >+ return; >+ } Empty ports doesn't mean null. >+ServiceWorkerMessageEvent::GetPorts(Nullable<nsTArray<RefPtr<MessagePort>>>& aPorts) > { >- MOZ_ASSERT(!mPorts && aPorts); >- mPorts = aPorts; >+ if (mPorts.IsEmpty()) { >+ aPorts.SetNull(); >+ return; >+ } ditto. Sounds like we're missing test for those events when null is passed as ports.
Attachment #8800378 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #8800378 -
Attachment is obsolete: true
Attachment #8800565 -
Flags: review?(bugs)
Comment 18•6 years ago
|
||
Comment on attachment 8800565 [details] [diff] [review] part 1 - MessagePortList removed from events > if (!subworker) { > info("Create a subworkers. ID: " + evt.data); > subworker = new Worker('worker_messageChannel.js'); > subworker.onmessage = function(e) { > info("Proxy a message to the parent."); >- postMessage(e.data, e.ports); >+ postMessage(e.data, e.ports ? e.ports : []); Why is this change needed.
Attachment #8800565 -
Flags: review?(bugs) → review+
Comment 19•6 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d090c04d8f Get rid of MessagePortList - part 1 - MessagePortList removed from events, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/36d6e588e7d4 Get rid of MessagePortList - part 2 - files removed, r=smaug
Assignee | ||
Comment 20•6 years ago
|
||
> Why is this change needed.
It's not needed. I reverted that change before landing the patches.
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f9d090c04d8f https://hg.mozilla.org/mozilla-central/rev/36d6e588e7d4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•