Add presubmit check and automatic update script for ExtensionPermission enum.

Add presubmit check and automatic update script for ExtensionPermission enum.
BUG=338781
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259302

[ahernandez, 2014-02-18 19:50:45]

Hi,

Could you please review these changes?

And, regarding these changes, update_extension_permission.py is a revised
version of update_extension_functions.py, so I believe there is a possibility of
factoring out code, but I am not sure what the best way would be. In addition,
perhaps tools/extra_imports could be better named?

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:25789: +  <int value="58"
label="OVERRIDE_BOOKMARKS_U_I"/>
When converting from the 'kLabelName' style to the all caps unix name style, it
causes weird things like this 'OVERRIDE_BOOKMARKS_U_I' label to appear. Should
there be special cases accounted for? Or should the naming conventions in
permission_message.h be changed to make it easier for automatic updating?

[rpaquay, 2014-02-18 21:39:26]

Thanks for doing this work! I think it is on the right track overall.

Note that I don't have python readability, so I would suggest adding someone
with python readability to the reviewer list.

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
File chrome/browser/extensions/PRESUBMIT.py (right):

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:33: results +=
_CheckHistogramEnumValue(input_api, output_api).Run()
nit: rename "_CheckHistogramEnumValue" as this line reads "funny". Maybe
something like "_CreateHistogramValueChecker"?

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
File chrome/browser/extensions/PRESUBMIT_test.py (right):

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT_test.py:147: mock_file =
MockFile(PRESUBMIT.HistogramValueChecker.HIST_VALUE_LOCAL_PATH,
Does this work? I am slightly confused to where HIST_VALUE_LOCAL_PATH is
defined.

Also, I think we should update the tests here to test maybe a single valid and
invalid change, since most of the test cases are now covered for the factored
tools/extra_imports/strict_enum_value_checker_test.py.

https://codereview.chromium.org/170233008/diff/1/extensions/common/permission...
File extensions/common/permissions/PRESUBMIT.py (right):

https://codereview.chromium.org/170233008/diff/1/extensions/common/permission...
extensions/common/permissions/PRESUBMIT.py:33: results +=
_CheckPermissionMessageEnumValue(input_api, output_api).Run()
nit: same remark as other file wrt to naming. Maybe
"_CreatePermissionMessageEnumChecker"?

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
File tools/extra_imports/strict_enum_value_checker.py (left):

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker.py:109: "|HistogramValue| enum
which should be edited in specific ways "
"HistogramValue" is too specific in this file, as this is the factored out code.

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker.py:181: "|HistogramValueChecker|
class.");
nit: HistogramValueChecker is too specific.

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
File tools/extra_imports/strict_enum_value_checker.py (right):

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker.py:1: # Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: remove the "(c"): see http://www.chromium.org/developers/coding-style
("file headers" section).

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker.py:27: '''Represents a range of
line numbers (1-based)'''
nit: is there a particular reason you use single quotes in this file? (it makes
the diff more difficult to read).

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
File tools/extra_imports/strict_enum_value_checker_test.py (right):

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker_test.py:2: # Copyright (c) 2014
The Chromium Authors. All rights reserved.
nit: not "(c)"

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker_test.py:78: result = ''
nit: why changing usage to single quote?

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:25789: +  <int value="58"
label="OVERRIDE_BOOKMARKS_U_I"/>
On 2014/02/18 19:50:46, ahernandez.miralles wrote:
> When converting from the 'kLabelName' style to the all caps unix name style,
it
> causes weird things like this 'OVERRIDE_BOOKMARKS_U_I' label to appear. Should
> there be special cases accounted for? Or should the naming conventions in
> permission_message.h be changed to make it easier for automatic updating?

I would argue for a special case, implementing along the lines of "if the enum
label is mixed case and contains "UI" uppercase, then don't split".

[ahernandez, 2014-02-20 17:23:41]

jyasskin,

Could you review this python code? I need to add someone with python
readability. If you are not the correct person to ask, if you could please
recommend someone who could review this I would appreciate it. Thank you.

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
File chrome/browser/extensions/PRESUBMIT_test.py (right):

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT_test.py:147: mock_file =
MockFile(PRESUBMIT.HistogramValueChecker.HIST_VALUE_LOCAL_PATH,
On 2014/02/18 21:39:26, rpaquay wrote:
> Does this work? I am slightly confused to where HIST_VALUE_LOCAL_PATH is
> defined.
> 
> Also, I think we should update the tests here to test maybe a single valid and
> invalid change, since most of the test cases are now covered for the factored
> tools/extra_imports/strict_enum_value_checker_test.py.

It appears I forgot to remove this file; all of these presubmit tests are
redundant and are covered in
toos/extra_imports/strict_enum_value_checker_test.py

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
File tools/extra_imports/strict_enum_value_checker.py (right):

https://codereview.chromium.org/170233008/diff/1/tools/extra_imports/strict_e...
tools/extra_imports/strict_enum_value_checker.py:27: '''Represents a range of
line numbers (1-based)'''
On 2014/02/18 21:39:26, rpaquay wrote:
> nit: is there a particular reason you use single quotes in this file? (it
makes
> the diff more difficult to read).

There really wasn't a reason other than a personal preference. I can easily
change it back to using double quotes.

[Jeffrey Yasskin, 2014-02-20 19:07:36]

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
File chrome/browser/extensions/PRESUBMIT.py (right):

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:22: sys.path = sys.path +
[input_api.os_path.join(
"sys.path.append(input_api...)"

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:23: input_api.PresubmitLocalPath(),
'../../../', 'tools', 'extra_imports')]
I'm nervous about this pattern for imports. However, I see that src/PRESUBMIT.py
does the same thing, so I guess you're ok.

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:23: input_api.PresubmitLocalPath(),
'../../../', 'tools', 'extra_imports')]
Please check with a src/PRESUBMIT.py owner to make sure this is the right
directory to create. maruel@ and joi@ are likely. I'd like one of them to lgtm
the new directory, but I haven't cc'ed them in yet so you can fix the other
comments first.

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:28: return
StrictEnumValueChecker(input_api, output_api, ENUM_START_MARKER,
Instead of defining constants to describe the strings, call
StrictEnumValueChecker like:

  StrictEnumValueChecker(
      input_api, output_api,
      start_marker='enum HistogramValue {', end_marker='  ENUM BOUNDARY',
      path='chrome/browser/extensions/extension_function_histogram_value.h')

https://codereview.chromium.org/170233008/diff/1/chrome/browser/extensions/PR...
chrome/browser/extensions/PRESUBMIT.py:39: results +=
_CheckHistogramEnumValue(input_api, output_api).Run()
Just "return _CheckHistogram..." should be equivalent and shorter.

https://codereview.chromium.org/170233008/diff/1/extensions/common/permission...
File extensions/common/permissions/PRESUBMIT.py (right):

https://codereview.chromium.org/170233008/diff/1/extensions/common/permission...
extensions/common/permissions/PRESUBMIT.py:28: return
StrictEnumValueChecker(input_api, output_api, ENUM_START_MARKER,
Same comments about the function call style and 'results'.

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/upd...
File tools/metrics/histograms/update_extension_permission.py (right):

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/upd...
tools/metrics/histograms/update_extension_permission.py:5: '''Updates
ExtensionPermission enum in histograms.xml file with values read from
Try to share the code between this file and the one you copied it from.

https://codereview.chromium.org/170233008/diff/1/tools/metrics/histograms/upd...
tools/metrics/histograms/update_extension_permission.py:40: return label[1] +
re.sub('([A-Z])', r'_\1', label[2:]).upper()
Probably assert that label.startswith('k') and label[1].isupper().

[ahernandez, 2014-02-20 22:37:28]

maruel@, joi@:

I created a new directory under src/tools called extra_imports, which contains a
class used by two presubmit scripts in other directories. If either of you could
review the new directory name/location, that would be great. Thank you.

[Jói, 2014-02-21 11:44:39]

Why the name extra_imports? The tool within this directory seems to be
named strict_enum_value_checker.py, why not name the directory the
same (minus the .py)?

On Thu, Feb 20, 2014 at 10:37 PM,  <ahernandez.miralles@gmail.com> wrote:
> maruel@, joi@:
>
> I created a new directory under src/tools called extra_imports, which
> contains a
> class used by two presubmit scripts in other directories. If either of you
> could
> review the new directory name/location, that would be great. Thank you.
>
> https://codereview.chromium.org/170233008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2014-02-26 09:07:52]

I'm fine with the new directory name and location, so LGTM.

Note however the comment below; you probably need to add a reviewer from
tools/metrics/OWNERS to make sure all of this is safe.

Cheers,
Jói

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
label="DESKTOP_CAPTURE"/>
I'm not the expert on this, you should check with
tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to change
labels after the fact, since older versions will be transmitting values with
different assumed labels.

[ahernandez, 2014-03-01 21:01:54]

isherman@, can you take a look at the histogram.xml file? I added a script to
automatically update the ExtensionPermission enum, but doing so changes a few of
the existing label names. Is this ok, or should a special case be added for
those existing labels?

jyasskin@, can you take a look at the python changes I made?

Thank you.

[ahernandez, 2014-03-08 19:06:58]

On 2014/03/01 21:01:54, ahernandez.miralles wrote:
> isherman@, can you take a look at the histogram.xml file? I added a script to
> automatically update the ExtensionPermission enum, but doing so changes a few
of
> the existing label names. Is this ok, or should a special case be added for
> those existing labels?
> 
> jyasskin@, can you take a look at the python changes I made?
> 
> Thank you.

ping?

[Jeffrey Yasskin, 2014-03-10 20:53:22]

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_functions.py (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_functions.py:5: '''Updates
ExtensionFunctions enum in histograms.xml file with values read from
Double quotes for multi-line strings are an order of magnitude more common than
single quotes. You should avoid changing this instance.

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_permission.py (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission.py:24: pattern =
'([A-HJ-Z])'
This looks flaky: Consider (and test!) "kUIWasIsolated" or similar. What's the
actual pattern you're going for? That contiguous capitals should be treated as
acronyms and kept together? Consider using something like
re.finditer([A-Z][a-z]*) and adding onto the result manually.

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:31: def
ReadHistogramValues(filename, start_marker, end_marker):
Set your --similarity low enough that `git cl upload` marks this as a copy. You
should be able to test with `git diff -C30% @{u}`.

[Ilya Sherman, 2014-03-10 22:21:53]

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
label="DESKTOP_CAPTURE"/>
On 2014/02/26 09:07:53, Jói wrote:
> I'm not the expert on this, you should check with
> tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to change
> labels after the fact, since older versions will be transmitting values with
> different assumed labels.

+1.  Is this a change that you're introducing in this CL, or is this reflecting
a change that previously landed?

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_permission.py (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission.py:16: '''Transforms labels
of form "kLabelName" to "LABEL_NAME"
Why is this transformation useful?  Why not just use labels of the form
kLabelName?

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission.py:30:
source_enum_path=('../../../extensions/common/permissions'
Have you verified that the forward slashes work on all platforms?  (Really, I'm
wondering about Windows.)

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission.py:31:
'/permission_message.h'),
nit: Why does this string start with a slash?

[ahernandez, 2014-03-13 19:11:24]

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
label="DESKTOP_CAPTURE"/>
On 2014/03/10 22:21:54, Ilya Sherman wrote:
> On 2014/02/26 09:07:53, Jói wrote:
> > I'm not the expert on this, you should check with
> > tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to change
> > labels after the fact, since older versions will be transmitting values with
> > different assumed labels.
> 
> +1.  Is this a change that you're introducing in this CL, or is this
reflecting
> a change that previously landed?

This is a change that I am introducing in this CL, but it reflects changes that
have occurred in the permission_message.h enum.

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_permission.py (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission.py:16: '''Transforms labels
of form "kLabelName" to "LABEL_NAME"
On 2014/03/10 22:21:54, Ilya Sherman wrote:
> Why is this transformation useful?  Why not just use labels of the form
> kLabelName? 

The ExtensionPermission enum already has it's entries styled as LABEL_NAME,
which is why I am preserving that style. If it is better to keep the style as
found in the permission_message.h enum, then that is an easy change to make

[rpaquay, 2014-03-13 20:27:45]

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
label="DESKTOP_CAPTURE"/>
On 2014/03/13 19:11:25, ahernandez.miralles wrote:
> On 2014/03/10 22:21:54, Ilya Sherman wrote:
> > On 2014/02/26 09:07:53, Jói wrote:
> > > I'm not the expert on this, you should check with
> > > tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to
change
> > > labels after the fact, since older versions will be transmitting values
with
> > > different assumed labels.
> > 
> > +1.  Is this a change that you're introducing in this CL, or is this
> reflecting
> > a change that previously landed?
> 
> This is a change that I am introducing in this CL, but it reflects changes
that
> have occurred in the permission_message.h enum.


A couple of things:

1) It is my understanding that changing labels (say from upper case to lower
case) is not a problem *in general*, as these labels are only used for
displaying data in the histogram web site.

2) In this case *in particular*, we should *not* change the content of the
"ExtensionPermission" because we have been recording data with "wrong" enum
values in past, as explained in
https://code.google.com/p/chromium/issues/detail?id=338008.

So, I think in this case, I propose we do the following
1) Create a new enum, let's say we call it "ExtensionPermissionNew".
2) Update this CL to update that enum only
3) Don't bother with trying to convert from lower case to upper case, and store
the enum values as is.
4) We will use then use the new enum defined in 1) as part of the fix for
crbug/338008.

