Add UMA logging of extension creation flags.

Add UMA logging of extension creation flags.

This CL adds an Extensions.LoadCreationFlags UMA histogram which logs an
aggregate of a user's extension's flags.

BUG=None

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

[benwells, 2014-04-22 06:41:44]

lgtm

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

https://codereview.chromium.org/240613004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:121: for (int i = 1; i <
Extension::LAST_INIT_FROM_VALUE_FLAGS; i <<= 1)
Nit: add a comment explaining what you're doing here / why you're doing this
loop.

[calamity, 2014-04-22 06:56:28]

+jar for histograms.xml OWNERS.

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

https://codereview.chromium.org/240613004/diff/60001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:121: for (int i = 1; i <
Extension::LAST_INIT_FROM_VALUE_FLAGS; i <<= 1)
On 2014/04/22 06:41:44, benwells wrote:
> Nit: add a comment explaining what you're doing here / why you're doing this
> loop.

Done.

[jar (doing other things), 2014-04-29 01:32:25]

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

https://codereview.chromium.org/240613004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:130:
"Extensions.LoadCreationFlags", i, highest_flag_bit + 1);
Other than |i| (which is the sample), the other arguments act as "definitions"
for this histogtram. As a result, you can't change them dynamically (either via
the macro, or by breaking-open the macro, and calling FactoryGet(...).)

Please try to define the histogram so that |higest_flag_bit| is a constant in
the program.  Most typically, it is a bounding value in an enumeration.

Reading the code, I *think* this is meant to be 11 or 12.  You should probably
define the list in an enumeration, and then expand it in the flag definition
(where you currently have the existing enum) using those enumerated values from
0-10 as "shift values" for defining your flags.  YMMV.

...but you probably shouldn't try to calculate the highest flag bit
*dynamically* found in a word.  That will give differing definitions, and mess
up the histogram.

[jar (doing other things), 2014-04-29 16:25:56]

As per comments, I suspect there are a few bugs here.

Please switch to constants to make this all much more readable.

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

https://codereview.chromium.org/240613004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:124: ++highest_flag_bit;
I didn't read this section of the code carefully...

As per your off-line comments.... this "would be" calculation does generate a
constant... and even is often optimized away by the compiler.

It would be much clearer to have a constant value here, rather than having to
read code to deduce that a const value is generated.

...and in fact...

Looking at your enums.... I can see that 
LAST_INIT_FROM_VALUE_FLAGS == 1 << 10

As a result, the loop will be executed a total of 9 times, and create the
constant:
 higest_flag_bit == 9.

...which does not seem to be as intended, and leads to further problems below.

It would be much simpler (clearer, and correct) to use a constant here.

https://codereview.chromium.org/240613004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:127: for (int i = 1; i <=
highest_flag_bit; ++i) {
Since higest_flag_bit == 9, this loop will only record samples for 
i == 1 ... 9

In contrast, I noted that you have a total 11 enums 
{ 1 << 0, ..., 1 << 10 }

As a result, you'll only test 9 of the 11 flags.

https://codereview.chromium.org/240613004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:128: if
(extension->creation_flags() & flag) {
Since you're only looking at 9 choices for |flag|, and the first choice is |1 <<
0|, it appears that you'll only check for flags 
{1 << 0, ... 1 << 8}

But then you'll populate the histogram with samples {1, ... 9}.
Looking at the xml file, it appears that a sample of 0 should have corresponded
to REQUIRED_KEY == 1 << 0, but you never have an i==0 recorded in the histogram.
The sample of 1 corresponds to a the first iteration through the loop, and hence
that records when the flag 1 << 0 is used.

I don't think this is as intended.

[calamity, 2014-05-12 03:39:09]

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

https://codereview.chromium.org/240613004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/installed_loader.cc:124: ++highest_flag_bit;
On 2014/04/29 16:25:56, jar wrote:
> I didn't read this section of the code carefully...
> 
> As per your off-line comments.... this "would be" calculation does generate a
> constant... and even is often optimized away by the compiler.
> 
> It would be much clearer to have a constant value here, rather than having to
> read code to deduce that a const value is generated.
> 
> ...and in fact...
> 
> Looking at your enums.... I can see that 
> LAST_INIT_FROM_VALUE_FLAGS == 1 << 10
> 
> As a result, the loop will be executed a total of 9 times, and create the
> constant:
>  higest_flag_bit == 9.
> 
> ...which does not seem to be as intended, and leads to further problems below.
> 
> It would be much simpler (clearer, and correct) to use a constant here.
> 

Done.

[Alexei Svitkine (slow), 2014-05-12 19:51:17]

lgtm

[calamity, 2014-05-13 02:38:14]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-05-13 02:38:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/calamity@chromium.org/240613004/100001

[commit-bot: I haz the power, 2014-05-13 05:34:20]

Change committed as 270029