ServiceWorker: Convert ServiceWorkerDatabase::Status to string for logging

content/browser/service_worker/service_worker_storage.cc

Index: content/browser/service_worker/service_worker_storage.cc
diff --git a/content/browser/service_worker/service_worker_storage.cc b/content/browser/service_worker/service_worker_storage.cc
index 54eacd657f51bf4593d953a1f8bdf61a97cf6656..63cb08df542d8e09271a876cc0abf9298bc07375 100644
--- a/content/browser/service_worker/service_worker_storage.cc
+++ b/content/browser/service_worker/service_worker_storage.cc
@@ -778,7 +778,7 @@ void ServiceWorkerStorage::DidFindRegistrationForDocument(
         "ServiceWorker",
         "ServiceWorkerStorage::FindRegistrationForDocument",
         base::Hash(document_url.spec()),
-        "Status", "OK");
+        "Status", ServiceWorkerDatabase::StatusToString(status));
     return;
   }
 
@@ -786,14 +786,16 @@ void ServiceWorkerStorage::DidFindRegistrationForDocument(
     // 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;
   }
 
@@ -804,7 +806,7 @@ void ServiceWorkerStorage::DidFindRegistrationForDocument(
       "ServiceWorker",
       "ServiceWorkerStorage::FindRegistrationForDocument",
       base::Hash(document_url.spec()),
-      "Status", status);
+      "Status", ServiceWorkerDatabase::StatusToString(status));
 }
 
 void ServiceWorkerStorage::DidFindRegistrationForPattern(