ServiceWorkerContainer::Promise #13419

Coder206 · 2016-09-26T01:42:41Z

The purpose of the code changes is to enable the use of promises in the Service Worker container. I also modified the Service Worker test in order to support the promises.

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

There are tests for these changes

This change is

highfive · 2016-09-26T01:42:46Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/package_commands.py
@KiChjang: components/script/dom/serviceworkercontainer.rs, components/script/dom/webidls/ServiceWorkerContainer.webidl

highfive · 2016-09-26T01:42:47Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

Coder206 · 2016-09-26T01:55:12Z

components/script/dom/serviceworkercontainer.rs

+
+        // New Step 1
+
+        let p = dom::promise::Promise


Is this right?

I recommend looking for uses of Promise::new in the codebase to base your changes off of.

Coder206 · 2016-09-26T01:55:39Z

components/script/dom/webidls/ServiceWorkerContainer.webidl

@@ -6,21 +6,21 @@
 [Pref="dom.serviceworker.enabled", Exposed=(Window,Worker)]
 interface ServiceWorkerContainer : EventTarget {
  [Unforgeable] readonly attribute ServiceWorker? controller;
-  //[SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;
+  [SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;


Just preparing for eventual functionality

This should be replacing the register underneath it.

Coder206 · 2016-09-26T01:57:38Z

It's the first time I do one of these, I am new to the process and I am having trouble understanding how to translate the pseudo code from Service Worker Nightly Container:Register into Rust for Servo

Coder206 · 2016-09-26T01:58:49Z

I think this PR is for @jdm

jdm

You will definitely need to be able to build Servo locally to be able to make these changes effectively.

jdm · 2016-09-26T02:17:44Z

components/script/dom/webidls/ServiceWorkerContainer.webidl

@@ -6,21 +6,21 @@
 [Pref="dom.serviceworker.enabled", Exposed=(Window,Worker)]
 interface ServiceWorkerContainer : EventTarget {
  [Unforgeable] readonly attribute ServiceWorker? controller;
-  //[SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;
+  [SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;


This should be replacing the register underneath it.

jdm · 2016-09-26T02:17:44Z

components/script/dom/serviceworkercontainer.rs

+        Let scopeURL be null.
+        If options.scope is present, set scopeURL to the result of parsing options.scope with the context object’s relevant settings object’s API base URL.
+        Invoke Start Register with scopeURL, scriptURL, p, client, client’s creation URL and options.type.
+        Return p.    */ 


As mentioned in #13409, I don't think there's any need to focus on the specification. It's enough to know that it says to create a promise and return it, and either resolve it with a successful service worker or else reject it with an error (instead of returning error values directly).

jdm · 2016-09-26T02:17:44Z

components/script/dom/webidls/ServiceWorkerContainer.webidl

-  //[NewObject] /*Promise<any>*/ any getRegistration(optional USVString clientURL = "");
-  //[NewObject] /* Promise */<sequence<ServiceWorkerRegistration>> getRegistrations();
+  [NewObject] Promise<any> any getRegistration(optional USVString clientURL = "");
+  [NewObject] Promise <sequence<ServiceWorkerRegistration>> getRegistrations();


We shouldn't be modifying any other methods or properties in this file for the purposes of these changes.

Coder206 · 2016-09-26T02:36:11Z

components/script/dom/webidls/ServiceWorkerContainer.webidl

-  //[SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;
-
-  [NewObject, Throws] ServiceWorkerRegistration register(USVString scriptURL, optional RegistrationOptions options);
+  [SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;


@jdm When running ./mach build -d, I am seeing this error:

WebIDL.WebIDLError: error: Unresolved type '<unresolved scope>::Promise'., /home/user/servo/components/script/dom/webidls/ServiceWorkerContainer.webidl line 9:34 [SameObject] readonly attribute Promise<ServiceWorkerRegistration> ready;

I am not sure how to solve it. Do you have any ideas?

Your branch is based off of a master commit from Sept 6. You need to be using 2b1a39c or more recent instead.

@Coder206 As an added note, We shouldn't be needing to make changes to the ready method for the relevant issue. You just have to make changes to the register api declaration from:

[NewObject, Throws] ServiceWorkerRegistration register(USVString scriptURL, optional RegistrationOptions options);

to

[NewObject] Promise<ServiceWorkerRegistration> register(USVString scriptURL, optional RegistrationOptions options);

This will result in a compilation error notifying you that, register method should return a Rc<Promise> type. So after that you need to use a combination of resolve_native and reject_error method to make the register method return a resolved/reject promise. Correct me if am wrong @jdm

Rebase your current branch to the master branch before making these changes as mentioned by jdm

@creativcoder I am not sure how to get started with resolve_native and reject_error. How do I implement that in my code? (does it go in the return)

We don't need to implement those methods, they are already implemented. In the Register method we just need to call reject_error on places where we return an Err value and resolve_native at places where we return an Ok value on the promise object.

You can have a look at my branch here, for how that is being done.

Coder206 · 2016-09-26T12:17:16Z

@jdm So I need to rebase my code to the newest commit?

jdm · 2016-09-26T14:48:43Z

@Coder206 That would be easiest, yes.

Coder206 · 2016-09-27T11:13:22Z

Looks like an extra commit was added here. I am going to remove it as soon as possible.

jdm · 2016-09-27T12:47:03Z

The test file mentioned in the original issue still needs modifying, too.

Coder206 · 2016-09-27T13:10:48Z

@jdm Thanks for the reminder

KiChjang · 2016-09-27T21:57:19Z

components/script/dom/serviceworkercontainer.rs

        }
        // Step 12
        if scope.path().to_ascii_lowercase().contains("%2f") ||
        scope.path().to_ascii_lowercase().contains("%5c") {
-            return Err(Error::Type("Scope URL contains forbidden characters".to_owned()));
+            return {


What's the purpose of this return keyword here and the block?

This should still be addressed.

bors-servo · 2016-09-28T19:52:16Z

☔ The latest upstream changes (presumably #13474) made this pull request unmergeable. Please resolve the merge conflicts.

jdm

This looks good with a few point addressed.

jdm · 2016-10-04T18:36:14Z

tests/html/service-worker/index.html

-    console.log("Registered script source: "+ navigator.serviceWorker.controller.scriptURL);
-    document.getElementById('iframe_sw').src = 'demo_iframe.html';
-    post_message('/demo_iframe');
+        /* global navigator */


I'm not sure what this comment means.

jdm · 2016-10-04T18:36:14Z

tests/html/service-worker/index.html

+            console.log("From client worker registered: ", navigator.serviceWorker.controller);
+            console.log("Registered script source: "+ navigator.serviceWorker.controller.scriptURL);
+        }).catch(function(error) {
+            console.log("Error in loading: " + navigator.serviceWorker.controller.scriptURL);


Same as previous.

jdm · 2016-10-04T18:36:14Z

tests/html/service-worker/index.html

+            console.log("Registered script source: "+ navigator.serviceWorker.controller.scriptURL);
+            post_message('/dashboard');
+        }).catch(function(error) {
+            console.log("Error in loading: " + navigator.serviceWorker.controller.scriptURL);


jdm · 2016-10-04T18:36:14Z

tests/html/service-worker/index.html

+            console.log("Registered script source: "+ navigator.serviceWorker.controller.scriptURL);
+            post_message('/profile');
+        }).catch(function(error) {
+            console.log("Error in loading: " + navigator.serviceWorker.controller.scriptURL);


jdm · 2016-10-27T15:19:45Z

@Coder206 Did you run the test after making the changes? My assumption when people make changes to test code is that the tests pass before they push their changes.

Coder206 · 2016-10-27T15:24:22Z

@jdm Yes, that's when I received the error from above. I haven't rebased though with the new changes. The next push will have the tests verified with the new master.

Coder206 · 2016-10-27T15:27:34Z

Sorry for the inconvenience.

Coder206 · 2016-10-30T14:35:36Z

I am unable to get the error messages that @KiChjang has shared with me. Here is the output of my attempt to test my changes. Please note, this is the result despite I only corrected the first error mentioned above:

Errors I am unsure what they mean:

▶ Unexpected subtest result in /_mozilla/mozilla/service-workers/service-worker-registration.html:
  │ FAIL [expected PASS] Test: Throws Error when Invalid Url Path
  │   → promise.then is not a function
  │ 
  │ promise_rejects@http://web-platform.test:8000/resources/testharness.js:547:16
  │ @http://web-platform.test:8000/_mozilla/mozilla/service-workers/service-worker-registration.html:34:10
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  └ promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:530:36

  ▶ Unexpected subtest result in /_mozilla/mozilla/service-workers/service-worker-registration.html:
  │ FAIL [expected PASS] Test: Throws Error when Invalid Scope
  │   → promise.then is not a function
  │ 
  │ promise_rejects@http://web-platform.test:8000/resources/testharness.js:547:16
  │ @http://web-platform.test:8000/_mozilla/mozilla/service-workers/service-worker-registration.html:40:10
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1401:20
  └ promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:530:36

Any ideas or suggestions?

jdm · 2016-10-30T14:45:34Z

@Coder206 The main problem you're seeing is due to using the test directory (which doesn't exist) instead of tests (which does exist).

Coder206 · 2016-10-30T17:41:33Z

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html

-              function() { var sw_reg = register_sw('./in%5Csome%5fdir/sw.js'); },
-              "Invalid URL Path");
+promise_test(function (p) {
+  promise_rejects(p, new TypeError(), register_sw('./in%5Csome%5fdir/sw.js'));


@jdm I don't fancy an inline code like this but the code below would fail:

promise_test(function (p) { promise_rejects(p, new TypeError(), function () { register_sw('./in%5Csome%5fdir/sw.js'); }); }, "Test: Throws Error when Invalid Url Path");

That's because it needs to return the result of register_sw.

@jdm I am still getting promise.then() is not a function when I return, I am trying to figure out why and how to correct it from the testharness.js from the web platfor test folder

Coder206 · 2016-10-30T17:41:34Z

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html

-              function() { var sw_reg = register_sw('sw.js', './mal%5fformed/sco%5Cpe/'); },
-              "Invalid URL Path");
+promise_test(function (p) {
+  return promise_rejects(p, new TypeError(), register_sw('sw.js', './mal%5fformed/sco%5Cpe/'));


@jdm Same for this one.

jdm

One teeny tiny change and then we can merge this!

jdm · 2016-10-31T13:26:33Z

tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html

 }, "Test: Active Service Worker ScriptURL property");

+function bar() {


This function isn't used anywhere.

@jdm Yes, sorry it was test code that escaped... Will remove it asap.


        Add Promises to Service Worker Container

The purpose of the code changes is to enable the use of promises in the Service Worker container. I also modified the Service Worker test in order to support the promises.

Coder206 · 2016-10-31T15:25:56Z

@jdm So I made the last changes requested and tested mach build -d and ./mach test-wpt tests/wpt/mozilla/tests/mozilla/service-workers/service-worker-registration.html both yielded success. Anything else to test or fix?

jdm · 2016-10-31T15:37:27Z

@bors-servo: r+

bors-servo · 2016-10-31T15:37:28Z

📌 Commit e6b8790 has been approved by jdm

bors-servo · 2016-10-31T16:19:02Z

⌛ Testing commit e6b8790 with merge 0d46c7c...


        Auto merge of #13419 - Coder206:swPromise, r=jdm

ServiceWorkerContainer::Promise   - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13409 (github issue number if applicable).  - [ ] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13419)

bors-servo · 2016-10-31T17:01:45Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-31T17:01:47Z

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

jdm · 2016-10-31T17:30:07Z

@bors-servo: retry

#10473

bors-servo · 2016-10-31T17:30:09Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt...

bors-servo · 2016-10-31T18:03:30Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned metajack Sep 26, 2016

highfive added the S-awaiting-review label Sep 26, 2016

Coder206 reviewed Sep 26, 2016

View changes

jdm requested changes Sep 26, 2016

View changes

jdm assigned jdm and unassigned metajack Sep 26, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 26, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 26, 2016

Coder206 reviewed Sep 26, 2016

View changes

jdm added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Sep 27, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 27, 2016

KiChjang reviewed Sep 27, 2016

View changes

highfive added the S-needs-rebase label Sep 28, 2016

jdm requested changes Oct 4, 2016

View changes

highfive added S-awaiting-review and removed S-tests-failed labels Oct 30, 2016

Coder206 reviewed Oct 30, 2016

View changes

jdm requested changes Oct 31, 2016

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 31, 2016

Add Promises to Service Worker Container

Loading status checks…

e6b8790

The purpose of the code changes is to enable the use of promises in the Service Worker container. I also modified the Service Worker test in order to support the promises.

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 31, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 31, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 31, 2016

bors-servo merged commit e6b8790 into servo:master Oct 31, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

		}, "Test: Active Service Worker ScriptURL property");

		function bar() {

servo / servo

Join GitHub today

ServiceWorkerContainer::Promise #13419

ServiceWorkerContainer::Promise #13419

Conversation

Coder206 commented Sep 26, 2016 • edited

The purpose of the code changes is to enable the use of promises in the Service Worker container. I also modified the Service Worker test in order to support the promises.

highfive commented Sep 26, 2016

highfive commented Sep 26, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coder206 Sep 26, 2016 • edited

This comment has been minimized.

Coder206 commented Sep 26, 2016 • edited

Coder206 commented Sep 26, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coder206 Sep 26, 2016 • edited

This comment has been minimized.

This comment has been minimized.

creativcoder Sep 26, 2016 • edited

This comment has been minimized.

This comment has been minimized.

Coder206 commented Sep 26, 2016

jdm commented Sep 26, 2016

Coder206 commented Sep 27, 2016 • edited

jdm commented Sep 27, 2016

Coder206 commented Sep 27, 2016

This comment has been minimized.

KiChjang Sep 27, 2016 • edited

This comment has been minimized.

bors-servo commented Sep 28, 2016

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Oct 27, 2016

Coder206 commented Oct 27, 2016

Coder206 commented Oct 27, 2016

Coder206 commented Oct 30, 2016

jdm commented Oct 30, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

This comment has been minimized.

This comment has been minimized.

Coder206 commented Oct 31, 2016

jdm commented Oct 31, 2016

bors-servo commented Oct 31, 2016

bors-servo commented Oct 31, 2016

bors-servo commented Oct 31, 2016

highfive commented Oct 31, 2016

jdm commented Oct 31, 2016

bors-servo commented Oct 31, 2016

bors-servo commented Oct 31, 2016

Coder206 commented Sep 26, 2016 •

edited

Coder206 Sep 26, 2016 •

edited

Coder206 commented Sep 26, 2016 •

edited

Coder206 Sep 26, 2016 •

edited

creativcoder Sep 26, 2016 •

edited

Coder206 commented Sep 27, 2016 •

edited

KiChjang Sep 27, 2016 •

edited