[ServiceWorkerCache] Cache and CacheStorage creation UMA

We're finding corrupted backends in the wild. As a first measurement
step, find out how often opens and creation fail.

Also refactored casing of an enum.

BUG=461654
Committed: https://crrev.com/f04de1ef2aa2bae922839ab62cb9f2efb8578a10
Cr-Commit-Position: refs/heads/master@{#320893}

[jkarlin, 2015-03-06 15:53:10]

michaeln: Please review for service_worker/
isherman: Please review histograms.xml

Thanks!

[michaeln, 2015-03-13 19:47:30]

On 2015/03/06 15:53:10, jkarlin wrote:
> michaeln: Please review for service_worker/
> isherman: Please review histograms.xml
> 
> Thanks!

Oh, just noticing this! Did you mean to send this out for review, there's nobody
in the reviewers list but the comment sure sounds like you did?

[jkarlin, 2015-03-13 19:51:22]

jkarlin@chromium.org changed reviewers:
+ isherman@chromium.org, michaeln@chromium.org

[jkarlin, 2015-03-13 19:51:22]

Somehow the reviewers didn't get added. Thanks for noticing michaeln!

[michaeln, 2015-03-13 22:47:16]

lgtm (ptal at the naming comments?) and fyi @jsbell

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache_storage.cc:617:
UMA_HISTOGRAM_BOOLEAN("ServiceWorkerCache.CreateCacheStorageResult",
Should the label be "CreateCacheResult"?

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache_storage.cc:634: bool
success) {
unrelated but why is 'success' not important here?

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33547: +    The value of creating
creating a new ServiceWorkerCacheStorage and its
In the code, it looks like this is the result of creating an individual named
cache (ServiceWorkerCache) instance, yet this comment makes it sounnd like its
the result of creating the cache storage area for an origin
(ServiceWorkerCacheStorage).

also nit: creating creating

[Ilya Sherman, 2015-03-13 22:58:20]

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:12: #include
"base/metrics/histogram.h"
Please include histogram_macros instead.

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1229: error,
ErrorType::ERROR_TYPE_LAST);
The final argument should be larger than any valid enum value.  I'd recommend
changing ERROR_TYPE_LAST to ERROR_TYPE_COUNT, so that it's one-past-the-end
already; but you could also change this code to ERROR_TYPE_LAST + 1.

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33547: +    The value of creating
creating a new ServiceWorkerCacheStorage and its
nit: duplicate "creating"

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33548: +    directory. The value is
Created (1) if the storage was able to be created.
nit: I'd change this summary to something a little bit more like "Whether a new
ServiceWorkerCacheStorage and its directory were successfully created."  The
enum labels can speak for the rest.

[jkarlin, 2015-03-14 01:47:19]

Thanks all! PTAL isherman@

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:12: #include
"base/metrics/histogram.h"
On 2015/03/13 22:58:19, Ilya Sherman wrote:
> Please include histogram_macros instead.

Done.

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1229: error,
ErrorType::ERROR_TYPE_LAST);
On 2015/03/13 22:58:19, Ilya Sherman wrote:
> The final argument should be larger than any valid enum value.  I'd recommend
> changing ERROR_TYPE_LAST to ERROR_TYPE_COUNT, so that it's one-past-the-end
> already; but you could also change this code to ERROR_TYPE_LAST + 1.

