Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276663002:
Script to populate the XML for the CSS property UseCounter. (Closed)
|
Created:
6 years, 7 months ago by Mike Lawther (Google) Modified:
6 years, 7 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@update-css-histogram Visibility:
Public. |
DescriptionScript to populate the XML for the CSS property UseCounter.
Also includes an updated histograms.xml after running the script.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272474
Patch Set 1 #
Total comments: 19
Patch Set 2 : fixed whitespace issues #
Total comments: 28
Patch Set 3 : review fixes #Patch Set 4 : review nit fixes #Patch Set 5 : comment typo fix #Patch Set 6 : rebased #Patch Set 7 : made update_use_counter_css.py executable #
Messages
Total messages: 21 (0 generated)
I manually checked all the diffs that the script generated in histograms.xml. They're all legit due to to the CSS property name changing in Blink and the XML not being updated.
https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:26: def print_enum_for_dashboard(enum_dict): I wasn't sure where to factor this function to. Suggestions? https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:54: parser = optparse.OptionParser() Is it worth factoring these lines out somewhere?
https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_histogram_enum.py:183: source_enum_path) 4 space indent plz http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent... and http://www.chromium.org/chromium-os/python-style-guidelines https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:30: two vertical spaces plz http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:31: def enum_to_css(enum_name): enum_to_css_property https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:32: """Converts a camel cased enum name to the lower case CSS property""" period plz https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:34: ditto https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:49: bucket_dict[bucket] = enum_to_css(css_property) this call looks weird. how about s/css_property/enum_name/? https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:52: ditto https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:67: 'MappedCSSProperties', read_css_properties(USE_COUNTER_CPP_PATH), 4 space indentation
Thanks for the review! https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_histogram_enum.py:183: source_enum_path) On 2014/05/08 08:06:53, tyoshino wrote: > 4 space indent plz Done. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Indent... > > and > > http://www.chromium.org/chromium-os/python-style-guidelines Thanks for the reference :) https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:26: def print_enum_for_dashboard(enum_dict): On 2014/05/08 07:29:50, Mike Lawther (Google) wrote: > I wasn't sure where to factor this function to. Suggestions? I ended up just pulling it from the original location. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:30: On 2014/05/08 08:06:53, tyoshino wrote: > two vertical spaces plz > > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... Done. It looks like the presubmit picks up some double verts and not others. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:31: def enum_to_css(enum_name): On 2014/05/08 08:06:53, tyoshino wrote: > enum_to_css_property Done. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:32: """Converts a camel cased enum name to the lower case CSS property""" On 2014/05/08 08:06:53, tyoshino wrote: > period plz Done. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:34: On 2014/05/08 08:06:53, tyoshino wrote: > ditto Done. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:49: bucket_dict[bucket] = enum_to_css(css_property) On 2014/05/08 08:06:53, tyoshino wrote: > this call looks weird. > > how about s/css_property/enum_name/? Done. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:52: On 2014/05/08 08:06:53, tyoshino wrote: > ditto Done. https://codereview.chromium.org/276663002/diff/1/tools/metrics/histograms/upd... tools/metrics/histograms/update_use_counter_css.py:67: 'MappedCSSProperties', read_css_properties(USE_COUNTER_CPP_PATH), On 2014/05/08 08:06:53, tyoshino wrote: > 4 space indentation Done.
Hi, apologies for being slow to respond. I haven't had a chance to review this yet, but it's definitely on my radar. I'm aiming to get to it early next week. Thanks for your patience!
Thanks for the script! Looks pretty good -- I just have a bunch of little comments. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36668: <!-- See http://src.chromium.org/viewvc/blink/trunk/Source/core/page/UseCounter.cpp --> nit: Is this line still needed? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_histogram_enum.py:140: source_enum_path): nit: Please document the source_enum_path parameter. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_histogram_enum.py:142: from the {value: 'key'} dictionary |source_enum_values| nit: Please end the sentence with a period. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:29: return re.sub(r'([a-zA-Z])([A-Z])', r'\1-\2', enum_name).lower() Hmm, why does the first captured group have uppercase letters? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:32: def read_css_properties(filename): nit: I think function names are supposed to be written in CamelCase. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') nit: Could you write this as a multi-line regex, so that it's easier to read? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') nit: Do you really need the leading and trailing .*'s? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') Optional nit: I'd name this something like PROPERTY_PATTERN or BUCKET_PATTERN. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:40: bucket_dict = {} Optional nit: I'd name this "properties" https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:44: enum_name = bucket_match.group(1) Optional nit: I'd name this "property"
Thanks for the review! Apologies for my own delay, I was traveling to/from BlinkOn last week. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:36668: <!-- See http://src.chromium.org/viewvc/blink/trunk/Source/core/page/UseCounter.cpp --> On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: Is this line still needed? Not any more :) Removed. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_histogram_enum.py:140: source_enum_path): On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: Please document the source_enum_path parameter. Done. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_histogram_enum.py:142: from the {value: 'key'} dictionary |source_enum_values| On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: Please end the sentence with a period. Done. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:29: return re.sub(r'([a-zA-Z])([A-Z])', r'\1-\2', enum_name).lower() On 2014/05/13 03:11:33, Ilya Sherman wrote: > Hmm, why does the first captured group have uppercase letters? Good question! This was to deal with the property 'z-index', which gets camelCased as ZIndex. This was the only property for which my original regex (I think I just had r'([a-z])([A-Z])') didn't work. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:32: def read_css_properties(filename): On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: I think function names are supposed to be written in CamelCase. Oh OK. I'd seen others in lower case (eg in update_use_counter_feature_enum.py), and reverse engineered the reasoning that lower case was intended to be 'private' and CamelCase was intended to be used externally. But happy to update all these to CamelCase. I'll change update_use_counter_features.py as well. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/13 03:11:33, Ilya Sherman wrote: > Optional nit: I'd name this something like PROPERTY_PATTERN or BUCKET_PATTERN. Did you mean break the regex string itself out and name it? Or the compiled regex? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: Could you write this as a multi-line regex, so that it's easier to read? As in use re.MULTILINE so I can use ^ and $? Now that I've removed the leading and trailing .*'s, is it easier to read? I'm only checking a single line of text at a time. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/13 03:11:33, Ilya Sherman wrote: > nit: Do you really need the leading and trailing .*'s? No. Removed. I had them there because I was using match instead of search down below, and I had it in my head that I was searching from the beginning of the string. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:40: bucket_dict = {} On 2014/05/13 03:11:33, Ilya Sherman wrote: > Optional nit: I'd name this "properties" Agreed. I also changed some of the other names in here to remove 'bucket'. wdyt? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:44: enum_name = bucket_match.group(1) On 2014/05/13 03:11:33, Ilya Sherman wrote: > Optional nit: I'd name this "property" I've left this as 'enum_name', because later down this gets passed into 'enum_to_css_property' to be turned into a property name. Does that still sound OK?
https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:29: return re.sub(r'([a-zA-Z])([A-Z])', r'\1-\2', enum_name).lower() On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > Hmm, why does the first captured group have uppercase letters? > > Good question! This was to deal with the property 'z-index', which gets > camelCased as ZIndex. This was the only property for which my original regex (I > think I just had r'([a-z])([A-Z])') didn't work. Hmm, how odd. Could you please add a comment explaining this? https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > Optional nit: I'd name this something like PROPERTY_PATTERN or BUCKET_PATTERN. > > Did you mean break the regex string itself out and name it? Or the compiled > regex? I meant the compiled regex. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > nit: Could you write this as a multi-line regex, so that it's easier to read? > > As in use re.MULTILINE so I can use ^ and $? Now that I've removed the leading > and trailing .*'s, is it easier to read? I'm only checking a single line of text > at a time. I meant using verbose mode: https://docs.python.org/2/library/re.html#re.VERBOSE https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:40: bucket_dict = {} On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > Optional nit: I'd name this "properties" > > Agreed. I also changed some of the other names in here to remove 'bucket'. wdyt? LG, thanks :) https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:44: enum_name = bucket_match.group(1) On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > Optional nit: I'd name this "property" > > I've left this as 'enum_name', because later down this gets passed into > 'enum_to_css_property' to be turned into a property name. Does that still sound > OK? Ok.
https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/update_use_counter_css.py (right): https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:29: return re.sub(r'([a-zA-Z])([A-Z])', r'\1-\2', enum_name).lower() On 2014/05/20 14:13:17, Ilya Sherman wrote: > On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > > Hmm, why does the first captured group have uppercase letters? > > > > Good question! This was to deal with the property 'z-index', which gets > > camelCased as ZIndex. This was the only property for which my original regex > (I > > think I just had r'([a-z])([A-Z])') didn't work. > > Hmm, how odd. Could you please add a comment explaining this? Done. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/20 14:13:17, Ilya Sherman wrote: > On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > > Optional nit: I'd name this something like PROPERTY_PATTERN or > BUCKET_PATTERN. > > > > Did you mean break the regex string itself out and name it? Or the compiled > > regex? > > I meant the compiled regex. Done. I called it 'ENUM_REGEX', as that's what it's searching for. https://codereview.chromium.org/276663002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/update_use_counter_css.py:38: bucket_finder = re.compile(r'.*CSSProperty(.*):\s*return\s*([0-9]+).*') On 2014/05/20 14:13:17, Ilya Sherman wrote: > On 2014/05/20 07:45:03, Mike Lawther (Google) wrote: > > On 2014/05/13 03:11:33, Ilya Sherman wrote: > > > nit: Could you write this as a multi-line regex, so that it's easier to > read? > > > > As in use re.MULTILINE so I can use ^ and $? Now that I've removed the leading > > and trailing .*'s, is it easier to read? I'm only checking a single line of > text > > at a time. > > I meant using verbose mode: https://docs.python.org/2/library/re.html#re.VERBOSE Gotcha. Done.
LGTM, thanks!
The CQ bit was checked by mikelawther@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikelawther@chromium.org/276663002/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7860) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10670)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
lgtm
The CQ bit was checked by mikelawther@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikelawther@chromium.org/276663002/100001
The CQ bit was checked by mikelawther@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mikelawther@chromium.org/276663002/120001
Message was sent while issue was closed.
Change committed as 272474 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