[ahernandez, 2014-03-13 20:41:20]

On 2014/03/13 20:27:45, rpaquay wrote:
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
> label="DESKTOP_CAPTURE"/>
> On 2014/03/13 19:11:25, ahernandez.miralles wrote:
> > On 2014/03/10 22:21:54, Ilya Sherman wrote:
> > > On 2014/02/26 09:07:53, Jói wrote:
> > > > I'm not the expert on this, you should check with
> > > > tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to
> change
> > > > labels after the fact, since older versions will be transmitting values
> with
> > > > different assumed labels.
> > > 
> > > +1.  Is this a change that you're introducing in this CL, or is this
> > reflecting
> > > a change that previously landed?
> > 
> > This is a change that I am introducing in this CL, but it reflects changes
> that
> > have occurred in the permission_message.h enum.
> 
> 
> A couple of things:
> 
> 1) It is my understanding that changing labels (say from upper case to lower
> case) is not a problem *in general*, as these labels are only used for
> displaying data in the histogram web site.
> 
> 2) In this case *in particular*, we should *not* change the content of the
> "ExtensionPermission" because we have been recording data with "wrong" enum
> values in past, as explained in
> https://code.google.com/p/chromium/issues/detail?id=338008.
> 
> So, I think in this case, I propose we do the following
> 1) Create a new enum, let's say we call it "ExtensionPermissionNew".
> 2) Update this CL to update that enum only
> 3) Don't bother with trying to convert from lower case to upper case, and
store
> the enum values as is.
> 4) We will use then use the new enum defined in 1) as part of the fix for
> crbug/338008.

