[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None

Review-Url: https://codereview.chromium.org/2606303002
Cr-Commit-Position: refs/heads/master@{#442822}
Committed: https://chromium.googlesource.com/chromium/src/+/e2f14475e68deef7b541c5454c4a340112a7378a

[leonhsl(Using Gerrit), 2017-01-03 06:33:06]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 06:33:11]

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, 2017-01-03 06:44:05]

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

[commit-bot: I haz the power, 2017-01-03 06:44:05]

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

[xiaofengzhang, 2017-01-03 07:06:40]

Patchset #1 (id:1) has been deleted

[leonhsl(Using Gerrit), 2017-01-03 07:15:19]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 07:15:26]

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

[xiaofengzhang, 2017-01-03 08:27:57]

Patchset #2 (id:40001) has been deleted

[xiaofengzhang, 2017-01-03 08:31:18]

Description was changed from

==========
[ServiceWorker] Optimize the code in ServiceWorkerContextClient

This CL includes two optimizations:
  Avoiding multiple calls to the same method.
  Use DCHECK() as assertion.

BUG=None
==========

to

==========
[ServiceWorker] Optimize the code in ServiceWorkerContextClient

This CL includes two optimizations:
  Avoiding multiple calls to the same method.
  Use DCHECK() as assertion.

BUG=None
==========

[xiaofengzhang, 2017-01-03 08:31:18]

xiaofeng.zhang@intel.com changed reviewers:
+ falken@chromium.org, horo@chromium.org

[xiaofengzhang, 2017-01-03 08:33:42]

Hi falken@, horo@
Could you please help to review? Thanks.

[falken, 2017-01-05 01:53:00]

Thanks for your contribution!

I have some questions in-line.

Also, can you clarify what you mean by optimization by avoiding calling the same
function multiple times? Of course at runtime since it's an if/else the function
is only called once. I suppose the compiler might emit shorter code if it's
written in the way of the patch though. This all seems like more of a style
change vs a significant optimization though.

https://codereview.chromium.org/2606303002/diff/20001/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/2606303002/diff/20001/AUTHORS#newcode731
AUTHORS:731: Xiaofeng Zhang <xiaofeng.zhang@intel.com>
Note: CLA confirmed.

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:689:
base::Time::FromDoubleT(event_dispatch_time));
If we're rewriting this code I'd actually prefer it to be a switch statement so
that if WebServiceWorkerEventResult gains a new type we'll catch it.

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
How do you know this DCHECK is safe? I assume the if (!callback) was written for
a reason.

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:775:
DCHECK(callback);
Ditto.

[xiaofengzhang, 2017-01-05 02:02:13]

On 2017/01/05 01:53:00, falken wrote:
> Thanks for your contribution!
> 
> I have some questions in-line.
> 
> Also, can you clarify what you mean by optimization by avoiding calling the
same
> function multiple times? Of course at runtime since it's an if/else the
function
> is only called once. I suppose the compiler might emit shorter code if it's
> written in the way of the patch though. This all seems like more of a style
> change vs a significant optimization though.
> 

yeah, thanks, it is indeed is a style change, I just think the change is better
than before. I can change the patch title.

> https://codereview.chromium.org/2606303002/diff/20001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/2606303002/diff/20001/AUTHORS#newcode731
> AUTHORS:731: Xiaofeng Zhang <mailto:xiaofeng.zhang@intel.com>
> Note: CLA confirmed.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:689:
> base::Time::FromDoubleT(event_dispatch_time));
> If we're rewriting this code I'd actually prefer it to be a switch statement
so
> that if WebServiceWorkerEventResult gains a new type we'll catch it.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:734:
> DCHECK(callback);
> How do you know this DCHECK is safe? I assume the if (!callback) was written
for
> a reason.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:775:
> DCHECK(callback);
> Ditto.

