Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1525743002:
Allow ServiceWorker events to have custom durations (Closed)
|
Created:
5 years ago by jkarlin Modified:
5 years ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jkarlin+watch_chromium.org, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, maxbogue+watch_chromium.org, michaeln, nhiroki, plaree+watch_chromium.org, pvalenzuela+watch_chromium.org, serviceworker-reviews, tim+watch_chromium.org, tzik, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow ServiceWorker events to have custom durations
BackgroundSync events need to be able to run for a different amount of
time than other events, such as push or fetch. This CL allows each event
request to set its own expiration time and stores them in a priority
queue based on increasing expiration time. It adds some tests to the
ServiceWorkerVersion changes and lets BackgroundSync set its own
custom time (by default 3 minutes but field trails will
be able to change it in an upcoming CL).
This CL also adds the ability for selected types of requests to not stop
the service worker if they time out. Right now this only pertains to the
sync event.
BUG=569001
Committed: https://crrev.com/264567afb2a2b59a2f155d50463d94476cbef6bf
Cr-Commit-Position: refs/heads/master@{#366105}
Patch Set 1 #Patch Set 2 : Tests #Patch Set 3 : Rebase #Patch Set 4 : Nits #Patch Set 5 : Mock background_sync_dispatcher_ #Patch Set 6 : Comments #
Total comments: 7
Patch Set 7 : Keep running if bgsync fails, but not other requests #Patch Set 8 : Logic bug #
Total comments: 8
Patch Set 9 : Address comments from PS8 #
Total comments: 7
Patch Set 10 : Address comments from PS9 #Patch Set 11 : Use std::move instead of .Pass #
Dependent Patchsets: Messages
Total messages: 32 (16 generated)
Description was changed from ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default the same 5 minutes but field trails will soon be able to change it in an upcoming CL). BUG=569001 ========== to ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default the same 5 minutes but field trails will be able to change it in an upcoming CL). BUG=569001 ==========
jkarlin@chromium.org changed reviewers: + falken@chromium.org
falken: PTAL at everything, thanks!
mek@chromium.org changed reviewers: + mek@chromium.org
just some drive-by comments https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2150: if (requests_.empty() && request_timed_out && running_status() != STOPPING) Not stopping the worker when a request was timed out means that now the browser process will DCHECK (by reaching a NOTREACHED) if/when the response to one of these events finally does arrive. All the On*EventFinished methods assume that the request they get an IPC for still exists in their request map. So even if not stopping if there still are pending events is the right thing to do, that would at least need more changes than just this. https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:211: mojo::InterfaceRequest<BackgroundSyncServiceClient> service_request = FYI, after I clean up and land https://codereview.chromium.org/1264333002, there will be a more clean way to have mock services for tests like this. https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:213: // The MocKBackgroundSyncServiceClient is bound to the client, and will be s/MocK/Mock/
https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2150: if (requests_.empty() && request_timed_out && running_status() != STOPPING) On 2015/12/16 19:09:57, Marijn Kruisselbrink wrote: > Not stopping the worker when a request was timed out means that now the browser > process will DCHECK (by reaching a NOTREACHED) if/when the response to one of > these events finally does arrive. All the On*EventFinished methods assume that > the request they get an IPC for still exists in their request map. > > So even if not stopping if there still are pending events is the right thing to > do, that would at least need more changes than just this. Good catch. It seems reasonable to me to remove the NOTREACHED calls.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Thanks for the drive by mek :) https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2150: if (requests_.empty() && request_timed_out && running_status() != STOPPING) On 2015/12/16 19:29:56, jkarlin wrote: > On 2015/12/16 19:09:57, Marijn Kruisselbrink wrote: > > Not stopping the worker when a request was timed out means that now the > browser > > process will DCHECK (by reaching a NOTREACHED) if/when the response to one of > > these events finally does arrive. All the On*EventFinished methods assume that > > the request they get an IPC for still exists in their request map. > > > > So even if not stopping if there still are pending events is the right thing > to > > do, that would at least need more changes than just this. > > Good catch. It seems reasonable to me to remove the NOTREACHED calls. I've changed it to only allow the service worker to continue running if bgsync times out, and removed its NOTREACHED call. I've also added a test for this exact scenario. The other requests have unmodified behavior. https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:211: mojo::InterfaceRequest<BackgroundSyncServiceClient> service_request = On 2015/12/16 19:09:57, Marijn Kruisselbrink wrote: > FYI, after I clean up and land https://codereview.chromium.org/1264333002, there > will be a more clean way to have mock services for tests like this. Nice! https://codereview.chromium.org/1525743002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:213: // The MocKBackgroundSyncServiceClient is bound to the client, and will be On 2015/12/16 19:09:57, Marijn Kruisselbrink wrote: > s/MocK/Mock/ Done.
Description was changed from ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default the same 5 minutes but field trails will be able to change it in an upcoming CL). BUG=569001 ========== to ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default the same 5 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ==========
falken@: PTAL, thanks!
Sorry for the late review, I was off today. This is looking good, just some questions. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:821: handle_id, last_chance, expiration, callback))); This means the expiration includes the time to start the worker, which can be a few seconds in some cases (max before start worker expires is 10 seconds). If you wanted to not include it, you could instead pass the duration for the sync event, and compute expiration = Now() + duration after this if statement. The other request timeouts work that way. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1102: SetAllRequestExpirations( So this means the sync event's expiration is not honored if devtools is attached and detached, right? That's probably fine. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2146: info.type, NUM_REQUEST_TYPES); Do you have/want UMA to track how frequent the sync event times out? This just records the raw number of timeouts. For fetch/push we can compare with UMA FetchEvent.*.Status, PushEvent.Time to see the frequency (kind of convoluted). https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2315: // Note, returning true for a type means that the On*EventFinished should not returning false?
Patchset #9 (id:200001) has been deleted
falken@: Sorry, didn't mean for you to look on your day off! PTAL when you get back from vacation :) https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:821: handle_id, last_chance, expiration, callback))); On 2015/12/17 13:54:32, falken wrote: > This means the expiration includes the time to start the worker, which can be a > few seconds in some cases (max before start worker expires is 10 seconds). If > you wanted to not include it, you could instead pass the duration for the sync > event, and compute expiration = Now() + duration after this if statement. The > other request timeouts work that way. Good idea. Done. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1102: SetAllRequestExpirations( On 2015/12/17 13:54:32, falken wrote: > So this means the sync event's expiration is not honored if devtools is attached > and detached, right? That's probably fine. Ack. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2146: info.type, NUM_REQUEST_TYPES); On 2015/12/17 13:54:32, falken wrote: > Do you have/want UMA to track how frequent the sync event times out? This just > records the raw number of timeouts. For fetch/push we can compare with UMA > FetchEvent.*.Status, PushEvent.Time to see the frequency (kind of convoluted). Added BackgroundSyncEvent.Time. https://codereview.chromium.org/1525743002/diff/180001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2315: // Note, returning true for a type means that the On*EventFinished should not On 2015/12/17 13:54:32, falken wrote: > returning false? Thanks. Done.
jkarlin@chromium.org changed reviewers: + isherman@chromium.org
isherman@: PTAL at histograms.xml, thanks!
histograms lgtm w/ a nit: https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43044: +<histogram name="ServiceWorker.BackgroundSyncEvent.Time" units="milliseconds"> nit: Perhaps "Duration" rather than "Time"?
lgtm https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2332: case NUM_REQUEST_TYPES: nit: NUM_REQUEST_TYPES being passed is a bug, should hit the NOTREACHED() instead https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:218: // Create a mock BackgroundSyncServiceClient apologetic nit: add period to sentence https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43044: +<histogram name="ServiceWorker.BackgroundSyncEvent.Time" units="milliseconds"> On 2015/12/17 21:41:37, Ilya Sherman wrote: > nit: Perhaps "Duration" rather than "Time"? That's probably better, but the other SW UMAs use "Time" so it might be better to keep it for consistency.
Description was changed from ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default the same 5 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ========== to ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default 3 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ==========
Thanks everyone! https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:2332: case NUM_REQUEST_TYPES: On 2015/12/18 01:28:47, falken wrote: > nit: NUM_REQUEST_TYPES being passed is a bug, should hit the NOTREACHED() > instead Done. https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/1525743002/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_version_unittest.cc:218: // Create a mock BackgroundSyncServiceClient On 2015/12/18 01:28:48, falken wrote: > apologetic nit: add period to sentence Done. https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1525743002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43044: +<histogram name="ServiceWorker.BackgroundSyncEvent.Time" units="milliseconds"> On 2015/12/18 01:28:48, falken wrote: > On 2015/12/17 21:41:37, Ilya Sherman wrote: > > nit: Perhaps "Duration" rather than "Time"? > > That's probably better, but the other SW UMAs use "Time" so it might be better > to keep it for consistency. Agree with both. Keeping with "Time" for consistency.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1525743002/#ps240001 (title: "Address comments from PS9")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525743002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525743002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1525743002/#ps260001 (title: "Use std::move instead of .Pass")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525743002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525743002/260001
Message was sent while issue was closed.
Description was changed from ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default 3 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ========== to ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default 3 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default 3 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 ========== to ========== Allow ServiceWorker events to have custom durations BackgroundSync events need to be able to run for a different amount of time than other events, such as push or fetch. This CL allows each event request to set its own expiration time and stores them in a priority queue based on increasing expiration time. It adds some tests to the ServiceWorkerVersion changes and lets BackgroundSync set its own custom time (by default 3 minutes but field trails will be able to change it in an upcoming CL). This CL also adds the ability for selected types of requests to not stop the service worker if they time out. Right now this only pertains to the sync event. BUG=569001 Committed: https://crrev.com/264567afb2a2b59a2f155d50463d94476cbef6bf Cr-Commit-Position: refs/heads/master@{#366105} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/264567afb2a2b59a2f155d50463d94476cbef6bf Cr-Commit-Position: refs/heads/master@{#366105} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
