ServiceWorker: Convert ServiceWorkerDatabase::Status to string for logging

content/browser/service_worker/service_worker_storage.cc

 // Copyright 2013 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/browser/service_worker/service_worker_storage.h"
 
 #include <string>
 
 #include "base/bind_helpers.h"
 #include "base/debug/trace_event.h"
@@ skipping 736 matching lines @@
   DCHECK_EQ(INITIALIZING, state_);
 
   if (status == ServiceWorkerDatabase::STATUS_OK) {
     next_registration_id_ = data->next_registration_id;
     next_version_id_ = data->next_version_id;
     next_resource_id_ = data->next_resource_id;
     registered_origins_.swap(data->origins);
     state_ = INITIALIZED;
   } else {
     // TODO(nhiroki): Stringify |status| using StatusToString() defined in
     // service_worker_database.cc.

[nhiroki, 2014/10/16 04:21:07]

Can you address this TODO?

[shimazu, 2014/10/16 09:01:53]

Done.

     DVLOG(2) << "Failed to initialize: " << status;
     ScheduleDeleteAndStartOver();
   }
 
   for (std::vector<base::Closure>::const_iterator it = pending_tasks_.begin();
        it != pending_tasks_.end(); ++it) {
     RunSoon(FROM_HERE, *it);
   }
   pending_tasks_.clear();
 }
 
 void ServiceWorkerStorage::DidFindRegistrationForDocument(
     const GURL& document_url,
     const FindRegistrationCallback& callback,
     const ServiceWorkerDatabase::RegistrationData& data,
     const ResourceList& resources,
     ServiceWorkerDatabase::Status status) {
   if (status == ServiceWorkerDatabase::STATUS_OK) {
     ReturnFoundRegistration(callback, data, resources);
     TRACE_EVENT_ASYNC_END1(
         "ServiceWorker",
         "ServiceWorkerStorage::FindRegistrationForDocument",
         base::Hash(document_url.spec()),
-        "Status", "OK");
+        "Status", ServiceWorkerDatabase::StatusToString(status));
     return;
   }
 
   if (status == ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND) {
     // Look for something currently being installed.
     scoped_refptr<ServiceWorkerRegistration> installing_registration =
         FindInstallingRegistrationForDocument(document_url);
-    callback.Run(installing_registration.get() ? SERVICE_WORKER_OK
-                                               : SERVICE_WORKER_ERROR_NOT_FOUND,
-                 installing_registration);
-    TRACE_EVENT_ASYNC_END1(
+    ServiceWorkerStatusCode installing_status = installing_registration.get() ?
+        SERVICE_WORKER_OK : SERVICE_WORKER_ERROR_NOT_FOUND;
+    callback.Run(installing_status, installing_registration);
+    TRACE_EVENT_ASYNC_END2(
         "ServiceWorker",
         "ServiceWorkerStorage::FindRegistrationForDocument",
         base::Hash(document_url.spec()),
-        "Status", status);
+        "Status", ServiceWorkerDatabase::StatusToString(status),
+        "Found installing registration",
+        installing_status == SERVICE_WORKER_OK);

[nhiroki, 2014/10/16 04:21:07]

It seems more natural that "Status" value corresponds to a status code passed to
the |callback.Run()|, and "Found installing registration" would slightly be too
specific label name, so how about this?

  "Status", ServiceWorkerStatusToString(status),
  "Info", "Found installing registration"

(fyi: "Info" label is used in an other CL:
https://codereview.chromium.org/637243003/)

[shimazu, 2014/10/16 09:01:53]

This looks good! I fixed it.

[nhiroki, 2014/10/16 09:20:51]

Oops... sorry, I meant |ServiceWorkerStatusToString(installing_status)|.
|ServiceWorkerDatabase::StatusToString(status)| is NOT_FOUND even if we found
one from |installing_registrations|.

[shimazu, 2014/10/17 01:54:46]

I think "Found/Not Found" is better than
|ServiceWorkerStatusToString(installing_status)| here because status must be
OK/NOT_FOUND, and "OK" means not only registration is found, but "installing"
registration is found.
In addition, I think users of about:tracing (most of them would be chromium
developers) can understand what happened if "Status" is NOT_FOUND and "Info" is
"Installing registration is found.".

[nhiroki, 2014/10/17 03:24:02]

(This comment is based on patchset 4)

Hmm... I guess both conversion functions return OK/NOT_FOUND in this case...?

This trace is for FindRegistrationForDocument(), so IMHO |status| would be the
final result of the function and "Info" could show the source of the
registration (ie. "Found from installing registration") if necessary.

Anyway, that seems to be just a matter of preference and it's okay to land this
as is :)

     return;
   }
 
   ScheduleDeleteAndStartOver();
   callback.Run(DatabaseStatusToStatusCode(status),
                scoped_refptr<ServiceWorkerRegistration>());
   TRACE_EVENT_ASYNC_END1(
       "ServiceWorker",
       "ServiceWorkerStorage::FindRegistrationForDocument",
       base::Hash(document_url.spec()),
-      "Status", status);
+      "Status", ServiceWorkerDatabase::StatusToString(status));
 }
 
 void ServiceWorkerStorage::DidFindRegistrationForPattern(
     const GURL& scope,
     const FindRegistrationCallback& callback,
     const ServiceWorkerDatabase::RegistrationData& data,
     const ResourceList& resources,
     ServiceWorkerDatabase::Status status) {
   if (status == ServiceWorkerDatabase::STATUS_OK) {
     ReturnFoundRegistration(callback, data, resources);
@@ skipping 602 matching lines @@
     // Give up the corruption recovery until the browser restarts.
     LOG(ERROR) << "Failed to delete the diskcache.";
     callback.Run(SERVICE_WORKER_ERROR_FAILED);
     return;
   }
   DVLOG(1) << "Deleted ServiceWorkerDiskCache successfully.";
   callback.Run(SERVICE_WORKER_OK);
 }
 
 }  // namespace content