[xiaofengzhang, 2017-01-05 02:07:49]

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:689:
base::Time::FromDoubleT(event_dispatch_time));
On 2017/01/05 01:53:00, falken wrote:
> If we're rewriting this code I'd actually prefer it to be a switch statement
so
> that if WebServiceWorkerEventResult gains a new type we'll catch it.

yes, agree, the best option would be to use a switch() if there will be a new
type.
But I just want keep current unification with less changes. Do you think it's
need to use switch currently for all such codes?

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/05 01:53:00, falken wrote:
> How do you know this DCHECK is safe? I assume the if (!callback) was written
for
> a reason.

thanks a lot for your kindly review.
This is come from the discussion in  https://codereview.chromium.org/2534403002,
see #34, horo's comments .

[falken, 2017-01-05 02:20:28]

Thank you for the reply. Yep, I'd prefer the patch description be revised to
de-emphasize "optimization".

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:689:
base::Time::FromDoubleT(event_dispatch_time));
On 2017/01/05 02:07:49, xiaofeng.zhang wrote:
> On 2017/01/05 01:53:00, falken wrote:
> > If we're rewriting this code I'd actually prefer it to be a switch statement
> so
> > that if WebServiceWorkerEventResult gains a new type we'll catch it.
> 
> yes, agree, the best option would be to use a switch() if there will be a new
> type.
> But I just want keep current unification with less changes. Do you think it's
> need to use switch currently for all such codes?

Yes, we should change to switch everywhere here. It'll set a good example for
future code.

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> On 2017/01/05 01:53:00, falken wrote:
> > How do you know this DCHECK is safe? I assume the if (!callback) was written
> for
> > a reason.
> 
> thanks a lot for your kindly review.
> This is come from the discussion in 
https://codereview.chromium.org/2534403002,
> see #34, horo's comments .

OK I'll defer to horo/shimazu. We also should confirm changing
didHandleSyncEvent is correct. I agree that if the callback is never null, we
should do a DCHECK rather than if-return.

[xiaofengzhang, 2017-01-05 02:58:10]

xiaofeng.zhang@intel.com changed reviewers:
+ iclelland@chromium.org

[xiaofengzhang, 2017-01-05 02:58:11]

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/05 02:20:28, falken wrote:
> On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > On 2017/01/05 01:53:00, falken wrote:
> > > How do you know this DCHECK is safe? I assume the if (!callback) was
written
> > for
> > > a reason.
> > 
> > thanks a lot for your kindly review.
> > This is come from the discussion in 
> https://codereview.chromium.org/2534403002,
> > see #34, horo's comments .
> 
> OK I'll defer to horo/shimazu. We also should confirm changing
> didHandleSyncEvent is correct. I agree that if the callback is never null, we
> should do a DCHECK rather than if-return.

@iclelland
How do you think this change?

[horo, 2017-01-05 03:00:14]

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/05 02:20:28, falken wrote:
> On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > On 2017/01/05 01:53:00, falken wrote:
> > > How do you know this DCHECK is safe? I assume the if (!callback) was
written
> > for
> > > a reason.
> > 
> > thanks a lot for your kindly review.
> > This is come from the discussion in 
> https://codereview.chromium.org/2534403002,
> > see #34, horo's comments .
> 
> OK I'll defer to horo/shimazu. We also should confirm changing
> didHandleSyncEvent is correct. I agree that if the callback is never null, we
> should do a DCHECK rather than if-return.

l-g-t-m for DCHECKs.

[leonhsl(Using Gerrit), 2017-01-06 05:23:25]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 05:23:38]

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

[xiaofengzhang, 2017-01-06 05:27:08]

Description was changed from

==========
[ServiceWorker] Optimize the code in ServiceWorkerContextClient

This CL includes two optimizations:
  Avoiding multiple calls to the same method.
  Use DCHECK() as assertion.

BUG=None
==========

to

==========
[ServiceWorker] Change to use switch statement in ServiceWorkerContextClient

Use switch is the best option here to get compile time verification, and it
will become significant if a third entry in WebServiceWorkerEventResult gets
added.

