Closed
Bug 1311324
Opened 7 years ago
Closed 6 years ago
Update ServiceWorker/ExtendableMessageEvent interfaces
Categories
(Core :: DOM: Service Workers, defect, P3)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: catalinb, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(4 files, 9 obsolete files)
6.39 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
11.33 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
5.46 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
Some minor changes were made to the webidl. See https://github.com/w3c/ServiceWorker/issues/989
Updated•7 years ago
|
Priority: P4 → P3
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
This patch updates our MessageEvent webidl and implementation. Main differences are: 1) origin attribute should be USVString 2) source attribute should permit ServiceWorker values
Attachment #8845466 -
Attachment is obsolete: true
Attachment #8845514 -
Flags: review?(amarchesini)
Assignee | ||
Comment 3•6 years ago
|
||
Replace our use of ServiceWorkerMessageEvent with MessageEvent. This matches the current spec in step 8.1 of: https://w3c.github.io/ServiceWorker/#client-postmessage
Attachment #8845519 -
Flags: review?(amarchesini)
Assignee | ||
Comment 4•6 years ago
|
||
Remove ServiceWorkerMessageEvent now that its not used any more.
Attachment #8845522 -
Flags: review?(amarchesini)
Assignee | ||
Comment 5•6 years ago
|
||
Update WPT test expectations now that we pass these two tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce56b3cad1003796bc2536a8e99256f6de3aeb17
Attachment #8845523 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•6 years ago
|
||
Note, there is a reference to ServiceWorkerMessageEvent in a devtools debugger.js, but they tell me I can ignore that.
Assignee | ||
Updated•6 years ago
|
Attachment #8845514 -
Flags: review?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Attachment #8845519 -
Flags: review?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Attachment #8845522 -
Flags: review?(amarchesini)
Assignee | ||
Updated•6 years ago
|
Attachment #8845523 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•6 years ago
|
||
There is some spec debate about Window vs WindowProxy here. I spoke with bz and he is ok with using WindowProxy if we convert to storing the outer window. I think this will also fix crashes in the last try build.
Attachment #8845514 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Attachment #8845540 -
Attachment is obsolete: true
Assignee | ||
Comment 9•6 years ago
|
||
Attachment #8845519 -
Attachment is obsolete: true
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8845522 -
Attachment is obsolete: true
Assignee | ||
Comment 11•6 years ago
|
||
Attachment #8845523 -
Attachment is obsolete: true
Assignee | ||
Comment 12•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7287e08098d47189bd2f200786aa5f9907bb658
Assignee | ||
Comment 13•6 years ago
|
||
Try is looking pretty green. I think this is ready for review. Boris, the patches here do the following: P1: Updates MessageEvent.webidl to spec. As we discussed, this means using WindowProxy for the source and storing the outer window. P2: Replace usage of ServiceWorkerMessageEvent with MessageEvent. The corresponding spec is step 8 here: https://w3c.github.io/ServiceWorker/#client-postmessage P3: Remove ServiceWorkerMessageEvent has its no longer used in gecko and does not exist in the spec any more. P4: Update WPT test expectations now that we pass more tests. Thanks for your help.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8845541 -
Attachment is obsolete: true
![]() |
||
Comment 15•6 years ago
|
||
Comment on attachment 8845589 [details] [diff] [review] P1 Update the MessageEvent webidl and implementation class. r=bz > MessageEvent::InitMessageEvent(JSContext* aCx, const nsAString& aType, You need to null out mServiceWorker where we null out mWindowSource/mPortSource. Worth adding a test for all three, perhaps. >+ RefPtr<mozilla::dom::workers::ServiceWorker> mServiceWorker; This is all in namespace mozilla::dom already. Just RefPtr<workers::ServiceWorker>, please. r=me with those fixed.
Attachment #8845589 -
Flags: review+
![]() |
||
Comment 16•6 years ago
|
||
Comment on attachment 8845542 [details] [diff] [review] P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=bz >+ if (NS_WARN_IF(rv.Failed())) { >+ xpc::Throw(aCx, rv.StealNSResult()); >+ return NS_ERROR_FAILURE; In practice, MessageEvent::Constructor never fails. How about we change MessageEvent's Constructor that takes an EventTarget not take an ErrorResult, to make this clearer. Then you won't need this code here. r=me with that.
Attachment #8845542 -
Flags: review+
![]() |
||
Comment 17•6 years ago
|
||
Comment on attachment 8845543 [details] [diff] [review] P3 Remove ServiceWorkerMessageEvent interface. r=bz r=me
Attachment #8845543 -
Flags: review+
![]() |
||
Comment 18•6 years ago
|
||
Comment on attachment 8845544 [details] [diff] [review] P4 Update WPT test expectations. r=bz Presumably this should actually be merged into part 2 or so? Either way, I guess; I just don't like known-failing stuff in bisect if it can be easily avoided. r=me
Flags: needinfo?(bzbarsky)
Attachment #8845544 -
Flags: review+
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #18) > Presumably this should actually be merged into part 2 or so? Either way, I > guess; I just don't like known-failing stuff in bisect if it can be easily > avoided. I didn't think that was an issue if it all landed in the same push. I thought we only built artifacts on push, not revisions within the push.
Assignee | ||
Comment 20•6 years ago
|
||
Updated: 1) Rename mServiceWorker to mServiceWorkerSource to match other source members. 2) Null out mServiceWorkerSource where other sources are cleared. 3) Cycle collect mServiceWorkerSource
Attachment #8845589 -
Attachment is obsolete: true
Attachment #8845615 -
Flags: review+
Assignee | ||
Comment 21•6 years ago
|
||
Updated to not require ErrorResult in MessageEvent::Constructor().
Attachment #8845542 -
Attachment is obsolete: true
Attachment #8845617 -
Flags: review+
Comment 22•6 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc235b8f878 P1 Update the MessageEvent webidl and implementation class. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/a5eb9246d24a P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/6c98344f65d4 P3 Remove ServiceWorkerMessageEvent interface. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/2269c901720f P4 Update WPT test expectations. r=bz
![]() |
||
Comment 23•6 years ago
|
||
> I thought we only built artifacts on push, not revisions within the push.
For our CI, yes. For someone doing a local bisect later, this will have a changeset with random test failures, which may or may not affect tests they are caring about during their bisect....
Comment 24•6 years ago
|
||
Backout by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/06fe334e9988 Backout 3cc235b8f878 to 2269c901720f for build bustage r=me
Assignee | ||
Comment 25•6 years ago
|
||
The bustage appears to be because this landed in m-c directly, but not m-i: https://hg.mozilla.org/mozilla-central/rev/c40ca7a1bdd9 And my patch shifted stuff around in binding directory such that it blew up there.
Assignee | ||
Comment 26•6 years ago
|
||
I verified locally that the patches build in m-i after c40ca7a1bdd9 was merged. The tree is closed for taskcluster issues, though, so flagging as checkin-needed.
Keywords: checkin-needed
Comment 27•6 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/658ee6463bec P1 Update the MessageEvent webidl and implementation class. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/d1db3a8f86d6 P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/baeb310cfabf P3 Remove ServiceWorkerMessageEvent interface. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdd2431a30d P4 Update WPT test expectations. r=bz
Keywords: checkin-needed
Comment 28•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/658ee6463bec https://hg.mozilla.org/mozilla-central/rev/d1db3a8f86d6 https://hg.mozilla.org/mozilla-central/rev/baeb310cfabf https://hg.mozilla.org/mozilla-central/rev/fbdd2431a30d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 29•6 years ago
|
||
I have documented this change. First of all, I have completed the documentation for MessageEvent, along with including details about the origin and source changes (most of it was missing): https://developer.mozilla.org/en-US/docs/Web/API/MessageEvent I have also updated all (I think) relevant SW pages with details about the deprecation of ServiceWorkerMessageEvent: https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerMessageEvent (and child pages) https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerContainer/onmessage https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/onmessage https://developer.mozilla.org/en-US/docs/Web/API/Client/postMessage Finally, I've added two separate notes to the Fx55 rel notes to explain the change: https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM https://developer.mozilla.org/en-US/Firefox/Releases/55#Service_Workers Can I get a tech review please? Thanks!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•