Service Worker: Coalesce the start worker timer into the timeout timer.

Service Worker: Coalesce the start worker timer into the timeout timer.

Just a refactoring. It's easier to reason about a single timeout timer
than multiple ones.

BUG=448003, 372436
Committed: https://crrev.com/088f04fe87b349c4e3f0593f57413a24b4eaf2f1
Cr-Commit-Position: refs/heads/master@{#319851}

[falken, 2015-03-09 08:48:08]

falken@chromium.org changed reviewers:
+ kinuko@chromium.org, nhiroki@chromium.org

[falken, 2015-03-09 08:48:08]

could one of you review this?

[kinuko, 2015-03-09 09:30:41]

lgtm, modulo some nits for readability

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:812: ping_state_ =
PING_STARTED;
nit: This line (or enum name) may look a bit cryptic as people would expect line
1484 to be the point when we start pinging.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1456: if
(GetTickDuration(start_time_) >
nit: Could we have one-line comments for each of these timeouts, e.g. 'starting
a worker hasn't finished within a certain period', 'the worker has been in idle
for longer than a certain period', 'the worker hasn't responded to our ping
within a certain period' etc.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1468: return;
nit: could this check be done before we check start_time?

[falken, 2015-03-09 10:10:55]

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:812: ping_state_ =
PING_STARTED;
On 2015/03/09 09:30:40, kinuko wrote:
> nit: This line (or enum name) may look a bit cryptic as people would expect
line
> 1484 to be the point when we start pinging.

Agreed. I renamed the enums and added a comment.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1456: if
(GetTickDuration(start_time_) >
On 2015/03/09 09:30:40, kinuko wrote:
> nit: Could we have one-line comments for each of these timeouts, e.g.
'starting
> a worker hasn't finished within a certain period', 'the worker has been in
idle
> for longer than a certain period', 'the worker hasn't responded to our ping
> within a certain period' etc.

Done.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1468: return;
On 2015/03/09 09:30:40, kinuko wrote:
> nit: could this check be done before we check start_time?

Thanks for pointing this out. The start_time timeout is different from the
others: it doesn't just stop the worker but also invokes start callbacks with
ERROR_TIMEOUT. This means StartWorker(); StopWorker(); will be able to fail
callbacks even if we got stuck at some point.

But there's a bug with the current patch: if ping_time expires happens before
start_time, it's possible that start callbacks will never be invoked with
ERROR_TIMEOUT.

I think I have to redesign this so the timeout timer continues running even
after one thing times out.

[nhiroki, 2015-03-10 00:37:10]

LGTM after kinuko's comments are addressed.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:62: const int
kDestructedStartingWorkerTimeoutThresholdSeconds = 5;  // 5 secs.
nit: This comment "// 5 secs." would be pointless because the const name
explains it.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version_unittest.cc:470:
base::TimeDelta::FromMinutes(6);  // over kStartWorkerTimeoutMinutes
This could get broken when kStartWorkerTimeoutMinutes is modified. How about
calculating "kStartWorkerTimeoutMinutes + 1"?

[falken, 2015-03-10 06:56:36]

Thanks.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:62: const int
kDestructedStartingWorkerTimeoutThresholdSeconds = 5;  // 5 secs.
On 2015/03/10 00:37:10, nhiroki wrote:
> nit: This comment "// 5 secs." would be pointless because the const name
> explains it.

Done.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:1468: return;
On 2015/03/09 10:10:55, falken wrote:
> On 2015/03/09 09:30:40, kinuko wrote:
> > nit: could this check be done before we check start_time?
> 
> Thanks for pointing this out. The start_time timeout is different from the
> others: it doesn't just stop the worker but also invokes start callbacks with
> ERROR_TIMEOUT. This means StartWorker(); StopWorker(); will be able to fail
> callbacks even if we got stuck at some point.
> 
> But there's a bug with the current patch: if ping_time expires happens before
> start_time, it's possible that start callbacks will never be invoked with
> ERROR_TIMEOUT.
> 
> I think I have to redesign this so the timeout timer continues running even
> after one thing times out.

Updated this... the timeout timer now continues until the worker is actually
stopped.

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version_unittest.cc (right):

https://codereview.chromium.org/991743002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version_unittest.cc:470:
base::TimeDelta::FromMinutes(6);  // over kStartWorkerTimeoutMinutes
On 2015/03/10 00:37:10, nhiroki wrote:
> This could get broken when kStartWorkerTimeoutMinutes is modified. How about
> calculating "kStartWorkerTimeoutMinutes + 1"?

Done.

[falken, 2015-03-10 06:57:11]

The CQ bit was checked by falken@chromium.org

[falken, 2015-03-10 06:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/991743002/#ps20001 (title:
"review comments")

[commit-bot: I haz the power, 2015-03-10 06:57:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/991743002/20001

[commit-bot: I haz the power, 2015-03-10 08:34:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-10 08:34:52]

Patchset 2 (id:??) landed as
https://crrev.com/088f04fe87b349c4e3f0593f57413a24b4eaf2f1
Cr-Commit-Position: refs/heads/master@{#319851}