Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2504663002:
Added a local tick clock to ServiceWorkerVersion. (Closed)
|
Created:
4 years, 1 month ago by harkness Modified:
4 years ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, horo+watch_chromium.org, jam, jsbell+serviceworker_chromium.org, kinuko+watch, kinuko+serviceworker, michaeln, nhiroki, serviceworker-reviews, shimazu+serviceworker_chromium.org, tzik Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a local tick clock to ServiceWorkerVersion.
As part of testing for functionality that uses StartRequestWithCustomTimout,
a way to trigger the timeout was needed on ServiceWorkerVersion. This CL
adds a clock that can be overridden by testing. It also updates the
ServiceWorker version tests to test multiple entries in the timeout request
queue and that they time out in the correct order.
BUG=649403
Committed: https://crrev.com/641c3b9629668ad19be1ee9ba8936624ce364bfa
Cr-Commit-Position: refs/heads/master@{#435929}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Code review comments #Patch Set 3 : Code review comments #Patch Set 4 : Fix test issues and rebase #
Messages
Total messages: 34 (21 generated)
harkness@chromium.org changed reviewers: + johnme@chromium.org, peter@chromium.org
https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1161: int64_t callback_id = tick_clock_->NowTicks().ToInternalValue(); If tick_clock_ is overridden by a test clock which doesn't increase between multiple calls, we could end up with multiple callback_ids which are the same. However, that's only used to record the tracing events, so it should be safe to allow collisions in testing.
The CQ bit was checked by harkness@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.
harkness@chromium.org changed reviewers: + falken@chromium.org
Hi falken@, I wrote this patch because we were trying to test code that uses StartRequestWithCustomTimeout. It turns out that it's extremely difficult to test this end to end through the PushMessaging system, so I'm not going to be pursuing that right now. I didn't see that you had any other tests that validated the custom timeout code, so even though I don't need it anymore, it seemed like a worthwhile thing to send to you for review. If you already have coverage for this that I didn't spot or you don't want to add a local clock, just let me know and I'll drop the review. Thanks, Jen
Sorry I didn't get to this today and am ooo tomorrow. Will review as soon as I can.
Thanks for taking a look, there's no rush for it. On Wed, Nov 30, 2016 at 11:59 AM, <falken@chromium.org> wrote: > Sorry I didn't get to this today and am ooo tomorrow. Will review as soon > as I > can. > > https://codereview.chromium.org/2504663002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by harkness@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.
FWIW I'm actually using this in some tests for timeout behavior for message and foreignfetch events I'm implementing :) https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version_unittest.cc:941: SERVICE_WORKER_ERROR_NETWORK; // dummy value nit: new code generally uses SERVICE_WORKER_ERROR_MAX_VALUE as dummy value
Thanks for the patch, this looks useful. lgtm https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.h:594: // The following methods all rely on the internal clock for the current time. nit: add "|tick_clock_|" for clarity?
https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.h (right): https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.h:594: // The following methods all rely on the internal clock for the current time. On 2016/12/02 09:12:28, falken wrote: > nit: add "|tick_clock_|" for clarity? Done. https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version_unittest.cc (right): https://codereview.chromium.org/2504663002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version_unittest.cc:941: SERVICE_WORKER_ERROR_NETWORK; // dummy value On 2016/12/02 00:15:52, Marijn Kruisselbrink wrote: > nit: new code generally uses SERVICE_WORKER_ERROR_MAX_VALUE as dummy value Done.
The CQ bit was checked by harkness@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/2504663002/#ps40001 (title: "Code review comments")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Proof that I should not upload patches before morning caffeine.
The CQ bit was checked by harkness@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.
The CQ bit was checked by harkness@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/2504663002/#ps60001 (title: "Fix test issues and rebase")
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": 60001, "attempt_start_ts": 1480685320629220,
"parent_rev": "16ff9ef439919b268d05a37fd22df6577e9eef70", "commit_rev":
"97cb2c187af6e4f5a8c975b831415b3330ba7839"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added a local tick clock to ServiceWorkerVersion. As part of testing for functionality that uses StartRequestWithCustomTimout, a way to trigger the timeout was needed on ServiceWorkerVersion. This CL adds a clock that can be overridden by testing. It also updates the ServiceWorker version tests to test multiple entries in the timeout request queue and that they time out in the correct order. BUG=649403 ========== to ========== Added a local tick clock to ServiceWorkerVersion. As part of testing for functionality that uses StartRequestWithCustomTimout, a way to trigger the timeout was needed on ServiceWorkerVersion. This CL adds a clock that can be overridden by testing. It also updates the ServiceWorker version tests to test multiple entries in the timeout request queue and that they time out in the correct order. BUG=649403 Committed: https://crrev.com/641c3b9629668ad19be1ee9ba8936624ce364bfa Cr-Commit-Position: refs/heads/master@{#435929} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/641c3b9629668ad19be1ee9ba8936624ce364bfa Cr-Commit-Position: refs/heads/master@{#435929} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
