Closed
Bug 1353638
Opened 4 years ago
Closed 4 years ago
do not ship boolean service worker useCache option
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bkelly, Assigned: bhsu)
References
Details
Attachments
(2 files, 4 obsolete files)
1.40 KB,
patch
|
bkelly
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
12.93 KB,
patch
|
bhsu
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•4 years 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•4 years 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•4 years ago
|
||
Sure, let me handle this.
Assignee: nobody → bhsu
Flags: needinfo?(bhsu)
Assignee | ||
Comment 4•4 years ago
|
||
Assignee | ||
Comment 5•4 years ago
|
||
Assignee | ||
Comment 6•4 years ago
|
||
Hi Ben, Do you mind reviewing those patches? I think they are ready.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 7•4 years ago
|
||
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
Comment on attachment 8854786 [details] [diff] [review] P1: Hide useCache registrationOptions wrong bug id...
Attachment #8854786 -
Attachment is obsolete: true
Assignee | ||
Comment 10•4 years ago
|
||
Comment on attachment 8854787 [details] [diff] [review] P2: Disable a testcase wrong bug id...
Attachment #8854787 -
Attachment is obsolete: true
Reporter | ||
Comment 11•4 years 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•4 years 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•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Attachment #8855593 -
Attachment description: P2: Remove a testcase → (V2) P2: Remove a testcase
Attachment #8855593 -
Flags: review+
Assignee | ||
Updated•4 years ago
|
Attachment #8854923 -
Attachment is obsolete: true
Assignee | ||
Comment 14•4 years ago
|
||
Since review is completed, I think we can remove the NI now :)
Flags: needinfo?(bkelly)
Reporter | ||
Comment 15•4 years ago
|
||
Ho-Pang pushed to try, but its the end of the day in Taiwan. I'll finish getting this landed.
Comment 16•4 years 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•4 years 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•4 years 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•4 years 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•4 years ago
|
||
Assignee | ||
Comment 22•4 years 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•4 years ago
|
Attachment #8855593 -
Attachment is obsolete: true
Attachment #8855593 -
Flags: approval-mozilla-beta?
Attachment #8855593 -
Flags: approval-mozilla-aurora?
Comment 23•4 years 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.
![]() |
||
Comment 25•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/29a0b481d8f3 https://hg.mozilla.org/mozilla-central/rev/67b87beb9234
Status: NEW → RESOLVED
Closed: 4 years ago
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-firefox54:
--- → +
Comment 28•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ba57c64105b https://hg.mozilla.org/releases/mozilla-aurora/rev/1d2549a6a16d
Comment 29•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7bdd4de24923 https://hg.mozilla.org/releases/mozilla-beta/rev/51468c2a2371
Comment 30•4 years 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
Flags: needinfo?(bhsu)
Reporter | ||
Comment 31•4 years 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.
Reporter | ||
Comment 33•4 years 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 34•4 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/9a08d0537ce14edb0f0bda3066366dae736ff79c remote: https://hg.mozilla.org/releases/mozilla-beta/rev/73696638f90109cfb05599ab28bd79faad8cf20d
Reporter | ||
Comment 35•4 years ago
|
||
Filed bug 1355087 about the WPT linter missing this problem on m-c.
See Also: → 1355087
Comment 36•4 years ago
|
||
(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.
Description
•