|
|
Created:
4 years, 3 months ago by horo Modified:
4 years, 3 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, kinuko+watch, tzik, Marijn Kruisselbrink, iclelland Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse InstallEvent interface even if ForeignFetch is disabled
InstallEvent interface was introduced only for ForeignFetch.
https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e44245840cb5d4e
But according to the spec we should use InstallEvent interface for install event.
https://w3c.github.io/ServiceWorker/#execution-context-events
So we will use InstallEvent interface even if ForeignFetch feature is disabled.
BUG=
Committed: https://crrev.com/707632096793d6ff645a1a4583956ab53f2398ab
Cr-Commit-Position: refs/heads/master@{#418725}
Patch Set 1 #Patch Set 2 : call V8InstallEvent::installForeignFetch() from installOriginTrialsForModules() #Patch Set 3 : update global-interface-listing-service-worker-expected.txt #Patch Set 4 : fix global-interface-listing-service-worker-expected.txt #
Messages
Total messages: 35 (21 generated)
Description was changed from ========== Use InstallEvent even if ForeignFetch is disabled BUG= ========== to ========== Use InstallEvent interface even if ForeignFetch is disabled BUG= ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ PTAL.
For the historical record, can you add to the CL description why this change is needed? I guess, InstallEvent was implemented for foreign fetch only, but it's actually long been part of the service worker spec. I think we have test coverage with virtual/stable/http/tests/serviceworker/webexposed. Those expectations needs updating no?
Description was changed from ========== Use InstallEvent interface even if ForeignFetch is disabled BUG= ========== to ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ==========
Description was changed from ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ========== to ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced only for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ==========
The CQ bit was checked by horo@chromium.org to run a CQ dry run
On 2016/09/14 06:57:03, falken wrote: > For the historical record, can you add to the CL description why this change is > needed? I guess, InstallEvent was implemented for foreign fetch only, but it's > actually long been part of the service worker spec. > > I think we have test coverage with > virtual/stable/http/tests/serviceworker/webexposed. Those expectations needs > updating no? done
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks, lgtm
The CQ bit was checked by horo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
horo@chromium.org changed reviewers: + haraken@chromium.org
haraken@ Could you please review this?
LGTM
The CQ bit was unchecked by horo@chromium.org
The CQ bit was checked by horo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2339083002/#ps60001 (title: "fix global-interface-listing-service-worker-expected.txt")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
horo@chromium.org changed reviewers: + tkent@chromium.org
tkent@ Could you please review global-interface-listing-service-worker-expected.txt?
lgtm. This is a minor change, and I think we don't need intent-to-ship.
The CQ bit was checked by horo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced only for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ========== to ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced only for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced only for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= ========== to ========== Use InstallEvent interface even if ForeignFetch is disabled InstallEvent interface was introduced only for ForeignFetch. https://github.com/w3c/ServiceWorker/commit/98843d032cd668cda4c7e46f0e4424584... But according to the spec we should use InstallEvent interface for install event. https://w3c.github.io/ServiceWorker/#execution-context-events So we will use InstallEvent interface even if ForeignFetch feature is disabled. BUG= Committed: https://crrev.com/707632096793d6ff645a1a4583956ab53f2398ab Cr-Commit-Position: refs/heads/master@{#418725} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/707632096793d6ff645a1a4583956ab53f2398ab Cr-Commit-Position: refs/heads/master@{#418725} |