Is "ExtensionPermissionNew" the actual name we want to use, or just an example?

[Ilya Sherman, 2014-03-13 20:42:35]

On 2014/03/13 20:41:20, ahernandez.miralles wrote:
> On 2014/03/13 20:27:45, rpaquay wrote:
> >
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
> > label="DESKTOP_CAPTURE"/>
> > On 2014/03/13 19:11:25, ahernandez.miralles wrote:
> > > On 2014/03/10 22:21:54, Ilya Sherman wrote:
> > > > On 2014/02/26 09:07:53, Jói wrote:
> > > > > I'm not the expert on this, you should check with
> > > > > tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea to
> > change
> > > > > labels after the fact, since older versions will be transmitting
values
> > with
> > > > > different assumed labels.
> > > > 
> > > > +1.  Is this a change that you're introducing in this CL, or is this
> > > reflecting
> > > > a change that previously landed?
> > > 
> > > This is a change that I am introducing in this CL, but it reflects changes
> > that
> > > have occurred in the permission_message.h enum.
> > 
> > 
> > A couple of things:
> > 
> > 1) It is my understanding that changing labels (say from upper case to lower
> > case) is not a problem *in general*, as these labels are only used for
> > displaying data in the histogram web site.
> > 
> > 2) In this case *in particular*, we should *not* change the content of the
> > "ExtensionPermission" because we have been recording data with "wrong" enum
> > values in past, as explained in
> > https://code.google.com/p/chromium/issues/detail?id=338008.
> > 
> > So, I think in this case, I propose we do the following
> > 1) Create a new enum, let's say we call it "ExtensionPermissionNew".
> > 2) Update this CL to update that enum only
> > 3) Don't bother with trying to convert from lower case to upper case, and
> store
> > the enum values as is.
> > 4) We will use then use the new enum defined in 1) as part of the fix for
> > crbug/338008.
> 
> Is "ExtensionPermissionNew" the actual name we want to use, or just an
example?