Here also replace some if-return with DCHECK.

BUG=None
==========

[xiaofengzhang, 2017-01-06 05:35:29]

Description was changed from

==========
[ServiceWorker] Change to use switch statement in ServiceWorkerContextClient

Use switch is the best option here to get compile time verification, and it
will become significant if a third entry in WebServiceWorkerEventResult gets
added.

Here also replace some if-return with DCHECK.

BUG=None
==========

to

==========
[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None
==========

[xiaofengzhang, 2017-01-06 05:45:33]

Description was changed from

==========
[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None
==========

to

==========
[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None
==========

[commit-bot: I haz the power, 2017-01-06 05:56:24]

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

[commit-bot: I haz the power, 2017-01-06 05:56:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[leonhsl(Using Gerrit), 2017-01-06 06:19:34]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 06:19:52]

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, 2017-01-06 07:41:24]

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

[commit-bot: I haz the power, 2017-01-06 07:41:25]

Dry run: This issue passed the CQ dry run.

[xiaofengzhang, 2017-01-06 08:10:56]

Hi falken

Uploaded ps#2 and ps#3 to address your comments, PTAL, Thanks.

On 2017/01/05 01:53:00, falken wrote:
> Thanks for your contribution!
> 
> I have some questions in-line.
> 
> Also, can you clarify what you mean by optimization by avoiding calling the
same
> function multiple times? Of course at runtime since it's an if/else the
function
> is only called once. I suppose the compiler might emit shorter code if it's
> written in the way of the patch though. This all seems like more of a style
> change vs a significant optimization though.
> 
> https://codereview.chromium.org/2606303002/diff/20001/AUTHORS
> File AUTHORS (right):
> 
> https://codereview.chromium.org/2606303002/diff/20001/AUTHORS#newcode731
> AUTHORS:731: Xiaofeng Zhang <mailto:xiaofeng.zhang@intel.com>
> Note: CLA confirmed.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:689:
> base::Time::FromDoubleT(event_dispatch_time));
> If we're rewriting this code I'd actually prefer it to be a switch statement
so
> that if WebServiceWorkerEventResult gains a new type we'll catch it.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:734:
> DCHECK(callback);
> How do you know this DCHECK is safe? I assume the if (!callback) was written
for
> a reason.
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:775:
> DCHECK(callback);
> Ditto.

[falken, 2017-01-06 08:27:32]

https://codereview.chromium.org/2606303002/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:692: }
Since this is repeated in other functions, how about a helper function like
EventResultToStatus(). It could also get around the need to initialize with OK:

switch (result) {
  case blink::WebServiceWorkerEventResultCompleted:
    return SERVICE_WORKER_OK;
  case blink::WebServiceWorkerEventResultRejected:
    return SERVICE_WORKER_ERROR_EVENT_WAIT_UNTIL_REJECTED;
}
NOTREACHED() << result;
return SERVICE_WORKER_ERROR_FAILED;

[iclelland, 2017-01-06 18:07:49]

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/05 03:00:14, horo wrote:
> On 2017/01/05 02:20:28, falken wrote:
> > On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > > On 2017/01/05 01:53:00, falken wrote:
> > > > How do you know this DCHECK is safe? I assume the if (!callback) was
> written
> > > for
> > > > a reason.
> > > 
> > > thanks a lot for your kindly review.
> > > This is come from the discussion in 
> > https://codereview.chromium.org/2534403002,
> > > see #34, horo's comments .
> > 
> > OK I'll defer to horo/shimazu. We also should confirm changing
> > didHandleSyncEvent is correct. I agree that if the callback is never null,
we
> > should do a DCHECK rather than if-return.
> 
> l-g-t-m for DCHECKs.

Agreed -- it would be a bug here if callback was null (this would mean that the
event was somehow handled twice, or the event id was bogus). DCHECK will crash
in debug builds, but the immediate use should also cause the renderer to crash
in release builds.

My only concern would be that such bugs might be difficult to find in tests --
especially if it's the result of a race -- will this make the browser less
stable?

[xiaofengzhang, 2017-01-09 02:55:22]

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:734:
DCHECK(callback);
On 2017/01/06 18:07:49, iclelland wrote:
> On 2017/01/05 03:00:14, horo wrote:
> > On 2017/01/05 02:20:28, falken wrote:
> > > On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > > > On 2017/01/05 01:53:00, falken wrote:
> > > > > How do you know this DCHECK is safe? I assume the if (!callback) was
> > written
> > > > for
> > > > > a reason.
> > > > 
> > > > thanks a lot for your kindly review.
> > > > This is come from the discussion in 
> > > https://codereview.chromium.org/2534403002,
> > > > see #34, horo's comments .
> > > 
> > > OK I'll defer to horo/shimazu. We also should confirm changing
> > > didHandleSyncEvent is correct. I agree that if the callback is never null,
> we
> > > should do a DCHECK rather than if-return.
> > 
> > l-g-t-m for DCHECKs.
> 
> Agreed -- it would be a bug here if callback was null (this would mean that
the
> event was somehow handled twice, or the event id was bogus). DCHECK will crash
> in debug builds, but the immediate use should also cause the renderer to crash
> in release builds.
> 
> My only concern would be that such bugs might be difficult to find in tests --
> especially if it's the result of a race -- will this make the browser less
> stable?

Thanks iclelland for your kindly review.
For "it would be a bug here if callback was null", have you ever met such bug
before? In which case it would happens?

https://codereview.chromium.org/2606303002/diff/80001/content/renderer/servic...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:692: }
On 2017/01/06 08:27:32, falken wrote:
> Since this is repeated in other functions, how about a helper function like
> EventResultToStatus(). It could also get around the need to initialize with
OK:
> 
> switch (result) {
>   case blink::WebServiceWorkerEventResultCompleted:
>     return SERVICE_WORKER_OK;
>   case blink::WebServiceWorkerEventResultRejected:
>     return SERVICE_WORKER_ERROR_EVENT_WAIT_UNTIL_REJECTED;
> }
> NOTREACHED() << result;
> return SERVICE_WORKER_ERROR_FAILED;
>    

Acknowledged.

https://codereview.chromium.org/2606303002/diff/80001/content/renderer/servic...
content/renderer/service_worker/service_worker_context_client.cc:692: }
On 2017/01/06 08:27:32, falken wrote:
> Since this is repeated in other functions, how about a helper function like
> EventResultToStatus(). It could also get around the need to initialize with
OK:
> 
> switch (result) {
>   case blink::WebServiceWorkerEventResultCompleted:
>     return SERVICE_WORKER_OK;
>   case blink::WebServiceWorkerEventResultRejected:
>     return SERVICE_WORKER_ERROR_EVENT_WAIT_UNTIL_REJECTED;
> }
> NOTREACHED() << result;
> return SERVICE_WORKER_ERROR_FAILED;
>    

