Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(435)

Issue 2469353002: Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. (Closed)

Created:
4 years, 1 month ago by Łukasz Anforowicz
Modified:
4 years, 1 month ago
Reviewers:
brettw, jbudorick, alexmos, sky
CC:
chromium-reviews, jam, darin-cc_chromium.org, nasko, site-isolation-reviews_chromium.org, slan, servolk, Paweł Hajdan Jr.
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Convert Site Isolation Win bot to --isolate-extensions [2nd attempt]. This is an attempt to reland https://crrev.com/2459813003 after: 1) proper merging (i.e. moving the diff hunks further down so they apply to Site Isolation Win config, not to Site Isolation Linux as they've accidentally been merged by CQ in the earlier CL) 2) adding data dependencies for the filter files (with extra refactoring / consolidation to hopefully make things easier / less surprising in the future). BUG=545200 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/e8b43a499acd3df1ff12f08f2652d4acca2695fd Cr-Commit-Position: refs/heads/master@{#430603}

Patch Set 1 #

Patch Set 2 : Fixing a silly mistake from the previous patchset - using absolute paths as needed. #

Total comments: 4

Patch Set 3 : Added reference to bug 658853 in the comments. #

Total comments: 4

Patch Set 4 : Introducing testing/buildbot/filters/BUILD.gn #

Patch Set 5 : Rebasing to minimize chance of merge conflicts (and of another https://crbug.com/661447). #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -26 lines) Patch
M chrome/test/BUILD.gn View 1 2 3 4 3 chunks +3 lines, -9 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 3 chunks +1 line, -2 lines 0 comments Download
M testing/buildbot/chromium.fyi.json View 1 2 3 4 13 chunks +15 lines, -15 lines 0 comments Download
A testing/buildbot/filters/BUILD.gn View 1 2 3 1 chunk +40 lines, -0 lines 1 comment Download

Messages

Total messages: 50 (30 generated)
Łukasz Anforowicz
alexmos@, can you take a look please?
4 years, 1 month ago (2016-11-03 00:17:41 UTC) #9
alexmos
LGTM, and thanks for also cleaning up BUILD.gn to add all filter files as dependencies. ...
4 years, 1 month ago (2016-11-03 01:12:15 UTC) #10
Łukasz Anforowicz
Thanks for the review. On 2016/11/03 01:12:15, alexmos wrote: > LGTM, and thanks for also ...
4 years, 1 month ago (2016-11-03 17:44:59 UTC) #13
Łukasz Anforowicz
+slan@, servolk@, phajdan.jr@ to CC / as FYI https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/20001/content/test/BUILD.gn#newcode741 content/test/BUILD.gn:741: "//testing/buildbot/filters/cast-linux.content_browsertests.filter", ...
4 years, 1 month ago (2016-11-03 17:45:47 UTC) #14
Łukasz Anforowicz
sky@, can you take a look please? (BTW: I was a little surprised that both ...
4 years, 1 month ago (2016-11-03 18:07:11 UTC) #16
sky
This looks reasonable to me, but I'm not sure if there is some reason that ...
4 years, 1 month ago (2016-11-03 20:36:08 UTC) #18
Łukasz Anforowicz
brettw@, can you take a look please?
4 years, 1 month ago (2016-11-04 17:08:01 UTC) #21
brettw
https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn#newcode1278 chrome/test/BUILD.gn:1278: # as a dependency. Can you mention here what ...
4 years, 1 month ago (2016-11-04 22:13:10 UTC) #24
Łukasz Anforowicz
Thanks for review - can you take another look please? https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/40001/chrome/test/BUILD.gn#newcode1278 ...
4 years, 1 month ago (2016-11-04 23:27:59 UTC) #27
brettw
lgtm
4 years, 1 month ago (2016-11-07 19:17:15 UTC) #30
Łukasz Anforowicz
Thanks. sky@, could you please take a look at and stamp the changes in testing/buildbot/chromium.fyi.json?
4 years, 1 month ago (2016-11-07 21:52:23 UTC) #33
sky
LGTM
4 years, 1 month ago (2016-11-07 22:05:46 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2469353002/80001
4 years, 1 month ago (2016-11-08 14:04:55 UTC) #40
Łukasz Anforowicz
The failure in linux_chromium_browser_side_navigation_rel is in layout tests and therefore 1) unlikely to be related ...
4 years, 1 month ago (2016-11-08 14:06:16 UTC) #41
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 1 month ago (2016-11-08 14:09:10 UTC) #43
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/e8b43a499acd3df1ff12f08f2652d4acca2695fd Cr-Commit-Position: refs/heads/master@{#430603}
4 years, 1 month ago (2016-11-08 14:10:48 UTC) #45
jbudorick
https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filters/BUILD.gn File testing/buildbot/filters/BUILD.gn (right): https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filters/BUILD.gn#newcode31 testing/buildbot/filters/BUILD.gn:31: source_set("content_browsertests") { Can we rename these source sets? They ...
4 years, 1 month ago (2016-11-09 18:20:44 UTC) #47
Łukasz Anforowicz
On 2016/11/09 18:20:44, jbudorick wrote: > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filters/BUILD.gn > File testing/buildbot/filters/BUILD.gn (right): > > https://codereview.chromium.org/2469353002/diff/80001/testing/buildbot/filters/BUILD.gn#newcode31 > ...
4 years, 1 month ago (2016-11-09 18:32:11 UTC) #48
jbudorick
On 2016/11/09 18:32:11, Łukasz Anforowicz wrote: > On 2016/11/09 18:20:44, jbudorick wrote: > > > ...
4 years, 1 month ago (2016-11-09 18:32:58 UTC) #49
Łukasz Anforowicz
4 years, 1 month ago (2016-11-09 20:28:15 UTC) #50
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2488963003/ by lukasza@chromium.org.

The reason for reverting is: amineer@ points out that https://crbug.com/663785
also affects Canary and Dev builds - the safest / fastest thing to do seems to
be to just revert..

Powered by Google App Engine
This is Rietveld 408576698