Closed Bug 1311324 Opened 9 years ago Closed 8 years ago

Update ServiceWorker/ExtendableMessageEvent interfaces

Categories

(Core :: DOM: Service Workers, defect, P3)

defect

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
Priority: P4 → P3
Blocks: 1293277
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)
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.
Blocks: 1345943
Attachment #8845514 - Flags: review?(amarchesini)
Attachment #8845519 - Flags: review?(amarchesini)
Attachment #8845522 - Flags: review?(amarchesini)
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+
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....
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
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.

Attachment

General

Created:
Updated:
Size: