Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement NavigationPreloadManager for ServiceWorker #22218

Merged
merged 1 commit into from Dec 10, 2018

Conversation

@CYBAI
Copy link
Collaborator

CYBAI commented Nov 18, 2018


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix part of #19302 .
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Nov 18, 2018

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/mod.rs, components/script/dom/webidls/NavigationPreloadManager.webidl, components/script/dom/webidls/ServiceWorkerRegistration.webidl, components/script/dom/navigationpreloadmanager.rs, components/script/dom/serviceworkerregistration.rs
  • @KiChjang: components/script/dom/mod.rs, components/script/dom/webidls/NavigationPreloadManager.webidl, components/script/dom/webidls/ServiceWorkerRegistration.webidl, components/script/dom/navigationpreloadmanager.rs, components/script/dom/serviceworkerregistration.rs
@highfive
Copy link

highfive commented Nov 18, 2018

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@CYBAI CYBAI mentioned this pull request Nov 18, 2018
5 of 6 tasks complete
@CYBAI
Copy link
Collaborator Author

CYBAI commented Nov 18, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2018

Trying commit aaf1034 with merge 16c4c6c...

bors-servo added a commit that referenced this pull request Nov 18, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@CYBAI CYBAI force-pushed the CYBAI:sw-up-to-date branch from aaf1034 to 159bb38 Nov 19, 2018
@CYBAI
Copy link
Collaborator Author

CYBAI commented Nov 19, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Nov 19, 2018

Trying commit 159bb38 with merge 1214fef...

bors-servo added a commit that referenced this pull request Nov 19, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 19, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@paulrouget paulrouget assigned asajeffrey and unassigned paulrouget Nov 21, 2018
@@ -8,7 +8,7 @@ interface ServiceWorkerRegistration : EventTarget {
readonly attribute ServiceWorker? installing;
readonly attribute ServiceWorker? waiting;
readonly attribute ServiceWorker? active;
// [SameObject] readonly attribute NavigationPreloadManager navigationPreload;
[SameObject] readonly attribute NavigationPreloadManager navigationPreload;

This comment has been minimized.

Copy link
@nox

nox Nov 27, 2018

Member

What happens if the pref from Pref="dom.serviceworker.enabled" on the interface isn't enabled? And shouldn't that preference be set when running tests related to that feature?

This comment has been minimized.

Copy link
@CYBAI

CYBAI Nov 27, 2018

Author Collaborator

Ah, I misunderstood the config for mozilla wpt was for web-platform-tests which is located at

prefs: ["dom.serviceworker.enabled:true"]

Let me update the PR to turn on the config for web-platform-tests for SW! Thanks for the good catch!

This comment has been minimized.

Copy link
@jdm

jdm Nov 27, 2018

Member

There's probably no real point; we don't implement enough of the SW API to make tests run to completion.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Nov 27, 2018

Author Collaborator

@jdm Yes, if we turned on SW tests, we'll have other FAIL cases. Or maybe let me keep working on it and turn it on when we have basic implementation for SW? cc @nox

This comment has been minimized.

Copy link
@jdm

jdm Nov 27, 2018

Member

Actually, it's possible that we might be able to get meaningful results out of the upstream SW tests now. One reason we created our own tests was that:

  • we didn't support promises yet
  • we didn't support HTTPS tests

Both of those have been fixed, so there's no good reason not to start relying on the upstream tests that require both of those features.

This comment has been minimized.

Copy link
@CYBAI

CYBAI Nov 27, 2018

Author Collaborator

It sounds reasonable for me to start relying on upstream tests if we've already supported these 2 points. How about letting me turn SW upstream tests on in another PR?

This comment has been minimized.

Copy link
@jdm

jdm Nov 27, 2018

Member

Sure!

This comment has been minimized.

Copy link
@CYBAI

CYBAI Nov 27, 2018

Author Collaborator

Opened at #22278 !

@CYBAI CYBAI mentioned this pull request Nov 27, 2018
3 of 3 tasks complete
bors-servo added a commit that referenced this pull request Nov 27, 2018
Turn on upstream SW wpt tests

Generated these metadata with following command

```sh
$ ./mach test-wpt --log-raw=update.log tests/wpt/web-platform-tests/service-workers
$ ./mach update-wpt update.log
```

Ref: #22218 (comment)

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] This PR will turn on `ServiceWorker` for upstream WPT tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22278)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Nov 28, 2018
Turn on upstream SW wpt tests

Generated these metadata with following command

```sh
$ ./mach test-wpt --log-raw=update.log tests/wpt/web-platform-tests/service-workers
$ ./mach update-wpt update.log
```

Ref: #22218 (comment)

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] This PR will turn on `ServiceWorker` for upstream WPT tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22278)
<!-- Reviewable:end -->
@CYBAI CYBAI force-pushed the CYBAI:sw-up-to-date branch from 159bb38 to e2cb292 Nov 28, 2018
@CYBAI
Copy link
Collaborator Author

CYBAI commented Nov 28, 2018

@bors-servo try=wpt

  • Just rebased to latest master but I think we need more implementation for SW so that we can have more PASS tests
@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2018

Trying commit e2cb292 with merge c97f093...

bors-servo added a commit that referenced this pull request Nov 28, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 28, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

Copy link
Member

jdm left a comment

Looks good overall!

components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
components/script/dom/serviceworkerregistration.rs Outdated Show resolved Hide resolved
components/script/dom/serviceworkerregistration.rs Outdated Show resolved Hide resolved
@jdm jdm assigned jdm and unassigned asajeffrey Nov 28, 2018
components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
components/script/dom/navigationpreloadmanager.rs Outdated Show resolved Hide resolved
@CYBAI CYBAI force-pushed the CYBAI:sw-up-to-date branch from b8b1100 to 4073495 Dec 8, 2018
@CYBAI CYBAI force-pushed the CYBAI:sw-up-to-date branch from 4073495 to 235fb59 Dec 8, 2018
@CYBAI
Copy link
Collaborator Author

CYBAI commented Dec 9, 2018

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2018

Trying commit 235fb59 with merge 3190167...

bors-servo added a commit that referenced this pull request Dec 9, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2018

☀️ Test successful - linux-rel-css, linux-rel-wpt
State: approved= try=True

@jdm
Copy link
Member

jdm commented Dec 9, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2018

📌 Commit 235fb59 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2018

Testing commit 235fb59 with merge 0a33f86...

bors-servo added a commit that referenced this pull request Dec 9, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 9, 2018

💔 Test failed - mac-rel-css1

@jdm
Copy link
Member

jdm commented Dec 10, 2018

@bors-servo retry

  • restarted servo-mac8
@bors-servo
Copy link
Contributor

bors-servo commented Dec 10, 2018

Testing commit 235fb59 with merge ddfc799...

bors-servo added a commit that referenced this pull request Dec 10, 2018
Implement NavigationPreloadManager for ServiceWorker

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #19302 .
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22218)
<!-- Reviewable:end -->
@bors-servo bors-servo merged commit 235fb59 into servo:master Dec 10, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@CYBAI CYBAI deleted the CYBAI:sw-up-to-date branch Dec 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.