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

content/renderer/service_worker/service_worker_context_client.cc

Index: content/renderer/service_worker/service_worker_context_client.cc
diff --git a/content/renderer/service_worker/service_worker_context_client.cc b/content/renderer/service_worker/service_worker_context_client.cc
index 715a35eab81fc59620d0caaf7ceeaedd7a0efd06..bb15820fa69e184148e928dfc57f48ae39347f91 100644
--- a/content/renderer/service_worker/service_worker_context_client.cc
+++ b/content/renderer/service_worker/service_worker_context_client.cc
@@ -683,13 +683,10 @@ void ServiceWorkerContextClient::didHandleExtendableMessageEvent(
   const DispatchExtendableMessageEventCallback* callback =
       context_->message_event_callbacks.Lookup(request_id);
   DCHECK(callback);
-  if (result == blink::WebServiceWorkerEventResultCompleted) {
-    callback->Run(SERVICE_WORKER_OK,
-                  base::Time::FromDoubleT(event_dispatch_time));
-  } else {
-    callback->Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
-                  base::Time::FromDoubleT(event_dispatch_time));
-  }
+  callback->Run(result == blink::WebServiceWorkerEventResultCompleted
+                    ? SERVICE_WORKER_OK
+                    : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
+                base::Time::FromDoubleT(event_dispatch_time));

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

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.

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

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?

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

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

   context_->message_event_callbacks.Remove(request_id);
 }
 
@@ -734,9 +731,7 @@ void ServiceWorkerContextClient::didHandleFetchEvent(
   }
   const FetchCallback* callback =
       context_->fetch_event_callbacks.Lookup(fetch_event_id);
-  if (!callback)
-    return;
-
+  DCHECK(callback);

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

How do you know this DCHECK is safe? I assume the if (!callback) was written for
a reason.

[xiaofengzhang, 2017/01/05 02:07:48]

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]

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:11]

@iclelland
How do you think this change?

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

l-g-t-m for DCHECKs.

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

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]

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?

   callback->Run(result == blink::WebServiceWorkerEventResultCompleted
                     ? SERVICE_WORKER_OK
                     : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
@@ -777,15 +772,11 @@ void ServiceWorkerContextClient::didHandleSyncEvent(
     double event_dispatch_time) {
   const SyncCallback* callback =
       context_->sync_event_callbacks.Lookup(request_id);
-  if (!callback)
-    return;
-  if (result == blink::WebServiceWorkerEventResultCompleted) {
-    callback->Run(SERVICE_WORKER_OK,
-                  base::Time::FromDoubleT(event_dispatch_time));
-  } else {
-    callback->Run(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
-                  base::Time::FromDoubleT(event_dispatch_time));
-  }
+  DCHECK(callback);

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

Ditto.

+  callback->Run(result == blink::WebServiceWorkerEventResultCompleted
+                    ? SERVICE_WORKER_OK
+                    : SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED,
+                base::Time::FromDoubleT(event_dispatch_time));
   context_->sync_event_callbacks.Remove(request_id);
 }