Add ManagedUserService for profile-specific managed user data.

Add ManagedUserService for profile-specific managed user data.

Because ManagedModeURLFilter is now owned by ProfileIOData, make it refcounted, so ProfileImplIOData and OffTheRecordProfileIOData can share the same filter.

TBR=ben@chromium.org,evan@chromium.org

BUG=169819
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177813

[Bernhard Bauer, 2013-01-14 09:47:07]

Please review.

[Pam (message me for reviews), 2013-01-14 14:12:41]

Needs a bug number and TEST line.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode.h:37: // The ManagedMode class provides
methods to check whether the browser is in
Reword this to match the new world.

Would it be worth renaming the class and methods to match that too? e.g.
ManagedMode --> ManagedUserLock
EnterManagedMode --> LockManagedUser
LeaveManagedMode --> UnlockManagedUser
that kind of thing?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (left):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:91: virtual void
DidStartProvisionalLoadForFrame(
OK, but not really related to this change?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.cc:165: base::ListValue
whitelist;
Is this cleanup actually related to the rest of this change?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.h:27: // on the same thread.
Sorry, this comment isn't clear to me.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:41:
ui_url_filter_->SetDefaultFilteringBehavior(behavior);
Is it worth making sure that we're on the UI thread in these methods?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:55:
ScopedVector<ManagedModeSiteList> site_lists_copy;
You make this copy, then don't do anything with it.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:124: return
profile_->GetPrefs()->GetBoolean(prefs::kProfileIsManaged);
What sets kProfileIsManaged? Will it be possible to manage a previously existing
profile (by installing a content pack and optionally setting the behavior), or
to un-manage a managed one (by uninstalling all content packs)? Or is the
preference kept 1:1 with "there's a content pack installed"?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:130: 2,
Ew. Please at least attach a comment saying what this corresponds to. Even
better would be to cast it from the enum.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:164: 
Extra blank line

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:219: return "Managed Mode";
"Managed User Service"

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2013
...even though most of this comes straight from older files.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:23: class ManagedUserService
: public ProfileKeyedService,
Please add a brief comment describing what this thing is. Also, it would be
helpful to add a couple of words explaining the separation of responsibility
between this and ManagedMode -- the latter does say what it handles, but having
this say "...and this does everything else related to managed users" would help
people know where to add things in the future.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:44: // Returns the URL's
category.
...obtained from one of the installed content-pack site lists.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:47: // Called in the critical
path of drawing the history UI, so needs to be fast.
Returns the list of all known human-readable category names, sorted by ID
number. Called...

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:89: // ManagedModeURLFilter
living on the IO thread.
More than that, really. A bridge from ManagedMode to both URLFilters, one on the
IO thread and one on the UI thread, mediating access and making sure they're
kept in sync.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:107:
scoped_refptr<ManagedModeURLFilter> ui_url_filter_;
It would be worth explaining a little bit here why they're refcounted and what
the ProfileIOData needs them for.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:120: // This method should
only be called if managed mode is active.
I'd say that managed mode is always active in a managed profile, now. (Or that
"managed mode" no longer exists.) What's the current restriction on when this
should be called, and why?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:135: // Returns a copy of the
manual whitelist which is stored in each profile.
whitelist -> blacklist

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:138: // Owns us.
Not anymore.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service_factory.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Guess what?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.cc:29:
ProfileDependencyManager::GetInstance()) {}
Don't we have some dependencies to register? Extensions, at least. Or am I
missing something?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service_factory.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service_unittest.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_unittest.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Hum de dum.

[Bernhard Bauer, 2013-01-15 14:24:44]

On 2013/01/14 14:12:41, Pam wrote:
> Needs a bug number and TEST line.

Added a bug number, but aren't TEST= lines somewhat deprecated (see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/3MotUyKGUp4/fvvrt...

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode.h:37: // The ManagedMode class provides
methods to check whether the browser is in
On 2013/01/14 14:12:42, Pam wrote:
> Reword this to match the new world.

Reworded it a bit. For now though, there still is a managed mode, so I'd like to
keep the rest of the comment until we remove the class entirely.

> Would it be worth renaming the class and methods to match that too? e.g.
> ManagedMode --> ManagedUserLock
> EnterManagedMode --> LockManagedUser
> LeaveManagedMode --> UnlockManagedUser
> that kind of thing?

