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

Plumb outside settings object for ServiceWorkerRegistration#update() #20170

Merged
merged 2 commits into from Nov 19, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Nov 8, 2019

This is similar to crrev.com/c/1861294 but for update(). Script
fetches for update check happen on the browser and are managed
by ServiceWorkerUpdateChecker so pass some information to
ServiceWorkerUpdateChecker too.

This CL has a behavioral change. The 'Referer' header of the request
triggered by update() in a document/service worker will be changed.
Before this CL it was the url of the top-level script but after this
CL it is document/service worker's url. This behavior is consistent
with the spec.

Bug: 937177
Change-Id: I57e9ce7c91a748ad549a49e4e46c7890dcca2903
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1905028
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716516}

Copy link
Collaborator

wpt-pr-bot left a comment

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1905028 branch 2 times, most recently from 18de849 to b355120 Nov 8, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1905028 branch from b355120 to 5f83297 Nov 19, 2019
This is similar to crrev.com/c/1861294 but for update(). Script
fetches for update check happen on the browser and are managed
by ServiceWorkerUpdateChecker so pass some information to
ServiceWorkerUpdateChecker too.

This CL has a behavioral change. The 'Referer' header of the request
triggered by update() in a document/service worker will be changed.
Before this CL it was the url of the top-level script but after this
CL it is document/service worker's url. This behavior is consistent
with the spec.

Bug: 937177
Change-Id: I57e9ce7c91a748ad549a49e4e46c7890dcca2903
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1905028
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716516}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1905028 branch from 5f83297 to 8e5c6ee Nov 19, 2019
@KyleJu KyleJu closed this Nov 19, 2019
@KyleJu KyleJu reopened this Nov 19, 2019
@KyleJu
Copy link

KyleJu commented Nov 19, 2019

The PR is causing the following test to timeout on chrome dev

Slow tests

Test Result Longest duration (ms) Timeout (ms)
/service-workers/service-worker/referer-toplevel-script-fetch.https.html OK 10088 10000
@KyleJu
Copy link

KyleJu commented Nov 19, 2019

@Hexcles for this failure in Taskcluster, I see messages like "Running tests in a loop 10 times : PASS Running tests in a loop with restarts 5 times : FAIL". IIUC, only flaky tests block Taskcluster but this one looks like test timeout/failures?

@KyleJu
Copy link

KyleJu commented Nov 19, 2019

The following test is flaky on Firefox

Unstable results

Test Subtest Results Messages
/service-workers/service-worker/update-no-cache-request-headers.https.html headers in no-cache mode FAIL: 1/10, PASS: 9/10 assert_equals: expected (string) "etag" but got (undefined) undefined
@KyleJu
Copy link

KyleJu commented Nov 19, 2019

The unstable result seems unrelated but I don't see the any existing flakiness on the same result. @stephenmcgruer do you think we could force merge it?

@stephenmcgruer
Copy link
Contributor

stephenmcgruer commented Nov 19, 2019

It could have affected it by the changing timestamps, but I do lean towards it being existing flake (maybe rare?). Can we reproduce locally?

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 295f22b into master Nov 19, 2019
7 of 10 checks passed
7 of 10 checks passed
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview failed
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview failed
Details
Azure Pipelines in progress
Details
wpt.fyi - chrome[experimental] Chrome results
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1905028 branch Nov 19, 2019
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

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