Thanks!

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache_storage.cc:617:
UMA_HISTOGRAM_BOOLEAN("ServiceWorkerCache.CreateCacheStorageResult",
On 2015/03/13 22:47:16, michaeln wrote:
> Should the label be "CreateCacheResult"?

The names are a bit misleading. ServiceWorkerCache::Init creates the actual
SimpleCache backend, and we have UMA there. 

This is for ServiceWorkerCacheStorage::SimpleCacheLoader (as opposed to
MemoryLoader), which preps the directory and write's the cache's name to an
index file. That's what this histogram is for.

https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_cache_storage.cc:634: bool
success) {
On 2015/03/13 22:47:16, michaeln wrote:
> unrelated but why is 'success' not important here?

Eek. Added a todo.

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33547: +    The value of creating
creating a new ServiceWorkerCacheStorage and its
On 2015/03/13 22:58:20, Ilya Sherman wrote:
> nit: duplicate "creating"

Done.

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33547: +    The value of creating
creating a new ServiceWorkerCacheStorage and its
On 2015/03/13 22:47:16, michaeln wrote:
> In the code, it looks like this is the result of creating an individual named
> cache (ServiceWorkerCache) instance, yet this comment makes it sounnd like its
> the result of creating the cache storage area for an origin
> (ServiceWorkerCacheStorage).
> 
> also nit: creating creating

I cleared up the naming confusion in the comment on
service_worker_cache_storage.cc

Got the nit, thanks.

https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:33548: +    directory. The value is
Created (1) if the storage was able to be created.
On 2015/03/13 22:58:20, Ilya Sherman wrote:
> nit: I'd change this summary to something a little bit more like "Whether a
new
> ServiceWorkerCacheStorage and its directory were successfully created."  The
> enum labels can speak for the rest.

Done.

[jkarlin, 2015-03-16 21:08:17]

On 2015/03/14 01:47:19, jkarlin wrote:
> Thanks all! PTAL isherman@
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> File content/browser/service_worker/service_worker_cache.cc (right):
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_cache.cc:12: #include
> "base/metrics/histogram.h"
> On 2015/03/13 22:58:19, Ilya Sherman wrote:
> > Please include histogram_macros instead.
> 
> Done.
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_cache.cc:1229: error,
> ErrorType::ERROR_TYPE_LAST);
> On 2015/03/13 22:58:19, Ilya Sherman wrote:
> > The final argument should be larger than any valid enum value.  I'd
recommend
> > changing ERROR_TYPE_LAST to ERROR_TYPE_COUNT, so that it's one-past-the-end
> > already; but you could also change this code to ERROR_TYPE_LAST + 1.
> 
> Thanks!
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> File content/browser/service_worker/service_worker_cache_storage.cc (right):
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_cache_storage.cc:617:
> UMA_HISTOGRAM_BOOLEAN("ServiceWorkerCache.CreateCacheStorageResult",
> On 2015/03/13 22:47:16, michaeln wrote:
> > Should the label be "CreateCacheResult"?
> 
> The names are a bit misleading. ServiceWorkerCache::Init creates the actual
> SimpleCache backend, and we have UMA there. 
> 
> This is for ServiceWorkerCacheStorage::SimpleCacheLoader (as opposed to
> MemoryLoader), which preps the directory and write's the cache's name to an
> index file. That's what this histogram is for.
> 
>
https://codereview.chromium.org/985053002/diff/20001/content/browser/service_...
> content/browser/service_worker/service_worker_cache_storage.cc:634: bool
> success) {
> On 2015/03/13 22:47:16, michaeln wrote:
> > unrelated but why is 'success' not important here?
> 
> Eek. Added a todo.
> 
>
https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:33547: +    The value of creating
> creating a new ServiceWorkerCacheStorage and its
> On 2015/03/13 22:58:20, Ilya Sherman wrote:
> > nit: duplicate "creating"
> 
> Done.
> 
>
https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:33547: +    The value of creating
> creating a new ServiceWorkerCacheStorage and its
> On 2015/03/13 22:47:16, michaeln wrote:
> > In the code, it looks like this is the result of creating an individual
named
> > cache (ServiceWorkerCache) instance, yet this comment makes it sounnd like
its
> > the result of creating the cache storage area for an origin
> > (ServiceWorkerCacheStorage).
> > 
> > also nit: creating creating
> 
> I cleared up the naming confusion in the comment on
> service_worker_cache_storage.cc
> 
> Got the nit, thanks.
> 
>
https://codereview.chromium.org/985053002/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:33548: +    directory. The value is
> Created (1) if the storage was able to be created.
> On 2015/03/13 22:58:20, Ilya Sherman wrote:
> > nit: I'd change this summary to something a little bit more like "Whether a
> new
> > ServiceWorkerCacheStorage and its directory were successfully created."  The
> > enum labels can speak for the rest.
> 
> Done.

Ping isherman@, thanks!

[Ilya Sherman, 2015-03-17 01:47:23]

histograms lgtm, thanks

[jkarlin, 2015-03-17 10:12:56]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-03-17 10:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/985053002/#ps60001 (title:
"Address comments from PS 2")

[commit-bot: I haz the power, 2015-03-17 10:13:26]

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

[commit-bot: I haz the power, 2015-03-17 11:28:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-17 11:29:18]

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