Use ExtendableMessageEvent for messageerror in service workers #25241 #26607

nosark · 2020-05-21T16:55:04Z

added function dispatch_error to the ExtendableMessageEvent implmentation and replaced the MessageEvent dispatch error call with the ExtendableMessageEvent dispatch error call in serviceworkerglobalscope.rs

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #25241 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-05-21T16:55:13Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @nox (or someone else) soon.

highfive · 2020-05-21T16:55:14Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/extendablemessageevent.rs, components/script/dom/serviceworkerglobalscope.rs
@KiChjang: components/script/dom/extendablemessageevent.rs, components/script/dom/serviceworkerglobalscope.rs

jdm · 2020-05-21T16:56:18Z

r? @gterzian

nosark · 2020-05-21T17:29:57Z

@jdm I followed the coding style in both files, but tidy didn't like, should I alter the format for the changes to please tidy, or leave it as is?

jdm · 2020-05-21T17:57:34Z

Please run ./mach fmt so it will automatically be adjusted to match the style that tidy checks for. Thanks!

nosark · 2020-05-21T19:28:04Z

Okay, I ran ./ mach fmt and everything should be good to go. If any other edits are needed please let me know!

gterzian · 2020-05-22T04:23:09Z

Thank you, I will look at it. First question: are the webgl and taskcluster changes related?

gterzian

Looks good, and I think we can further leverage WebIDL to do some work for us.

It also looks to me like some unrelated changes slipped into the PR.

gterzian · 2020-05-22T04:43:18Z

components/script/dom/extendablemessageevent.rs

@@ -123,6 +123,21 @@ impl ExtendableMessageEvent {
        );
        Extendablemessageevent.upcast::<Event>().fire(target);
    }