I'd recommend "ExtensionPermission2", so that you don't end up with "NewNew" if
you ever need to rename the histogram again in the future.

[rpaquay, 2014-03-13 20:56:34]

On 2014/03/13 20:42:35, Ilya Sherman wrote:
> On 2014/03/13 20:41:20, ahernandez.miralles wrote:
> > On 2014/03/13 20:27:45, rpaquay wrote:
> > >
> >
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> > > File tools/metrics/histograms/histograms.xml (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/170233008/diff/260001/tools/metrics/histogram...
> > > tools/metrics/histograms/histograms.xml:26252: +  <int value="8"
> > > label="DESKTOP_CAPTURE"/>
> > > On 2014/03/13 19:11:25, ahernandez.miralles wrote:
> > > > On 2014/03/10 22:21:54, Ilya Sherman wrote:
> > > > > On 2014/02/26 09:07:53, Jói wrote:
> > > > > > I'm not the expert on this, you should check with
> > > > > > tools/metrics/histograms/OWNERS, but I'm not sure it's a good idea
to
> > > change
> > > > > > labels after the fact, since older versions will be transmitting
> values
> > > with
> > > > > > different assumed labels.
> > > > > 
> > > > > +1.  Is this a change that you're introducing in this CL, or is this
> > > > reflecting
> > > > > a change that previously landed?
> > > > 
> > > > This is a change that I am introducing in this CL, but it reflects
changes
> > > that
> > > > have occurred in the permission_message.h enum.
> > > 
> > > 
> > > A couple of things:
> > > 
> > > 1) It is my understanding that changing labels (say from upper case to
lower
> > > case) is not a problem *in general*, as these labels are only used for
> > > displaying data in the histogram web site.
> > > 
> > > 2) In this case *in particular*, we should *not* change the content of the
> > > "ExtensionPermission" because we have been recording data with "wrong"
enum
> > > values in past, as explained in
> > > https://code.google.com/p/chromium/issues/detail?id=338008.
> > > 
> > > So, I think in this case, I propose we do the following
> > > 1) Create a new enum, let's say we call it "ExtensionPermissionNew".
> > > 2) Update this CL to update that enum only
> > > 3) Don't bother with trying to convert from lower case to upper case, and
> > store
> > > the enum values as is.
> > > 4) We will use then use the new enum defined in 1) as part of the fix for
> > > crbug/338008.
> > 
> > Is "ExtensionPermissionNew" the actual name we want to use, or just an
> example?
> 
> I'd recommend "ExtensionPermission2", so that you don't end up with "NewNew"
if
> you ever need to rename the histogram again in the future.