Nice, thanks

[falken, 2017-01-09 13:30:29]

On 2017/01/06 18:07:49, iclelland wrote:
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> content/renderer/service_worker/service_worker_context_client.cc:734:
> DCHECK(callback);
> On 2017/01/05 03:00:14, horo wrote:
> > On 2017/01/05 02:20:28, falken wrote:
> > > On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > > > On 2017/01/05 01:53:00, falken wrote:
> > > > > How do you know this DCHECK is safe? I assume the if (!callback) was
> > written
> > > > for
> > > > > a reason.
> > > > 
> > > > thanks a lot for your kindly review.
> > > > This is come from the discussion in 
> > > https://codereview.chromium.org/2534403002,
> > > > see #34, horo's comments .
> > > 
> > > OK I'll defer to horo/shimazu. We also should confirm changing
> > > didHandleSyncEvent is correct. I agree that if the callback is never null,
> we
> > > should do a DCHECK rather than if-return.
> > 
> > l-g-t-m for DCHECKs.
> 
> Agreed -- it would be a bug here if callback was null (this would mean that
the
> event was somehow handled twice, or the event id was bogus). DCHECK will crash
> in debug builds, but the immediate use should also cause the renderer to crash
> in release builds.
> 
> My only concern would be that such bugs might be difficult to find in tests --
> especially if it's the result of a race -- will this make the browser less
> stable?

