Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699803005:
DevTools: add PRESUBMIT hook for browser_protocol.json in Source/core/inspector (Closed)
|
Created:
3 years, 10 months ago by chenwilliam Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: add PRESUBMIT hook for browser_protocol.json in Source/core/inspector
Previously, if you only changed browser_protocol.json, there would be no
PRESUBMIT hook that ran closure compiler.
This simplifies the logic in Source/devtools so it always runs closure compiler.
It's much faster now and the previous way of whitelisting files made it error-prone.
BUG=none
Review-Url: https://codereview.chromium.org/2699803005
Cr-Commit-Position: refs/heads/master@{#451508}
Committed: https://chromium.googlesource.com/chromium/src/+/202dea0022d76d633088ca326452b87b69da768c
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix #
Total comments: 2
Patch Set 3 : address CL feedback #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== DevTools: add PRESUBMIT hook for browser_protocol.json in Source/core Previously, if you only changed browser_protocol.json, there would be no PRESUBMIT hook that ran closure compiler. This simplifies the logic in Source/devtools so it always runs closure compiler. It's much faster now and the previous way of whitelisting files made it error-prone. BUG=none ========== to ========== DevTools: add PRESUBMIT hook for browser_protocol.json in Source/core/inspector Previously, if you only changed browser_protocol.json, there would be no PRESUBMIT hook that ran closure compiler. This simplifies the logic in Source/devtools so it always runs closure compiler. It's much faster now and the previous way of whitelisting files made it error-prone. BUG=none ==========
chenwilliam@chromium.org changed reviewers: + dgozman@chromium.org, pfeldman@chromium.org
ptal!
https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:5: compile_note = "Be sure to run your patch with Source/devtools/scripts/compile_frontend.py script prior to committing!" Drop this. https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:11: devtools = input_api.os_path.join("third_party", "WebKit", "Source", "devtools") I wonder whether we should instead have a single presubmit check in WebKit/ or WebKit/Source. We can put the actual code into devtools/scripts. WDYT? https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/PRESUBMIT.py:37: compile_note = "Be sure to run your patch by the compile_frontend.py script prior to committing!" Seems unnecessary now.
https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:5: compile_note = "Be sure to run your patch with Source/devtools/scripts/compile_frontend.py script prior to committing!" On 2017/02/17 01:20:18, dgozman wrote: > Drop this. Done. https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:11: devtools = input_api.os_path.join("third_party", "WebKit", "Source", "devtools") On 2017/02/17 01:20:18, dgozman wrote: > I wonder whether we should instead have a single presubmit check in WebKit/ or > WebKit/Source. We can put the actual code into devtools/scripts. WDYT? I considered it and it would reduce code duplication. I like the idea of keeping the presubmit hook in Source/devtools so that one day we can run PRESUBMIT checks on just the devtools frontend checkout. I know it's an idealistic / long-term goal so I'm OK doing it the DRYer way. https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/PRESUBMIT.py:37: compile_note = "Be sure to run your patch by the compile_frontend.py script prior to committing!" On 2017/02/17 01:20:18, dgozman wrote: > Seems unnecessary now. Done.
lgtm https://codereview.chromium.org/2699803005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:9: devtools = input_api.os_path.join("third_party", "WebKit", "Source", "devtools") Let's change this to |input_api.PresubmitLocalPath() + '../../devtools'|
done https://codereview.chromium.org/2699803005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/PRESUBMIT.py (right): https://codereview.chromium.org/2699803005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/PRESUBMIT.py:9: devtools = input_api.os_path.join("third_party", "WebKit", "Source", "devtools") On 2017/02/17 21:38:46, dgozman wrote: > Let's change this to |input_api.PresubmitLocalPath() + '../../devtools'| Done.
The CQ bit was checked by chenwilliam@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2699803005/#ps40001 (title: "address CL feedback")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chenwilliam@chromium.org
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by chenwilliam@chromium.org
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": 40001, "attempt_start_ts": 1487471436073970,
"parent_rev": "232b610b1a9ae4893764f54a0c952f11e1619686", "commit_rev":
"202dea0022d76d633088ca326452b87b69da768c"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: add PRESUBMIT hook for browser_protocol.json in Source/core/inspector Previously, if you only changed browser_protocol.json, there would be no PRESUBMIT hook that ran closure compiler. This simplifies the logic in Source/devtools so it always runs closure compiler. It's much faster now and the previous way of whitelisting files made it error-prone. BUG=none ========== to ========== DevTools: add PRESUBMIT hook for browser_protocol.json in Source/core/inspector Previously, if you only changed browser_protocol.json, there would be no PRESUBMIT hook that ran closure compiler. This simplifies the logic in Source/devtools so it always runs closure compiler. It's much faster now and the previous way of whitelisting files made it error-prone. BUG=none Review-Url: https://codereview.chromium.org/2699803005 Cr-Commit-Position: refs/heads/master@{#451508} Committed: https://chromium.googlesource.com/chromium/src/+/202dea0022d76d633088ca326452... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/202dea0022d76d633088ca326452... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
