Make installed_loader UMA less wasteful.

Make installed_loader UMA less wasteful.

R=jar@chromium.org, rdevlin.cronin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272192

[Devlin, 2014-05-21 19:54:09]

lg so far, but I'll wait until jar reviews in case there are more changes.

https://codereview.chromium.org/299853002/diff/1/extensions/browser/admin_pol...
File extensions/browser/admin_policy.cc (right):

https://codereview.chromium.org/299853002/diff/1/extensions/browser/admin_pol...
extensions/browser/admin_policy.cc:101: break;
Hey, this break statement actually does something now! ;)

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h
File extensions/common/manifest.h (right):

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h...
extensions/common/manifest.h:66: NUM_LOAD_TYPES
nit: We should probably include the standard comment about "any new types should
go above this one"

[jar (doing other things), 2014-05-21 22:12:45]

Thanks for taking the time to do this.

LGTM, % nits below.

It won't surprise me if you expose an issue when you add the histograms.xml
entry.  You might want to add a todo, but it is your choice.

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:377: 10);
nit: I was curious about this bound (10)... but I think the histogram has not
yet been added to the xml file... so I really can't see if we've overrun, or
what is going on.

Please (in this CL) at least add this name and description to the xml file....
I'm fine with leaving this line as is for now (this CL)....

but if we really have large numbers, we'd probably switch to using:
UMA_HISTOGRAM_SPARSE_SLOWLY()
...but that is certainly a different CL.

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h
File extensions/common/manifest.h (right):

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h...
extensions/common/manifest.h:66: NUM_LOAD_TYPES
On 2014/05/21 19:54:09, Devlin wrote:
> nit: We should probably include the standard comment about "any new types
should
> go above this one"

+1

[not at google - send to devlin, 2014-05-21 22:31:07]

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:164:
content::RecordAction(UserMetricsAction("Extensions.IDChangedError"));
(see below... what even is "content::RecordAction"?)

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:377: 10);
On 2014/05/21 22:12:45, jar wrote:
> nit: I was curious about this bound (10)... but I think the histogram has not
> yet been added to the xml file... so I really can't see if we've overrun, or
> what is going on.
> 
> Please (in this CL) at least add this name and description to the xml file....
> I'm fine with leaving this line as is for now (this CL)....
> 
> but if we really have large numbers, we'd probably switch to using:
> UMA_HISTOGRAM_SPARSE_SLOWLY()
> ...but that is certainly a different CL.

Looks like there are actually 4 values that are being logged in this file but
don't have entries in histograms.xml:

1. Extensions.IDChangedError
2. Extensions.LoadAllUser
3. Extensions.ManifestReloadEnumValue
4. Extensions.ManifestVersion


(1) I might just delete, nobody has cared about it for a long time (the guy who
added it is long gone).
(2) I might add to histograms.xml in this patch
(3) I have no idea what this is even really trying to do, so I'd have problems
adding it to the histograms.xml file. I might just delete it (SGTY? It'll clean
up a bit more code in the process).
(4) We can actually get a decent approximation of this data by looking through
the items that are checked into the webstore, but it's conceivable we'll want to
know outside webstore, so I'll add to this patch.

[not at google - send to devlin, 2014-05-21 23:17:22]

ptal

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/299853002/diff/1/chrome/browser/extensions/in...
chrome/browser/extensions/installed_loader.cc:377: 10);
On 2014/05/21 22:31:07, kalman wrote:
> On 2014/05/21 22:12:45, jar wrote:
> > nit: I was curious about this bound (10)... but I think the histogram has
not
> > yet been added to the xml file... so I really can't see if we've overrun, or
> > what is going on.
> > 
> > Please (in this CL) at least add this name and description to the xml
file....
> > I'm fine with leaving this line as is for now (this CL)....
> > 
> > but if we really have large numbers, we'd probably switch to using:
> > UMA_HISTOGRAM_SPARSE_SLOWLY()
> > ...but that is certainly a different CL.
> 
> Looks like there are actually 4 values that are being logged in this file but
> don't have entries in histograms.xml:
> 
> 1. Extensions.IDChangedError
> 2. Extensions.LoadAllUser
> 3. Extensions.ManifestReloadEnumValue
> 4. Extensions.ManifestVersion
> 
> 
> (1) I might just delete, nobody has cared about it for a long time (the guy
who
> added it is long gone).
> (2) I might add to histograms.xml in this patch
> (3) I have no idea what this is even really trying to do, so I'd have problems
> adding it to the histograms.xml file. I might just delete it (SGTY? It'll
clean
> up a bit more code in the process).
> (4) We can actually get a decent approximation of this data by looking through
> the items that are checked into the webstore, but it's conceivable we'll want
to
> know outside webstore, so I'll add to this patch.

Actually I'm going to delete (2). It's not very interesting, we already have
other data which is basically the same and clearly nobody has cared about it.

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h
File extensions/common/manifest.h (right):

https://codereview.chromium.org/299853002/diff/1/extensions/common/manifest.h...
extensions/common/manifest.h:66: NUM_LOAD_TYPES
On 2014/05/21 22:12:45, jar wrote:
> On 2014/05/21 19:54:09, Devlin wrote:
> > nit: We should probably include the standard comment about "any new types
> should
> > go above this one"
> 
> +1
> 

Done.

[Devlin, 2014-05-21 23:23:37]

lgtm

https://codereview.chromium.org/299853002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/299853002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:64: NUM_BACKGROUND_PAGE_TYPES
nit: New enum values must go above here.

[not at google - send to devlin, 2014-05-21 23:36:04]

https://codereview.chromium.org/299853002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/installed_loader.cc (right):

https://codereview.chromium.org/299853002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:64: NUM_BACKGROUND_PAGE_TYPES
On 2014/05/21 23:23:37, Devlin wrote:
> nit: New enum values must go above here.

done and done and done

[not at google - send to devlin, 2014-05-21 23:36:07]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-05-21 23:42:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/299853002/40001

[jar (doing other things), 2014-05-22 02:09:02]

lgtm

[commit-bot: I haz the power, 2014-05-22 08:47:48]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-22 08:52:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 08:52:10]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[not at google - send to devlin, 2014-05-22 15:02:39]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-05-22 15:03:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/299853002/60001

[not at google - send to devlin, 2014-05-22 15:35:24]

Committed patchset #4 manually as r272192 (presubmit successful).