Add presubmit scripts to check histograms.xml if bad_message.h changes.

Add presubmit scripts to check histograms.xml if bad_message.h changes.

BUG=None
R=dgozman

Review-Url: https://codereview.chromium.org/2653273003
Cr-Commit-Position: refs/heads/master@{#447531}
Committed: https://chromium.googlesource.com/chromium/src/+/b53dc5998db146031a36b39d774ff95d7bfdc687

[dougt, 2017-01-25 22:39:56]

dgozman, what do you think of something like this? We can add similar presubmits
to: 

chrome/browser/bad_message.h
components/nacl/browser/bad_message.h
components/password_manager/content/browser/bad_message.h   
extensions/browser/bad_message.h

[dougt, 2017-01-25 22:40:03]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 22:41:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 23:17:11]

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

[commit-bot: I haz the power, 2017-01-25 23:17:11]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-26 23:36:07]

lgtm, thank you!

On 2017/01/25 22:39:56, dougt wrote:
> dgozman, what do you think of something like this? We can add similar
presubmits
> to: 
> 
> chrome/browser/bad_message.h
> components/nacl/browser/bad_message.h
> components/password_manager/content/browser/bad_message.h   
> extensions/browser/bad_message.h

Perhaps, it would make sense to add a helper in this case?
We can also use it in third_party/WebKit/Source/core/frame/PRESUBMIT.py.

[dougt, 2017-01-26 23:40:09]

On 2017/01/26 23:36:07, dgozman wrote:
> lgtm, thank you!
> 
> On 2017/01/25 22:39:56, dougt wrote:
> > dgozman, what do you think of something like this? We can add similar
> presubmits
> > to: 
> > 
> > chrome/browser/bad_message.h
> > components/nacl/browser/bad_message.h
> > components/password_manager/content/browser/bad_message.h   
> > extensions/browser/bad_message.h
> 
> Perhaps, it would make sense to add a helper in this case?
> We can also use it in third_party/WebKit/Source/core/frame/PRESUBMIT.py.

sgtm.  I was thinking about just making script that wraps up the logic and have
the presubmit call in to that.  stay tuned.

[dougt, 2017-01-27 20:51:57]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 20:52:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 21:04:29]

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

[commit-bot: I haz the power, 2017-01-27 21:04:29]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-01-27 21:10:15]

dgozman, please take a look.

fwiw, I am not sure converting third_party/WebKit/Source/core/frame/PRESUBMIT.py
makes sense at this point.

[dougt, 2017-01-27 23:06:28]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 23:07:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 23:18:57]

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

[commit-bot: I haz the power, 2017-01-27 23:18:58]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-27 23:19:31]

dgozman@chromium.org changed reviewers:
+ isherman@chromium.org

[dgozman, 2017-01-27 23:19:32]

[+isherman for histograms]
Looks good, but you need more owners.

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:16: if
f.LocalPath().endswith('bad_message.h'):
Let's pass this as a parameter.

[dougt, 2017-01-27 23:25:46]

dougt@chromium.org changed reviewers:
+ brettw@chromium.org

[dougt, 2017-01-27 23:25:47]

brettw@chromium.org: ptal?

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:16: if
f.LocalPath().endswith('bad_message.h'):
On 2017/01/27 23:19:32, dgozman wrote:
> Let's pass this as a parameter.

Acknowledged. It's always bad_message.h at this point, and this precheck is
about "bad message".

[Ilya Sherman, 2017-01-27 23:35:13]

Thanks!  I'm not very familiar with the details of presubmit scripts, so
apologies if some of the comments inline are off base.

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:7: bad_messages.h also
include the generated changes to histograms.xml
nit: bad_messages or bad_message, as typed below?

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:18: break
There are multiple bad_message files in the repository, right?  What if multiple
are changed in a single CL?  Will this break stmt mean that only the first one
is ever considered?

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:20: # If bad message
wasn't change, then there is nothing to check.
nit: s/change/changed

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:20: # If bad message
wasn't change, then there is nothing to check.
nit: s/bad message/bad_message.h

[dougt, 2017-01-28 00:58:12]

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:7: bad_messages.h also
include the generated changes to histograms.xml
On 2017/01/27 23:35:13, Ilya Sherman wrote:
> nit: bad_messages or bad_message, as typed below?

Done.

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:18: break
On 2017/01/27 23:35:13, Ilya Sherman wrote:
> There are multiple bad_message files in the repository, right?  What if
multiple
> are changed in a single CL?  Will this break stmt mean that only the first one
> is ever considered?

You will get one warning that asks you to run 'python
tools/metrics/histograms/update_bad_message_reasons.py'. Doing this will update
the histogram with the content from all bad_message.h files in the repo.

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:20: # If bad message
wasn't change, then there is nothing to check.
On 2017/01/27 23:35:13, Ilya Sherman wrote:
> nit: s/bad message/bad_message.h

Done.

