Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
Committed: https://crrev.com/e545dcd96514138d5bef849b4a1a44030fa539d8
Cr-Commit-Position: refs/heads/master@{#413136}

[horo, 2016-08-16 05:49:51]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 05:50:06]

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

[commit-bot: I haz the power, 2016-08-16 06:10:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-16 06:10:03]

Dry run: Exceeded global retry quota

[horo, 2016-08-17 05:13:40]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 05:13:56]

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

[horo, 2016-08-17 05:42:21]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 05:42:41]

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

[horo, 2016-08-17 06:22:45]

Patchset #2 (id:20001) has been deleted

[horo, 2016-08-17 06:22:48]

Patchset #1 (id:1) has been deleted

[horo, 2016-08-17 06:26:04]

Description was changed from

==========
use optional for has_fetch_handler

BUG=
==========

to

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only when the install event
has been executed in a new version or when the install event has been executed
in a new version.

BUG=
==========

[horo, 2016-08-17 06:26:22]

Description was changed from

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only when the install event
has been executed in a new version or when the install event has been executed
in a new version.

BUG=
==========

to

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when the install event has been executed in a new version.

BUG=
==========

[horo, 2016-08-17 06:27:07]

Description was changed from

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when the install event has been executed in a new version.

BUG=
==========

to

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

BUG=
==========

[horo, 2016-08-17 06:35:45]

Description was changed from

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

BUG=
==========

to

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

[horo, 2016-08-17 06:40:38]

horo@chromium.org changed reviewers:
+ shimazu@chromium.org

[horo, 2016-08-17 06:40:39]

shimazu@
Could you please review?

[commit-bot: I haz the power, 2016-08-17 07:39:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 07:39:05]

Dry run: This issue passed the CQ dry run.

[shimazu, 2016-08-18 04:47:03]

https://codereview.chromium.org/2251633002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2251633002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:909:
InstallTestHelper("/service_worker/fetch_event.js", SERVICE_WORKER_OK);
How about adding EXPECT_EQ(base::nullopt, version_->has_fetch_handler()) before
installing?

[Marijn Kruisselbrink, 2016-08-18 05:01:36]

Just a drive-by comment, but is an base::Optional<bool> really the best thing to
use here? It results in some really awkward code, like if (has_fetch_handler())
{ } being considered "true" when there is no fetch handler. Wouldn't it make
more sense to just use an enum with three values rather than trying to encode
the three values with base::Optional<bool>. That way at least at call-sites it
will be clearer what checks are actually being done.
And if you decide to do go ahead with a base::Optional<bool>, you'll definitely
need to rename has_fetch_handler(), as that name just too strongly implies that
the return value is true/false depending on if a fetch handler is present,
rather than true-ish regardless of presence of a fetch handler.

[horo, 2016-08-18 07:51:41]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 07:51:55]

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

[horo, 2016-08-18 08:03:17]

On 2016/08/18 05:01:36, Marijn Kruisselbrink wrote:
> Just a drive-by comment, but is an base::Optional<bool> really the best thing
to
> use here? It results in some really awkward code, like if
(has_fetch_handler())
> { } being considered "true" when there is no fetch handler. Wouldn't it make
> more sense to just use an enum with three values rather than trying to encode
> the three values with base::Optional<bool>. That way at least at call-sites it
> will be clearer what checks are actually being done.
> And if you decide to do go ahead with a base::Optional<bool>, you'll
definitely
> need to rename has_fetch_handler(), as that name just too strongly implies
that
> the return value is true/false depending on if a fetch handler is present,
> rather than true-ish regardless of presence of a fetch handler.

Using an enum with three values sounds good to me.
I uploaded Patch Set 3 using the enum.

[horo, 2016-08-18 08:03:27]

https://codereview.chromium.org/2251633002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2251633002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:909:
InstallTestHelper("/service_worker/fetch_event.js", SERVICE_WORKER_OK);
On 2016/08/18 04:47:03, shimazu wrote:
> How about adding EXPECT_EQ(base::nullopt, version_->has_fetch_handler())
before
> installing?

Done by using tri-state enum.

[horo, 2016-08-18 08:05:25]

Description was changed from

==========
Use base::Optional<bool> for ServiceWorkerVersion::has_fetch_handler_

To make sure that set_has_fetch_handler() is called only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

to

==========
Use tri-state enum (UNKNOWN,EXISTS,DOES_NOT_EXIST) for FetchHandlerStatus

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

[commit-bot: I haz the power, 2016-08-18 08:22:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 08:22:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-08-18 08:28:20]

Description was changed from

==========
Use tri-state enum (UNKNOWN,EXISTS,DOES_NOT_EXIST) for FetchHandlerStatus

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

to

==========
Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

[horo, 2016-08-18 13:38:54]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 13:39:12]

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

[commit-bot: I haz the power, 2016-08-18 13:55:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 13:55:10]

Dry run: This issue passed the CQ dry run.

[horo, 2016-08-18 14:42:47]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 14:43:09]

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

[commit-bot: I haz the power, 2016-08-18 17:24:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 17:24:51]

Dry run: This issue passed the CQ dry run.

[shimazu, 2016-08-19 02:14:36]

I also feel enum is clearer, too. Thanks:)

https://codereview.chromium.org/2251633002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2251633002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:148: FetchHandlerStatus
fetch_handler_status() const {
I think Status is a bit ambiguous...
How about Existence (fetch_handler_existence(), FetchHandlerExistence)?

[horo, 2016-08-19 05:54:56]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 05:55:18]

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

[horo, 2016-08-19 07:53:14]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 07:53:27]

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

[commit-bot: I haz the power, 2016-08-19 08:02:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 08:02:45]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2016-08-19 08:04:07]

Patchset #5 (id:120001) has been deleted

[horo, 2016-08-19 08:04:53]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 08:05:17]

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

[horo, 2016-08-19 08:07:02]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 08:07:14]

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

[horo, 2016-08-19 08:48:05]

https://codereview.chromium.org/2251633002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/2251633002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_version.h:148: FetchHandlerStatus
fetch_handler_status() const {
On 2016/08/19 02:14:36, shimazu wrote:
> I think Status is a bit ambiguous...
> How about Existence (fetch_handler_existence(), FetchHandlerExistence)?

Done.

[shimazu, 2016-08-19 08:54:30]

lgtm

[commit-bot: I haz the power, 2016-08-19 09:55:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 09:55:46]

Dry run: This issue passed the CQ dry run.

[horo, 2016-08-19 14:20:08]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-08-19 14:20:23]

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

[commit-bot: I haz the power, 2016-08-19 14:25:46]

Description was changed from

==========
Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

to

==========
Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

[commit-bot: I haz the power, 2016-08-19 14:25:50]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-08-19 14:27:51]

Description was changed from

==========
Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209
==========

to

==========
Use tri-state enum for the existence of Fetch Handler in ServiceWorkerVersion.

To make sure that the status of fetch handler is set only
when the install event has been executed in a new version or
when an installed version is loaded from the storage.

https://codereview.chromium.org/2250623003/ depends on this CL.

BUG=561209

Committed: https://crrev.com/e545dcd96514138d5bef849b4a1a44030fa539d8
Cr-Commit-Position: refs/heads/master@{#413136}
==========

[commit-bot: I haz the power, 2016-08-19 14:27:52]

Patchset 5 (id:??) landed as
https://crrev.com/e545dcd96514138d5bef849b4a1a44030fa539d8
Cr-Commit-Position: refs/heads/master@{#413136}