Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2200003002:
Automatically trigger spv2 tryjobs on paint-related changes (Closed)
DescriptionAutomatically trigger spv2 tryjobs on paint-related changes
This patch adds a presubmit hook for adding a spv2 cq tryjob to all
patches affecting "paint-related" directories which currently includes
core/paint, core/layout, platform/graphics/paint, and
platform/graphics/compositing.
BUG=601275
Committed: https://crrev.com/b02023adefda06d9afa8ec55ff892221b073e7bd
Cr-Commit-Position: refs/heads/master@{#410932}
Patch Set 1 #Patch Set 2 : Disable pylint check C0103 #
Total comments: 1
Patch Set 3 : Also include core/layout/compositing in the trigger #
Total comments: 2
Patch Set 4 : 80 columns ftw #Patch Set 5 : 80 columns, take 2 #Patch Set 6 : Cleanup style #Messages
Total messages: 24 (7 generated)
pdr@chromium.org changed reviewers: + chrishtr@chromium.org, dpranke@chromium.org
Tests: positive: https://codereview.chromium.org/2202753003 negative: https://codereview.chromium.org/2194363003 (Please do not CQ yet)
https://codereview.chromium.org/2200003002/diff/20001/third_party/WebKit/PRES... File third_party/WebKit/PRESUBMIT.py (right): https://codereview.chromium.org/2200003002/diff/20001/third_party/WebKit/PRES... third_party/WebKit/PRESUBMIT.py:332: os.path.join('third_party', 'WebKit', 'Source', 'core', 'paint') in file_path): Also: Source/core/layout/compositing/
Description was changed from ========== Automatically trigger spv2 tryjobs on paint-related changes This patch adds a presubmit hook for adding a spv2 cq tryjob to all patches affecting "paint-related" directories which currently includes core/paint, platform/graphics/paint, and platform/graphics/compositing. BUG=601275 ========== to ========== Automatically trigger spv2 tryjobs on paint-related changes This patch adds a presubmit hook for adding a spv2 cq tryjob to all patches affecting "paint-related" directories which currently includes core/paint, core/layout, platform/graphics/paint, and platform/graphics/compositing. BUG=601275 ==========
On 2016/08/01 at 23:44:35, chrishtr wrote: > https://codereview.chromium.org/2200003002/diff/20001/third_party/WebKit/PRES... > File third_party/WebKit/PRESUBMIT.py (right): > > https://codereview.chromium.org/2200003002/diff/20001/third_party/WebKit/PRES... > third_party/WebKit/PRESUBMIT.py:332: os.path.join('third_party', 'WebKit', 'Source', 'core', 'paint') in file_path): > Also: Source/core/layout/compositing/ Good idea, done.
dpranke@chromium.org changed reviewers: + tandrii@chromium.org
I'm not sure this mechanism even still works for this purpose. You should check w/ tandrii@.
https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... File third_party/WebKit/PRESUBMIT.py (right): https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... third_party/WebKit/PRESUBMIT.py:330: if (os.path.join('third_party', 'WebKit', 'Source', 'platform', 'graphics', 'compositing') in file_path or nit: 80 char limit here and below. https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... third_party/WebKit/PRESUBMIT.py:338: def PostUploadHook(cl, change, output_api): # pylint: disable=C0103 frankly, i don't understand why this should happen post upload, and not before when user has a chance to edit description. But other PRESUBMIT.py in chromium achieve this the same way, so let's keep consistency. I filed a bug for improvement: http://crbug.com/633572
On 2016/08/02 at 13:52:37, tandrii wrote: > https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... > File third_party/WebKit/PRESUBMIT.py (right): > > https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... > third_party/WebKit/PRESUBMIT.py:330: if (os.path.join('third_party', 'WebKit', 'Source', 'platform', 'graphics', 'compositing') in file_path or > nit: 80 char limit here and below. Done. I think splitting this into 4 if statements is the cleanest to read but I wouldn't mind changing that to something else if you'd like. > > https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... > third_party/WebKit/PRESUBMIT.py:338: def PostUploadHook(cl, change, output_api): # pylint: disable=C0103 > frankly, i don't understand why this should happen post upload, and not before when user has a chance to edit description. But other PRESUBMIT.py in chromium achieve this the same way, so let's keep consistency. > > I filed a bug for improvement: http://crbug.com/633572 Yeah :/ Another issue is the style suggested in the docs is different from webkit's python style which is why I had to add "pylint: disable=C0103".
On 2016/08/02 18:31:07, pdr. wrote: > On 2016/08/02 at 13:52:37, tandrii wrote: > > > https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... > > File third_party/WebKit/PRESUBMIT.py (right): > > > > > https://codereview.chromium.org/2200003002/diff/40001/third_party/WebKit/PRES... > > third_party/WebKit/PRESUBMIT.py:330: if (os.path.join('third_party', 'WebKit', > 'Source', 'platform', 'graphics', 'compositing') in file_path or > > nit: 80 char limit here and below. > > Done. I think splitting this into 4 if statements is the cleanest to read but I > wouldn't mind changing that to something else if you'd like. Personally, I'd create a set of path parts as a list before for loop, s.t.: parts = [ os.path.join(........), ..., ] for affected_file in change.AffectedFiles(): file_path = affected_file.LocalPath() if any(x in file_path for x in parts): return True return False
ANd anyway, LGTM
On 2016/08/02 at 18:43:04, tandrii wrote: > ANd anyway, LGTM Thanks! In the latest patchset I refactored this to work like you suggested. Mind one more review for a python noob?
On 2016/08/02 18:52:29, pdr. wrote: > On 2016/08/02 at 18:43:04, tandrii wrote: > > ANd anyway, LGTM > > Thanks! In the latest patchset I refactored this to work like you suggested. > Mind one more review for a python noob? Good. LGTM!
The CQ bit was checked by pdr@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/08/09 at 22:56:33, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) @chrishtr, I think I need your owner approval. This is the message I sent to paint-dev: https://groups.google.com/a/chromium.org/forum/#!topic/paint-dev/s2pcooZuJRw
lgtm
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Automatically trigger spv2 tryjobs on paint-related changes This patch adds a presubmit hook for adding a spv2 cq tryjob to all patches affecting "paint-related" directories which currently includes core/paint, core/layout, platform/graphics/paint, and platform/graphics/compositing. BUG=601275 ========== to ========== Automatically trigger spv2 tryjobs on paint-related changes This patch adds a presubmit hook for adding a spv2 cq tryjob to all patches affecting "paint-related" directories which currently includes core/paint, core/layout, platform/graphics/paint, and platform/graphics/compositing. BUG=601275 Committed: https://crrev.com/b02023adefda06d9afa8ec55ff892221b073e7bd Cr-Commit-Position: refs/heads/master@{#410932} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b02023adefda06d9afa8ec55ff892221b073e7bd Cr-Commit-Position: refs/heads/master@{#410932} |
