1311324 - Update ServiceWorker/ExtendableMessageEvent interfaces

Status: RESOLVED FIXED

Description

[Cătălin Badea (:catalinb)]

Some minor changes were made to the webidl. See
https://github.com/w3c/ServiceWorker/issues/989

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=baku

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=baku

This patch updates our MessageEvent webidl and implementation.  Main differences are:

1) origin attribute should be USVString
2) source attribute should permit ServiceWorker values

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=baku

Replace our use of ServiceWorkerMessageEvent with MessageEvent.  This matches the current spec in step 8.1 of:

https://w3c.github.io/ServiceWorker/#client-postmessage

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove ServiceWorkerMessageEvent interface. r=baku

Remove ServiceWorkerMessageEvent now that its not used any more.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update WPT test expectations. r=baku

Update WPT test expectations now that we pass these two tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce56b3cad1003796bc2536a8e99256f6de3aeb17

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Note, there is a reference to ServiceWorkerMessageEvent in a devtools debugger.js, but they tell me I can ignore that.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=bz

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=bz

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=bz

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Remove ServiceWorkerMessageEvent interface. r=bz

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update WPT test expectations. r=bz

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7287e08098d47189bd2f200786aa5f9907bb658

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=bz

Comment 15

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 17

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 8845543 [details] [diff] [review]
P3 Remove ServiceWorkerMessageEvent interface. r=bz

r=me

Comment 18

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update the MessageEvent webidl and implementation class. r=bz

Updated:

1) Rename mServiceWorker to mServiceWorkerSource to match other source members.
2) Null out mServiceWorkerSource where other sources are cleared.
3) Cycle collect mServiceWorkerSource

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Replace usage of ServiceWorkerMessageEvent with MessageEvent. r=bz

Updated to not require ErrorResult in MessageEvent::Constructor().

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> 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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06fe334e9988
Backout 3cc235b8f878 to 2269c901720f for build bustage r=me

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Chris Mills (Mozilla, MDN editor) [:cmills]]

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!