It’s a judgement call but in general I’d opt for the DCHECK over the if-return
for an “it’d be a bug” situation like this. The DCHECK is more likely to surface
a bug since we’d get the crash report for release builds (and there’s been some
plans to enable DCHECKs on Canary). The if-return also is confusing
documentation since one is left guessing whether it’s legitimately possible or
written just in case of a bug.

[iclelland, 2017-01-09 15:55:15]

On 2017/01/09 13:30:29, falken wrote:
> On 2017/01/06 18:07:49, iclelland wrote:
> >
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> > File content/renderer/service_worker/service_worker_context_client.cc
(right):
> > 
> >
>
https://codereview.chromium.org/2606303002/diff/20001/content/renderer/servic...
> > content/renderer/service_worker/service_worker_context_client.cc:734:
> > DCHECK(callback);
> > On 2017/01/05 03:00:14, horo wrote:
> > > On 2017/01/05 02:20:28, falken wrote:
> > > > On 2017/01/05 02:07:48, xiaofeng.zhang wrote:
> > > > > On 2017/01/05 01:53:00, falken wrote:
> > > > > > How do you know this DCHECK is safe? I assume the if (!callback) was
> > > written
> > > > > for
> > > > > > a reason.
> > > > > 
> > > > > thanks a lot for your kindly review.
> > > > > This is come from the discussion in 
> > > > https://codereview.chromium.org/2534403002,
> > > > > see #34, horo's comments .
> > > > 
> > > > OK I'll defer to horo/shimazu. We also should confirm changing
> > > > didHandleSyncEvent is correct. I agree that if the callback is never
null,
> > we
> > > > should do a DCHECK rather than if-return.
> > > 
> > > l-g-t-m for DCHECKs.
> > 
> > Agreed -- it would be a bug here if callback was null (this would mean that
> the
> > event was somehow handled twice, or the event id was bogus). DCHECK will
crash
> > in debug builds, but the immediate use should also cause the renderer to
crash
> > in release builds.
> > 
> > My only concern would be that such bugs might be difficult to find in tests
--
> > especially if it's the result of a race -- will this make the browser less
> > stable?
> 
> It’s a judgement call but in general I’d opt for the DCHECK over the if-return
> for an “it’d be a bug” situation like this. The DCHECK is more likely to
surface
> a bug since we’d get the crash report for release builds (and there’s been
some
> plans to enable DCHECKs on Canary). The if-return also is confusing
> documentation since one is left guessing whether it’s legitimately possible or
> written just in case of a bug.

Agreed, then -- DCHECK L G T M.

[leonhsl(Using Gerrit), 2017-01-10 04:37:03]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 04:38:15]

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

[falken, 2017-01-10 06:34:04]

Not sure this is ready yet but it caught my eye. Don't feel obligated to wait
for the CQ dry run to finish before requesting review, if there's an error the
CQ will catch it before committing.

