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

content/renderer/service_worker/service_worker_context_client.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/service_worker/service_worker_context_client.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/lazy_instance.h"
@@ skipping 665 matching lines @@
       base::Time::FromDoubleT(event_dispatch_time)));
 }
 
 void ServiceWorkerContextClient::didHandleExtendableMessageEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
     double event_dispatch_time) {
   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);
 }
 
 void ServiceWorkerContextClient::didHandleInstallEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
     double event_dispatch_time) {
   Send(new ServiceWorkerHostMsg_InstallEventFinished(
       GetRoutingID(), request_id, result, proxy_->hasFetchEventHandler(),
       base::Time::FromDoubleT(event_dispatch_time)));
@@ skipping 24 matching lines @@
     blink::WebServiceWorkerEventResult result,
     double event_dispatch_time) {
   if (context_->preload_requests.Lookup(fetch_event_id)) {
     // Deletes NavigationPreloadRequest. If the network request is ongoing, it
     // will be canceled by deleting the mojom::URLLoaderPtr in the
     // NavigationPreloadRequest.
     context_->preload_requests.Remove(fetch_event_id);
   }
   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,
                 base::Time::FromDoubleT(event_dispatch_time));
   context_->fetch_event_callbacks.Remove(fetch_event_id);
 }
 
 void ServiceWorkerContextClient::didHandleNotificationClickEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
@@ skipping 20 matching lines @@
       GetRoutingID(), request_id, result,
       base::Time::FromDoubleT(event_dispatch_time)));
 }
 
 void ServiceWorkerContextClient::didHandleSyncEvent(
     int request_id,
     blink::WebServiceWorkerEventResult result,
     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);
 }
 
 blink::WebServiceWorkerNetworkProvider*
 ServiceWorkerContextClient::createServiceWorkerNetworkProvider(
     blink::WebDataSource* data_source) {
   DCHECK(main_thread_task_runner_->RunsTasksOnCurrentThread());
 
   // Create a content::ServiceWorkerNetworkProvider for this data source so
   // we can observe its requests.
@@ skipping 481 matching lines @@
 }
 
 base::WeakPtr<ServiceWorkerContextClient>
 ServiceWorkerContextClient::GetWeakPtr() {
   DCHECK(worker_task_runner_->RunsTasksOnCurrentThread());
   DCHECK(context_);
   return context_->weak_factory.GetWeakPtr();
 }
 
 }  // namespace content