Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360853002:
Better handle safe_browsing logic in build dependencies. (Closed)
DescriptionBetter handle safe_browsing logic in build dependencies.
//chrome/browser/resources was depending on targets only needed
if safe_browsing_mode != 0. Also rearrange the logic in
build/config/features.gni to turn off safe_browsing on all
Chromecast builds, including those for Android TV.
BUG=internal b/31340856
Committed: https://crrev.com/3596104c020f13b18131180228bdbddf0e78c0c8
Cr-Commit-Position: refs/heads/master@{#420751}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use integer comparison in grd file #
Total comments: 1
Messages
Total messages: 24 (9 generated)
sanfin@chromium.org changed reviewers: + mbjorge@chromium.org, slan@chromium.org
This Works. Android unittests build on local outside of a virtualenv!
lgtm for the trybots, but you'll need an OWNER. https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni#... chrome/common/features.gni:62: "safe_browsing_mode=$safe_browsing_mode", hmm... AFAIK, this string will resolve to something containing an int (ex. "safe_browsing_mode=2") I'm not 100% positive of that, but I don't know how else it would know to resolve this as a bool. You probably need to do something like (safe_browsing_mode != 0) You can check by calling print(chrome_grit_defines) below. P.S. I'm also not sure how grit interprets an int... Clearly it seems to be working ok
https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni#... chrome/common/features.gni:62: "safe_browsing_mode=$safe_browsing_mode", On 2016/09/21 at 23:41:59, slan wrote: > hmm... AFAIK, this string will resolve to something containing an int (ex. "safe_browsing_mode=2") > > I'm not 100% positive of that, but I don't know how else it would know to resolve this as a bool. You probably need to do something like (safe_browsing_mode != 0) > > You can check by calling print(chrome_grit_defines) below. > > P.S. I'm also not sure how grit interprets an int... Clearly it seems to be working ok https://www.chromium.org/developers/tools-we-use-in-chromium/grit/grit-users-... """ The 'if' element has a single attribute 'expr', the contents of which are evaluated as a Python expression, and the resources within the 'if' element are only output if the result of that expression is true. """
On 2016/09/22 15:02:50, mbjorge wrote: > the contents of which are evaluated as a Python expression, Cool, thanks. I think this is fine then. I will defer to chrome OWNERS on this.
lgtm
Description was changed from ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 ========== to ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 ==========
sanfin@chromium.org changed reviewers: + estade@chromium.org, scottmg@chromium.org
sanfin@chromium.org changed reviewers: + calamity@chromium.org
On 2016/09/21 23:41:59, slan wrote: > lgtm for the trybots, but you'll need an OWNER. > > https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni > File chrome/common/features.gni (right): > > https://codereview.chromium.org/2360853002/diff/1/chrome/common/features.gni#... > chrome/common/features.gni:62: "safe_browsing_mode=$safe_browsing_mode", > hmm... AFAIK, this string will resolve to something containing an int (ex. > "safe_browsing_mode=2") > > I'm not 100% positive of that, but I don't know how else it would know to > resolve this as a bool. You probably need to do something like > (safe_browsing_mode != 0) > > You can check by calling print(chrome_grit_defines) below. > > P.S. I'm also not sure how grit interprets an int... Clearly it seems to be > working ok I tried a build where the integer was set to a nonzero number, and it failed, so I changed the expression to use an integer comparison. I verified that this works; you can take a look at https://cs.chromium.org/chromium/src/tools/grit/grit/node/base.py?l=445 to see how expressions in the grd file are evaluated (basically, it calls Python's eval() function, so we can use just about any Python expression, as long as we define the variable names).
I'm here for build/ I guess? That file lgtm. https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... chrome/browser/browser_resources.grd:115: <if expr="safe_browsing_mode != 0"> I have no idea, but I would have just written <if expr="safe_browsing_mode"> assuming that it's likely python-ish, and that looks like the other <if> conditions nearby.
On 2016/09/22 22:19:21, scottmg wrote: > I'm here for build/ I guess? That file lgtm. > > https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... > File chrome/browser/browser_resources.grd (right): > > https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... > chrome/browser/browser_resources.grd:115: <if expr="safe_browsing_mode != 0"> > I have no idea, but I would have just written > > <if expr="safe_browsing_mode"> > > assuming that it's likely python-ish, and that looks like the other <if> > conditions nearby. The comment above left by sanfin@ explains this line. The code checks that the value returned by eval is a bool: https://cs.chromium.org/chromium/src/tools/grit/grit/node/base.py?rcl=0&l=502
On 2016/09/22 22:27:24, slan wrote: > On 2016/09/22 22:19:21, scottmg wrote: > > I'm here for build/ I guess? That file lgtm. > > > > > https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... > > File chrome/browser/browser_resources.grd (right): > > > > > https://codereview.chromium.org/2360853002/diff/20001/chrome/browser/browser_... > > chrome/browser/browser_resources.grd:115: <if expr="safe_browsing_mode != 0"> > > I have no idea, but I would have just written > > > > <if expr="safe_browsing_mode"> > > > > assuming that it's likely python-ish, and that looks like the other <if> > > conditions nearby. > > The comment above left by sanfin@ explains this line. The code checks that the > value returned by eval is a bool: > > https://cs.chromium.org/chromium/src/tools/grit/grit/node/base.py?rcl=0&l=502 Ah ok. Sorry, I should read the older comments more carefully.
what should I be looking at?
slan@chromium.org changed reviewers: + sky@chromium.org
estade@, I believe you are an OWNER for chrome/browser/browser_resources.grd. +sky@ for OWNERS stamp on chrome/common/features.gni.
.gni LGTM
The CQ bit was checked by sanfin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from slan@chromium.org, mbjorge@chromium.org Link to the patchset: https://codereview.chromium.org/2360853002/#ps20001 (title: "Use integer comparison in grd file")
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.
Description was changed from ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 ========== to ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 ========== to ========== Better handle safe_browsing logic in build dependencies. //chrome/browser/resources was depending on targets only needed if safe_browsing_mode != 0. Also rearrange the logic in build/config/features.gni to turn off safe_browsing on all Chromecast builds, including those for Android TV. BUG=internal b/31340856 Committed: https://crrev.com/3596104c020f13b18131180228bdbddf0e78c0c8 Cr-Commit-Position: refs/heads/master@{#420751} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/3596104c020f13b18131180228bdbddf0e78c0c8 Cr-Commit-Position: refs/heads/master@{#420751} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