I am ok with "ExtensionPermission2".

[ahernandez, 2014-03-13 21:39:26]

@rpaquay, @jyasskin, @isherman, could you please review these changes? Thank
you.

[rpaquay, 2014-03-13 22:31:11]

lgtm minus maybe python readability, which I can't really comment on.

[Ilya Sherman, 2014-03-14 23:19:34]

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27806: +<enum
name="ExtensionPermission2" type="int">
Shouldn't there be a histogram that uses this enum?  Are you planning to add
that in a separate CL?

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
File tools/metrics/histograms/update_extension_permission_2.py (right):

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/update_extension_permission_2.py:3: # found in the
LICENSE file.
I would name this file "update_extension_permissions.py", i.e. replace the "_2"
with an "s".  The "_2" is very much an implementation detail that most people
can safely ignore.

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (right):

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:30: 
nit: Please leave two blank lines between top-level definitions (applies
throughout).

https://codereview.chromium.org/170233008/diff/320001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:34: '''
nit: Double quotes for this sort of comment string.  (Applies throughout.)

[ahernandez, 2014-03-15 17:55:10]

@isherman, the current plan as I understand it is to upload a separate CL that
introduces usage of the new enum.

[Ilya Sherman, 2014-03-17 22:21:09]

This should hopefully be the last round of review.  Thanks for taking on this
task and for your patience :)

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (left):

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:9: """
Docs for this file would still be useful.

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (right):

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:73: a string, defaulting
labelTransformer to str() effectively does nothing.
Is the labelTransformer parameter still needed?

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:106:
UpdateHistogramDefinitions.
Ditto.

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:108: HISTOGRAMS_PATH =
'histograms.xml'
Would be nice to share this logic:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...

If you prefer to defer that to a follow-up CL, please add a TODO here.

https://codereview.chromium.org/170233008/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:127: source_enum_path,
histograms_doc, labelTransformer)
nit: Please align the arguments, so either indent this line to the paren, or
wrap all the args to be indented by four spaces.

[ahernandez, 2014-03-17 22:58:41]

@jyasskin, when you get a chance, can you please give a (hopefully) final look
at the python code? Thank you.

https://codereview.chromium.org/170233008/diff/400001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (right):

https://codereview.chromium.org/170233008/diff/400001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:107: HISTOGRAMS_PATH =
'histograms.xml'
This seems like it would be better in a separate CL. TODO added.

[Ilya Sherman, 2014-03-17 23:30:34]

tools/metrics/histograms LGTM.  Thanks!

[Jeffrey Yasskin, 2014-03-25 01:26:36]

The python style lgtm. Sorry for the delay.

https://codereview.chromium.org/170233008/diff/400001/tools/metrics/histogram...
File tools/metrics/histograms/update_histogram_enum.py (right):

https://codereview.chromium.org/170233008/diff/400001/tools/metrics/histogram...
tools/metrics/histograms/update_histogram_enum.py:34: return m.group(1)
Note that this will throw an IndexError if the regex matched but didn't contain
any groups. http://docs.python.org/2/library/re.html#re.MatchObject.group

https://codereview.chromium.org/170233008/diff/400001/tools/strict_enum_value...
File tools/strict_enum_value_checker/strict_enum_value_checker_test.py (right):

https://codereview.chromium.org/170233008/diff/400001/tools/strict_enum_value...
tools/strict_enum_value_checker/strict_enum_value_checker_test.py:174: 'We
should get no warning for simple edits.')
As before, don't make this sort of cleanup change in the middle of a big CL,
except possibly where you're editing the lines anyway.

[ahernandez, 2014-03-25 17:21:05]

The CQ bit was checked by ahernandez.miralles@gmail.com

[commit-bot: I haz the power, 2014-03-25 17:23:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ahernandez.miralles@gmail.com/17023300...

[commit-bot: I haz the power, 2014-03-25 19:49:10]

Change committed as 259302