do not ship boolean service worker useCache option

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: bkelly, Assigned: HoPang)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 unaffected, firefox53+ fixed, firefox54+ fixed, firefox55 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

11 months ago
As described in bug 1353636, the service worker useCache option has been changed from a boolean to an enum.  We should not ship our boolean implementation in FF53.  Instead, lets simply revert back to defaulting to using the cache and hide the option from the webidl.  We can then implement the new solution in trunk in bug 1353636.

Ho-Pang, can you do this ASAP?  We need to land and uplift to FF53 this week before the release candidate is made.
(Reporter)

Comment 1

11 months ago
Ho-Pang, can you do this ASAP?  We need to land and uplift to FF53 this week before the release candidate is made.
Flags: needinfo?(bhsu)
(Reporter)

Comment 2

11 months ago
We need to get this web compat issue fixed before FF53 is released.  This bug should uplift the minimal amount of code to disable the boolean useCache option and just default to using the cache instead.
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
tracking-firefox53: --- → ?
(Assignee)

Comment 3

11 months ago
Sure, let me handle this.
Assignee: nobody → bhsu
Flags: needinfo?(bhsu)
(Assignee)

Comment 4

11 months ago
Created attachment 8854786 [details] [diff] [review]
P1: Hide useCache registrationOptions
(Assignee)

Comment 5

11 months ago
Created attachment 8854787 [details] [diff] [review]
P2: Disable a testcase
(Assignee)

Comment 6

11 months ago
Hi Ben,

Do you mind reviewing those patches? I think they are ready.
Flags: needinfo?(bkelly)
(Assignee)

Comment 7

11 months ago
Created attachment 8854922 [details] [diff] [review]
P1: Hide useCache registrationOptions
(Assignee)

Comment 8

11 months ago
Created attachment 8854923 [details] [diff] [review]
P2: Disable a testcase
(Assignee)

Comment 9

11 months ago
Comment on attachment 8854786 [details] [diff] [review]
P1: Hide useCache registrationOptions

wrong bug id...
Attachment #8854786 - Attachment is obsolete: true
(Assignee)

Comment 10

11 months ago
Comment on attachment 8854787 [details] [diff] [review]
P2: Disable a testcase

wrong bug id...
Attachment #8854787 - Attachment is obsolete: true
(Reporter)

Comment 11

11 months ago
Comment on attachment 8854922 [details] [diff] [review]
P1: Hide useCache registrationOptions

Review of attachment 8854922 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the quick turn around!
Attachment #8854922 - Flags: review+
(Reporter)

Comment 12

11 months ago
Comment on attachment 8854923 [details] [diff] [review]
P2: Disable a testcase

Review of attachment 8854923 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove the test files as well.  Just updating the manifest makes it too easy to add the test back with --manifest-update.  Also, removing the test will end up removing it from upstream so other browsers don't test against the obsolete boolean definition here.

We can then re-land the test in bug 1353636 using the new enumeration definition.

r=me with that change.
Attachment #8854923 - Flags: review+
(Assignee)

Comment 13

11 months ago
Created attachment 8855593 [details] [diff] [review]
(V2) P2: Remove a testcase
(Assignee)

Updated

11 months ago
Attachment #8855593 - Attachment description: P2: Remove a testcase → (V2) P2: Remove a testcase
Attachment #8855593 - Flags: review+
(Assignee)

Updated

11 months ago
Attachment #8854923 - Attachment is obsolete: true
(Assignee)

Comment 14

11 months ago
Since review is completed, I think we can remove the NI now :)
Flags: needinfo?(bkelly)
(Reporter)

Comment 15

11 months ago
Ho-Pang pushed to try, but its the end of the day in Taiwan.  I'll finish getting this landed.

Comment 16

11 months ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/321a37f440e8
P1: Hide useCache registrationOptions. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc210ac61f96
P2: Remove a testcase. r=bkelly
(Reporter)

Comment 17

11 months ago
Comment on attachment 8854922 [details] [diff] [review]
P1: Hide useCache registrationOptions

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1290944.
[User impact if declined]: Web compat.  Bug 1290944 added a new webidl feature to service workers that had spec agreement.  At the spec meeting this week, however, we decided to change that part of the API.  To avoid web compat issues we want to hide our implementation from bug 1290944 until we can switch to the new spec'd API.  We understand this is very late in the release cycle, but we think uplifting this small change is better than having to fix a released web compat issue.
[Is this code covered by automated tests?]: There are tests for this part of the code.  The new test added in bug 1290944 is removed in P2 of this bug.
[Has the fix been verified in Nightly?]: It has not merged to nightly yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: We are simply removing an option from webidl and switching the default value back to the way it was before bug 1290944.  There should be very minimal risk.
[String changes made/needed]: None.
Attachment #8854922 - Flags: approval-mozilla-beta?
Attachment #8854922 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 18

