Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149503005:
Change actions.txt to actions.xml (Closed)
|
Created:
6 years, 10 months ago by yao Modified:
6 years, 9 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org, M-A Ruel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChange the user action file format from .txt to .xml.
In this way, more information can be added (currently added 'description' and 'owner' for each action)
A few functions are moved from tools/metrics/histograms to tools/metrics/common to be shared by tools/metrics and tools/histograms.
BUG=340735
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255357
Patch Set 1 : Refactor the code. #Patch Set 2 : Make pretty-print histograms work. #Patch Set 3 : Read in txt and output xml. #Patch Set 4 : Read in .xml and output .xml. #Patch Set 5 : Reupload #Patch Set 6 : Update comment. #Patch Set 7 : Update OWNERS file. #Patch Set 8 : Put <owner> before <description>. #
Total comments: 4
Patch Set 9 : Change --similarity to 10. #Patch Set 10 : Add copyright header. #Patch Set 11 : Reupload(Resolve old chunk mismatch) #
Total comments: 12
Patch Set 12 : Adress comments. #Patch Set 13 : Add multiple <owner> tag under <owners> tag. #Patch Set 14 : Rebase, and re-generate actions.xml #Patch Set 15 : Set similarity to 50% #Patch Set 16 : Change Indentation of PrettyPrintNode just for review. #Patch Set 17 : Indent PrettyPrintNode back. #
Total comments: 6
Patch Set 18 : Added presubmit check for actions.xml. Moved histogram style parameter to a new file. #Patch Set 19 : Rebase. #
Total comments: 55
Patch Set 20 : Address code review comments. #
Total comments: 8
Patch Set 21 : Address code review comments 2. #Patch Set 22 : Add 'obsolete' tag. #Patch Set 23 : Add unit test. #Patch Set 24 : Fix function name. #
Total comments: 13
Patch Set 25 : Added unittests. #
Total comments: 2
Patch Set 26 : Preserve top-level comments & update general presubmit. #
Total comments: 22
Patch Set 27 : Address comments #Patch Set 28 : Delete chromeactions.txt #Patch Set 29 : Address comments #Patch Set 30 : Address comments #
Total comments: 14
Patch Set 31 : Address comments for pathch 30. #
Total comments: 2
Patch Set 32 : Change back to direct string search. #
Total comments: 2
Patch Set 33 : Nit fix #
Total comments: 2
Patch Set 34 : Delay file loading to until it's needed. #
Total comments: 3
Patch Set 35 : address comment on patch 34 #Messages
Total messages: 53 (0 generated)
https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:1: import logging Needs some comments / copyright block at the top. https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:45: def PrettyPrintNode(node, xml_style, indent=0): Can you make it so this file is marked as being branched/copied from tools/metrics/histograms/pretty_print.py so the diff is easier to read?
https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:1: import logging On 2014/01/31 19:56:58, Alexei Svitkine wrote: > Needs some comments / copyright block at the top. Done. https://codereview.chromium.org/149503005/diff/140001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:45: def PrettyPrintNode(node, xml_style, indent=0): On 2014/01/31 19:56:58, Alexei Svitkine wrote: > Can you make it so this file is marked as being branched/copied from > tools/metrics/histograms/pretty_print.py so the diff is easier to read? Done.
Generally, looks good. Thanks for working on this! Adding Ilya to help with the review. Also, can you file a crbug and set it in the CL description? https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/O... File tools/metrics/actions/OWNERS (left): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/O... tools/metrics/actions/OWNERS:2: per-file chromeactions.txt=* This CL doesn't seem to be removing chromeactions.txt. Are you planning to keep both files in parallel during a transition period? If not, the old file should be removed at the time that the new file is added. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:41: import diffutil Hmm, can diffutil and pretty_print_xml use the same naming convention? (i.e. standardize on either underscores, or no underscores). https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:632: logging.error('More than 1 child node exist under %s' % tag_name) Nit: exist -> exists https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:633: raise Error() Can't the message above be included in the Error? https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:5: """Utility file for pretty print xml file.""" This description should be more clear, otherwise it's just repeating the file name. For example, mention that this function is used for formatting both histograms and actions XML files. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:51: def PrettyPrintNode(node, xml_style, indent=0): Now that you have the XmlStyle object, can you make this a method of that class, rather than a free-standing function?
The --similarity param for copy/rename detection is set back to 50%. After making PrettyPrintNode as a member function of class XmlStyle (put 2 extra blanks before each line of the function), git won't detect pretty_print_xml.py as a copy from pretty_print.py even if I set --similarity to 5%, and 5% is already way too low. See what has been changed since last review: https://codereview.chromium.org/149503005/diff2/380001:590001/tools/metrics/c... See what has been changed after code is refactored: https://codereview.chromium.org/149503005/diff2/1:590001/tools/metrics/common... (Patch 16 is created only for review, and from path 16 to patch 17, only function PrettyPrintNode's indentation is changed, nothing else.) https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/O... File tools/metrics/actions/OWNERS (left): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/O... tools/metrics/actions/OWNERS:2: per-file chromeactions.txt=* On 2014/02/03 19:37:34, Alexei Svitkine wrote: > This CL doesn't seem to be removing chromeactions.txt. > > Are you planning to keep both files in parallel during a transition period? > > If not, the old file should be removed at the time that the new file is added. Done. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:41: import diffutil On 2014/02/03 19:37:34, Alexei Svitkine wrote: > Hmm, can diffutil and pretty_print_xml use the same naming convention? (i.e. > standardize on either underscores, or no underscores). Done. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:632: logging.error('More than 1 child node exist under %s' % tag_name) On 2014/02/03 19:37:34, Alexei Svitkine wrote: > Nit: exist -> exists Done. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:633: raise Error() On 2014/02/03 19:37:34, Alexei Svitkine wrote: > Can't the message above be included in the Error? Done. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:5: """Utility file for pretty print xml file.""" On 2014/02/03 19:37:34, Alexei Svitkine wrote: > This description should be more clear, otherwise it's just repeating the file > name. For example, mention that this function is used for formatting both > histograms and actions XML files. Done. https://codereview.chromium.org/149503005/diff/380001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:51: def PrettyPrintNode(node, xml_style, indent=0): On 2014/02/03 19:37:34, Alexei Svitkine wrote: > Now that you have the XmlStyle object, can you make this a method of that class, > rather than a free-standing function? Done.
One thing I don't see in this CL is changes to the presubmit that validates the formatting of the XML file, like there for histograms.xml. Can you add that? https://codereview.chromium.org/149503005/diff/650001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/650001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:765: pretty = xml_style.PrettyPrintNode(new_dom) Can you extract the code from line 750 up to this line into a helper function? https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/pretty_print.py (right): https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:17: import diffutil Should this be removed? https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:29: import diffutil Isn't this changed to diff_util? But I don't see where this file actually uses diffutil. Maybe this can be removed too?
https://codereview.chromium.org/149503005/diff/650001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/650001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:765: pretty = xml_style.PrettyPrintNode(new_dom) On 2014/02/11 19:05:31, Alexei Svitkine wrote: > Can you extract the code from line 750 up to this line into a helper function? Done. https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... File tools/metrics/histograms/pretty_print.py (right): https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:17: import diffutil On 2014/02/11 19:05:31, Alexei Svitkine wrote: > Should this be removed? Done. https://codereview.chromium.org/149503005/diff/650001/tools/metrics/histogram... tools/metrics/histograms/pretty_print.py:29: import diffutil On 2014/02/11 19:05:31, Alexei Svitkine wrote: > Isn't this changed to diff_util? > > But I don't see where this file actually uses diffutil. Maybe this can be > removed too? This worked because of the .pyc file. So it's still needed.
Looks good. A few more comments. Any chance you could add some simple sanity tests for this? (e.g. that descriptions/owners are preserved when adding new actions and the like) https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:623: class Action(object): Can you move this closer to where it's used? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:640: as_list: If set to True, returns a list of string. Otherwise, returns a nit: "a list of string" -> " a list of strings" https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:741: xml_style = pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER, I noticed you moved this to its own print_style file for histograms? Maybe do the same here, so that it's consistent? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:761: # Parse and store the XML data currently stored in file. Nit: "in file" -> "in the file" https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:768: actions_dict[action_name] = Action(action_name, description, owners) Nit: Actually, I'd also make a function for this block you're adding, and have it return the two outputs (actions and actions_dict). https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:806: shutil.move(actions_xml_path, 'actions.before.xml') I'd name this actions.old.xml. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... File tools/metrics/histograms/print_style.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. Nit: 2014
Ilya, could you also take a look when you have the chance? I think this is pretty close to be ready to land. Exciting!
https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/O... File tools/metrics/actions/OWNERS (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/O... tools/metrics/actions/OWNERS:2: per-file actions.xml=* I think we should enforce strict OWNERS reviews of this file, at least for a little while, if we actually want people to fill in meaningful owners and descriptions. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... File tools/metrics/actions/PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... tools/metrics/actions/PRESUBMIT.py:13: """Checks that actions.xml is up to date and pretty-printed.""" This PRESUBMIT script only checks that actions.xml is up-to-date if the CL already includes changes to that file. What are we doing to make sure that people don't forget to update the file when they add actions? Note that today, I can forget to add my actions to the file, and the next person to come along can do it for me. Presumably this won't work once we start requiring descriptions and owners for actions. Moreover, a previous developer having forgotten to update actions.xml can cause pain for the next developer who adds actions. Am I misunderstanding something? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... tools/metrics/actions/PRESUBMIT.py:24: 'run extract_actions.py to fix')] Please separate out the cases of not up-to-date vs. not formatted correctly. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3: <!--User action XML--> nit: This comment doesn't seem very useful. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:15: </action> Yikes, this change causes the file to grow more than 10x in length. Chrome on my beefy Mac Pro takes nearly a minute to load this file on the coderview site. Can we go with something more compact? One simple option is to remove the <owners> tag, and not require the <description> to wrap when it doesn't need to. That gives something like: <action name="AboutChrome"> <owner>Please specify the owners of this user action.</owner> <owner>Add more owner tags as needed.</owner> <description>Please enter the description of this user action.</description> </action> Even better IMO would be to not use two lines per default template entry, e.g. by omitting the second <owner> tag (which should be enforceable by reviewers instead). That's still a 5x growth in the file size; but hey, at least that's a 2x improvement... https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:20: function to make sure it's well formatted. If the file content needs be changed, nit: "needs be" -> "needs to be" https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:132: 'Add more owner tags as needed')} nit: Please end the sentence with a period. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:135: _DOC = 'User action XML' nit: Why the leading underscore? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:634: def _ExtractText(parent_dom, tag_name, as_list=True): nit: Why the underscores before the method names? (Alternately, should the other methods in this file be prefixed with underscores?) https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:634: def _ExtractText(parent_dom, tag_name, as_list=True): Having as_list here seems pretty hacky. Why not just have the client code fetch the first element from the list, if need be? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:656: return texts[0] Should it be an error for tests to have a size not equal to 1 in this case? Otherwise, I don't understand why I'd ever want to call this method with as_list set to False -- I'd potentially be losing data. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:668: <owner>Owner </owner> nit: Spurious space after "Owner"? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:675: If action_name is in actions_dict, the values to be inserted is based on the nit: "values ... is" -> "values ... are" https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:803: sys.exit(0) This should probably be sys.exit(1), since the script failed. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:810: print ('Updated %s. Do not forget to add it to your changelist' % Optional nit: "Do not" -> "Don't" (reads more naturally, IMO) https://codereview.chromium.org/149503005/diff/800001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:3: # found in the LICENSE file. I assume that the contents of this file are not actually brand new. Please re-upload with a lower --similarity index so that it's easier to review this file. Thanks. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... File tools/metrics/histograms/print_style.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:3: # found in the LICENSE file. nit: Please leave a blank line after this one. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:4: """Holds the constants for pretty printing histograms.xml.""" Ditto. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... File tools/metrics/histograms/update_policies.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/update_policies.py:26: POLICY_TEMPLATES_PATH =( nit: Please leave a space between the '=' and the '('
https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:15: </action> On 2014/02/22 07:07:37, Ilya Sherman wrote: > Yikes, this change causes the file to grow more than 10x in length. Chrome on > my beefy Mac Pro takes nearly a minute to load this file on the coderview site. > Can we go with something more compact? One simple option is to remove the > <owners> tag, and not require the <description> to wrap when it doesn't need to. > That gives something like: > > <action name="AboutChrome"> > <owner>Please specify the owners of this user action.</owner> > <owner>Add more owner tags as needed.</owner> > <description>Please enter the description of this user action.</description> > </action> > > Even better IMO would be to not use two lines per default template entry, e.g. > by omitting the second <owner> tag (which should be enforceable by reviewers > instead). That's still a 5x growth in the file size; but hey, at least that's a > 2x improvement... FWIW: It only takes 14s to load on my not-very-beefy MBP at home. Are you using any extension that manipulates things on rietveld?
https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:15: </action> On 2014/02/22 15:04:07, Alexei Svitkine wrote: > On 2014/02/22 07:07:37, Ilya Sherman wrote: > > Yikes, this change causes the file to grow more than 10x in length. Chrome on > > my beefy Mac Pro takes nearly a minute to load this file on the coderview > site. > > Can we go with something more compact? One simple option is to remove the > > <owners> tag, and not require the <description> to wrap when it doesn't need > to. > > That gives something like: > > > > <action name="AboutChrome"> > > <owner>Please specify the owners of this user action.</owner> > > <owner>Add more owner tags as needed.</owner> > > <description>Please enter the description of this user action.</description> > > </action> > > > > Even better IMO would be to not use two lines per default template entry, e.g. > > by omitting the second <owner> tag (which should be enforceable by reviewers > > instead). That's still a 5x growth in the file size; but hey, at least that's > a > > 2x improvement... > > FWIW: It only takes 14s to load on my not-very-beefy MBP at home. Are you using > any extension that manipulates things on rietveld? I'm realizing that this load time is only relevant for this first review -- after that, it'll generate incremental diffs instead -- so apologies for bringing that up. Nevertheless, there are plenty of other advantages to having a more compact output, including smaller file size (less likely to make AppEngine fail) and easier scanability.
https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... File tools/metrics/actions/PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... tools/metrics/actions/PRESUBMIT.py:13: """Checks that actions.xml is up to date and pretty-printed.""" On 2014/02/22 07:07:37, Ilya Sherman wrote: > This PRESUBMIT script only checks that actions.xml is up-to-date if the CL > already includes changes to that file. What are we doing to make sure that > people don't forget to update the file when they add actions? > > Note that today, I can forget to add my actions to the file, and the next person > to come along can do it for me. Presumably this won't work once we start > requiring descriptions and owners for actions. Moreover, a previous developer > having forgotten to update actions.xml can cause pain for the next developer who > adds actions. Am I misunderstanding something? I agree that it's a valid point and it would be good for a general presubmit to detect when these are changed without the actions XML file being changed - but I'm not sure it's in scope for this change. We already have this problem for histograms, for example. Perhaps we can address this by more communication / dev education - e.g. send an email to chromium-dev when this lands. (The reason I'm not just advocating for just doing it in a presubmit for all code is that I'm worried how much time this would add to all developers' workflows - for example I've seen output saying that some check for some NS stuff took many seconds to run. I don't want us to add more overhead like that.)
https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... File tools/metrics/actions/PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... tools/metrics/actions/PRESUBMIT.py:13: """Checks that actions.xml is up to date and pretty-printed.""" On 2014/02/24 18:32:35, Alexei Svitkine wrote: > On 2014/02/22 07:07:37, Ilya Sherman wrote: > > This PRESUBMIT script only checks that actions.xml is up-to-date if the CL > > already includes changes to that file. What are we doing to make sure that > > people don't forget to update the file when they add actions? > > > > Note that today, I can forget to add my actions to the file, and the next > person > > to come along can do it for me. Presumably this won't work once we start > > requiring descriptions and owners for actions. Moreover, a previous developer > > having forgotten to update actions.xml can cause pain for the next developer > who > > adds actions. Am I misunderstanding something? > > I agree that it's a valid point and it would be good for a general presubmit to > detect when these are changed without the actions XML file being changed - but > I'm not sure it's in scope for this change. We already have this problem for > histograms, for example. > > Perhaps we can address this by more communication / dev education - e.g. send an > email to chromium-dev when this lands. > > (The reason I'm not just advocating for just doing it in a presubmit for all > code is that I'm worried how much time this would add to all developers' > workflows - for example I've seen output saying that some check for some NS > stuff took many seconds to run. I don't want us to add more overhead like that.) For histograms.xml, we don't have any automated script to add histograms to the XML file. That results in many histograms not being mapped (:sadface:), but also means that if I'm adding a new histogram to the file, the only diffs I see are related to my changes. With actions.txt, we had a script that would just automatically add the mapping for you. Often people would forget to run it; but the next person to run it would automatically fix that up. It created a bit of confusion for people to see unrelated changes in their uploaded actions.txt files, but it was easy to convince folks that this was ok. Now, forgotten metrics will each show up in later reviews as a huge block of text asking for information from the current developer. We can still tell people that this is ok, but I suspect it's going to cause a lot more confusion that the status quo does. I agree that we shouldn't slow down presubmit times globally. Maybe it would not be too slow to run the crawler over just the files that were actually modified? If so, we might even be able to add a similar crawler + PRESUBMIT for histograms, which would be a big win IMO.
I will create a separate CL for pre-submit check. In the meanwhile, other comments have been addressed. Thanks. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/O... File tools/metrics/actions/OWNERS (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/O... tools/metrics/actions/OWNERS:2: per-file actions.xml=* On 2014/02/22 07:07:37, Ilya Sherman wrote: > I think we should enforce strict OWNERS reviews of this file, at least for a > little while, if we actually want people to fill in meaningful owners and > descriptions. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... File tools/metrics/actions/PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/P... tools/metrics/actions/PRESUBMIT.py:24: 'run extract_actions.py to fix')] On 2014/02/22 07:07:37, Ilya Sherman wrote: > Please separate out the cases of not up-to-date vs. not formatted correctly. It's not trivial to differentiate between 'not up to date' and 'not formatted correctly' based on the code now. Given the fact that running extract_actions.py fixes both problem together, is it necessary to differentiate them? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:3: <!--User action XML--> On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: This comment doesn't seem very useful. Since the structure of this file is rather straight forward, it should be OK to just remove it I guess. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:15: </action> On 2014/02/22 22:04:42, Ilya Sherman wrote: > On 2014/02/22 15:04:07, Alexei Svitkine wrote: > > On 2014/02/22 07:07:37, Ilya Sherman wrote: > > > Yikes, this change causes the file to grow more than 10x in length. Chrome > on > > > my beefy Mac Pro takes nearly a minute to load this file on the coderview > > site. > > > Can we go with something more compact? One simple option is to remove the > > > <owners> tag, and not require the <description> to wrap when it doesn't need > > to. > > > That gives something like: > > > > > > <action name="AboutChrome"> > > > <owner>Please specify the owners of this user action.</owner> > > > <owner>Add more owner tags as needed.</owner> > > > <description>Please enter the description of this user > action.</description> > > > </action> > > > > > > Even better IMO would be to not use two lines per default template entry, > e.g. > > > by omitting the second <owner> tag (which should be enforceable by reviewers > > > instead). That's still a 5x growth in the file size; but hey, at least > that's > > a > > > 2x improvement... > > > > FWIW: It only takes 14s to load on my not-very-beefy MBP at home. Are you > using > > any extension that manipulates things on rietveld? > > I'm realizing that this load time is only relevant for this first review -- > after that, it'll generate incremental diffs instead -- so apologies for > bringing that up. Nevertheless, there are plenty of other advantages to having > a more compact output, including smaller file size (less likely to make > AppEngine fail) and easier scanability. I compacted <description> tag, which becomes 1 line now. For <owner> tag, ideally, if most histograms have the owners field specified in the future, it will be shown in one line as <owner>owner_name</owner> (specified by TAGS_THAT_ALLOW_SINGLE_LINE in extract_actions.py). It's also possible to shorten the default value to "Specify user action's owners. Add more owner tags as needed." which will fit in one line. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:20: function to make sure it's well formatted. If the file content needs be changed, On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: "needs be" -> "needs to be" Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:132: 'Add more owner tags as needed')} On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:135: _DOC = 'User action XML' On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Why the leading underscore? Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:623: class Action(object): On 2014/02/20 22:50:55, Alexei Svitkine wrote: > Can you move this closer to where it's used? Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:634: def _ExtractText(parent_dom, tag_name, as_list=True): On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Why the underscores before the method names? (Alternately, should the > other methods in this file be prefixed with underscores?) In google3, internal/private function names should start with an underscore. It seems there's no such requirement in Chromium? https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:634: def _ExtractText(parent_dom, tag_name, as_list=True): On 2014/02/22 07:07:37, Ilya Sherman wrote: > Having as_list here seems pretty hacky. Why not just have the client code fetch > the first element from the list, if need be? Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:640: as_list: If set to True, returns a list of string. Otherwise, returns a On 2014/02/20 22:50:55, Alexei Svitkine wrote: > nit: "a list of string" -> " a list of strings" Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:656: return texts[0] On 2014/02/22 07:07:37, Ilya Sherman wrote: > Should it be an error for tests to have a size not equal to 1 in this case? > Otherwise, I don't understand why I'd ever want to call this method with as_list > set to False -- I'd potentially be losing data. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:668: <owner>Owner </owner> On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Spurious space after "Owner"? Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:675: If action_name is in actions_dict, the values to be inserted is based on the On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: "values ... is" -> "values ... are" Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:741: xml_style = pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER, On 2014/02/20 22:50:55, Alexei Svitkine wrote: > I noticed you moved this to its own print_style file for histograms? Maybe do > the same here, so that it's consistent? Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:761: # Parse and store the XML data currently stored in file. On 2014/02/20 22:50:55, Alexei Svitkine wrote: > Nit: "in file" -> "in the file" Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:768: actions_dict[action_name] = Action(action_name, description, owners) On 2014/02/20 22:50:55, Alexei Svitkine wrote: > Nit: Actually, I'd also make a function for this block you're adding, and have > it return the two outputs (actions and actions_dict). Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:803: sys.exit(0) On 2014/02/22 07:07:37, Ilya Sherman wrote: > This should probably be sys.exit(1), since the script failed. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:806: shutil.move(actions_xml_path, 'actions.before.xml') On 2014/02/20 22:50:55, Alexei Svitkine wrote: > I'd name this actions.old.xml. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/actions/e... tools/metrics/actions/extract_actions.py:810: print ('Updated %s. Do not forget to add it to your changelist' % On 2014/02/22 07:07:37, Ilya Sherman wrote: > Optional nit: "Do not" -> "Don't" (reads more naturally, IMO) Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/common/pr... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/common/pr... tools/metrics/common/pretty_print_xml.py:3: # found in the LICENSE file. On 2014/02/22 07:07:37, Ilya Sherman wrote: > I assume that the contents of this file are not actually brand new. Please > re-upload with a lower --similarity index so that it's easier to review this > file. Thanks. Hi, Since I indented most of the lines by 2, decreasing --similarity didn't work. Please point to here to see what I have changed: https://codereview.chromium.org/149503005/diff2/1:590001/tools/metrics/common... It's the diff between path 1 and patch 16. Patch 16 is created only for review, and since path 16, only function PrettyPrintNode's indentation is changed, nothing else. Please let me know anything else I could do to make it clearer. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... File tools/metrics/histograms/print_style.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:1: # Copyright 2013 The Chromium Authors. All rights reserved. On 2014/02/20 22:50:55, Alexei Svitkine wrote: > Nit: 2014 Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:3: # found in the LICENSE file. On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/print_style.py:4: """Holds the constants for pretty printing histograms.xml.""" On 2014/02/22 07:07:37, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... File tools/metrics/histograms/update_policies.py (right): https://codereview.chromium.org/149503005/diff/800001/tools/metrics/histogram... tools/metrics/histograms/update_policies.py:26: POLICY_TEMPLATES_PATH =( On 2014/02/22 07:07:37, Ilya Sherman wrote: > nit: Please leave a space between the '=' and the '(' Done.
Note sure if you saw my message at the top of my earlier reply: [Any chance you could add some simple sanity tests for this? (e.g. that descriptions/owners are preserved when adding new actions and the like)] On Mon, Feb 24, 2014 at 2:16 PM, <yiyaoliu@chromium.org> wrote: > I will create a separate CL for pre-submit check. In the meanwhile, other > comments have been addressed. > > Thanks. > > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/OWNERS > File tools/metrics/actions/OWNERS (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/OWNERS#newcode2 > tools/metrics/actions/OWNERS:2: per-file actions.xml=* > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> I think we should enforce strict OWNERS reviews of this file, at least >> > for a > >> little while, if we actually want people to fill in meaningful owners >> > and > >> descriptions. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/PRESUBMIT.py > File tools/metrics/actions/PRESUBMIT.py (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/PRESUBMIT.py#newcode24 > tools/metrics/actions/PRESUBMIT.py:24: 'run extract_actions.py to fix')] > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> Please separate out the cases of not up-to-date vs. not formatted >> > correctly. > > It's not trivial to differentiate between 'not up to date' and 'not > formatted correctly' based on the code now. > Given the fact that running extract_actions.py fixes both problem > together, is it necessary to differentiate them? > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/actions.xml > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/actions.xml#newcode3 > tools/metrics/actions/actions.xml:3: <!--User action XML--> > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: This comment doesn't seem very useful. >> > > Since the structure of this file is rather straight forward, it should > be OK to just remove it I guess. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/actions.xml#newcode15 > tools/metrics/actions/actions.xml:15: </action> > On 2014/02/22 22:04:42, Ilya Sherman wrote: > >> On 2014/02/22 15:04:07, Alexei Svitkine wrote: >> > On 2014/02/22 07:07:37, Ilya Sherman wrote: >> > > Yikes, this change causes the file to grow more than 10x in >> > length. Chrome > >> on >> > > my beefy Mac Pro takes nearly a minute to load this file on the >> > coderview > >> > site. >> > > Can we go with something more compact? One simple option is to >> > remove the > >> > > <owners> tag, and not require the <description> to wrap when it >> > doesn't need > >> > to. >> > > That gives something like: >> > > >> > > <action name="AboutChrome"> >> > > <owner>Please specify the owners of this user action.</owner> >> > > <owner>Add more owner tags as needed.</owner> >> > > <description>Please enter the description of this user >> action.</description> >> > > </action> >> > > >> > > Even better IMO would be to not use two lines per default template >> > entry, > >> e.g. >> > > by omitting the second <owner> tag (which should be enforceable by >> > reviewers > >> > > instead). That's still a 5x growth in the file size; but hey, at >> > least > >> that's >> > a >> > > 2x improvement... >> > >> > FWIW: It only takes 14s to load on my not-very-beefy MBP at home. >> > Are you > >> using >> > any extension that manipulates things on rietveld? >> > > I'm realizing that this load time is only relevant for this first >> > review -- > >> after that, it'll generate incremental diffs instead -- so apologies >> > for > >> bringing that up. Nevertheless, there are plenty of other advantages >> > to having > >> a more compact output, including smaller file size (less likely to >> > make > >> AppEngine fail) and easier scanability. >> > > I compacted <description> tag, which becomes 1 line now. > > For <owner> tag, ideally, if most histograms have the owners field > specified in the future, it will be shown in one line as > <owner>owner_name</owner> (specified by TAGS_THAT_ALLOW_SINGLE_LINE in > extract_actions.py). > > It's also possible to shorten the default value to "Specify user > action's owners. Add more owner tags as needed." which will fit in one > line. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py > File tools/metrics/actions/extract_actions.py (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode20 > tools/metrics/actions/extract_actions.py:20: function to make sure it's > well formatted. If the file content needs be changed, > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: "needs be" -> "needs to be" >> > > Done. > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode132 > tools/metrics/actions/extract_actions.py:132: 'Add more owner tags as > needed')} > > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Please end the sentence with a period. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode135 > tools/metrics/actions/extract_actions.py:135: _DOC = 'User action XML' > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Why the leading underscore? >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode623 > tools/metrics/actions/extract_actions.py:623: class Action(object): > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> Can you move this closer to where it's used? >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode634 > tools/metrics/actions/extract_actions.py:634: def > _ExtractText(parent_dom, tag_name, as_list=True): > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Why the underscores before the method names? (Alternately, >> > should the > >> other methods in this file be prefixed with underscores?) >> > > In google3, internal/private function names should start with an > underscore. It seems there's no such requirement in Chromium? > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode634 > tools/metrics/actions/extract_actions.py:634: def > _ExtractText(parent_dom, tag_name, as_list=True): > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> Having as_list here seems pretty hacky. Why not just have the client >> > code fetch > >> the first element from the list, if need be? >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode640 > tools/metrics/actions/extract_actions.py:640: as_list: If set to True, > returns a list of string. Otherwise, returns a > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> nit: "a list of string" -> " a list of strings" >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode656 > tools/metrics/actions/extract_actions.py:656: return texts[0] > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> Should it be an error for tests to have a size not equal to 1 in this >> > case? > >> Otherwise, I don't understand why I'd ever want to call this method >> > with as_list > >> set to False -- I'd potentially be losing data. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode668 > tools/metrics/actions/extract_actions.py:668: <owner>Owner </owner> > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Spurious space after "Owner"? >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode675 > tools/metrics/actions/extract_actions.py:675: If action_name is in > actions_dict, the values to be inserted is based on the > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: "values ... is" -> "values ... are" >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode741 > tools/metrics/actions/extract_actions.py:741: xml_style = > pretty_print_xml.XmlStyle(ATTRIBUTE_ORDER, > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> I noticed you moved this to its own print_style file for histograms? >> > Maybe do > >> the same here, so that it's consistent? >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode761 > tools/metrics/actions/extract_actions.py:761: # Parse and store the XML > data currently stored in file. > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> Nit: "in file" -> "in the file" >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode768 > tools/metrics/actions/extract_actions.py:768: actions_dict[action_name] > = Action(action_name, description, owners) > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> Nit: Actually, I'd also make a function for this block you're adding, >> > and have > >> it return the two outputs (actions and actions_dict). >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode803 > tools/metrics/actions/extract_actions.py:803: sys.exit(0) > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> This should probably be sys.exit(1), since the script failed. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode806 > tools/metrics/actions/extract_actions.py:806: > shutil.move(actions_xml_path, 'actions.before.xml') > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> I'd name this actions.old.xml. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/actions/extract_actions.py#newcode810 > tools/metrics/actions/extract_actions.py:810: print ('Updated %s. Do not > forget to add it to your changelist' % > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> Optional nit: "Do not" -> "Don't" (reads more naturally, IMO) >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/common/pretty_print_xml.py > File tools/metrics/common/pretty_print_xml.py (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/common/pretty_print_xml.py#newcode3 > tools/metrics/common/pretty_print_xml.py:3: # found in the LICENSE file. > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> I assume that the contents of this file are not actually brand new. >> > Please > >> re-upload with a lower --similarity index so that it's easier to >> > review this > >> file. Thanks. >> > > Hi, > > Since I indented most of the lines by 2, decreasing --similarity didn't > work. > > Please point to here to see what I have changed: > https://codereview.chromium.org/149503005/diff2/1:590001/ > tools/metrics/common/pretty_print_xml.py > > It's the diff between path 1 and patch 16. Patch 16 is created only for > review, and since path 16, only function PrettyPrintNode's indentation > is changed, nothing else. > > Please let me know anything else I could do to make it clearer. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/print_style.py > File tools/metrics/histograms/print_style.py (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/print_style.py#newcode1 > tools/metrics/histograms/print_style.py:1: # Copyright 2013 The Chromium > Authors. All rights reserved. > On 2014/02/20 22:50:55, Alexei Svitkine wrote: > >> Nit: 2014 >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/print_style.py#newcode3 > tools/metrics/histograms/print_style.py:3: # found in the LICENSE file. > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Please leave a blank line after this one. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/print_style.py#newcode4 > tools/metrics/histograms/print_style.py:4: """Holds the constants for > pretty printing histograms.xml.""" > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> Ditto. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/update_policies.py > File tools/metrics/histograms/update_policies.py (right): > > https://codereview.chromium.org/149503005/diff/800001/ > tools/metrics/histograms/update_policies.py#newcode26 > tools/metrics/histograms/update_policies.py:26: POLICY_TEMPLATES_PATH =( > On 2014/02/22 07:07:37, Ilya Sherman wrote: > >> nit: Please leave a space between the '=' and the '(' >> > > Done. > > https://codereview.chromium.org/149503005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/24 19:16:22, yao wrote: > I will create a separate CL for pre-submit check. In the meanwhile, other > comments have been addressed. Thanks. IMO it's wise to land the presubmit check first, so that someone adding new user actions is always reminded to update actions.txt / actions.xml if they haven't already done so. Otherwise, when we switch to the XML format, folks who are doing the right thing (running the script to update actions.xml) will see diffs left over from previous developers' mistakes (i.e. not running the script). These diffs ask questions, like "Who owns this metric?" This creates extra work for someone trying to be a good citizen, which would be a shame. https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/actions.xml:7: needed. A suggestion for a briefer rewording: "Please list the metric's owners. Add more owner tags as needed." Together with the suggestion below, this makes the default <owner> tag fit on a single line. https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9: </owners> I still think that the <owners> tag isn't really worth spending two extra lines on. What's the reason not to go with a flatter structure like the below? <action name="AboutChrome"> <owner>example1@chromium.org</owner> <owner>example2@chromium.org</owner> <description>...</description> </action> https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:635: def _ExtractText(parent_dom, tag_name): nit: It's atypical for this to be declared below the first use (on line 622). https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/common/p... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/common/p... tools/metrics/common/pretty_print_xml.py:8: and actions.xml file. nit: "formatting both histograms.xml and actions.xml file." -> "formatting both histograms.xml and actions.xml."
https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/actions.xml:7: needed. On 2014/02/24 20:38:41, Ilya Sherman wrote: > A suggestion for a briefer rewording: "Please list the metric's owners. Add more > owner tags as needed." Together with the suggestion below, this makes the > default <owner> tag fit on a single line. Done. https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/actions.xml:9: </owners> On 2014/02/24 20:38:41, Ilya Sherman wrote: > I still think that the <owners> tag isn't really worth spending two extra lines > on. What's the reason not to go with a flatter structure like the below? > > <action name="AboutChrome"> > <owner>example1@chromium.org</owner> > <owner>example2@chromium.org</owner> > <description>...</description> > </action> Done. https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:635: def _ExtractText(parent_dom, tag_name): On 2014/02/24 20:38:41, Ilya Sherman wrote: > nit: It's atypical for this to be declared below the first use (on line 622). Done. https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/common/p... File tools/metrics/common/pretty_print_xml.py (right): https://codereview.chromium.org/149503005/diff/1110001/tools/metrics/common/p... tools/metrics/common/pretty_print_xml.py:8: and actions.xml file. On 2014/02/24 20:38:41, Ilya Sherman wrote: > nit: "formatting both histograms.xml and actions.xml file." -> "formatting both > histograms.xml and actions.xml." Done.
Sorry I forgot to add the test. Will push another patch. Please not review yet. On Tue, Feb 25, 2014 at 3:08 PM, <yiyaoliu@chromium.org> wrote: > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/actions/actions.xml > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/actions/actions.xml#newcode7 > tools/metrics/actions/actions.xml:7: needed. > On 2014/02/24 20:38:41, Ilya Sherman wrote: > >> A suggestion for a briefer rewording: "Please list the metric's >> > owners. Add more > >> owner tags as needed." Together with the suggestion below, this makes >> > the > >> default <owner> tag fit on a single line. >> > > Done. > > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/actions/actions.xml#newcode9 > tools/metrics/actions/actions.xml:9: </owners> > On 2014/02/24 20:38:41, Ilya Sherman wrote: > >> I still think that the <owners> tag isn't really worth spending two >> > extra lines > >> on. What's the reason not to go with a flatter structure like the >> > below? > > <action name="AboutChrome"> >> <owner>example1@chromium.org</owner> >> <owner>example2@chromium.org</owner> >> <description>...</description> >> </action> >> > > Done. > > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/actions/extract_actions.py > File tools/metrics/actions/extract_actions.py (right): > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/actions/extract_actions.py#newcode635 > tools/metrics/actions/extract_actions.py:635: def > _ExtractText(parent_dom, tag_name): > On 2014/02/24 20:38:41, Ilya Sherman wrote: > >> nit: It's atypical for this to be declared below the first use (on >> > line 622). > > Done. > > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/common/pretty_print_xml.py > File tools/metrics/common/pretty_print_xml.py (right): > > https://codereview.chromium.org/149503005/diff/1110001/ > tools/metrics/common/pretty_print_xml.py#newcode8 > tools/metrics/common/pretty_print_xml.py:8: and actions.xml file. > On 2014/02/24 20:38:41, Ilya Sherman wrote: > >> nit: "formatting both histograms.xml and actions.xml file." -> >> > "formatting both > >> histograms.xml and actions.xml." >> > > Done. > > https://codereview.chromium.org/149503005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry for the mistake just now. It's ready for review now. Thanks!
Thanks, this generally looks good! Some nits and comments on testing; but in addition to that, I still think it would be a good idea to get the PRESUBMIT working prior to landing this change. What do you guys think? https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:661: # There are at most one obsolete tag for each user action. nit: "There are ... one" -> "There is ... one". https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:684: <obsolete> is an optional tag. It's added to user actions that are no long nit: "no long" -> "no longer" https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:2: # Copyright (c) 2014 The Chromium Authors. All rights reserved. Ultra nit: Just "Copyright 2014" (i.e. no "(c)") https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:19: </action> Please test edge cases as well. What happens if there is just one <owner> tag? What about 0? What happens if there is an <obsolete> tag? What happens if a required tag is missing, or duplicated? What happens if a tag's value is empty? What formatting requirements are there for the contents of an <owner> tag? https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: actions, actions_dict = extract_actions._ParseActionFile(file_name) Rather than testing the internal function, it's most helpful to test the effective public API. In this case, I think that would essentially be testing that the system is able to start with an actions.xml file, slurp it in, add some automatically aggregated actions, and write it out... plus doing some data validation on the side.
Hi, Sorry I forget to say this last time, I will make sure the presubmit CL will land before this one. Thanks, Yiyao https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:661: # There are at most one obsolete tag for each user action. On 2014/02/26 01:32:18, Ilya Sherman wrote: > nit: "There are ... one" -> "There is ... one". Done. https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:684: <obsolete> is an optional tag. It's added to user actions that are no long On 2014/02/26 01:32:18, Ilya Sherman wrote: > nit: "no long" -> "no longer" Done. https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:2: # Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/26 01:32:18, Ilya Sherman wrote: > Ultra nit: Just "Copyright 2014" (i.e. no "(c)") Done. https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: actions, actions_dict = extract_actions._ParseActionFile(file_name) On 2014/02/26 01:32:18, Ilya Sherman wrote: > Rather than testing the internal function, it's most helpful to test the > effective public API. In this case, I think that would essentially be testing > that the system is able to start with an actions.xml file, slurp it in, add some > automatically aggregated actions, and write it out... plus doing some data > validation on the side. Could you give me a more concrete idea of how you want it to be tested? Since most of the function in main are either hard coded (like AddAboutFlagsActions) or dependent on the file system. It wound't easy to test them as a whole, cause the code base can always be changed and we won't be able to know which action will be there in the future. In the main function, I only added 2 functions, _ParseActionFile and _PrettyPrint. I feel these 2 functions are relatively public. If it's the starting underscore in the function name that makes you uncomfortable, it could be easily changed. The previous comment will be addressed after I understand how test should be done.
https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: actions, actions_dict = extract_actions._ParseActionFile(file_name) On 2014/02/27 14:45:21, yao wrote: > On 2014/02/26 01:32:18, Ilya Sherman wrote: > > Rather than testing the internal function, it's most helpful to test the > > effective public API. In this case, I think that would essentially be testing > > that the system is able to start with an actions.xml file, slurp it in, add > some > > automatically aggregated actions, and write it out... plus doing some data > > validation on the side. > > Could you give me a more concrete idea of how you want it to be tested? > > Since most of the function in main are either hard coded (like > AddAboutFlagsActions) or dependent on the file system. It wound't easy to test > them as a whole, cause the code base can always be changed and we won't be able > to know which action will be there in the future. > > In the main function, I only added 2 functions, _ParseActionFile and > _PrettyPrint. I feel these 2 functions are relatively public. If it's the > starting underscore in the function name that makes you uncomfortable, it could > be easily changed. > > The previous comment will be addressed after I understand how test should be > done. Actually I agree that if a function will be used anywhere else outside of the file it's defined, the function name shouldn't start with an underscore. Will wait for your opinion before I change things.
https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: actions, actions_dict = extract_actions._ParseActionFile(file_name) On 2014/02/27 14:50:55, yao wrote: > On 2014/02/27 14:45:21, yao wrote: > > On 2014/02/26 01:32:18, Ilya Sherman wrote: > > > Rather than testing the internal function, it's most helpful to test the > > > effective public API. In this case, I think that would essentially be > testing > > > that the system is able to start with an actions.xml file, slurp it in, add > > some > > > automatically aggregated actions, and write it out... plus doing some data > > > validation on the side. > > > > Could you give me a more concrete idea of how you want it to be tested? > > > > Since most of the function in main are either hard coded (like > > AddAboutFlagsActions) or dependent on the file system. It wound't easy to test > > them as a whole, cause the code base can always be changed and we won't be > able > > to know which action will be there in the future. > > > > In the main function, I only added 2 functions, _ParseActionFile and > > _PrettyPrint. I feel these 2 functions are relatively public. If it's the > > starting underscore in the function name that makes you uncomfortable, it > could > > be easily changed. > > > > The previous comment will be addressed after I understand how test should be > > done. > > Actually I agree that if a function will be used anywhere else outside of the > file it's defined, the function name shouldn't start with an underscore. > Will wait for your opinion before I change things. I'm not going to be too picky about test coverage -- some is better than none, and I don't want to block this CL indefinitely. I'll defer to Alexei's judgement as to what level of testing is enough for now. But, if you wanted to pursue testing along the lines that I described, I think what you'd need is some way to mock out the input that the extract_actions.py script runs over. Currently, that's essentially hardcoded to be the files in the repository under the 'src' directory. The code would be much more testable -- and probably easier to integrate with PRESUBMIT.py as well -- if it took an input iterator over file- or string-like objects that it could run the various grep steps over.
https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:19: </action> On 2014/02/26 01:32:18, Ilya Sherman wrote: > Please test edge cases as well. What happens if there is just one <owner> tag? > What about 0? What happens if there is an <obsolete> tag? What happens if a > required tag is missing, or duplicated? What happens if a tag's value is empty? > What formatting requirements are there for the contents of an <owner> tag? Edge cases like 0, 1, or more <owner>, <description> and <obsolete> tags are tested. Currently, there's format requirement for <owner> tag and we'll make it pass if the a tag value is empty.
https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1530001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:19: </action> On 2014/03/03 20:44:22, yiyaoliu wrote: > On 2014/02/26 01:32:18, Ilya Sherman wrote: > > Please test edge cases as well. What happens if there is just one <owner> > tag? > > What about 0? What happens if there is an <obsolete> tag? What happens if a > > required tag is missing, or duplicated? What happens if a tag's value is > empty? Sorry typo, there's no format requirement for <owner> tag. > > What formatting requirements are there for the contents of an <owner> tag? > > Edge cases like 0, 1, or more <owner>, <description> and <obsolete> tags are > tested. > Currently, there's format requirement for <owner> tag and we'll make it pass if > the a tag value is empty.
This CL should also update the src/PRESUBMIT.py check you landed. https://codereview.chromium.org/149503005/diff/1600002/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1600002/tools/metrics/actions/... tools/metrics/actions/actions.xml:1: <actions> I think this should have file-level comment. At least, the standard Chromium copyright block and maybe a line or two about the purpose of this file. Speaking of comments, does the current script preserve any XML <!-- --> comments? Ideally, it should along with some tests for that. But if it's not easy to do, we could punt on it for now.
https://codereview.chromium.org/149503005/diff/1600002/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1600002/tools/metrics/actions/... tools/metrics/actions/actions.xml:1: <actions> On 2014/03/03 20:55:53, Alexei Svitkine wrote: > I think this should have file-level comment. > > At least, the standard Chromium copyright block and maybe a line or two about > the purpose of this file. > > Speaking of comments, does the current script preserve any XML <!-- --> > comments? Ideally, it should along with some tests for that. But if it's not > easy to do, we could punt on it for now. No it doesn't preserve XML comments. I also think that it's better if it preserves comments, but I didn't think of an easy way to do so. I modified the code to preserve top-level comments, but it doesn't preserve comments at other places. Is that acceptable? I'll put a todo here.
LGTM % the remaining comments. Thanks. https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py#newcode1034 PRESUBMIT.py:1034: if name_pattern not in current_actions: Hmm, it would probably be more efficient to parse the XML file and build a single set of action names, rather than doing a huge string search each time -- no? https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16: TODO: TODOs should have author names, like so: TODO(yiyaoliu): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:645: # Get top-level comments. It is assumed that all comments are places before nit: "places" -> "placed" https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:35: <action name="action1">\n{owners}{obsolete}{description} nit: Please leave a blank line after the \n, unless that breaks expectations somehow. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:44: ' Add more owner tags as needed.</owner>\n' nit: Odd indentation on this line.
https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py#newcode1034 PRESUBMIT.py:1034: if name_pattern not in current_actions: On 2014/03/03 23:17:34, Ilya Sherman wrote: > Hmm, it would probably be more efficient to parse the XML file and build a > single set of action names, rather than doing a huge string search each time -- > no? Agree that it would be quicker if a search needed. I put the parsing inside the loop to prevent an unnecessary parse when the code does not touch UserMetricsAction function, which should be most of the cases. In this way, the for loop becomes bigger, and a new dependency minidom is introduced. Not sure if the previous way or the current way is better. Redirect to Alexei to make the decision. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/actions.xml:16: TODO: On 2014/03/03 23:17:34, Ilya Sherman wrote: > TODOs should have author names, like so: TODO(yiyaoliu): Done. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:645: # Get top-level comments. It is assumed that all comments are places before On 2014/03/03 23:17:34, Ilya Sherman wrote: > nit: "places" -> "placed" Done. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:35: <action name="action1">\n{owners}{obsolete}{description} On 2014/03/03 23:17:34, Ilya Sherman wrote: > nit: Please leave a blank line after the \n, unless that breaks expectations > somehow. Done. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:44: ' Add more owner tags as needed.</owner>\n' On 2014/03/03 23:17:34, Ilya Sherman wrote: > nit: Odd indentation on this line. I put the blank up to the line above. Is this what you meant?
https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py#newcode1034 PRESUBMIT.py:1034: if name_pattern not in current_actions: On 2014/03/03 23:17:34, Ilya Sherman wrote: > Hmm, it would probably be more efficient to parse the XML file and build a > single set of action names, rather than doing a huge string search each time -- > no? I'm curious as to the answer here as well. Could you try both approaches and report what the perf looks like? (Parsing the XML file will also be more correct since it will ensure the string appears as an action entry, rather than e.g. in a description somewhere.) https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: ACTIONS_XML = """ This should either have a more meaningful name (e.g. call it a format string) or at least a comment. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:119: def _assertActionResult(self, owner, description, obsolete, expected_result, Please add some comments for this method to explain the params and what it does. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:123: with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: How hard would it be to expose a extract_actions.ParseActionString(string) method from the implementation file, so that the tests don't have to bother with this tempfile stuff? https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:130: self.assertEqual(expected_result, pretty) I think it's clearer to have this assert line be in the test bodies themselves and have this function simply return |pretty|. Then, it would be more straightforward to understand what the tests are doing, e.g. xml_result = self._ParseAndFormatXML(...) self.assertEqual(NO_OWNER_EXPECTED_XML, xml_result) https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:167: with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: Seems this logic is common between several places. Can you make a separate helper method for this? (i.e. the formatting above and the tempfile stuff) https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:170: with self.assertRaises(SystemExit) as cm: Can you add a comment explaining what this is doing?
https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1690001/PRESUBMIT.py#newcode1034 PRESUBMIT.py:1034: if name_pattern not in current_actions: On 2014/03/04 15:32:13, Alexei Svitkine wrote: > On 2014/03/03 23:17:34, Ilya Sherman wrote: > > Hmm, it would probably be more efficient to parse the XML file and build a > > single set of action names, rather than doing a huge string search each time > -- > > no? > > I'm curious as to the answer here as well. Could you try both approaches and > report what the perf looks like? (Parsing the XML file will also be more correct > since it will ensure the string appears as an action entry, rather than e.g. in > a description somewhere.) I tried 0, 1, 2, 4 invocations of UserMetricsAction. There's no very big difference. It seems that when there's 1 invocation, parsing xml takes more time (about 2.7s vs 2.5s), and when there's more invocations, parxing xml takes less time(about 2.8s vs 3.1s). (I highly doubt how representative my experiment result is, for the exactly same path, the presubmit could take 2,4s or 3.2s. I could only say there's no very big difference when the number of invocation to UserMetricsAction is small.) Besides, parsing XML is truly more accurate. Let's go with the current parsing XML version? https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:31: ACTIONS_XML = """ On 2014/03/04 15:32:13, Alexei Svitkine wrote: > This should either have a more meaningful name (e.g. call it a format string) or > at least a comment. Done. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:119: def _assertActionResult(self, owner, description, obsolete, expected_result, On 2014/03/04 15:32:13, Alexei Svitkine wrote: > Please add some comments for this method to explain the params and what it does. Done. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:123: with tempfile.NamedTemporaryFile(mode='w', delete=False) as f: On 2014/03/04 15:32:13, Alexei Svitkine wrote: > How hard would it be to expose a extract_actions.ParseActionString(string) > method from the implementation file, so that the tests don't have to bother with > this tempfile stuff? It's actually better to pass in a string. Changed as you suggested. https://codereview.chromium.org/149503005/diff/1690001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:130: self.assertEqual(expected_result, pretty) On 2014/03/04 15:32:13, Alexei Svitkine wrote: > I think it's clearer to have this assert line be in the test bodies themselves > and have this function simply return |pretty|. > > Then, it would be more straightforward to understand what the tests are doing, > e.g. > > xml_result = self._ParseAndFormatXML(...) > self.assertEqual(NO_OWNER_EXPECTED_XML, xml_result) Done.
https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:636: file_content: a string representing the action XML file content. Nit: representing -> containing https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:6: import tempfile Do you still need this? https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:121: def _getProcessedAction(self, owner, description, obsolete, new_action=None, Nit: capitalize, I think. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:123: """Form an actions XML string and returns it after processing. Nit: "Form" -> "Forms" https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:125: It parses the original XML string, add new user actions (if there is any), Nit: "add" -> "adds" and "pretty print" -> "pretty prints" below (same verb tense as "parses"). https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:133: new_action: optional. List of new user actions' names to be inserted. If it's a list, use plural (new_actions) https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:147: self.assertEqual(expected_result, pretty) Delete line.
https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... File tools/metrics/actions/extract_actions.py (right): https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions.py:636: file_content: a string representing the action XML file content. On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Nit: representing -> containing Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... File tools/metrics/actions/extract_actions_test.py (right): https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:6: import tempfile On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Do you still need this? Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:121: def _getProcessedAction(self, owner, description, obsolete, new_action=None, On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Nit: capitalize, I think. Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:123: """Form an actions XML string and returns it after processing. On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Nit: "Form" -> "Forms" Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:125: It parses the original XML string, add new user actions (if there is any), On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Nit: "add" -> "adds" and "pretty print" -> "pretty prints" below (same verb > tense as "parses"). Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:133: new_action: optional. List of new user actions' names to be inserted. On 2014/03/04 18:40:22, Alexei Svitkine wrote: > If it's a list, use plural (new_actions) Done. https://codereview.chromium.org/149503005/diff/1750001/tools/metrics/actions/... tools/metrics/actions/extract_actions_test.py:147: self.assertEqual(expected_result, pretty) On 2014/03/04 18:40:22, Alexei Svitkine wrote: > Delete line. Done.
LGTM, great work! Please sync and update the XML file to make sure you have the latest values, after which it should be good to land.
+maruel Could you please take a look at src/PRESUBMIT.py file? Thanks! Yiyao On Tue, Mar 4, 2014 at 2:01 PM, <asvitkine@chromium.org> wrote: > LGTM, great work! > > Please sync and update the XML file to make sure you have the latest > values, > after which it should be good to land. > > https://codereview.chromium.org/149503005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
-maruel since maruel@ is OOO. +joi. Hi joi, Could you please take a look at file src/PRESUBMIT.py? Thanks! Yiyao On Tue, Mar 4, 2014 at 2:08 PM, Yiyao Liu <yiyaoliu@google.com> wrote: > +maruel > > Could you please take a look at src/PRESUBMIT.py file? Thanks! > > Yiyao > > > > On Tue, Mar 4, 2014 at 2:01 PM, <asvitkine@chromium.org> wrote: > >> LGTM, great work! >> >> Please sync and update the XML file to make sure you have the latest >> values, >> after which it should be good to land. >> >> https://codereview.chromium.org/149503005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py#newcode1035 PRESUBMIT.py:1035: minidom.parse('tools/metrics/actions/actions.xml'). I am concerned about the performance of parsing an XML file every time presubmit checks are run on a change with files that contain actions. An alternative would be to search for name="[action name]" in the file, which would only require a line-by-line scan or just a slurp into memory and a string.contains(). Could you do a quick measurement where you hack PRESUBMIT.py to run _CheckUserActionUpdate 100 times instead of once, and logs the total time it took to run, on a change that includes some new actions?
https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py#newcode1035 PRESUBMIT.py:1035: minidom.parse('tools/metrics/actions/actions.xml'). On 2014/03/05 09:09:35, Jói wrote: > I am concerned about the performance of parsing an XML file every time presubmit > checks are run on a change with files that contain actions. > > An alternative would be to search for name="[action name]" in the file, which > would only require a line-by-line scan or just a slurp into memory and a > string.contains(). > > Could you do a quick measurement where you hack PRESUBMIT.py to run > _CheckUserActionUpdate 100 times instead of once, and logs the total time it > took to run, on a change that includes some new actions? With 1 or 5 user actions code added, the presubmit takes around 14s for both when _CheckUserActionUpdate runs for 100 times
OK, so this adds 140 ms to the presubmit checks any time there is an
action in one of the changed files.
A presubmit check of a single-file changelist takes about 1.4s on my
machine, so this is 10% on top of that, although probably more like
2-5% on top of most typical presubmit checks.
Do you mind doing a comparison with slurping the file into memory and
doing a .contains("name=\"%s\"") on the slurped-in string?
Cheers,
Jói
On Wed, Mar 5, 2014 at 3:07 PM, <yiyaoliu@google.com> wrote:
>
> https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py
> File PRESUBMIT.py (right):
>
>
https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py#newcode1035
> PRESUBMIT.py:1035: minidom.parse('tools/metrics/actions/actions.xml').
> On 2014/03/05 09:09:35, Jói wrote:
>>
>> I am concerned about the performance of parsing an XML file every time
>
> presubmit
>>
>> checks are run on a change with files that contain actions.
>
>
>> An alternative would be to search for name="[action name]" in the
>
> file, which
>>
>> would only require a line-by-line scan or just a slurp into memory and
>
> a
>>
>> string.contains().
>
>
>> Could you do a quick measurement where you hack PRESUBMIT.py to run
>> _CheckUserActionUpdate 100 times instead of once, and logs the total
>
> time it
>>
>> took to run, on a change that includes some new actions?
>
>
> With 1 or 5 user actions code added, the presubmit takes around 14s for
> both when _CheckUserActionUpdate runs for 100 times
>
> https://codereview.chromium.org/149503005/
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Hi, It took less than 4s, you were right, this way is faster. I modified it back to direct string search. Please take a look! Thanks! On Wed, Mar 5, 2014 at 10:39 AM, Jói Sigurðsson <joi@chromium.org> wrote: > OK, so this adds 140 ms to the presubmit checks any time there is an > action in one of the changed files. > > A presubmit check of a single-file changelist takes about 1.4s on my > machine, so this is 10% on top of that, although probably more like > 2-5% on top of most typical presubmit checks. > > Do you mind doing a comparison with slurping the file into memory and > doing a .contains("name=\"%s\"") on the slurped-in string? > > Cheers, > Jói > > > On Wed, Mar 5, 2014 at 3:07 PM, <yiyaoliu@google.com> wrote: > > > > https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py > > File PRESUBMIT.py (right): > > > > > https://codereview.chromium.org/149503005/diff/1770001/PRESUBMIT.py#newcode1035 > > PRESUBMIT.py:1035: minidom.parse('tools/metrics/actions/actions.xml'). > > On 2014/03/05 09:09:35, Jói wrote: > >> > >> I am concerned about the performance of parsing an XML file every time > > > > presubmit > >> > >> checks are run on a change with files that contain actions. > > > > > >> An alternative would be to search for name="[action name]" in the > > > > file, which > >> > >> would only require a line-by-line scan or just a slurp into memory and > > > > a > >> > >> string.contains(). > > > > > >> Could you do a quick measurement where you hack PRESUBMIT.py to run > >> _CheckUserActionUpdate 100 times instead of once, and logs the total > > > > time it > >> > >> took to run, on a change that includes some new actions? > > > > > > With 1 or 5 user actions code added, the presubmit takes around 14s for > > both when _CheckUserActionUpdate runs for 100 times > > > > https://codereview.chromium.org/149503005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Still LGTM % comment below https://codereview.chromium.org/149503005/diff/1830001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1830001/PRESUBMIT.py#newcode14 PRESUBMIT.py:14: from xml.dom import minidom Nit: Remove.
Thanks for making the change, shaving 100 ms off the presubmit runtime is definitely a good thing. There is one last thing I'd ask you to change. https://codereview.chromium.org/149503005/diff/1850001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1850001/PRESUBMIT.py#newcode1023 PRESUBMIT.py:1023: with open('tools/metrics/actions/actions.xml') as f: Please delay reading the file until you have a match (i.e. until line 1032), but load it only once if it is needed.
https://codereview.chromium.org/149503005/diff/1830001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1830001/PRESUBMIT.py#newcode14 PRESUBMIT.py:14: from xml.dom import minidom On 2014/03/05 16:06:37, Alexei Svitkine wrote: > Nit: Remove. Done. https://codereview.chromium.org/149503005/diff/1850001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1850001/PRESUBMIT.py#newcode1023 PRESUBMIT.py:1023: with open('tools/metrics/actions/actions.xml') as f: On 2014/03/05 16:17:45, Jói wrote: > Please delay reading the file until you have a match (i.e. until line 1032), but > load it only once if it is needed. Done.
https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py#newcode1032 PRESUBMIT.py:1032: current_actions Instead, can you put this above the loop: current_actions = None and then change this to just be an if statement rather than relying on an exception?
https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py#newcode1032 PRESUBMIT.py:1032: current_actions On 2014/03/05 18:23:36, Alexei Svitkine wrote: > Instead, can you put this above the loop: > > current_actions = None > > and then change this to just be an if statement rather than relying on an > exception? +1
https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/149503005/diff/1910001/PRESUBMIT.py#newcode1032 PRESUBMIT.py:1032: current_actions On 2014/03/06 12:46:49, Jói wrote: > On 2014/03/05 18:23:36, Alexei Svitkine wrote: > > Instead, can you put this above the loop: > > > > current_actions = None > > > > and then change this to just be an if statement rather than relying on an > > exception? > > +1 Done.
LGTM, thanks
LGTM, thanks!
The CQ bit was checked by yiyaoliu@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yiyaoliu@chromium.org/149503005/1930001
Message was sent while issue was closed.
Change committed as 255357 |
