Add PRESUBMIT check if modified UMA histogram name can be found (2)

Add PRESUBMIT check if modified UMA histogram name can be found (2)

This CL is an iteration of http://crrev.com/766713004, that got reverted
after http://crbug.com/445265 because it could not cope correctly
with structures of the type:

<histogram_suffixes name="SafeBrowsingStores.SBWhiteLists" separator=".">
  <suffix name="CSD" label="CSD"/>
  <suffix name="DownloadWhitelist" label="DownloadWhitelist"/>
  <affected-histogram name="SB2.DatabaseKilobytes"/>
</histogram_suffixes>

(in particular: affected-histogram-suffixes).

Limitation that should be solved now.

Original description ---------------------------------------------------

Add PRESUBMIT check if modified UMA histogram name can be found

This Presubmit checks if some diffs affect any UMA_HISTOGRAM_*
macro and, if so, checks if the histogram name is to be found
in either tools/metrics/histograms/histograms.xml or in the
CL diffs.

Addresses the problem of someone modifying code and
inadvertently forgetting a corresponding histograms.xml
adaptation, that has happened in the past.

BUG=434420, 445265
Committed: https://crrev.com/b7440c2829b8fae8467c110235a7aa8363644ae0
Cr-Commit-Position: refs/heads/master@{#314557}

[mcasas, 2015-01-30 02:34:27]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-01-30 02:36:10]

mcasas@chromium.org changed reviewers:
+ asvitkine@chromium.org, phajdan.jr@chromium.org

[mcasas, 2015-01-30 02:36:10]

phajdan.jr@ Low Prio Review, PTAL

asvitkine@ FYI/PTAL.

[Alexei Svitkine (slow), 2015-01-30 15:15:37]

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode361
PRESUBMIT.py:361: # Helper function to smartly try to find a histogram name or
prefix in a line.
Make this a """ comment inside the function.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode364
PRESUBMIT.py:364: return histogram_name in line;
Nit: No need for trailing ;

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode371
PRESUBMIT.py:371: 
Nit: Two empty lines between functions.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode376
PRESUBMIT.py:376: 
Nit: No blank line.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode382
PRESUBMIT.py:382: if (f.LocalPath().endswith(('histograms.xml'))):
Nit: No need for outer parens in Python. Same for the if below.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode404
PRESUBMIT.py:404: with open('tools/metrics/histograms/histograms.xml') as
histograms_xml:
It seems inefficient to open the file and iterate through all of its lines
potentially multiple times.

Can you collect the list of histograms that weren't found and look for them all
at once with a single iteration over histograms.xml lines?

[mcasas, 2015-01-30 23:45:49]

folks PTAL.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode361
PRESUBMIT.py:361: # Helper function to smartly try to find a histogram name or
prefix in a line.
On 2015/01/30 15:15:36, Alexei Svitkine wrote:
> Make this a """ comment inside the function.

Done.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode364
PRESUBMIT.py:364: return histogram_name in line;
On 2015/01/30 15:15:37, Alexei Svitkine wrote:
> Nit: No need for trailing ;

Done.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode371
PRESUBMIT.py:371: 
On 2015/01/30 15:15:36, Alexei Svitkine wrote:
> Nit: Two empty lines between functions.

Done.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode376
PRESUBMIT.py:376: 
On 2015/01/30 15:15:37, Alexei Svitkine wrote:
> Nit: No blank line.

Done.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode382
PRESUBMIT.py:382: if (f.LocalPath().endswith(('histograms.xml'))):
On 2015/01/30 15:15:36, Alexei Svitkine wrote:
> Nit: No need for outer parens in Python. Same for the if below.

Done.

https://codereview.chromium.org/885783007/diff/20001/PRESUBMIT.py#newcode404
PRESUBMIT.py:404: with open('tools/metrics/histograms/histograms.xml') as
histograms_xml:
On 2015/01/30 15:15:37, Alexei Svitkine wrote:
> It seems inefficient to open the file and iterate through all of its lines
> potentially multiple times.
> 
> Can you collect the list of histograms that weren't found and look for them
all
> at once with a single iteration over histograms.xml lines?

Done.

[Alexei Svitkine (slow), 2015-02-02 15:12:08]

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode364
PRESUBMIT.py:364: line."""
Nit: No fit into one line - e.g. no need to start with "Helper function.", just
... "Tries to find a histogram name"...

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode404
PRESUBMIT.py:404: touched_histograms.remove(histogram_info)
Is it actually safe to remove an item from the list you're iterating over?

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode410
PRESUBMIT.py:410: histogram_name_found = False;
No ;

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode416
PRESUBMIT.py:416: continue
Nit: Just reverse the condition and put the next line in the if.

[mcasas, 2015-02-02 19:24:14]

guys PTAL.

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode364
PRESUBMIT.py:364: line."""
On 2015/02/02 15:12:08, asvitkine (slow this week) wrote:
> Nit: No fit into one line - e.g. no need to start with "Helper function.",
just
> ... "Tries to find a histogram name"...

Done.

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode404
PRESUBMIT.py:404: touched_histograms.remove(histogram_info)
On 2015/02/02 15:12:07, asvitkine (slow this week) wrote:
> Is it actually safe to remove an item from the list you're iterating over?

It turns out that the iterator-like semantics are a 
bit more involved than what we thought. My idea was to
avoid creating a new list of unmatched-histograms but
meh. 

I changed it to building up a list of unmatched 
histograms.

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode410
PRESUBMIT.py:410: histogram_name_found = False;
On 2015/02/02 15:12:08, asvitkine (slow this week) wrote:
> No ;

Argh! Done :)

https://codereview.chromium.org/885783007/diff/40001/PRESUBMIT.py#newcode416
PRESUBMIT.py:416: continue
On 2015/02/02 15:12:08, asvitkine (slow this week) wrote:
> Nit: Just reverse the condition and put the next line in the if.

Done.

[Alexei Svitkine (slow), 2015-02-02 20:25:15]

lgtm

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py#newcode395
PRESUBMIT.py:395: problems = []
Nit: Move this down to closer where it's used.

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py#newcode411
PRESUBMIT.py:411: histogram_name_found =
_FindHistogramNameInLine(histogram_name, line);
Nit: Another ; should go away ;)

[mcasas, 2015-02-02 20:30:56]

mcasas@chromium.org changed reviewers:
+ jochen@chromium.org
- phajdan.jr@chromium.org

[mcasas, 2015-02-02 20:30:57]

jochen@ OWNERS RS/PTAL.

(Pawel is OOO).

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py#newcode395
PRESUBMIT.py:395: problems = []
On 2015/02/02 20:25:15, asvitkine (slow this week) wrote:
> Nit: Move this down to closer where it's used.

Done.

https://codereview.chromium.org/885783007/diff/60001/PRESUBMIT.py#newcode411
PRESUBMIT.py:411: histogram_name_found =
_FindHistogramNameInLine(histogram_name, line);
On 2015/02/02 20:25:15, asvitkine (slow this week) wrote:
> Nit: Another ; should go away ;)

Done.

[jochen (gone - plz use gerrit), 2015-02-04 12:35:49]

lgtm

[mcasas, 2015-02-04 14:16:03]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2015-02-04 14:17:07]

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

[commit-bot: I haz the power, 2015-02-04 14:52:50]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-04 14:53:43]

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