Sounds good, but probably for another CL?

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_navigation_observer.h (left):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_navigation_observer.h:91: virtual void
DidStartProvisionalLoadForFrame(
On 2013/01/14 14:12:42, Pam wrote:
> OK, but not really related to this change?

Hm, fair enough. Removed.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.cc:165: base::ListValue
whitelist;
On 2013/01/14 14:12:42, Pam wrote:
> Is this cleanup actually related to the rest of this change?

Well, I'm changing the interface to SetManualLists so that I can call it without
passing ownership, which allows me to update both the UI thread and the IO
thread filter using the same ListValue. I could do it in a separate CL, but I
think it makes this one slightly easier.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.h:27: // on the same thread.
On 2013/01/14 14:12:42, Pam wrote:
> Sorry, this comment isn't clear to me.

I tried to clear it up a bit. What I wanted to convey was the combination of
RefCountedThreadSafe and NonThreadSafe (i.e. the refcounting is done in a
threadsafe way, but member access isn't).

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/14 14:12:42, Pam wrote:
> 2013

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:41:
ui_url_filter_->SetDefaultFilteringBehavior(behavior);
On 2013/01/14 14:12:42, Pam wrote:
> Is it worth making sure that we're on the UI thread in these methods?

The filter will check that itself.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:55:
ScopedVector<ManagedModeSiteList> site_lists_copy;
On 2013/01/14 14:12:42, Pam wrote:
> You make this copy, then don't do anything with it.

Oops. Fixed.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:124: return
profile_->GetPrefs()->GetBoolean(prefs::kProfileIsManaged);
On 2013/01/14 14:12:42, Pam wrote:
> What sets kProfileIsManaged?

It's set by the profile creation mechanism when the checkbox is ticked.

> Will it be possible to manage a previously existing
> profile (by installing a content pack and optionally setting the behavior), or
> to un-manage a managed one (by uninstalling all content packs)?

In theory the kProfileIsManaged flag could be set or reset after profile
creation, there just is no UI for that right now.

> Or is the
> preference kept 1:1 with "there's a content pack installed"?

No, the kProfileIsManaged preference is independent of whether there are content
packs installed or not (a managed user could have only manual whitelist entries,
and in theory content packs could also be used by non-managed users).

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:130: 2,
On 2013/01/14 14:12:42, Pam wrote:
> Ew. Please at least attach a comment saying what this corresponds to. Even
> better would be to cast it from the enum.

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:164: 
On 2013/01/14 14:12:42, Pam wrote:
> Extra blank line

Removed.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:219: return "Managed Mode";
On 2013/01/14 14:12:42, Pam wrote:
> "Managed User Service"

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:23: class ManagedUserService
: public ProfileKeyedService,
On 2013/01/14 14:12:42, Pam wrote:
> Please add a brief comment describing what this thing is. Also, it would be
> helpful to add a couple of words explaining the separation of responsibility
> between this and ManagedMode -- the latter does say what it handles, but
having
> this say "...and this does everything else related to managed users" would
help
> people know where to add things in the future.

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:44: // Returns the URL's
category.
On 2013/01/14 14:12:42, Pam wrote:
> ...obtained from one of the installed content-pack site lists.

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:47: // Called in the critical
path of drawing the history UI, so needs to be fast.
On 2013/01/14 14:12:42, Pam wrote:
> Returns the list of all known human-readable category names, sorted by ID
> number. Called...

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:107:
scoped_refptr<ManagedModeURLFilter> ui_url_filter_;
On 2013/01/14 14:12:42, Pam wrote:
> It would be worth explaining a little bit here why they're refcounted and what
> the ProfileIOData needs them for.

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:120: // This method should
only be called if managed mode is active.
On 2013/01/14 14:12:42, Pam wrote:
> I'd say that managed mode is always active in a managed profile, now. (Or that
> "managed mode" no longer exists.) What's the current restriction on when this
> should be called, and why?

The restriction is not really needed anymore (it would now be, "This method
should only be called for a managed profile", but there's really nothing
speaking against installing content packs in a non-managed profile), so removed.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:135: // Returns a copy of the
manual whitelist which is stored in each profile.
On 2013/01/14 14:12:42, Pam wrote:
> whitelist -> blacklist

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.h:138: // Owns us.
On 2013/01/14 14:12:42, Pam wrote:
> Not anymore.

It does, via the ProfileKeyedService mechanism. I updated the comment to that
effect.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service_factory.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.cc:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/01/14 14:12:42, Pam wrote:
> Guess what?

Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.cc:29:
ProfileDependencyManager::GetInstance()) {}
On 2013/01/14 14:12:42, Pam wrote:
> Don't we have some dependencies to register? Extensions, at least. Or am I
> missing something?

True, we need ExtensionSystem. Done.

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service_factory.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service_factory.h:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/01/14 14:12:42, Pam wrote:
> 2013

Done.

[Pam (message me for reviews), 2013-01-15 14:47:52]

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode.h:37: // The ManagedMode class provides
methods to check whether the browser is in
> > ManagedMode --> ManagedUserLock
> > EnterManagedMode --> LockManagedUser
> > LeaveManagedMode --> UnlockManagedUser
> > that kind of thing?
> 
> Sounds good, but probably for another CL?

