Update ServiceWorker/ExtendableMessageEvent interfaces

RESOLVED FIXED in Firefox 55

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: catalinb, Assigned: bkelly)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments, 9 obsolete attachments)

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
(Reporter)

Description

3 years ago
Some minor changes were made to the webidl. See
https://github.com/w3c/ServiceWorker/issues/989
Priority: P4 → P3
(Assignee)

Updated

2 years ago
Blocks: 1293277
(Assignee)

Updated

2 years ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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)
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)
Remove ServiceWorkerMessageEvent now that its not used any more.
Attachment #8845522 - Flags: review?(amarchesini)
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)
Note, there is a reference to ServiceWorkerMessageEvent in a devtools debugger.js, but they tell me I can ignore that.
(Assignee)

Updated

2 years ago
Blocks: 1345943
(Assignee)

Updated

2 years ago
Attachment #8845514 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8845519 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8845522 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8845523 - Flags: review?(amarchesini)
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
Attachment #8845523 - Attachment is obsolete: true
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)
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 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 on attachment 8845543 [details] [diff] [review]
P3 Remove ServiceWorkerMessageEvent interface. r=bz

r=me
Attachment #8845543 - Flags: review+
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+
(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.
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+
Updated to not require ErrorResult in MessageEvent::Constructor().
Attachment #8845542 - Attachment is obsolete: true
Attachment #8845617 - Flags: review+

Comment 22

2 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
> 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

2 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
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.
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

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