Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827411:
Disable uses of URLBlacklist on platforms that don't support it. (Closed)
|
Created:
8 years, 4 months ago by Philippe Modified:
8 years, 4 months ago CC:
chromium-reviews, Ted C, David Trainor- moved to gerrit Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisable uses of URLBlacklist on platforms that don't support it.
This was causing linking issues on the Linux Redux bot.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152396
Patch Set 1 #Patch Set 2 : Fix unused variable error #Patch Set 3 : #Patch Set 4 : #
Total comments: 3
Messages
Total messages: 13 (0 generated)
https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... File chrome/browser/managed_mode_url_filter.cc (right): https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... chrome/browser/managed_mode_url_filter.cc:37: if (!policy::URLBlacklist::FilterToComponents( This is probably what causes the problem, and the reference below too. @bauerb: these calls should probably be moved elsewhere so that managed mode still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt?
https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... File chrome/browser/managed_mode_url_filter.cc (right): https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... chrome/browser/managed_mode_url_filter.cc:37: if (!policy::URLBlacklist::FilterToComponents( On 2012/08/20 14:41:03, Joao da Silva wrote: > This is probably what causes the problem, and the reference below too. > > @bauerb: these calls should probably be moved elsewhere so that managed mode > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? Hm. I don't think we need to support a configuration where Managed Mode works but policies don't. What I'm usually doing is excluding the whole file on platforms where ENABLE_CONFIGURATION_POLICY is not defined (see https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). We should probably key that exclude off enable_configuration_policy instead of OS="android".
https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... File chrome/browser/managed_mode_url_filter.cc (right): https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... chrome/browser/managed_mode_url_filter.cc:37: if (!policy::URLBlacklist::FilterToComponents( On 2012/08/20 14:48:05, Bernhard Bauer wrote: > On 2012/08/20 14:41:03, Joao da Silva wrote: > > This is probably what causes the problem, and the reference below too. > > > > @bauerb: these calls should probably be moved elsewhere so that managed mode > > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? > > Hm. I don't think we need to support a configuration where Managed Mode works > but policies don't. What I'm usually doing is excluding the whole file on > platforms where ENABLE_CONFIGURATION_POLICY is not defined (see > https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). > We should probably key that exclude off enable_configuration_policy instead of > OS="android". SGTM @pliard: chrome_browser.gypi already has some sources excluded when configuration_policy==0. Please add managed_mode.* and managed_mode_url_filter.* there too. Thanks!
On 2012/08/20 14:58:16, Joao da Silva wrote: > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > File chrome/browser/managed_mode_url_filter.cc (right): > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > chrome/browser/managed_mode_url_filter.cc:37: if > (!policy::URLBlacklist::FilterToComponents( > On 2012/08/20 14:48:05, Bernhard Bauer wrote: > > On 2012/08/20 14:41:03, Joao da Silva wrote: > > > This is probably what causes the problem, and the reference below too. > > > > > > @bauerb: these calls should probably be moved elsewhere so that managed mode > > > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? > > > > Hm. I don't think we need to support a configuration where Managed Mode works > > but policies don't. What I'm usually doing is excluding the whole file on > > platforms where ENABLE_CONFIGURATION_POLICY is not defined (see > > > https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). > > We should probably key that exclude off enable_configuration_policy instead of > > OS="android". > > SGTM > > @pliard: chrome_browser.gypi already has some sources excluded when > configuration_policy==0. Please add managed_mode.* and managed_mode_url_filter.* > there too. > > Thanks! I tried in my last patch set to GYP out the whole file which raises new linker issues (see linux_redux try bot). I'm afraid that solving this would add more #ifdefs. What do you think?
On 2012/08/20 16:12:08, Philippe wrote: > On 2012/08/20 14:58:16, Joao da Silva wrote: > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > File chrome/browser/managed_mode_url_filter.cc (right): > > > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > chrome/browser/managed_mode_url_filter.cc:37: if > > (!policy::URLBlacklist::FilterToComponents( > > On 2012/08/20 14:48:05, Bernhard Bauer wrote: > > > On 2012/08/20 14:41:03, Joao da Silva wrote: > > > > This is probably what causes the problem, and the reference below too. > > > > > > > > @bauerb: these calls should probably be moved elsewhere so that managed > mode > > > > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? > > > > > > Hm. I don't think we need to support a configuration where Managed Mode > works > > > but policies don't. What I'm usually doing is excluding the whole file on > > > platforms where ENABLE_CONFIGURATION_POLICY is not defined (see > > > > > > https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). > > > We should probably key that exclude off enable_configuration_policy instead > of > > > OS="android". > > > > SGTM > > > > @pliard: chrome_browser.gypi already has some sources excluded when > > configuration_policy==0. Please add managed_mode.* and > managed_mode_url_filter.* > > there too. > > > > Thanks! > > I tried in my last patch set to GYP out the whole file which raises new linker > issues (see linux_redux try bot). I'm afraid that solving this would add more > #ifdefs. What do you think? Hrm, maybe #ifdef'ing out parts of ManagedModeUrlFilter might be the easier thing after all. Generally, having a non-tree closing, non-CQ-running bot that builds a superset of targets that other bots (android) build seems like asking for trouble like this :-/
On 2012/08/20 16:35:05, Bernhard Bauer wrote: > On 2012/08/20 16:12:08, Philippe wrote: > > On 2012/08/20 14:58:16, Joao da Silva wrote: > > > > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > > File chrome/browser/managed_mode_url_filter.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > > chrome/browser/managed_mode_url_filter.cc:37: if > > > (!policy::URLBlacklist::FilterToComponents( > > > On 2012/08/20 14:48:05, Bernhard Bauer wrote: > > > > On 2012/08/20 14:41:03, Joao da Silva wrote: > > > > > This is probably what causes the problem, and the reference below too. > > > > > > > > > > @bauerb: these calls should probably be moved elsewhere so that managed > > mode > > > > > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? > > > > > > > > Hm. I don't think we need to support a configuration where Managed Mode > > works > > > > but policies don't. What I'm usually doing is excluding the whole file on > > > > platforms where ENABLE_CONFIGURATION_POLICY is not defined (see > > > > > > > > > > https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). > > > > We should probably key that exclude off enable_configuration_policy > instead > > of > > > > OS="android". > > > > > > SGTM > > > > > > @pliard: chrome_browser.gypi already has some sources excluded when > > > configuration_policy==0. Please add managed_mode.* and > > managed_mode_url_filter.* > > > there too. > > > > > > Thanks! > > > > I tried in my last patch set to GYP out the whole file which raises new linker > > issues (see linux_redux try bot). I'm afraid that solving this would add more > > #ifdefs. What do you think? > > Hrm, maybe #ifdef'ing out parts of ManagedModeUrlFilter might be the easier > thing after all. > > Generally, having a non-tree closing, non-CQ-running bot that builds a superset > of targets that other bots (android) build seems like asking for trouble like > this :-/ I agree that this is an unnecessary pain :/ Can we submit the patch set 4 then?
On 2012/08/20 16:41:31, Philippe wrote: > On 2012/08/20 16:35:05, Bernhard Bauer wrote: > > On 2012/08/20 16:12:08, Philippe wrote: > > > On 2012/08/20 14:58:16, Joao da Silva wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > > > File chrome/browser/managed_mode_url_filter.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10827411/diff/10001/chrome/browser/man... > > > > chrome/browser/managed_mode_url_filter.cc:37: if > > > > (!policy::URLBlacklist::FilterToComponents( > > > > On 2012/08/20 14:48:05, Bernhard Bauer wrote: > > > > > On 2012/08/20 14:41:03, Joao da Silva wrote: > > > > > > This is probably what causes the problem, and the reference below too. > > > > > > > > > > > > @bauerb: these calls should probably be moved elsewhere so that > managed > > > mode > > > > > > still works when ENABLE_CONFIGURATION_POLICY is disabled. wdyt? > > > > > > > > > > Hm. I don't think we need to support a configuration where Managed Mode > > > works > > > > > but policies don't. What I'm usually doing is excluding the whole file > on > > > > > platforms where ENABLE_CONFIGURATION_POLICY is not defined (see > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10824257/diff/13004/chrome/chrome_brow...). > > > > > We should probably key that exclude off enable_configuration_policy > > instead > > > of > > > > > OS="android". > > > > > > > > SGTM > > > > > > > > @pliard: chrome_browser.gypi already has some sources excluded when > > > > configuration_policy==0. Please add managed_mode.* and > > > managed_mode_url_filter.* > > > > there too. > > > > > > > > Thanks! > > > > > > I tried in my last patch set to GYP out the whole file which raises new > linker > > > issues (see linux_redux try bot). I'm afraid that solving this would add > more > > > #ifdefs. What do you think? > > > > Hrm, maybe #ifdef'ing out parts of ManagedModeUrlFilter might be the easier > > thing after all. > > > > Generally, having a non-tree closing, non-CQ-running bot that builds a > superset > > of targets that other bots (android) build seems like asking for trouble like > > this :-/ > > I agree that this is an unnecessary pain :/ Can we submit the patch set 4 then? ok. Patch set 4 LGTM!
lgtm stamp from me too then.
On 2012/08/20 16:43:26, Nico wrote: > lgtm stamp from me too then. Thanks guys!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10827411/10001
Change committed as 152396 |