BTW service_worker_context_client.h looks like it failed to upload, there is an
error "chunk mismatch". I think I've seen corporate proxies interfere with the
upload before like this.

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:893:
blink::WebServiceWorkerEventResult result) {
This could be a nonmember function in an unnamed namespace since it's not using
anything from ServiceWorkerContextClient.

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:900:
NOTREACHED() << "Got stray result: " << result;
"stray result" is a bit weird. "invalid result"

[commit-bot: I haz the power, 2017-01-10 06:39:50]

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

[commit-bot: I haz the power, 2017-01-10 06:39:51]

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

[xiaofengzhang, 2017-01-10 07:07:07]

On 2017/01/10 06:34:04, falken wrote:
> Not sure this is ready yet but it caught my eye. Don't feel obligated to wait
> for the CQ dry run to finish before requesting review, if there's an error the
> CQ will catch it before committing.
Thanks for you kindly reminding.

> BTW service_worker_context_client.h looks like it failed to upload, there is
an
> error "chunk mismatch". I think I've seen corporate proxies interfere with the
> upload before like this.
>
Sorry, my fault.
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.cc:893:
> blink::WebServiceWorkerEventResult result) {
> This could be a nonmember function in an unnamed namespace since it's not
using
> anything from ServiceWorkerContextClient.
> 
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.cc:900:
> NOTREACHED() << "Got stray result: " << result;
> "stray result" is a bit weird. "invalid result"

[xiaofengzhang, 2017-01-10 07:07:20]

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:893:
blink::WebServiceWorkerEventResult result) {
On 2017/01/10 06:34:03, falken wrote:
> This could be a nonmember function in an unnamed namespace since it's not
using
> anything from ServiceWorkerContextClient.

Acknowledged.

https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:900:
NOTREACHED() << "Got stray result: " << result;
On 2017/01/10 06:34:03, falken wrote:
> "stray result" is a bit weird. "invalid result"

Acknowledged.

[xiaofengzhang, 2017-01-10 08:49:30]

On 2017/01/10 07:07:20, xiaofengzhang wrote:
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> File content/renderer/service_worker/service_worker_context_client.cc (right):
> 
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.cc:893:
> blink::WebServiceWorkerEventResult result) {
> On 2017/01/10 06:34:03, falken wrote:
> > This could be a nonmember function in an unnamed namespace since it's not
> using
> > anything from ServiceWorkerContextClient.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2606303002/diff/100001/content/renderer/servi...
> content/renderer/service_worker/service_worker_context_client.cc:900:
> NOTREACHED() << "Got stray result: " << result;
> On 2017/01/10 06:34:03, falken wrote:
> > "stray result" is a bit weird. "invalid result"
> 
> Acknowledged.

@falken, please help to review again, thanks

[xiaofengzhang, 2017-01-10 11:01:28]

The CQ bit was checked by xiaofeng.zhang@intel.com to run a CQ dry run

[xiaofengzhang, 2017-01-10 11:01:41]

The CQ bit was unchecked by xiaofeng.zhang@intel.com

[falken, 2017-01-10 13:37:32]

lgtm, thanks for sticking with this patch.

[leonhsl(Using Gerrit), 2017-01-11 03:14:21]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 03:15:10]

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, 2017-01-11 04:01:00]

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

[commit-bot: I haz the power, 2017-01-11 04:01:01]

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

[xiaofengzhang, 2017-01-11 05:02:50]

The CQ bit was checked by xiaofeng.zhang@intel.com

[commit-bot: I haz the power, 2017-01-11 05:03:16]

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, 2017-01-11 06:18:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484110970615570,
"parent_rev": "9b3dd2bc8783876fdf3d9e242b4240a3537d19e2", "commit_rev":
"e2f14475e68deef7b541c5454c4a340112a7378a"}

[commit-bot: I haz the power, 2017-01-11 06:19:16]

Description was changed from

==========
[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None
==========

to

==========
[ServiceWorker] Some code clean-up for content::ServiceWorkerContextClient

Using switch is the best option here to get compile time verification, and
we can catch it if WebServiceWorkerEventResult gains a new type later.

Here also replace some if-return with DCHECK.

BUG=None

Review-Url: https://codereview.chromium.org/2606303002
Cr-Commit-Position: refs/heads/master@{#442822}
Committed:
https://chromium.googlesource.com/chromium/src/+/e2f14475e68deef7b541c5454c4a...
==========

[commit-bot: I haz the power, 2017-01-11 06:19:18]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/e2f14475e68deef7b541c5454c4a...