ServiceWorker: Convert ServiceWorkerDatabase::Status to string for logging

ServiceWorker: Convert status code to string for logging

BUG=N/A
TEST=N/A

Committed: https://crrev.com/5592d09963bdcb5069e6f98dca89d8070276a286
Cr-Commit-Position: refs/heads/master@{#300061}

[shimazu, 2014-10-14 10:08:37]

shimazu@chromium.org changed reviewers:
+ nhiroki@chromium.org

[shimazu, 2014-10-14 10:08:39]

PTAL

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

Looks good overall. You may need to rebase.

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:757: //
service_worker_database.cc.
Can you address this TODO?

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:798: installing_status
== SERVICE_WORKER_OK);
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 08:59:05]

Patchset #2 (id:20001) has been deleted

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

Thanks for your comments!
Please review again:)

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:757: //
service_worker_database.cc.
On 2014/10/16 04:21:07, nhiroki wrote:
> Can you address this TODO?

Done.

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:798: installing_status
== SERVICE_WORKER_OK);
On 2014/10/16 04:21:07, nhiroki wrote:
> 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/)

This looks good! I fixed it.

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

Let me add some more nits...

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:798: installing_status
== SERVICE_WORKER_OK);
On 2014/10/16 09:01:53, makoto wrote:
> On 2014/10/16 04:21:07, nhiroki wrote:
> > 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),

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

> >   "Info", "Found installing registration"
> > 
> > (fyi: "Info" label is used in an other CL:
> > https://codereview.chromium.org/637243003/)
> 
> This looks good! I fixed it.

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:282: "Status", (status
== SERVICE_WORKER_OK) ? "Found" : "Not Found");
Can you use |ServiceWorkerStatusToString(status)|?

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:815: "Found installing
registration" : "Not found any registrations");
nit: found -> find

[nhiroki, 2014-10-16 09:26:30]

REG: CL title, StatusToString() is now used not only for TRACE_EVENT but also
for other logging, so can you update it? (for example, "Convert
ServiceWorkerDatabase::Status to string for logging")

[shimazu, 2014-10-17 01:54:47]

Thanks!
Please review again:)

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:798: installing_status
== SERVICE_WORKER_OK);
On 2014/10/16 09:20:51, nhiroki wrote:
> On 2014/10/16 09:01:53, makoto wrote:
> > On 2014/10/16 04:21:07, nhiroki wrote:
> > > 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),
> 
> Oops... sorry, I meant |ServiceWorkerStatusToString(installing_status)|.
> |ServiceWorkerDatabase::StatusToString(status)| is NOT_FOUND even if we found
> one from |installing_registrations|.
> 
> > >   "Info", "Found installing registration"
> > > 
> > > (fyi: "Info" label is used in an other CL:
> > > https://codereview.chromium.org/637243003/)
> > 
> > This looks good! I fixed it.
> 

> |ServiceWorkerDatabase::StatusToString(status)| is NOT_FOUND even if we found
> one from |installing_registrations|.

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.".

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:282: "Status", (status
== SERVICE_WORKER_OK) ? "Found" : "Not Found");
On 2014/10/16 09:20:51, nhiroki wrote:
> Can you use |ServiceWorkerStatusToString(status)|?

Done.

https://codereview.chromium.org/654823002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:815: "Found installing
registration" : "Not found any registrations");
On 2014/10/16 09:20:51, nhiroki wrote:
> nit: found -> find

Done.

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

LGTM

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/654823002/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:798: installing_status
== SERVICE_WORKER_OK);
On 2014/10/17 01:54:46, makoto wrote:
> On 2014/10/16 09:20:51, nhiroki wrote:
> > On 2014/10/16 09:01:53, makoto wrote:
> > > On 2014/10/16 04:21:07, nhiroki wrote:
> > > > 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),
> > 
> > Oops... sorry, I meant |ServiceWorkerStatusToString(installing_status)|.
> > |ServiceWorkerDatabase::StatusToString(status)| is NOT_FOUND even if we
found
> > one from |installing_registrations|.
> > 
> > > >   "Info", "Found installing registration"
> > > > 
> > > > (fyi: "Info" label is used in an other CL:
> > > > https://codereview.chromium.org/637243003/)
> > > 
> > > This looks good! I fixed it.
> > 
> 
> > |ServiceWorkerDatabase::StatusToString(status)| is NOT_FOUND even if we
found
> > one from |installing_registrations|.
> 
> 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.".

(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 :)

[shimazu, 2014-10-17 03:28:29]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2014-10-17 03:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654823002/80001

[commit-bot: I haz the power, 2014-10-17 04:20:16]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-17 04:21:03]

Patchset 4 (id:??) landed as
https://crrev.com/5592d09963bdcb5069e6f98dca89d8070276a286
Cr-Commit-Position: refs/heads/master@{#300061}