Add pretty printing for rappor.xml

Add pretty printing for rappor.xml

BUG=381380
Committed: https://crrev.com/3edb8729b72e1b1c77cda6d2431094079d2a8fad
Cr-Commit-Position: refs/heads/master@{#316634}

[Steven Holte, 2015-02-13 03:08:46]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-02-13 14:36:38]

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/actions/ex...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:844: return PrettyPrint(actions,
actions_dict, comment_nodes)
Should the histograms presubmits also be refactored to use the new common code -
or is that sufficiently different that it doesn't make sense?

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/common/mod...
File tools/metrics/common/models.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/common/mod...
tools/metrics/common/models.py:15: def GetComments(node):
I think we have some server-side code that mirrors some of the code here (i.e.
for internal versions for these files).

Does it make sense to do the same changes there? If so, probably worth doing the
review there first for this refactoring.

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
File tools/metrics/rappor/pretty_print.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print.py:19: parametersType =
models.ObjectNodeType('parameters',
Nit: camelCase isn't a thing in Python style...

I think these should be CONSTANT_CASED, probably with leading _ to indicate
they're internal to this file.

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print.py:77: def HasMissingOwners(metrics):
Can you add some tests?

[Steven Holte, 2015-02-14 01:11:18]

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/actions/ex...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:844: return PrettyPrint(actions,
actions_dict, comment_nodes)
On 2015/02/13 14:36:37, Alexei Svitkine wrote:
> Should the histograms presubmits also be refactored to use the new common code
-
> or is that sufficiently different that it doesn't make sense?

Done.

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/common/mod...
File tools/metrics/common/models.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/common/mod...
tools/metrics/common/models.py:15: def GetComments(node):
On 2015/02/13 14:36:37, Alexei Svitkine wrote:
> I think we have some server-side code that mirrors some of the code here (i.e.
> for internal versions for these files).
> 
> Does it make sense to do the same changes there? If so, probably worth doing
the
> review there first for this refactoring.

I haven't refactored any existing code to use the code in this file.  Only the
rappor.xml is using these classes now, they are just intended to be re-usable.

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
File tools/metrics/rappor/pretty_print.py (right):

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print.py:19: parametersType =
models.ObjectNodeType('parameters',
On 2015/02/13 14:36:37, Alexei Svitkine wrote:
> Nit: camelCase isn't a thing in Python style...
> 
> I think these should be CONSTANT_CASED, probably with leading _ to indicate
> they're internal to this file.

Done.

https://codereview.chromium.org/925753002/diff/20001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print.py:77: def HasMissingOwners(metrics):
On 2015/02/13 14:36:37, Alexei Svitkine wrote:
> Can you add some tests?

Done.

[Alexei Svitkine (slow), 2015-02-17 01:36:54]

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
File tools/metrics/common/presubmit_util.py (right):

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
tools/metrics/common/presubmit_util.py:1: # Copyright (c) 2015 The Chromium
Authors. All rights reserved.
Nit: No (c) - remove throughout.

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
tools/metrics/common/presubmit_util.py:33: # If there is a histograms.xml in the
current working directory, use that.
Update comment.

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/rappor/pre...
File tools/metrics/rappor/pretty_print_test.py (right):

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print_test.py:2: # Copyright 2014 The Chromium
Authors. All rights reserved.
2015

[Alexei Svitkine (slow), 2015-02-17 01:38:00]

LGTM % above comments

[Steven Holte, 2015-02-17 19:33:20]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[Steven Holte, 2015-02-17 19:33:38]

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
File tools/metrics/common/presubmit_util.py (right):

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
tools/metrics/common/presubmit_util.py:1: # Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/02/17 01:36:54, Alexei Svitkine wrote:
> Nit: No (c) - remove throughout.

Done.

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/common/pre...
tools/metrics/common/presubmit_util.py:33: # If there is a histograms.xml in the
current working directory, use that.
On 2015/02/17 01:36:54, Alexei Svitkine wrote:
> Update comment.

Done.

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/rappor/pre...
File tools/metrics/rappor/pretty_print_test.py (right):

https://codereview.chromium.org/925753002/diff/40001/tools/metrics/rappor/pre...
tools/metrics/rappor/pretty_print_test.py:2: # Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/02/17 01:36:54, Alexei Svitkine wrote:
> 2015

Done.

[Steven Holte, 2015-02-17 19:33:50]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2015-02-17 19:35:25]

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

[commit-bot: I haz the power, 2015-02-17 19:55:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-17 19:55:54]

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

[falken, 2015-02-18 02:08:58]

On 2015/02/17 19:55:54, I haz the power (commit-bot) wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/3edb8729b72e1b1c77cda6d2431094079d2a8fad
> Cr-Commit-Position: refs/heads/master@{#316634}

This breaks extract_actions.py.

$ tools/metrics/actions/extract_actions.py
Traceback (most recent call last):
  File "tools/metrics/actions/extract_actions.py", line 852, in <module>
    sys.exit(main(sys.argv))
  File "tools/metrics/actions/extract_actions.py", line 848, in main
    presubmit_util.DoPresubmitMain(argv, 'actions.xml', 'actions.old.xml',
NameError: global name 'presubmit_util' is not defined

[falken, 2015-02-18 02:20:41]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/933103002/ by falken@chromium.org.

The reason for reverting is: This breaks extract_actions.py.

$ tools/metrics/actions/extract_actions.py
Traceback (most recent call last):
  File "tools/metrics/actions/extract_actions.py", line 852, in <module>
    sys.exit(main(sys.argv))
  File "tools/metrics/actions/extract_actions.py", line 848, in main
    presubmit_util.DoPresubmitMain(argv, 'actions.xml', 'actions.old.xml',
NameError: global name 'presubmit_util' is not defined.