+
+    pub fn dispatch_error(target: &EventTarget, scope: &GlobalScope, message: HandleValue) {
+        let ExtendableMsgEvent = ExtendableMessageEvent::new(


We can do something similar to what is currently done in messageevent.rs, by:

In ExtendableMessageEvent.webidl, un-comment the ports attribute of ExtendableMessageEventInit.

Here, use let init = ExtendableMessageEvent::MessageEventInit::empty();

then use the data from init when calling ExtendableMessageEvent::new.

Okay, I'll rewrite and cleanup the newlines. My bad. Thank you!

gterzian · 2020-05-22T04:43:18Z

components/script/dom/extendablemessageevent.rs

+            DOMString::new(),
+            Vec::<DomRoot<MessagePort>>::new(),
+        );
+


nit: I don't think we need the newline here(not something fmt will fix).

I'll clean it up. Thank you!

gterzian · 2020-05-22T04:43:18Z

components/script/dom/serviceworkerglobalscope.rs

+                    ExtendableMessageEvent::dispatch_error(
+                        target,
+                        scope.upcast(),
+                        message.handle(),


When we will be using ExtendableMessageEvent::MessageEventInit::empty(), we don't need to pass the HandleValue anymore, since it will be part of the empty initialized value.

Okay, for some reason I thought we needed the data for the error handling. I apologize I'm newer to the code base. I'll get this fixed right away. Thank you for the input!

gterzian · 2020-05-22T05:02:42Z

While we are at it, let's also add some spec links to https://github.com/servo/servo/pull/26607/files#diff-ee0d03fc9e0cdadcde40c50bd0f8369eL30

For the struct as a whole it would be https://w3c.github.io/ServiceWorker/#extendablemessageevent-interface

and you'll see below the links for the various members(not complete one to one match but you'll see), for example for data it would be https://w3c.github.io/ServiceWorker/#extendablemessage-event-data

The docs require a triple ///

nosark · 2020-05-22T05:03:16Z

@gterzian are the changes you're referencing already covered in the above comments? I didn't touch any other files other than manifest (with ./mach update-manifest), extendablemessageevent.rs, and serviceworkerglobalscope.rs. I did pull upstream to keep my fork synced, maybe that caused the unexpected changes?

gterzian · 2020-05-22T05:08:57Z

I did pull upstream to keep my fork synced, maybe that caused the unexpected changes?

Yes maybe, how did you do it, with a git merge? What we usually do is a rebase, although it's unnecessary unless there are some actual merge conflicts(there is a guide here https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing).

So the files I am referring to are:

etc/taskcluster/macos/Brewfile-build.lock.json
etc/taskcluster/macos/Brewfile.lock.json
tests/wpt/webgl/tests/resources/npot-video.theora.ogv
tests/wpt/webgl/tests/resources/red-green.theora.ogv

and I think those changes should be removed from the PR(some might have been generated by ./mach update-manifest, which is also unnecessary here since your change did not change any tests).

nosark · 2020-05-22T05:16:19Z

etc/taskcluster/macos/Brewfile-build.lock.json
etc/taskcluster/macos/Brewfile.lock.json
tests/wpt/webgl/tests/resources/npot-video.theora.ogv
tests/wpt/webgl/tests/resources/red-green.theora.ogv

the test files were changed on the pull from upstream. The etc/... files changed only upon setting up the fork to build in the readme for my environment. Weird.... I run git pull upstream master and I accept any incoming merges if conflicts arise but this last pull I didn't have to resolve any conflicts. I'll read the reabase and squashing doc

gterzian · 2020-05-22T05:32:53Z

Ok, I think if you rebase those change might go away, if they are already on master? Otherwise It think you can also manually undo them. Let me know if you run into some issues.

I run git pull upstream master and I accept any incoming merges

What I usually do is git fetch upstream(not pull), and then git rebase upstream/master and I only do it if I really want a fix from master, or if there is a merge conflict(which will show up on the PR).

nosark · 2020-05-22T05:59:31Z

@gterzian I'm not sure I'm following you in regards to the spec links and docs. Could you please elaborate? Should I be adding the spec link above the struct and any other respectively?

gterzian · 2020-05-22T06:26:38Z

Yes I'm referring to the ExtendableMessageEvent struct, so you could add a doc link for that, in the format

/// <https://w3c.github.io/ServiceWorker/#extendablemessageevent-interface>
#[dom_struct]
#[allow(non_snake_case)]
pub struct ExtendableMessageEvent {}

and then if you look at the members of the struct, you will see some match the IDL definition and have their own link in the spec, and could be separately documented like for example:

/// <https://w3c.github.io/ServiceWorker/#extendablemessage-event-origin>
#[ignore_malloc_size_of = "mozjs"]
data: Heap<JSVal>,

nosark · 2020-05-26T18:23:16Z

@gterzian I'll add the docs and get my branch rebased. Thank you!

servo-wpt-sync · 2020-05-26T18:31:28Z

Error syncing changes upstream. Logs saved in error-snapshot-1590517887990.

servo-wpt-sync · 2020-05-26T18:42:01Z

Error syncing changes upstream. Logs saved in error-snapshot-1590518520183.

gterzian · 2020-05-27T04:12:14Z

Great, the actual changes look good, and it looks like some extra commits slipped in as well.

I can't tell exactly what happened, my guess is a git rebase -i HEAD~97, followed by commenting out with # all the commits that are not for this PR, could help.

And then also you can squash your commits into one.

nosark · 2020-05-27T17:03:44Z

Yeah, I'm not sure. I'll try the rebase and then squash everything into one commit. Thanks @gterzian!

nosark · 2020-06-01T21:23:37Z

I believe everything should be good to go. If any changes need to be made, don't hesitate to ask!
Thanks for all of the help @jdm @gterzian

gterzian

Ok, thanks for adding the docs and using the empty init. One comment left, and we also need to remove those unrelated changes, I think this can simply be done manually?

And then finally, it will also require a rebase, like:

git fetch upstream(If you haven't added an upstream origin pointing to the main repo, see https://help.github.com/en/github/using-git/adding-a-remote).
git rebase upstream/master.
Fix the conflicts and do git add .(or filename)(make sure there are no other files that will be added, best to first do git status to check for that).
git rebase --continue.

Then it should be good to do.

gterzian · 2020-06-02T04:12:03Z

components/script/dom/extendablemessageevent.rs

+            init.data.handle(),
+            init.origin.clone().unwrap(),
+            init.lastEventId.clone().unwrap(),
+            Vec::<DomRoot<MessagePort>>::new(),


init.ports will be available, if you uncomment

servo/components/script/dom/webidls/ExtendableMessageEvent.webidl

Line 23 in 2b24cfe

// sequence<MessagePort>? ports;

Okay, I'll uncomment this and replace the initialized MessagePort vec with the parents port property.

gterzian · 2020-06-02T04:12:03Z

etc/taskcluster/macos/Brewfile-build.lock.json

@@ -0,0 +1,202 @@
+{


Let's also find a way to remove this file addition from the changes.

gterzian · 2020-06-02T04:12:03Z

etc/taskcluster/macos/Brewfile.lock.json

@@ -0,0 +1,169 @@
+{


same for this file.

gterzian · 2020-06-02T04:12:03Z

tests/wpt/webgl/meta/MANIFEST.json

@@ -8719,7 +8719,7 @@
     []
    ],
    "npot-video.theora.ogv": [
-     "4458678fbfd81997ee5eff54f8b5e7fbd855264a",
+     "e2e7a25177072b19cb21be65c57d3b60c48cc9ce",


Also this should be undone, I would suggest just manually re-adding the old hashes. And then we need to re-add the binary files that have been removed below.

Okay, I'll diff the changes to the Manifest.json and copy the old hashes in to replace the changed hashes.

nosark · 2020-06-26T06:09:41Z

@gterzian I'm running into an interesting error. It's the side effect of uncommenting the message port related line in the ExtendableMessageEvent web IDL. Any ideas as to where I should be focusing my effort? Thanks in advance!

error[E0412]: cannot find type `MessagePort` in this scope
   --> /Users/kylenosar/rust-projects/servo/servo/target/debug/build/script-afd592f7b933938c/out/Bindings/ExtendableMessageEventBinding.rs:275:42
    |
275 |     pub ports: Option<Option<Vec<DomRoot<MessagePort>> >>,
    |                                          ^^^^^^^^^^^ not found in this scope
    |
help: possible candidate is found in another module, you can import it into scope
    |
4   | use crate::dom::messageport::MessagePort;
    |
help: you might be missing a type parameter
    |
270 | pub struct ExtendableMessageEventInit<MessagePort> {
    |                                      ^^^^^^^^^^^^^

error[E0283]: type annotations needed
   --> /Users/kylenosar/rust-projects/servo/servo/target/debug/build/script-afd592f7b933938c/out/Bindings/ExtendableMessageEventBinding.rs:275:5
    |
275 |     pub ports: Option<Option<Vec<DomRoot<MessagePort>> >>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type
    |
    = note: cannot satisfy `_: std::default::Default`
    = note: required by `std::default::Default::default`
    = note: this error originates in a derive macro (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 2 previous errors

gterzian · 2020-06-26T06:31:20Z

Ok, the solution is the replace the un-commented line with sequence<MessagePort> ports = [];, see the equivalent in MessageEvent.webidl.

jdm · 2020-06-26T15:00:22Z

That probably won't be enough - I think our codegen is missing logic for importing types that are only specific in dictionaries. You can work around for right now by adding 'crate::dom::messageport::MessagePort' to

servo/components/script/dom/bindings/codegen/CodegenRust.py

Line 6213 in 040891d

'crate::dom::windowproxy::WindowProxy',

.

gterzian · 2020-07-20T04:19:46Z

components/script/dom/webidls/ExtendableMessageEvent.webidl

@@ -20,5 +20,5 @@ dictionary ExtendableMessageEventInit : ExtendableEventInit {
  DOMString origin;
  DOMString lastEventId;
  // (Client or ServiceWorker /*or MessagePort*/)? source;
-  // sequence<MessagePort>? ports;
+  sequence<MessagePort> ports;


We can add a bunch of default values to the init, so that it matches https://w3c.github.io/ServiceWorker/#extendablemessageevent

For example:

sequence<MessagePort> ports = [];

DOMString origin = "";

You can look at MessageEvent.webidl which does the same thing.

That way you can remove the various unwrap following the call to init above.

gterzian

Thanks for sticking through with this, looks like we've almost reached the finish line!

nosark · 2020-07-21T00:20:52Z

@gterzian No problem! we'll get there, so close lol

nosark · 2020-07-29T22:26:35Z

@gterzian @jdm I've added all of the defaults from the ExtendableMessageEvent.webidl and everything is good until I come acrossed the source attribute. It looks like we need to implement GetSource for ExtendableMessageEventMethods and a similar SrcObject enum like

servo/components/script/dom/messageevent.rs

Line 32 in a9a406e

enum SrcObject {

but for a ClientOrServiceWorkerOrMessagePort type similar to

servo/components/script/dom/messageevent.rs

Line 38 in a9a406e

impl From<&WindowProxyOrMessagePortOrServiceWorker> for SrcObject {

to implement GetSource(). Should I implement these as well to finish off extendablemessageevent.rs for now, or should I leave the source commented out in the ExtendableMessageEvent.webidl and commit? Thoughts? Thanks in advance!

gterzian · 2020-07-30T01:48:15Z

Thanks for looking into it, we can leave source commented-out in this PR, that work is tracked over at #24659

nosark · 2020-07-30T04:29:01Z

Okay cool. Thanks @gterzian !

gterzian

I think it would be good to go following a squash of the two commits into one.


        Use ExtendableMessageEvent for messageerror in service workers

nosark · 2020-07-30T14:05:10Z

@gterzian Done! Thanks for all of your help!

gterzian · 2020-07-31T02:43:35Z

Great work!

@bors-servo r+

bors-servo · 2020-07-31T02:43:36Z

📌 Commit 95ddcf5 has been approved by gterzian

bors-servo · 2020-07-31T02:43:43Z

⌛ Testing commit 95ddcf5 with merge edc7ca7...


        Auto merge of #26607 - nosark:master, r=gterzian

Use ExtendableMessageEvent for messageerror in service workers #25241  added function dispatch_error to the ExtendableMessageEvent implmentation and replaced the MessageEvent dispatch error call with the ExtendableMessageEvent dispatch error call in serviceworkerglobalscope.rs ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [X] These changes fix #25241 (GitHub issue number if applicable)  - [x] There are tests for these changes OR - [x] These changes do not require tests because ___

nosark · 2020-07-31T03:18:50Z

@gterzian thanks!

bors-servo · 2020-07-31T03:23:08Z

💔 Test failed - status-taskcluster

gterzian · 2020-07-31T04:58:01Z

@bors-servo retry

0 matches
1 unexpected results that are NOT known-intermittents:
  ▶ FAIL [expected PASS] /css/css-text/text-align/text-align-end-010.html
  │   → /css/css-text/text-align/text-align-end-010.html ['f391d36b6f045c5cf9c439e0086f5fab5194f0c0']
  └   → /css/css-text/text-align/reference/text-align-end-ref-010.html ['b8cc0962b8e55a9684b327cddb76f385328b328d']

I think this is #23290

bors-servo · 2020-07-31T04:58:07Z

⌛ Testing commit 95ddcf5 with merge 9b6b793...

nosark · 2020-07-31T05:03:17Z

@gterzian Okay, I saw the failure and was about to ask. If it isn't related to #23290, let me know and I'll take a look in the morning!

bors-servo · 2020-07-31T05:08:15Z

☀️ Test successful - status-taskcluster
Approved by: gterzian
Pushing 9b6b793 to master...

highfive assigned nox May 21, 2020

highfive added the S-awaiting-review label May 21, 2020

highfive assigned gterzian and unassigned nox May 21, 2020

gterzian suggested changes May 22, 2020

View changes

nosark force-pushed the nosark:master branch from 0e39654 to ac20509 Jun 2, 2020

gterzian suggested changes Jun 2, 2020

View changes

nosark reopened this Jul 19, 2020

gterzian reviewed Jul 20, 2020

View changes

gterzian suggested changes Jul 20, 2020

View changes

gterzian suggested changes Jul 30, 2020

View changes

Use ExtendableMessageEvent for messageerror in service workers

Loading status checks…

95ddcf5

nosark force-pushed the nosark:master branch from 2f5b73a to 95ddcf5 Jul 30, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 31, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 31, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 31, 2020

bors-servo merged commit 9b6b793 into servo:master Jul 31, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 31, 2020

servo / servo

Join GitHub today

Use ExtendableMessageEvent for messageerror in service workers #25241 #26607

Use ExtendableMessageEvent for messageerror in service workers #25241 #26607

Conversation

nosark commented May 21, 2020 • edited

highfive commented May 21, 2020

highfive commented May 21, 2020

jdm commented May 21, 2020

nosark commented May 21, 2020 • edited

jdm commented May 21, 2020

nosark commented May 21, 2020

gterzian commented May 22, 2020

gterzian left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian commented May 22, 2020

nosark commented May 22, 2020 • edited

gterzian commented May 22, 2020 • edited

nosark commented May 22, 2020

gterzian commented May 22, 2020 • edited

nosark commented May 22, 2020 • edited

gterzian commented May 22, 2020 • edited

nosark commented May 26, 2020 • edited

servo-wpt-sync commented May 26, 2020

servo-wpt-sync commented May 26, 2020

gterzian commented May 27, 2020 • edited

nosark commented May 27, 2020

nosark commented Jun 1, 2020

gterzian left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nosark commented Jun 26, 2020 • edited by jdm

gterzian commented Jun 26, 2020

jdm commented Jun 26, 2020

This comment has been minimized.

gterzian left a comment

nosark commented Jul 21, 2020

nosark commented Jul 29, 2020 • edited

gterzian commented Jul 30, 2020

nosark commented Jul 30, 2020

gterzian left a comment • edited

nosark commented Jul 30, 2020 • edited

gterzian commented Jul 31, 2020

bors-servo commented Jul 31, 2020

bors-servo commented Jul 31, 2020

nosark commented Jul 31, 2020

bors-servo commented Jul 31, 2020

gterzian commented Jul 31, 2020

bors-servo commented Jul 31, 2020

nosark commented Jul 31, 2020

bors-servo commented Jul 31, 2020

nosark commented May 21, 2020 •

edited

nosark commented May 21, 2020 •

edited

gterzian left a comment •

edited

nosark commented May 22, 2020 •

edited

gterzian commented May 22, 2020 •

edited

gterzian commented May 22, 2020 •

edited

nosark commented May 22, 2020 •

edited

gterzian commented May 22, 2020 •

edited

nosark commented May 26, 2020 •

edited

gterzian commented May 27, 2020 •

edited

gterzian left a comment •

edited

nosark commented Jun 26, 2020 •

edited by jdm

nosark commented Jul 29, 2020 •

edited

gterzian left a comment •

edited

nosark commented Jul 30, 2020 •

edited