11 months ago
Comment on attachment 8855593 [details] [diff] [review]
(V2) P2: Remove a testcase

See comment 17.
Attachment #8855593 - Flags: approval-mozilla-beta?
Attachment #8855593 - Flags: approval-mozilla-aurora?

Comment 19

11 months ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5649ff4a2b48
Backed out changeset dc210ac61f96 
https://hg.mozilla.org/integration/mozilla-inbound/rev/801bf613b8de
Backed out changeset 321a37f440e8 for linting failure in testing/web-platform/meta/MANIFEST.json. r=backout
Backed out for linting failure in testing/web-platform/meta/MANIFEST.json:

https://hg.mozilla.org/integration/mozilla-inbound/rev/801bf613b8de5c1393543509364e8e5eb2376edf
https://hg.mozilla.org/integration/mozilla-inbound/rev/5649ff4a2b48b8d745a81b5143c2b9a964ba2d18

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dc210ac61f96cb1c61a25994fae1922ed43d6321&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=89587054&repo=mozilla-inbound

[task 2017-04-07T16:11:32.087634Z]  0:06.37 LOG: MainThread ERROR Manifest /home/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json is outdated, use |mach wpt-manifest-update| to fix.
[task 2017-04-07T16:12:56.524846Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | service-workers/service-worker/resources/update-max-aged-worker-imported-script.py in manifest but removed from source. (wpt-manifest)
[task 2017-04-07T16:12:56.524978Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | service-workers/service-worker/resources/update-max-aged-worker.py in manifest but removed from source. (wpt-manifest)

This will likely get fixed if the manifest gets updated like described in https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/README.md
Flags: needinfo?(bhsu)
(Assignee)

Comment 21

11 months ago
Created attachment 8855886 [details] [diff] [review]
(V3) P2: Remove a testcase
(Assignee)

Comment 22

11 months ago
Comment on attachment 8855886 [details] [diff] [review]
(V3) P2: Remove a testcase

Please refer to comment 17.
Flags: needinfo?(bhsu)
Attachment #8855886 - Flags: review+
Attachment #8855886 - Flags: approval-mozilla-beta?
Attachment #8855886 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

11 months ago
Attachment #8855593 - Attachment is obsolete: true
Attachment #8855593 - Flags: approval-mozilla-beta?
Attachment #8855593 - Flags: approval-mozilla-aurora?

Comment 23

11 months ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a0b481d8f3
P1: Hide useCache registrationOptions. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/67b87beb9234
P2: Remove a testcase. r=bkelly
I'll check back later today or tomorrow to uplift once this lands and sticks on m-c.
https://hg.mozilla.org/mozilla-central/rev/29a0b481d8f3
https://hg.mozilla.org/mozilla-central/rev/67b87beb9234
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8854922 [details] [diff] [review]
P1: Hide useCache registrationOptions

Important web compat issue, we need to land this for 53. This should revert us to the same behavior as in 52.
Attachment #8854922 - Flags: approval-mozilla-beta?
Attachment #8854922 - Flags: approval-mozilla-beta+
Attachment #8854922 - Flags: approval-mozilla-aurora?
Attachment #8854922 - Flags: approval-mozilla-aurora+
Comment on attachment 8855886 [details] [diff] [review]
(V3) P2: Remove a testcase

Removes a new testcase from bug 1290944
Attachment #8855886 - Flags: approval-mozilla-beta?
Attachment #8855886 - Flags: approval-mozilla-beta+
Attachment #8855886 - Flags: approval-mozilla-aurora?
Attachment #8855886 - Flags: approval-mozilla-aurora+
tracking-firefox53: ? → +
tracking-firefox54: --- → +

Comment 30

11 months ago
sorry had to back this out from beta for wptlint error like https://treeherder.mozilla.org/logviewer.html#?job_id=90107221&repo=mozilla-beta
status-firefox53: fixed → affected
Flags: needinfo?(bhsu)
(Reporter)

Comment 31

11 months ago
(In reply to Carsten Book [:Tomcat] from comment #30)
> sorry had to back this out from beta for wptlint error like
> https://treeherder.mozilla.org/logviewer.html#?job_id=90107221&repo=mozilla-
> beta

I'll fix this and re-land.  We probably just need to update the manifest again.
(Assignee)

Comment 32

11 months ago
Thanks and sorry
Flags: needinfo?(bhsu)
(Reporter)

Comment 33

11 months ago
Seems registration-useCache.https.html was still in the manifest.  I wonder why the m-c and m-a wpt lint didn't catch that...
(Reporter)

Comment 35

11 months ago
Filed bug 1355087 about the WPT linter missing this problem on m-c.
See Also: → bug 1355087
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #17)
> [Is this code covered by automated tests?]: There are tests for this part of
> the code.  The new test added in bug 1290944 is removed in P2 of this bug.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Ben's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.