service worker: Refactor OnNoControllees

service worker: Refactor OnNoControllees

OnNoControllees doesn't need to unset the flags.
* If |is_uninstalling_| is set, the call to Clear() would unset it.
* If |should_activate_when_ready_| is set, the call to ActivateWaitingVersion() would
unset it. We also change Clear() to unset |should_activate_when_ready_| which
makes sense for any callsite of Clear().

Minor simplification in anticipation of changes for https://crbug.com/616331
Refactoring only; no behavior change.

BUG=616331
Committed: https://crrev.com/6c74874b9c6969a2afc6f195c96a1a3b15cda890
Cr-Commit-Position: refs/heads/master@{#403650}

[falken, 2016-07-04 07:33:53]

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

[falken, 2016-07-04 07:33:54]

nhiroki: Can you review this tiny patch?

[nhiroki, 2016-07-04 07:53:38]

lgtm

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_registration.cc:430: void
ServiceWorkerRegistration::Clear() {
How about checking pre-conditions before flipping |is_uninstalling_| and
|is_uninstalled_|?

DCHECK(is_uninstalling_);
DCHECK(!is_uninstalled_);

[falken, 2016-07-04 08:14:37]

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_registration.cc:430: void
ServiceWorkerRegistration::Clear() {
On 2016/07/04 07:53:38, nhiroki wrote:
> How about checking pre-conditions before flipping |is_uninstalling_| and
> |is_uninstalled_|?
> 
> DCHECK(is_uninstalling_);
> DCHECK(!is_uninstalled_);

Unfortunately I'm not 100% sure those hold. Since this patch doesn't change when
Clear() is called, I'll leave out the dchecks in this patch.

In the future, these uninstalling/uninstalled/deleting flags should be merged
into a single Status enum.

[falken, 2016-07-04 08:15:02]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-07-04 08:15:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nhiroki, 2016-07-04 08:39:05]

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_registration.cc:430: void
ServiceWorkerRegistration::Clear() {
On 2016/07/04 08:14:37, falken wrote:
> On 2016/07/04 07:53:38, nhiroki wrote:
> > How about checking pre-conditions before flipping |is_uninstalling_| and
> > |is_uninstalled_|?
> > 
> > DCHECK(is_uninstalling_);
> > DCHECK(!is_uninstalled_);
> 
> Unfortunately I'm not 100% sure those hold. Since this patch doesn't change
when
> Clear() is called, I'll leave out the dchecks in this patch.

Acknowledged.

> In the future, these uninstalling/uninstalled/deleting flags should be merged
> into a single Status enum.

It's great to merge them! Are you going to try it? :)
(I tried it before and gave up because of complex state transitions...)

[commit-bot: I haz the power, 2016-07-04 08:48:54]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-04 08:49:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 08:50:27]

Description was changed from

==========
service worker: Refactor OnNoControllees

OnNoControllees doesn't need to unset the flags.
* If |is_uninstalling_| is set, the call to Clear() would unset it.
* If |should_activate_when_ready_| is set, the call to ActivateWaitingVersion()
would
unset it. We also change Clear() to unset |should_activate_when_ready_| which
makes sense for any callsite of Clear().

Minor simplification in anticipation of changes for https://crbug.com/616331
Refactoring only; no behavior change.

BUG=616331
==========

to

==========
service worker: Refactor OnNoControllees

OnNoControllees doesn't need to unset the flags.
* If |is_uninstalling_| is set, the call to Clear() would unset it.
* If |should_activate_when_ready_| is set, the call to ActivateWaitingVersion()
would
unset it. We also change Clear() to unset |should_activate_when_ready_| which
makes sense for any callsite of Clear().

Minor simplification in anticipation of changes for https://crbug.com/616331
Refactoring only; no behavior change.

BUG=616331

Committed: https://crrev.com/6c74874b9c6969a2afc6f195c96a1a3b15cda890
Cr-Commit-Position: refs/heads/master@{#403650}
==========

[commit-bot: I haz the power, 2016-07-04 08:50:28]

Patchset 1 (id:??) landed as
https://crrev.com/6c74874b9c6969a2afc6f195c96a1a3b15cda890
Cr-Commit-Position: refs/heads/master@{#403650}

[falken, 2016-07-04 10:39:09]

On 2016/07/04 08:39:05, nhiroki wrote:
>
https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
> File content/browser/service_worker/service_worker_registration.cc (right):
> 
>
https://codereview.chromium.org/2115393002/diff/1/content/browser/service_wor...
> content/browser/service_worker/service_worker_registration.cc:430: void
> ServiceWorkerRegistration::Clear() {
> On 2016/07/04 08:14:37, falken wrote:
> > On 2016/07/04 07:53:38, nhiroki wrote:
> > > How about checking pre-conditions before flipping |is_uninstalling_| and
> > > |is_uninstalled_|?
> > > 
> > > DCHECK(is_uninstalling_);
> > > DCHECK(!is_uninstalled_);
> > 
> > Unfortunately I'm not 100% sure those hold. Since this patch doesn't change
> when
> > Clear() is called, I'll leave out the dchecks in this patch.
> 
> Acknowledged.
> 
> > In the future, these uninstalling/uninstalled/deleting flags should be
merged
> > into a single Status enum.
> 
> It's great to merge them! Are you going to try it? :)
> (I tried it before and gave up because of complex state transitions...)

Yea I plan to do this. It might lead to removing some complex code we no longer
need.