Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658603003:
ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope (Closed)
|
Created:
3 years, 11 months ago by nhiroki Modified:
3 years, 10 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, mlamouri+watch-content_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, jam, blink-reviews-api_chromium.org, haraken, dglazkov+blink, kinuko+serviceworker, horo+watch_chromium.org, blink-reviews, darin-cc_chromium.org, kinuko+watch, tzik, falken+watch_chromium.org, blink-worker-reviews_chromium.org, Rick Byers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope
DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo8c/edit?usp=sharing
This CL enables UseCounter for ServiceWorker. This works as follows:
- API use on ServiceWorkerGlobalScope is recorded in all documents controlled by
the worker. For example, when a service worker controlling 3 documents calls
some API, it's recorded in their UseCounter as if they all called the API:
[Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)]
(calls API X) ---> [Renderer(Document2)]
---> [Renderer(Document3)]
(each document records use of API X;
the total count of API X is 3)
- When a new document gets controlled by the existing service worker, its
counter is synchronized with the current use counts of the worker.
[Browser] ---> [Renderer(Document4)]
(new doc, but has the same counts with documents1-3;
the total count of API X is 4)
- By the specification, ServiceWorker can be terminated any time there is no
activity and restarted on demand. As long as on-memory representation of
service worker in the browser process (i.e., ServiceWorkerVersion) is alive,
the counter can outlive worker restart.
- The counter is persisted in a service worker storage on installation so that
API use during the install event can be counted even when a service worker
restarts from on-disk representation. This is necessary for avoiding API use
during installation from being underestimated compared to API use after
activation (installation happens only one time per service worker version).
See the design doc for a discussion of this model.
BUG=376039
Review-Url: https://codereview.chromium.org/2658603003
Cr-Commit-Position: refs/heads/master@{#450260}
Committed: https://chromium.googlesource.com/chromium/src/+/38e501dfecb476f9666f3833fd2a871a6a03b3c9
Patch Set 1 : ready to review #
Total comments: 27
Patch Set 2 : partially address review comments #Patch Set 3 : rebase #Patch Set 4 : tweak unittests #
Total comments: 12
Patch Set 5 : rebase on https://codereview.chromium.org/2680423006/ #Patch Set 6 : address review comments #Patch Set 7 : add nullptr check for fixing test failures #
Total comments: 4
Patch Set 8 : int32_t -> uint32_t #Depends on Patchset: Messages
Total messages: 124 (102 generated)
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - stop sending an IPC message to documents if it's already counted - add tests BUG=376039 ==========
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - stop sending an IPC message to documents if it's already counted - add tests BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored counts are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ==========
The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored counts are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored count are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ==========
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored count are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - DONE: store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored count are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ==========
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 nhiroki@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 nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nhiroki@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...
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope WIP TODO - DONE: store UseCounter in ServiceWorkerStorage and retrieve it on worker startup - DONE: stop sending an IPC message to documents if it's already counted - add tests - When a document gets controlled, a browser process sends an existing counts to the document via ServiceWorkerProviderHost::SetControllerVersionAttribute(). - When a service worker is installed, stores current counts in the registration storage. This stored count are never updated after installation. - When a service worker starts, retrieve the stored counts and notify them of all controlled documents. - When a service worker uses some API to be counted, it's notified to all controlled documents. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the spec of ServiceWorker, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as the service worker representation in the browser process (i.e., ServiceWorkerVersion) exists, the counter is kept over worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event are appropriately counted even when a service worker starts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use while a worker is running. This stored count are never updated after installation. See the design doc for the discussion on this model. BUG=376039 ==========
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, the worker notifies the document of the current use counts so that the document can synchronize with its UseCounter with others. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the spec of ServiceWorker, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as the service worker representation in the browser process (i.e., ServiceWorkerVersion) exists, the counter is kept over worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event are appropriately counted even when a service worker starts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use while a worker is running. This stored count are never updated after installation. See the design doc for the discussion on this model. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker starts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker starts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ==========
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:240001) has been deleted
Patchset #1 (id:260001) has been deleted
Patchset #1 (id:280001) has been deleted
nhiroki@chromium.org changed reviewers: + falken@chromium.org, kinuko@chromium.org, rbyers@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2658603003/diff/300001/content/child/service_... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/child/service_... content/child/service_worker/service_worker_dispatcher.cc:842: // Sync controllee's use counter with service worker's use counter. nit: controllee -> the controllee service worker's use counter -> the service worker's one
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:367: // to record an API use. Nit: I'd prefer preparing separate methods for countFeature and countDeprecation, instead of judging that by making countFeature call Deprecation::deprecationMessage.
Does this sync the use counter with the browser and other pages immediately on each feature use? I'm a bit worried about the number of many IPC messages. Do we have an idea of how often the use counter gets changed, and did you consider doing something like periodic syncing or syncing just when the worker dies? (The design document mentions this somewhat.) https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_database.proto (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_database.proto:55: // blink::UseCounter::Feature enum. This is the set of features the worker used up until the time installation completed right? Could the comment reflect that? https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:465: return; // Could be NULL in some tests. Is this really hit in some tests? I've come to dislike these test-only code in production because they add complexity and avoid them if possible. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:237: // Notifies an API use to the document. What is "the document" here? Does this mean this should only be used for SERVICE_WORKER_PROVIDER_FOR_WINDOW? https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:774: found_registration->waiting_version()->foreign_fetch_origins()[0]); Should we check the used features somewhere in this test? https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:1582: live_version->set_used_features({124, 901, 1019}); Just curious why we set the used features here. Should it be checked later? https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_version.h:854: // from blink::UseCounter::Feature enum. Can we mention this is the used features that were (1) used up until installation of this version completed, or (2) used during the lifetime of |this|. If that's correct? https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/usecounter-worker.js (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/usecounter-worker.js:4: let run_install_event = false; nit: maybe did_install or did_run_install_event is more clear? https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:31: let scope = 'resources/usecounter-window.html?basic'; const for these? https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:95: let scope = 'resources/usecounter-window.html?claim'; const https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:178: '&deprecated=' + kDeprecatedFeature; const? https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:317: // TODO(nhiroki): Test a case where ServiceWorker controles SharedWorker that nit: controls
The CQ bit was checked by nhiroki@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ==========
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
The CQ bit was checked by nhiroki@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...
Thanks for your comments. Updated. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_database.proto (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_database.proto:55: // blink::UseCounter::Feature enum. On 2017/02/08 05:01:53, falken wrote: > This is the set of features the worker used up until the time installation > completed right? Could the comment reflect that? That's right. Updated. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:465: return; // Could be NULL in some tests. On 2017/02/08 05:01:53, falken wrote: > Is this really hit in some tests? I've come to dislike these test-only code in > production because they add complexity and avoid them if possible. I hit this before but now cannot... so removed this check. I'll check others in this file later. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:237: // Notifies an API use to the document. On 2017/02/08 05:01:53, falken wrote: > What is "the document" here? Does this mean this should only be used for > SERVICE_WORKER_PROVIDER_FOR_WINDOW? No, this can be called for any controllee type. Updated this comment and added DCHECK. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_storage_unittest.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:774: found_registration->waiting_version()->foreign_fetch_origins()[0]); Yeah, I should have checked it. Done. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_storage_unittest.cc:1582: live_version->set_used_features({124, 901, 1019}); On 2017/02/08 05:01:53, falken wrote: > Just curious why we set the used features here. Should it be checked later? Yeah, this doesn't really make sense. I meant to check whether a new version overrides an incumbent version in terms of used_features, but it's obvious because used_features is owned by SWVersion and replacement of SWVersion is well-tested in other unittests and layouttests, so I removed this. https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_version.h:854: // from blink::UseCounter::Feature enum. On 2017/02/08 05:01:53, falken wrote: > Can we mention this is the used features that were (1) used up until > installation of this version completed, or (2) used during the lifetime of > |this|. > > If that's correct? That's correct. Updated. https://codereview.chromium.org/2658603003/diff/300001/content/child/service_... File content/child/service_worker/service_worker_dispatcher.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/child/service_... content/child/service_worker/service_worker_dispatcher.cc:842: // Sync controllee's use counter with service worker's use counter. On 2017/02/07 13:42:58, kinuko wrote: > nit: controllee -> the controllee > service worker's use counter -> the service worker's one Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/usecounter-worker.js (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/resources/usecounter-worker.js:4: let run_install_event = false; On 2017/02/08 05:01:54, falken wrote: > nit: maybe did_install or did_run_install_event is more clear? Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:31: let scope = 'resources/usecounter-window.html?basic'; On 2017/02/08 05:01:54, falken wrote: > const for these? Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:95: let scope = 'resources/usecounter-window.html?claim'; On 2017/02/08 05:01:54, falken wrote: > const Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:178: '&deprecated=' + kDeprecatedFeature; On 2017/02/08 05:01:54, falken wrote: > const? Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium/usecounter.html:317: // TODO(nhiroki): Test a case where ServiceWorker controles SharedWorker that On 2017/02/08 05:01:54, falken wrote: > nit: controls Done. https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right): https://codereview.chromium.org/2658603003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:367: // to record an API use. On 2017/02/08 04:28:45, haraken wrote: > > Nit: I'd prefer preparing separate methods for countFeature and > countDeprecation, instead of judging that by making countFeature call > Deprecation::deprecationMessage. Removed the switch instead of adding a separate path. That was just for bypassing DCHECK in UseCounter::count(Feature feature). I removed the check in a separate patch for SharedWorker: https://codereview.chromium.org/2586863002/#ps460001 In addition, calling countDeprecation() in document-side was wrong because it prints a duplicate warning message.
Did you see my top-level comment too?
On 2017/02/09 05:31:15, falken wrote: > Did you see my top-level comment too? That was a good question to consider, I have actually wondered the same.
On 2017/02/08 05:01:54, falken wrote: > Does this sync the use counter with the browser and other pages immediately on > each feature use? Yes. > I'm a bit worried about the number of many IPC messages. Do we > have an idea of how often the use counter gets changed, and did you consider > doing something like periodic syncing or syncing just when the worker dies? (The > design document mentions this somewhat.) I guess the number of IPC messages is not big for now because (1) most of blink::UseCounter::Features are available only on Document (e.g., DOM, CSS), (2) the counts are sent as a batch when a new document gets controlled, and (3) renderer(worker) and browser don't send an IPC message for already recorded features. Periodic syncing or finalizer-ish syncing would be a reasonable option but I feel it'd be over-optimization as a first edition.
Makes sense to me. lgtm. https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:238: // should be called only on controllee's provider host. It's a little hard to parse this. Suggestion: "Notifies the client that its controller used a feature, for UseCounter purposes. This can only be called if IsProviderForClient() is true." https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... content/child/service_worker/web_service_worker_provider_impl.cc:57: // Sync controllee's use counter with service worker's use counter. nit: I guess this should match the other file: "Sync the controllee's use counter with the service worker's one." https://codereview.chromium.org/2658603003/diff/360001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/common/service... content/common/service_worker/service_worker_messages.h:522: // Notifies an API use to a client document (browser->renderer). |feature| must Notifies a client that its controller used a feature, for UseCounter purposes (browser->renderer).
nhiroki@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng@, can you review content/common/service_worker/*_messages.h ? I'll address falken@'s comments tomorrow.
https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_version.h:470: void set_used_features(std::set<uint32_t> used_features) { Nit: Pass by const ref https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_version.h:473: std::set<uint32_t> used_features() const { return used_features_; } Consider returning by const ref here as well. https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... File content/child/service_worker/service_worker_provider_context.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... content/child/service_worker/service_worker_provider_context.h:75: std::set<uint32_t> used_features() const { return used_features_; } And here.
I think this introduces the same issue as https://codereview.chromium.org/2586863002/. This CL depends on MessagePort communication being in the same FIFO as the EmbeddedWorkerHostMsg_CountFeature IPC. That seems like something we shouldn't count on being true, and it is something I'm trying to make untrue ;-)
On 2017/02/09 23:20:15, darin (slow to review) wrote: > I think this introduces the same issue as > https://codereview.chromium.org/2586863002/. > > This CL depends on MessagePort communication being in the same FIFO as > the EmbeddedWorkerHostMsg_CountFeature IPC. That seems like something > we shouldn't count on being true, and it is something I'm trying to > make untrue ;-) Perhaps instead of using a MessagePort to signal when the usecounters have been updated, we should have a way to attach a listener on window.internal. Then we can just observe when usecounters get updated by shared workers or service workers.
On 2017/02/09 23:26:42, darin (slow to review) wrote: > On 2017/02/09 23:20:15, darin (slow to review) wrote: > > I think this introduces the same issue as > > https://codereview.chromium.org/2586863002/. > > > > This CL depends on MessagePort communication being in the same FIFO as > > the EmbeddedWorkerHostMsg_CountFeature IPC. That seems like something > > we shouldn't count on being true, and it is something I'm trying to > > make untrue ;-) Thank you for the information. Yes, these tests assume that MessagePort messages arrive after UseCounter messages. > Perhaps instead of using a MessagePort to signal when the usecounters > have been updated, we should have a way to attach a listener on > window.internal. Then we can just observe when usecounters get updated > by shared workers or service workers. Using a listener sounds reasonable. I'll make a separate patch soon.
On 2017/02/10 00:20:56, nhiroki (slow) wrote: > On 2017/02/09 23:26:42, darin (slow to review) wrote: > > On 2017/02/09 23:20:15, darin (slow to review) wrote: > > > I think this introduces the same issue as > > > https://codereview.chromium.org/2586863002/. > > > > > > This CL depends on MessagePort communication being in the same FIFO as > > > the EmbeddedWorkerHostMsg_CountFeature IPC. That seems like something > > > we shouldn't count on being true, and it is something I'm trying to > > > make untrue ;-) > > Thank you for the information. Yes, these tests assume that MessagePort messages > arrive after UseCounter messages. > > > Perhaps instead of using a MessagePort to signal when the usecounters > > have been updated, we should have a way to attach a listener on > > window.internal. Then we can just observe when usecounters get updated > > by shared workers or service workers. > > Using a listener sounds reasonable. I'll make a separate patch soon. Cool, thanks!
On 2017/02/10 00:28:49, darin (slow to review) wrote: > On 2017/02/10 00:20:56, nhiroki (slow) wrote: > > On 2017/02/09 23:26:42, darin (slow to review) wrote: > > > On 2017/02/09 23:20:15, darin (slow to review) wrote: > > > > I think this introduces the same issue as > > > > https://codereview.chromium.org/2586863002/. > > > > > > > > This CL depends on MessagePort communication being in the same FIFO as > > > > the EmbeddedWorkerHostMsg_CountFeature IPC. That seems like something > > > > we shouldn't count on being true, and it is something I'm trying to > > > > make untrue ;-) > > > > Thank you for the information. Yes, these tests assume that MessagePort > messages > > arrive after UseCounter messages. > > > > > Perhaps instead of using a MessagePort to signal when the usecounters > > > have been updated, we should have a way to attach a listener on > > > window.internal. Then we can just observe when usecounters get updated > > > by shared workers or service workers. > > > > Using a listener sounds reasonable. I'll make a separate patch soon. > > Cool, thanks! A patch is under review: https://codereview.chromium.org/2680423006/
The CQ bit was checked by nhiroki@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 nhiroki@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...
Thank you for the comments. Updated. https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:238: // should be called only on controllee's provider host. On 2017/02/09 06:03:58, falken wrote: > It's a little hard to parse this. Suggestion: "Notifies the client that its > controller used a feature, for UseCounter purposes. This can only be called if > IsProviderForClient() is true." Done. https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_version.h:470: void set_used_features(std::set<uint32_t> used_features) { On 2017/02/09 22:43:20, dcheng wrote: > Nit: Pass by const ref Done. https://codereview.chromium.org/2658603003/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_version.h:473: std::set<uint32_t> used_features() const { return used_features_; } On 2017/02/09 22:43:20, dcheng wrote: > Consider returning by const ref here as well. Done. https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... File content/child/service_worker/service_worker_provider_context.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... content/child/service_worker/service_worker_provider_context.h:75: std::set<uint32_t> used_features() const { return used_features_; } On 2017/02/09 22:43:20, dcheng wrote: > And here. Done. https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... File content/child/service_worker/web_service_worker_provider_impl.cc (right): https://codereview.chromium.org/2658603003/diff/360001/content/child/service_... content/child/service_worker/web_service_worker_provider_impl.cc:57: // Sync controllee's use counter with service worker's use counter. On 2017/02/09 06:03:58, falken wrote: > nit: I guess this should match the other file: "Sync the controllee's use > counter with the service worker's one." Done. https://codereview.chromium.org/2658603003/diff/360001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/2658603003/diff/360001/content/common/service... content/common/service_worker/service_worker_messages.h:522: // Notifies an API use to a client document (browser->renderer). |feature| must On 2017/02/09 06:03:58, falken wrote: > Notifies a client that its controller used a feature, for UseCounter purposes > (browser->renderer). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nhiroki@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...
https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2658603003/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:465: return; // Could be NULL in some tests. On 2017/02/09 05:11:31, nhiroki wrote: > On 2017/02/08 05:01:53, falken wrote: > > Is this really hit in some tests? I've come to dislike these test-only code in > > production because they add complexity and avoid them if possible. > > I hit this before but now cannot... so removed this check. > > I'll check others in this file later. Hm... browser_side_navigation_browser_tests on linux_chromium_rel_ng hit this. The patchset7 adds this check again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ipc lgtm https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1343: for (int32_t feature : registration.used_features) Nit: uint32_t feature https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:465: return; // Could be nullptr in some tests. Nit: I think "null for testing" makes it harder to understand code. I'm OK with landing as-is, but we should probably fix the tests if possible.
Thank you! https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... File content/browser/service_worker/service_worker_database.cc (right): https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... content/browser/service_worker/service_worker_database.cc:1343: for (int32_t feature : registration.used_features) On 2017/02/13 21:25:02, dcheng wrote: > Nit: uint32_t feature Good catch! Fixed. https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/2658603003/diff/420001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:465: return; // Could be nullptr in some tests. On 2017/02/13 21:25:02, dcheng wrote: > Nit: I think "null for testing" makes it harder to understand code. I'm OK with > landing as-is, but we should probably fix the tests if possible. Acknowledged. This pattern is somewhat widely used in SW code. I'll make a separate patch to fix them.
The CQ bit was checked by nhiroki@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...
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, falken@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2658603003/#ps440001 (title: "int32_t -> uint32_t")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1487049048689380,
"parent_rev": "1a5774e06f5d0a8794594e28a70e7c6b00106730", "commit_rev":
"38e501dfecb476f9666f3833fd2a871a6a03b3c9"}
Message was sent while issue was closed.
Description was changed from ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 ========== to ========== ServiceWorker: Enable UseCounter for ServiceWorkerGlobalScope DesignDoc: https://docs.google.com/document/d/1VyYZnhjBdk-MzCRAcX37TM5-yjwTY40U_J9rWnEAo... This CL enables UseCounter for ServiceWorker. This works as follows: - API use on ServiceWorkerGlobalScope is recorded in all documents controlled by the worker. For example, when a service worker controlling 3 documents calls some API, it's recorded in their UseCounter as if they all called the API: [Renderer(Worker)] ---> [Browser] ---> [Renderer(Document1)] (calls API X) ---> [Renderer(Document2)] ---> [Renderer(Document3)] (each document records use of API X; the total count of API X is 3) - When a new document gets controlled by the existing service worker, its counter is synchronized with the current use counts of the worker. [Browser] ---> [Renderer(Document4)] (new doc, but has the same counts with documents1-3; the total count of API X is 4) - By the specification, ServiceWorker can be terminated any time there is no activity and restarted on demand. As long as on-memory representation of service worker in the browser process (i.e., ServiceWorkerVersion) is alive, the counter can outlive worker restart. - The counter is persisted in a service worker storage on installation so that API use during the install event can be counted even when a service worker restarts from on-disk representation. This is necessary for avoiding API use during installation from being underestimated compared to API use after activation (installation happens only one time per service worker version). See the design doc for a discussion of this model. BUG=376039 Review-Url: https://codereview.chromium.org/2658603003 Cr-Commit-Position: refs/heads/master@{#450260} Committed: https://chromium.googlesource.com/chromium/src/+/38e501dfecb476f9666f3833fd2a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:440001) as https://chromium.googlesource.com/chromium/src/+/38e501dfecb476f9666f3833fd2a... |