[Ilya Sherman, 2017-01-28 02:53:35]

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:18: break
On 2017/01/28 00:58:11, dougt wrote:
> On 2017/01/27 23:35:13, Ilya Sherman wrote:
> > There are multiple bad_message files in the repository, right?  What if
> multiple
> > are changed in a single CL?  Will this break stmt mean that only the first
one
> > is ever considered?
> 
> You will get one warning that asks you to run 'python
> tools/metrics/histograms/update_bad_message_reasons.py'. Doing this will
update
> the histogram with the content from all bad_message.h files in the repo.

Why are you guaranteed to get this warning?  It looks like the warning is only
emitted if the check below fails.  But, if only one of the bad_message.h files
is checked, then couldn't the warning be missed entirely?  (Perhaps I'm
misunderstanding something about how this all works?)

[dougt, 2017-01-28 05:06:54]

On 2017/01/28 02:53:35, Ilya Sherman wrote:
>
https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):
> 
>
https://codereview.chromium.org/2653273003/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/presubmit_bad_message_reasons.py:18: break
> On 2017/01/28 00:58:11, dougt wrote:
> > On 2017/01/27 23:35:13, Ilya Sherman wrote:
> > > There are multiple bad_message files in the repository, right?  What if
> > multiple
> > > are changed in a single CL?  Will this break stmt mean that only the first
> one
> > > is ever considered?
> > 
> > You will get one warning that asks you to run 'python
> > tools/metrics/histograms/update_bad_message_reasons.py'. Doing this will
> update
> > the histogram with the content from all bad_message.h files in the repo.
> 
> Why are you guaranteed to get this warning?  It looks like the warning is only
> emitted if the check below fails.  But, if only one of the bad_message.h files
> is checked, then couldn't the warning be missed entirely?  (Perhaps I'm
> misunderstanding something about how this all works?)

Good question!

For each bad_message.h, we have an associated presubmit which is called anytime
you make a change in the directory containing bad_message.h.  This presubmit is
pretty dumb. It sets up the sys.path to make the call into the histogram code,
passes the name of the histogram we care about, and returns if the presubmit
should fail or not.

In tools/metrics/histograms/presubmit_bad_message_reasons.py (which is called by
each of these presubmits), the first thing we do is to check to see if there was
a change to bad_message.h.

To check for a file change in the CL, we use |input_api.AffectedFiles()| which
only lists files in the same directory as the current presubmit script:
 
https://cs.chromium.org/chromium/tools/depot_tools/presubmit_support.py?q=%22...


To demonstrate, suppose you have a change like:

------------------------------------------------------------------------
$ git diff HEAD~1
diff --git a/chrome/browser/bad_message.h b/chrome/browser/bad_message.h
index 3bdd0d2b3293..79a0245c0b50 100644
--- a/chrome/browser/bad_message.h
+++ b/chrome/browser/bad_message.h
@@ -30,6 +30,7 @@ enum BadMessageReason {
   BAD_MESSAGE_MAX
 };
 
+// just a comment and doesn't change BadMessageReason
 // Called when the browser receives a bad IPC message from a renderer process
on
 // the UI thread. Logs the event, records a histogram metric for the |reason|,
 // and terminates the process for |host|.
diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index 31530024e1b3..27ad1e143cff 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -183,6 +183,7 @@ enum BadMessageReason {
   RFH_DID_ADD_CONSOLE_MESSAGE_BAD_SEVERITY = 159,
   AIRH_VOLUME_OUT_OF_RANGE = 160,
   BDH_INVALID_DESCRIPTOR_ID = 161,
+  Jean Valjean = 24601,
 
   // Please add new elements here. The naming convention is abbreviated class

$ git cl presubmit --upload -v 
Running presubmit upload checks ...
Running /usr/local/google/home/dft/builds/chromium/src/PRESUBMIT.py
Running /usr/local/google/home/dft/builds/chromium/src/chrome/PRESUBMIT.py
Done processing
/usr/local/google/home/dft/builds/chromium/src/chrome/browser/bad_message.h
Running
/usr/local/google/home/dft/builds/chromium/src/chrome/browser/PRESUBMIT.py
[I2017-01-27 21:03:39,830 133393 139651295885120 update_histogram_enum.py]
Reading histogram enum definition from "chrome/browser/bad_message.h".
[I2017-01-27 21:03:39,831 133393 139651295885120 update_histogram_enum.py]
Reading existing histograms from
"/usr/local/google/home/dft/builds/chromium/src/tools/metrics/histograms/histograms.xml".
[I2017-01-27 21:03:42,042 133393 139651295885120 update_histogram_enum.py]
Comparing histograms enum with new enum definition.
Running
/usr/local/google/home/dft/builds/chromium/src/components/nacl/browser/PRESUBMIT.py
Running
/usr/local/google/home/dft/builds/chromium/src/components/password_manager/PRESUBMIT.py
Running
/usr/local/google/home/dft/builds/chromium/src/components/password_manager/content/browser/PRESUBMIT.py
Running
/usr/local/google/home/dft/builds/chromium/src/content/browser/PRESUBMIT.py
[I2017-01-27 21:03:47,121 133393 139651295885120 update_histogram_enum.py]
Reading histogram enum definition from "content/browser/bad_message.h".
[I2017-01-27 21:03:47,122 133393 139651295885120 update_histogram_enum.py]
Reading existing histograms from
"/usr/local/google/home/dft/builds/chromium/src/tools/metrics/histograms/histograms.xml".
[I2017-01-27 21:03:50,297 133393 139651295885120 update_histogram_enum.py]
Comparing histograms enum with new enum definition.
Running
/usr/local/google/home/dft/builds/chromium/src/extensions/browser/PRESUBMIT.py
Running
/usr/local/google/home/dft/builds/chromium/src/tools/metrics/histograms/PRESUBMIT.py

** Presubmit Messages **
/usr/local/google/home/dft/builds/chromium/src/chrome/browser/test_presubmit.py
(0.07s)

** Presubmit Warnings **
bad_messages.h has been updated but histogram.xml does not appear to be updated.
Please run:
  python tools/metrics/histograms/update_bad_message_reasons.py


Presubmit checks took 17.7s to calculate.

There were presubmit warnings. 
------------------------------------------------------------------------


WDYT?

[brettw, 2017-02-01 00:28:22]

lgtm

https://codereview.chromium.org/2653273003/diff/60001/components/nacl/browser...
File components/nacl/browser/PRESUBMIT.py (right):

https://codereview.chromium.org/2653273003/diff/60001/components/nacl/browser...
components/nacl/browser/PRESUBMIT.py:4: """Chromium presubmit script to check
that BadMessage enums in histograms.xml
Can you put a blank line between the copyright and the docstring?

Same in the other files.

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:32: 'bad_messages.h
has been updated but histogram.xml does not '
I'm not an expert at Python style (so if you know I'm wrong please ignore me),
but I generally expect lines like this wrapped to 4 spaces. Can you double-check
what style is correct here (and in the other added Python code).

[Ilya Sherman, 2017-02-01 00:35:13]

LGTM.  Sorry for the delay!  I thought I had already replied :-/

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:20: # If bad_message.h
wasn't changed, then there is nothing to check.
nit: It would be great to clarify in this documentation why this check makes
sense, i.e. that this function will be called once per bad_message.h-containing
directory.

[dougt, 2017-02-01 02:21:22]

Patchset #5 (id:80001) has been deleted

[dougt, 2017-02-01 02:22:44]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 02:23:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 02:44:46]

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

[commit-bot: I haz the power, 2017-02-01 02:44:47]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-02-01 02:49:07]

https://codereview.chromium.org/2653273003/diff/60001/components/nacl/browser...
File components/nacl/browser/PRESUBMIT.py (right):

https://codereview.chromium.org/2653273003/diff/60001/components/nacl/browser...
components/nacl/browser/PRESUBMIT.py:4: """Chromium presubmit script to check
that BadMessage enums in histograms.xml
On 2017/02/01 00:28:22, brettw (ping after 24h) wrote:
> Can you put a blank line between the copyright and the docstring?
> 
> Same in the other files.

Done.

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/presubmit_bad_message_reasons.py (right):

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:20: # If bad_message.h
wasn't changed, then there is nothing to check.
On 2017/02/01 00:35:13, Ilya Sherman wrote:
> nit: It would be great to clarify in this documentation why this check makes
> sense, i.e. that this function will be called once per
bad_message.h-containing
> directory.

Done.

https://codereview.chromium.org/2653273003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/presubmit_bad_message_reasons.py:32: 'bad_messages.h
has been updated but histogram.xml does not '
On 2017/02/01 00:28:22, brettw (ping after 24h) wrote:
> I'm not an expert at Python style (so if you know I'm wrong please ignore me),
> but I generally expect lines like this wrapped to 4 spaces. Can you
double-check
> what style is correct here (and in the other added Python code).

Done. The style is to indent 4 spaces (or align where approriate):

http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation

[dougt, 2017-02-01 16:19:21]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-02-01 16:19:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, brettw@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2653273003/#ps100001
(title: "fixups")

[commit-bot: I haz the power, 2017-02-01 16:19:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 16:25:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485965961002590,
"parent_rev": "b2368fffc991da192ed060db581841eef83b1f1f", "commit_rev":
"b53dc5998db146031a36b39d774ff95d7bfdc687"}

[commit-bot: I haz the power, 2017-02-01 16:25:36]

Description was changed from

==========
Add presubmit scripts to check histograms.xml if bad_message.h changes.

BUG=None
R=dgozman
==========

to

==========
Add presubmit scripts to check histograms.xml if bad_message.h changes.

BUG=None
R=dgozman

Review-Url: https://codereview.chromium.org/2653273003
Cr-Commit-Position: refs/heads/master@{#447531}
Committed:
https://chromium.googlesource.com/chromium/src/+/b53dc5998db146031a36b39d774f...
==========

[commit-bot: I haz the power, 2017-02-01 16:25:38]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b53dc5998db146031a36b39d774f...