+1

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.h:27: // on the same thread.
> What I wanted to convey was the combination of
> RefCountedThreadSafe and NonThreadSafe (i.e. the refcounting is done in a
> threadsafe way, but member access isn't).

That right there is clear!

// References to it can be passed around on different threads (the refcounting
// is thread-safe), but the object itself should always be accessed on the
// same thread (member access isn't thread-safe).

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:124: return
profile_->GetPrefs()->GetBoolean(prefs::kProfileIsManaged);
> No, the kProfileIsManaged preference is independent of whether there are
content
> packs installed or not (a managed user could have only manual whitelist
entries,
> and in theory content packs could also be used by non-managed users).

I'm not convinced that kProfileIsManaged is necessary. I envision the checkbox
more as a UI indication that says "I want to see the managed-user wizard",
rather than as creating a truly different kind of user. Is there a case where we
really need to know whether the user was created as a managed user, versus just
knowing whether there are whitelist entries or content packs or whatever
features we're dealing with at the moment? What does "is a managed profile" mean
on its own?

[Bernhard Bauer, 2013-01-15 14:59:55]

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_mode_url_filter.h (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_mode_url_filter.h:27: // on the same thread.
On 2013/01/15 14:47:53, Pam wrote:
> > What I wanted to convey was the combination of
> > RefCountedThreadSafe and NonThreadSafe (i.e. the refcounting is done in a
> > threadsafe way, but member access isn't).
> 
> That right there is clear!
> 
> // References to it can be passed around on different threads (the refcounting
> // is thread-safe), but the object itself should always be accessed on the
> // same thread (member access isn't thread-safe).

Done!

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
File chrome/browser/managed_mode/managed_user_service.cc (right):

https://codereview.chromium.org/11826059/diff/12001/chrome/browser/managed_mo...
chrome/browser/managed_mode/managed_user_service.cc:124: return
profile_->GetPrefs()->GetBoolean(prefs::kProfileIsManaged);
On 2013/01/15 14:47:53, Pam wrote:
> > No, the kProfileIsManaged preference is independent of whether there are
> content
> > packs installed or not (a managed user could have only manual whitelist
> entries,
> > and in theory content packs could also be used by non-managed users).
> 
> I'm not convinced that kProfileIsManaged is necessary. I envision the checkbox
> more as a UI indication that says "I want to see the managed-user wizard",
> rather than as creating a truly different kind of user. Is there a case where
we
> really need to know whether the user was created as a managed user, versus
just
> knowing whether there are whitelist entries or content packs or whatever
> features we're dealing with at the moment? What does "is a managed profile"
mean
> on its own?

* Can install extensions/content packs only after authenticating (although we
could do this potentially from outside the profile. Hm.)
* Can't create new profiles
* Opening the profile locks access to other profiles
(* Modifying the managed user settings could potentially happen outside of the
profile, so that's not necessarily something a property of a managed user) 

OTOH, even for things that would happen outside of the profile in the future, we
would need some additional metadata (if it isn't a flag for managed profiles, it
would at least be a list of profiles managed by the custodian user). Having a
boolean flag for now would allow us to migrate to a full-blown
managed-user-custodian relation eventually.

[Pam (message me for reviews), 2013-01-16 13:06:04]

LGTM.

> > What does "is a managed profile" mean on its own?
> 
> * Can install extensions/content packs only after authenticating (although we
> could do this potentially from outside the profile. Hm.)
> * Can't create new profiles
> * Opening the profile locks access to other profiles
> (* Modifying the managed user settings could potentially happen outside of the
> profile, so that's not necessarily something a property of a managed user)

OK, I'm convinced now: it means "show the lock and ask for authentication" (all
the places we need that). Thanks.

[Bernhard Bauer, 2013-01-16 14:39:49]

Adding OWNERS:

Will: ProfileIOData
Evan: WebUI TBR (IWYU-fix after refactoring)
Ben: File additions TBR

[willchan no longer on Chromium, 2013-01-16 17:15:40]

I'm sending most of these ProfileIOData reviews to mmenke nowadays.

https://codereview.chromium.org/11826059/diff/58002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11826059/diff/58002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:490: #if !defined(OS_ANDROID)
We should really do something instead like ENABLE_NOTIFICATIONS above, and then
set the value on a per platform basis in build/common.gypi. I've been lax about
this before, but it's really the right way to do it.

[Ben Goodger (Google), 2013-01-16 18:00:17]

gyp lgtm

[Bernhard Bauer, 2013-01-17 13:49:49]

https://codereview.chromium.org/11826059/diff/58002/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/11826059/diff/58002/chrome/browser/profiles/p...
chrome/browser/profiles/profile_io_data.h:490: #if !defined(OS_ANDROID)
On 2013/01/16 17:15:40, willchan wrote:
> We should really do something instead like ENABLE_NOTIFICATIONS above, and
then
> set the value on a per platform basis in build/common.gypi. I've been lax
about
> this before, but it's really the right way to do it.

Yeah, that's reasonable. Done!

[willchan no longer on Chromium, 2013-01-17 16:24:49]

lgtm

[mmenke, 2013-01-17 17:51:50]

Sorry I'm slow this week.  LGTM as well.

On 2013/01/17 16:24:49, willchan wrote:
> lgtm

[commit-bot: I haz the power, 2013-01-18 22:47:10]

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

[commit-bot: I haz the power, 2013-01-19 03:44:33]

Change committed as 177813

[not at google - send to devlin, 2013-02-01 19:35:50]

https://chromiumcodereview.appspot.com/11826059/diff/80001/chrome/browser/ren...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/11826059/diff/80001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:334:
io_data->managed_mode_url_filter()));
This broke compile with safe_browsing==0 since io_data is only defined if
FULL_SAFE_BROWSING is defined.