1353638 - do not ship boolean service worker useCache option

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[Ben Hsu [:HoPang]]

Sure, let me handle this.

Comment 4

[Ben Hsu [:HoPang]]

Attachment: P1: Hide useCache registrationOptions

Comment 5

[Ben Hsu [:HoPang]]

Attachment: P2: Disable a testcase

Comment 6

[Ben Hsu [:HoPang]]

Hi Ben,

Do you mind reviewing those patches? I think they are ready.

Comment 7

[Ben Hsu [:HoPang]]

Attachment: P1: Hide useCache registrationOptions

Comment 8

[Ben Hsu [:HoPang]]

Attachment: P2: Disable a testcase

Comment 9

[Ben Hsu [:HoPang]]

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

wrong bug id...

Comment 10

[Ben Hsu [:HoPang]]

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

wrong bug id...

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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

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

Thanks for the quick turn around!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Ben Hsu [:HoPang]]

Attachment: (V2) P2: Remove a testcase

Comment 14

[Ben Hsu [:HoPang]]

Since review is completed, I think we can remove the NI now :)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 17.

Comment 19

[Pulsebot]

Backout by [e-mail]:
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

Comment 20

[Sebastian Hengst [:aryx] (needinfo on intermittent or 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

Comment 21

[Ben Hsu [:HoPang]]

Attachment: (V3) P2: Remove a testcase

Comment 22

[Ben Hsu [:HoPang]]

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

Please refer to comment 17.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

I'll check back later today or tomorrow to uplift once this lands and sticks on m-c.

Comment 25

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/29a0b481d8f3
https://hg.mozilla.org/mozilla-central/rev/67b87beb9234

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 27

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Removes a new testcase from bug 1290944

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ba57c64105b
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d2549a6a16d

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7bdd4de24923
https://hg.mozilla.org/releases/mozilla-beta/rev/51468c2a2371

Comment 30

[Carsten Book [:Tomcat]]

sorry had to back this out from beta for wptlint error like https://treeherder.mozilla.org/logviewer.html#?job_id=90107221&repo=mozilla-beta

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 32

[Ben Hsu [:HoPang]]

Thanks and sorry

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

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...

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9a08d0537ce14edb0f0bda3066366dae736ff79c
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/73696638f90109cfb05599ab28bd79faad8cf20d

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Filed bug 1355087 about the WPT linter missing this problem on m-c.

Comment 36

[Andrei Vaida [:avaida]]

(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.