Closed Bug 1353638 Opened 3 years ago Closed 3 years ago

do not ship boolean service worker useCache option

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox53 + fixed
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: bkelly, Assigned: bhsu)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
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)
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.
Sure, let me handle this.
Assignee: nobody → bhsu
Flags: needinfo?(bhsu)
Attached patch P2: Disable a testcase (obsolete) — Splinter Review
Hi Ben,

Do you mind reviewing those patches? I think they are ready.
Flags: needinfo?(bkelly)
Attached patch P2: Disable a testcase (obsolete) — Splinter Review
Comment on attachment 8854786 [details] [diff] [review]
P1: Hide useCache registrationOptions

wrong bug id...
Attachment #8854786 - Attachment is obsolete: true
Comment on attachment 8854787 [details] [diff] [review]
P2: Disable a testcase

wrong bug id...
Attachment #8854787 - Attachment is obsolete: true
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+
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+
Attached patch (V2) P2: Remove a testcase (obsolete) — Splinter Review
Attachment #8855593 - Attachment description: P2: Remove a testcase → (V2) P2: Remove a testcase
Attachment #8855593 - Flags: review+
Attachment #8854923 - Attachment is obsolete: true
Since review is completed, I think we can remove the NI now :)
Flags: needinfo?(bkelly)
Ho-Pang pushed to try, but its the end of the day in Taiwan.  I'll finish getting this landed.
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?
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?
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)
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?
Attachment #8855593 - Attachment is obsolete: true
Attachment #8855593 - Flags: approval-mozilla-beta?
Attachment #8855593 - Flags: approval-mozilla-aurora?
I'll check back later today or tomorrow to uplift once this lands and sticks on m-c.
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+
sorry had to back this out from beta for wptlint error like https://treeherder.mozilla.org/logviewer.html#?job_id=90107221&repo=mozilla-beta
Flags: needinfo?(bhsu)
(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.
Thanks and sorry
Flags: needinfo?(bhsu)
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...
Filed bug 1355087 about the WPT linter missing this problem on m-c.
See Also: → 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.