Assert about ServiceWorkerRegisterJob internal state in terms of phases

Assert about ServiceWorkerRegisterJob internal state in terms of phases

Instead of ad-hoc DCHECKs of internal state throughout the job's
callbacks, introduce a phase system and add DCHECKs to internal state
getters/setters in terms of it.

BUG=349337
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262936

[falken, 2014-04-08 23:30:53]

PTL

Successor to https://codereview.chromium.org/222003010/

[dominicc (has gone to gerrit), 2014-04-09 00:09:58]

LGTM

I would like to have phase_ assigned directly in only two places: Constructor
and SetPhase, per the inline comment, but I leave that and the details up to
your discretion.

https://codereview.chromium.org/229403004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/229403004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:44: phase_ =
START;
It would be nice to have SetPhase which DCHECKs the previous state makes sense.
This switch statement should end up reading like a transition table reading

case x:
  DCHECK(phase == y);
  break;
...
phase_ = phase;

indicating transitions from x->y are possible.

[falken, 2014-04-09 01:33:19]

Thanks, makes sense.

kinuko: Can you please review as OWNER?

https://codereview.chromium.org/229403004/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/229403004/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:44: phase_ =
START;
On 2014/04/09 00:09:58, dominicc wrote:
> It would be nice to have SetPhase which DCHECKs the previous state makes
sense.
> This switch statement should end up reading like a transition table reading
> 
> case x:
>   DCHECK(phase == y);
>   break;
> ...
> phase_ = phase;
> 
> indicating transitions from x->y are possible.

Done.

[kinuko, 2014-04-09 02:43:30]

lgtm, can you rebase this on top of https://codereview.chromium.org/224733014/ ?

[falken, 2014-04-10 01:07:23]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-04-10 01:07:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/229403004/100001

[commit-bot: I haz the power, 2014-04-10 05:36:27]

Change committed as 262936