Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835733002:
Readdress use_counter_feature_enum issue (Closed)
|
Created:
3 years, 8 months ago by lunalu1 Modified:
3 years, 7 months ago Reviewers:
Ilya Sherman CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-frames_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReaddress use_counter_feature_enum issue brought in
https://codereview.chromium.org/2797043002
Made histogram checker raise exception for duplicated enum values.
BUG=711205
Review-Url: https://codereview.chromium.org/2835733002
Cr-Commit-Position: refs/heads/master@{#469169}
Committed: https://chromium.googlesource.com/chromium/src/+/647ac075be56779f27835fc08fd68de07c36d07c
Patch Set 1 #
Total comments: 6
Patch Set 2 : Codereview: nit + defined custom error #
Total comments: 5
Patch Set 3 : Codereview: nit #
Total comments: 4
Patch Set 4 : Codereview update (TODO: rebase after 2841823007) #Patch Set 5 : Rebase after 2841823007 #
Total comments: 4
Patch Set 6 : Codereview update #
Total comments: 1
Patch Set 7 : Codereview: nit (restore a newline) #Messages
Total messages: 38 (24 generated)
Description was changed from ========== Readdress use_counter_feature_num issue Made histogram checker raise exception for duplicated enum values. BUG=711205 ========== to ========== Readdress use_counter_feature_enum issue brought in https://codereview.chromium.org/2797043002 Made histogram checker raise exception for duplicated enum values. BUG=711205 ==========
lunalu@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, could you PTAL? Thanks
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi, thanks for coming back to this CL! https://codereview.chromium.org/2835733002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/PRESUBMIT.py (right): https://codereview.chromium.org/2835733002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/PRESUBMIT.py:90: except Exception as duplicated_value: # pylint: disable=W0703 Please catch only the specific type of exception that's relevant for duplicated_values, rather than swallowing all exceptions. (This is also what the Python warning is warning about. You should not disable this warning.) https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:49: 'k' should be stripped. Please add a "Raises:" section (see go/pystyle) to document the exception. https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:187: changed. Please update the documentation to list the potentially thrown exception. https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:199: source_enum_values, duplicated_values = ReadHistogramValues( Shouldn't this call site be updated? There's no longer a pair being returned, right? https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:202: return False, duplicated_values Please update this as well (presumably by omitting it, since the exception will just bubble up naturally). https://codereview.chromium.org/2835733002/diff/1/tools/metrics/histograms/up... tools/metrics/histograms/update_histogram_enum.py:206: return xml != new_xml, None Please update this as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi isherman. Thanks for your comments. Sorry for my lack of knowledge in Python. I have made the changes according to your review. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:28: def __init__(self, message): Hmm, why did you add extra indentation to all of the lines in this file? https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:34: nit: Please leave two blank lines between top-level definitions. https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:227: raise There's no need to use a try/except here. The natural exception handling flow will do exactly the same thing. (At least, I'm pretty sure that's true. Please let me know if you've noticed a difference in behavior between the two.) https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:278: UpdateHistogramFromDict(Histogram_enum_name, source_enum_values, nit: s/Histogram/histogram (text editor keyboard shortcut typo?)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Thanks for the comments. PTAL https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:28: def __init__(self, message): On 2017/04/26 at 00:18:50, Ilya Sherman wrote: > Hmm, why did you add extra indentation to all of the lines in this file? My bad, I didn't realize chromium and Blink had different style guide on Python.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:61: """ Why did you remove the Returns and Raises documentation here? https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:216: source_enum_path, start_marker, end_marker, strip_k_prefix) ReadHistogramValues now only returns the source_enum_values, and does not have a second duplicated_values return value. Right? Please update this code. (Also, I already mentioned this in a previous comment. Please reply 'Done' to addressed comments, so that it is easier to keep track of what has been addressed and what hasn't.)
Also, please note that you will likely need to rebase this CL on top of https://codereview.chromium.org/2841823007/
Hi isherman. Thanks for your patience reviewing this CL. I apologize for my careless mistakes. Please take another look at the CL and I will make sure I land this after rebasing the change in 2841823007 https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:61: """ On 2017/04/27 at 22:23:15, Ilya Sherman wrote: > Why did you remove the Returns and Raises documentation here? Sorry I must have missed it after reverting this file. I did a revert for the indentation. Done. https://codereview.chromium.org/2835733002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:216: source_enum_path, start_marker, end_marker, strip_k_prefix) On 2017/04/27 at 22:23:15, Ilya Sherman wrote: > ReadHistogramValues now only returns the source_enum_values, and does not have a second duplicated_values return value. Right? Please update this code. (Also, I already mentioned this in a previous comment. Please reply 'Done' to addressed comments, so that it is easier to keep track of what has been addressed and what hasn't.) Done.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Hi, I have rebased after 2841823007. Please take another look. Thanks
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (left): https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:221: return None Why did you remove this return stmt? https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:109: raise DuplicatedValue(result[enum_value], label) Please keep this after strip_k_prefix -- the order was intentionally changed in crrev.com/2841823007
Done. Please take another look. Thanks https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (left): https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:221: return None On 2017/04/28 20:59:49, Ilya Sherman wrote: > Why did you remove this return stmt? My bad, I thought it will just return None by default. Done. https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/update_histogram_enum.py (right): https://codereview.chromium.org/2835733002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/update_histogram_enum.py:109: raise DuplicatedValue(result[enum_value], label) On 2017/04/28 20:59:49, Ilya Sherman wrote: > Please keep this after strip_k_prefix -- the order was intentionally changed in > crrev.com/2841823007 That makes sense. Sorry for the mistake. Done.
LGTM, thanks. https://codereview.chromium.org/2835733002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/update_histogram_enum.py (left): https://codereview.chromium.org/2835733002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/update_histogram_enum.py:220: nit: Please restore this newline.
The CQ bit was checked by lunalu@chromium.org
The CQ bit was unchecked by lunalu@chromium.org
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2835733002/#ps110001 (title: "Codereview: nit (restore a newline)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 110001, "attempt_start_ts": 1493848811830340,
"parent_rev": "62575979076864abba50a3a523f70be61be56f94", "commit_rev":
"647ac075be56779f27835fc08fd68de07c36d07c"}
Message was sent while issue was closed.
Description was changed from ========== Readdress use_counter_feature_enum issue brought in https://codereview.chromium.org/2797043002 Made histogram checker raise exception for duplicated enum values. BUG=711205 ========== to ========== Readdress use_counter_feature_enum issue brought in https://codereview.chromium.org/2797043002 Made histogram checker raise exception for duplicated enum values. BUG=711205 Review-Url: https://codereview.chromium.org/2835733002 Cr-Commit-Position: refs/heads/master@{#469169} Committed: https://chromium.googlesource.com/chromium/src/+/647ac075be56779f27835fc08fd6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001) as https://chromium.googlesource.com/chromium/src/+/647ac075be56779f27835fc08fd6... |
