Add ManagedModeURLFilter.

Add ManagedModeURLFilter.

ManagedModeURLFilter will eventually allow special treatment (e.g. blocking, notifying) for URLs that aren't on a defined whitelist.

BUG=134417
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151993

[Bernhard Bauer, 2012-08-14 08:19:57]

Please review.

Will: IOThread/ProfileIOData
Dominic: Rest

Thanks!

[battre, 2012-08-14 11:58:33]

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:273: UpdateWhitelist();
missing a break here?

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:318: registrar_.Remove(this,
chrome::NOTIFICATION_EXTENSION_LOADED,
nit: -2 indent

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:340: // TODO(bauerb): Update URL filter with
whitelist.
is this TODO left intentionally?

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_url_filter.cc (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.cc:74: return true;
This looks controversial because a large set of URLs is whitelisted by default.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_url_filter.h (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:25: ~ManagedModeURLFilter();
virtual?

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:31: // won't match any URL.
nit: add comment that this is inactive by default?

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:31: // won't match any URL.
What does it mean "it won't match any URLs"? -> maybe "it reports all URLs as
whitelisted"?

[Bernhard Bauer, 2012-08-14 15:36:57]

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode.cc (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:273: UpdateWhitelist();
On 2012/08/14 11:58:33, battre wrote:
> missing a break here?

Oops. Done.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:318: registrar_.Remove(this,
chrome::NOTIFICATION_EXTENSION_LOADED,
On 2012/08/14 11:58:33, battre wrote:
> nit: -2 indent

Done.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode.cc:340: // TODO(bauerb): Update URL filter with
whitelist.
On 2012/08/14 11:58:33, battre wrote:
> is this TODO left intentionally?

Yes. At the moment the whitelist isn't populated yet, as support for the content
packs that define the whitelist isn't landed yet.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_url_filter.cc (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.cc:74: return true;
On 2012/08/14 11:58:33, battre wrote:
> This looks controversial because a large set of URLs is whitelisted by
default.

The idea was to only filter URLs that actually go over the network, and those
have standard schemes. Are there any specific schemes that you're concerned
about?

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
File chrome/browser/managed_mode_url_filter.h (right):

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:25: ~ManagedModeURLFilter();
On 2012/08/14 11:58:33, battre wrote:
> virtual?

This class is never destructed via a superclass, so a virtual destructor isn't
necessary. It also doesn't have any virtual methods yet, so making the
destructor virtual would add a vtable to the class, for no benefit.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:31: // won't match any URL.
On 2012/08/14 11:58:33, battre wrote:
> nit: add comment that this is inactive by default?

Done.

http://codereview.chromium.org/10824257/diff/12001/chrome/browser/managed_mod...
chrome/browser/managed_mode_url_filter.h:31: // won't match any URL.
On 2012/08/14 11:58:33, battre wrote:
> What does it mean "it won't match any URLs"? -> maybe "it reports all URLs as
> whitelisted"?

Done.

[willchan no longer on Chromium, 2012-08-14 17:12:29]

Please update the changelist description much more thoroughly. Explain what
ManagedModeURLFilter is.

Other than that, lgtm, although I'm not sure that the new files are located
appropriately. I assume a chrome/OWNERS reviewer will comment if needed.

http://codereview.chromium.org/10824257/diff/6002/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10824257/diff/6002/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:194: #if !defined(OS_ANDROID)
Why is this not on for Android?

[Bernhard Bauer, 2012-08-14 23:36:32]

http://codereview.chromium.org/10824257/diff/6002/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10824257/diff/6002/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:194: #if !defined(OS_ANDROID)
On 2012/08/14 17:12:29, willchan wrote:
> Why is this not on for Android?

ManagedModeURLFilter depends on policy code for whitelisting (they share the
pattern syntax, for example), which isn't included on Android. It probably makes
more sense to key it off ENABLE_CONFIGURATION_POLICY than OS_ANDROID though.

[battre, 2012-08-16 14:35:25]

lgtm

[Bernhard Bauer, 2012-08-16 14:47:05]

Ben, can I get an OWNERS review for chrome/?

[Bernhard Bauer, 2012-08-16 14:47:31]

*Ahem* actually adding Ben.

Ben, can I get an OWNERS review for chrome/?

[Ben Goodger (Google), 2012-08-16 17:59:50]

lgtm

[commit-bot: I haz the power, 2012-08-16 18:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10824257/6002

[commit-bot: I haz the power, 2012-08-16 18:48:43]

Try job failure for 10824257-6002 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-08-16 19:24:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/10824257/13004

[commit-bot: I haz the power, 2012-08-17 00:08:20]

Change committed as 151993