Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

TBR=calamity@chromium.org
BUG=704648
Review-Url: https://codereview.chromium.org/2776493005
Cr-Commit-Position: refs/heads/master@{#460940}
Committed: https://chromium.googlesource.com/chromium/src/+/db2637ff32cb9a619f1e2920293eb2e68f20ac4c

[mmenke, 2017-03-24 19:35:56]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 19:36:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 19:49:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 19:49:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-03-27 14:42:35]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 14:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 14:45:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 14:45:57]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2017-03-27 15:03:49]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 15:04:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 15:18:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 15:18:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-03-27 15:26:53]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 15:27:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 15:39:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 15:39:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-03-27 15:57:53]

Description was changed from

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext (As there's
now only one URLFilter, not one for the UI thread and one for the IO
thread).

BUG=704648
==========

to

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
copy std::maps when updating the filter, etc.

BUG=704648
==========

[mmenke, 2017-03-27 15:58:02]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[mmenke, 2017-03-27 15:58:11]

Description was changed from

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
copy std::maps when updating the filter, etc.

BUG=704648
==========

to

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

BUG=704648
==========

[commit-bot: I haz the power, 2017-03-27 15:58:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-03-27 16:01:47]

Patchset #6 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-27 16:10:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 16:10:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-03-27 16:26:48]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:27:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-03-27 16:27:38]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:28:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 16:42:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 16:42:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-03-27 16:44:58]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:45:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 16:57:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 16:57:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2017-03-27 17:10:40]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 17:11:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 17:39:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 17:39:14]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2017-03-27 18:18:50]

Patchset #7 (id:140001) has been deleted

[mmenke, 2017-03-27 18:18:58]

Patchset #2 (id:20001) has been deleted

[mmenke, 2017-03-27 18:21:18]

Description was changed from

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

BUG=704648
==========

to

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

BUG=704648
==========

[mmenke, 2017-03-27 18:21:34]

Patchset #7 (id:180001) has been deleted

[mmenke, 2017-03-27 18:21:47]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 18:22:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-03-27 18:34:31]

mmenke@chromium.org changed reviewers:
+ treib@chromium.org

[mmenke, 2017-03-27 18:34:32]

[treib]:  Since there are already browser tests, and they're all passing, I'm
assuming this CL doesn't break anything, though the WebUI change does mean it
will show more changes than it used to.

I don't see any browser tests for the WebUI stuff, and am not sure how to test
that locally, other than to fool around with it a bit, but I'm not a supervised
user, which makes it a bit less meaningful.

I did a little cleanup, and more may be doable, but think its beyond the scope
of this CL (Could perhaps merge the NavigationThrottle and NavigationObserver,
for instance, or merge the filter with the service)

There's no rush on this review (I'll ping if I don't hear back in a week, but
this isn't blocking anything at all).

Once you sign off, I plan to TBR the rest of the files, since I own profiles/
and loader/, chrome_content_browser_client.cc has an * rule, and the rest you
own or is just an API rename.  Just mentioning now in case you disagree, and so
you know I plan to only use you as a reviewer.

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.h
(right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Not sure if I should update this - this file started life as the old
ResourceThrottle, so not really a new source file.

[commit-bot: I haz the power, 2017-03-27 20:07:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 20:07:46]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-03-28 08:37:52]

Some early comments before starting the actual review.

On 2017/03/27 18:34:32, mmenke wrote:
> [treib]:  Since there are already browser tests, and they're all passing, I'm
> assuming this CL doesn't break anything, though the WebUI change does mean it
> will show more changes than it used to.
> 
> I don't see any browser tests for the WebUI stuff, and am not sure how to test
> that locally, other than to fool around with it a bit, but I'm not a
supervised
> user, which makes it a bit less meaningful.

You can create a supervised user: In the "create profile" dialog (from
chrome://settings, or via avatar menu -> Manage People -> Add Person) there's a
checkbox "Supervise this user". (You have to be signed-in to Chrome for that to
work.)

> I did a little cleanup, and more may be doable, but think its beyond the scope
> of this CL (Could perhaps merge the NavigationThrottle and NavigationObserver,
> for instance, or merge the filter with the service)

I don't know about NavigationThrottle and NavigationObserver, maybe there's a
reason we have both? At least one of them exists on both the UI and the IO
thread, maybe that's related?

I'm pretty sure we want to keep the filter separate from the service :)

> There's no rush on this review (I'll ping if I don't hear back in a week, but
> this isn't blocking anything at all).

Thanks for the heads-up! I'll still get started, no point in delaying :)

> Once you sign off, I plan to TBR the rest of the files, since I own profiles/
> and loader/, chrome_content_browser_client.cc has an * rule, and the rest you
> own or is just an API rename.  Just mentioning now in case you disagree, and
so
> you know I plan to only use you as a reviewer.

Yup, that's fine. I'll add bauerb@ though, since I seem to dimly remember that
we considered this change before, but had some reason for not doing it.

[Marc Treib, 2017-03-28 10:22:57]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2017-03-28 10:22:58]

Thanks! Very nice cleanup, just a few minor comments.

Also +bauerb for his thoughts on NavigationThrottle vs ResourceThrottle. I
*think* we had some reasons for staying with ResourceThrottle, but I can't
recall the details or find a bug...

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
(left):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:138: //
Can't use base::MakeUnique because the constructor is private.
nit: Leave the comment?

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.h
(right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.h:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2017/03/27 18:34:32, mmenke wrote:
> Not sure if I should update this - this file started life as the old
> ResourceThrottle, so not really a new source file.

Eh.. either's fine. It's not like anyone particularly cares about the years :)

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service_factory.h (right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service_factory.h:22:
content::BrowserContext* browser_context);
Is this change actually required? If so, the method should probably be renamed
to GetForBrowserContext, but updating all callers won't be fun... maybe just
have both?

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (left):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_url_filter.cc:254:
DCHECK(CalledOnValidThread());
Any reason for removing this?

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/supervised_user_internals_message_handler.h
(right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/supervised_user_internals_message_handler.h:64: bool
is_watching_filter_;
Would a ScopedObserver work here?

[Bernhard Bauer, 2017-03-28 11:16:05]

Oh, neat! I didn't realize that this would allow us to remove the second URL
filter on the IO thread.

https://crbug.com/576604 is the old bug about doing this. The only issue was
that it would now be more work to start filtering resource loads (images, etc.),
but we can cross that bridge if we ever come to it.

[Bernhard Bauer, 2017-03-28 11:18:46]

On 2017/03/27 18:34:32, mmenke wrote:
> [treib]:  Since there are already browser tests, and they're all passing, I'm
> assuming this CL doesn't break anything, though the WebUI change does mean it
> will show more changes than it used to.
> 
> I don't see any browser tests for the WebUI stuff, and am not sure how to test
> that locally, other than to fool around with it a bit, but I'm not a
supervised
> user, which makes it a bit less meaningful.

You can create legacy supervised user profiles for any Google account from the
user manager in Chrome, although asynchronous filtering is only enabled for
child accounts. Ping Marc or me if you need credentials for a testing child
account or want to create one yourself.

[mmenke, 2017-03-28 12:06:29]

A couple quick responses.  I'll update code and do some manual testing later
today.

On 2017/03/28 11:16:05, Bernhard Bauer wrote:
> Oh, neat! I didn't realize that this would allow us to remove the second URL
> filter on the IO thread.
> 
> https://crbug.com/576604 is the old bug about doing this. The only issue was
> that it would now be more work to start filtering resource loads (images,
etc.),
> but we can cross that bridge if we ever come to it.

Certainly true.  We'll probably need to provide an API that can be used to do
that for existing consumers - extensions do it, but it will potentially add
network process to browser process round trips.  May be ways to avoid the round
trips  Early days yet, and I'm just tackling some of the easier consumers so we
can get a better handle on what APIs we currently need.

> I don't know about NavigationThrottle and NavigationObserver, maybe there's a
> reason we have both? At least one of them exists on both the UI and the IO
> thread, maybe that's related?

NavigationThrottle is a new class - having separate classes may have been
necessary because ResourceThrottle lives on the IO thread.  Now that everything
is on the UI thread, and we're using the NavigationThrottle API, it may also be
called when the NavigationObserver is.  Anyhow, I haven't thought about or
investigated this, just thought I'd suggest it as an option.

[Marc Treib, 2017-03-28 12:11:49]

On 2017/03/28 12:06:29, mmenke wrote:
> A couple quick responses.  I'll update code and do some manual testing later
> today.
> 
> On 2017/03/28 11:16:05, Bernhard Bauer wrote:
> > Oh, neat! I didn't realize that this would allow us to remove the second URL
> > filter on the IO thread.
> > 
> > https://crbug.com/576604 is the old bug about doing this. The only issue was
> > that it would now be more work to start filtering resource loads (images,
> etc.),
> > but we can cross that bridge if we ever come to it.
> 
> Certainly true.  We'll probably need to provide an API that can be used to do
> that for existing consumers - extensions do it, but it will potentially add
> network process to browser process round trips.  May be ways to avoid the
round
> trips  Early days yet, and I'm just tackling some of the easier consumers so
we
> can get a better handle on what APIs we currently need.
> 
> > I don't know about NavigationThrottle and NavigationObserver, maybe there's
a
> > reason we have both? At least one of them exists on both the UI and the IO
> > thread, maybe that's related?
> 
> NavigationThrottle is a new class - having separate classes may have been
> necessary because ResourceThrottle lives on the IO thread.  Now that
everything
> is on the UI thread, and we're using the NavigationThrottle API, it may also
be
> called when the NavigationObserver is.  Anyhow, I haven't thought about or
> investigated this, just thought I'd suggest it as an option.

If it's possible to merge the two, I'd certainly be in favor of that - every
time I've had to change anything about the filtering, I had to look at both to
find out where the new code needed to go. I could never remember which of the
two does what exactly.
Anyway, that's for another CL :)

[Bernhard Bauer, 2017-03-28 16:44:28]

On 2017/03/28 12:11:49, Marc Treib wrote:
> On 2017/03/28 12:06:29, mmenke wrote:
> > A couple quick responses.  I'll update code and do some manual testing later
> > today.
> > 
> > On 2017/03/28 11:16:05, Bernhard Bauer wrote:
> > > Oh, neat! I didn't realize that this would allow us to remove the second
URL
> > > filter on the IO thread.
> > > 
> > > https://crbug.com/576604 is the old bug about doing this. The only issue
was
> > > that it would now be more work to start filtering resource loads (images,
> > etc.),
> > > but we can cross that bridge if we ever come to it.
> > 
> > Certainly true.  We'll probably need to provide an API that can be used to
do
> > that for existing consumers - extensions do it, but it will potentially add
> > network process to browser process round trips.  May be ways to avoid the
> round
> > trips  Early days yet, and I'm just tackling some of the easier consumers so
> we
> > can get a better handle on what APIs we currently need.
> > 
> > > I don't know about NavigationThrottle and NavigationObserver, maybe
there's
> a
> > > reason we have both? At least one of them exists on both the UI and the IO
> > > thread, maybe that's related?
> > 
> > NavigationThrottle is a new class - having separate classes may have been
> > necessary because ResourceThrottle lives on the IO thread.  Now that
> everything
> > is on the UI thread, and we're using the NavigationThrottle API, it may also
> be
> > called when the NavigationObserver is.  Anyhow, I haven't thought about or
> > investigated this, just thought I'd suggest it as an option.
> 
> If it's possible to merge the two, I'd certainly be in favor of that - every
> time I've had to change anything about the filtering, I had to look at both to
> find out where the new code needed to go. I could never remember which of the
> two does what exactly.
> Anyway, that's for another CL :)

One thing to note is that the NavigationObserver corresponds to one WebContents
and stores e.g. information about blocked navigations. The NavigationThrottle
only exists for the duration of a navigation (and is it even created for a
pushState() navigation?).

[mmenke, 2017-03-28 16:52:20]

On 2017/03/28 16:44:28, Bernhard Bauer wrote:
> On 2017/03/28 12:11:49, Marc Treib wrote:
> > On 2017/03/28 12:06:29, mmenke wrote:
> > > A couple quick responses.  I'll update code and do some manual testing
later
> > > today.
> > > 
> > > On 2017/03/28 11:16:05, Bernhard Bauer wrote:
> > > > Oh, neat! I didn't realize that this would allow us to remove the second
> URL
> > > > filter on the IO thread.
> > > > 
> > > > https://crbug.com/576604 is the old bug about doing this. The only issue
> was
> > > > that it would now be more work to start filtering resource loads
(images,
> > > etc.),
> > > > but we can cross that bridge if we ever come to it.
> > > 
> > > Certainly true.  We'll probably need to provide an API that can be used to
> do
> > > that for existing consumers - extensions do it, but it will potentially
add
> > > network process to browser process round trips.  May be ways to avoid the
> > round
> > > trips  Early days yet, and I'm just tackling some of the easier consumers
so
> > we
> > > can get a better handle on what APIs we currently need.
> > > 
> > > > I don't know about NavigationThrottle and NavigationObserver, maybe
> there's
> > a
> > > > reason we have both? At least one of them exists on both the UI and the
IO
> > > > thread, maybe that's related?
> > > 
> > > NavigationThrottle is a new class - having separate classes may have been
> > > necessary because ResourceThrottle lives on the IO thread.  Now that
> > everything
> > > is on the UI thread, and we're using the NavigationThrottle API, it may
also
> > be
> > > called when the NavigationObserver is.  Anyhow, I haven't thought about or
> > > investigated this, just thought I'd suggest it as an option.
> > 
> > If it's possible to merge the two, I'd certainly be in favor of that - every
> > time I've had to change anything about the filtering, I had to look at both
to
> > find out where the new code needed to go. I could never remember which of
the
> > two does what exactly.
> > Anyway, that's for another CL :)
> 
> One thing to note is that the NavigationObserver corresponds to one
WebContents
> and stores e.g. information about blocked navigations. The NavigationThrottle
> only exists for the duration of a navigation (and is it even created for a
> pushState() navigation?).

I have no idea about pushState navigations.  NavigationThrottles do indeed only
last for the duration of the navigation.  I was mainy thinking that the logic in
SupervisedUserNavigationObserver::DidFinishNavigation seemed like it would make
sense in a throttle, but am not familiar enough with the web platform to know if
there are navigations that doesn't cover.

[mmenke, 2017-03-28 17:53:27]

On 2017/03/28 08:37:52, Marc Treib wrote:
> Some early comments before starting the actual review.
> 
> On 2017/03/27 18:34:32, mmenke wrote:
> > [treib]:  Since there are already browser tests, and they're all passing,
I'm
> > assuming this CL doesn't break anything, though the WebUI change does mean
it
> > will show more changes than it used to.
> > 
> > I don't see any browser tests for the WebUI stuff, and am not sure how to
test
> > that locally, other than to fool around with it a bit, but I'm not a
> supervised
> > user, which makes it a bit less meaningful.
> 
> You can create a supervised user: In the "create profile" dialog (from
> chrome://settings, or via avatar menu -> Manage People -> Add Person) there's
a
> checkbox "Supervise this user". (You have to be signed-in to Chrome for that
to
> work.)

This doesn't seem to work.  Adding a user who's supervised shows a message
demanding I sign in to Chrome to supervise a user.  Signing in to Chrome doesn't
seem to work with chromium:  I go to settings, click sign in to chromium, enter
my credentials, click on...And nothing happens.  Tried both personal and corp
credentials.

I don't get any visible error message, but looking at the network responses, I'm
getting a JSON file with the text "The OAuth client was not found." as a
response to one request.  Other requests are being responded to with rather less
intelligible responses.

> > I did a little cleanup, and more may be doable, but think its beyond the
scope
> > of this CL (Could perhaps merge the NavigationThrottle and
NavigationObserver,
> > for instance, or merge the filter with the service)
> 
> I don't know about NavigationThrottle and NavigationObserver, maybe there's a
> reason we have both? At least one of them exists on both the UI and the IO
> thread, maybe that's related?
> 
> I'm pretty sure we want to keep the filter separate from the service :)
> 
> > There's no rush on this review (I'll ping if I don't hear back in a week,
but
> > this isn't blocking anything at all).
> 
> Thanks for the heads-up! I'll still get started, no point in delaying :)
> 
> > Once you sign off, I plan to TBR the rest of the files, since I own
profiles/
> > and loader/, chrome_content_browser_client.cc has an * rule, and the rest
you
> > own or is just an API rename.  Just mentioning now in case you disagree, and
> so
> > you know I plan to only use you as a reviewer.
> 
> Yup, that's fine. I'll add bauerb@ though, since I seem to dimly remember that
> we considered this change before, but had some reason for not doing it.

[mmenke, 2017-03-28 17:55:05]

I've responded to comments, but I remain stumped as to how to check this
manually.

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
(left):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:138: //
Can't use base::MakeUnique because the constructor is private.
On 2017/03/28 10:22:58, Marc Treib wrote:
> nit: Leave the comment?

Thanks for catching this, done.

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service_factory.h (right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service_factory.h:22:
content::BrowserContext* browser_context);
On 2017/03/28 10:22:58, Marc Treib wrote:
> Is this change actually required? If so, the method should probably be renamed
> to GetForBrowserContext, but updating all callers won't be fun... maybe just
> have both?

It's not really needed, I just was horrified by:

SupervisedUserServiceFactory::GetForProfile(navigation_handle->GetWebContents()->GetBrowserContext())->GetURLFilter()

And thought throwing in a Profile::FromBrowserContext in there made the line
even worse, particularly as SupervisedUserServiceFactory::GetForProfile actually
uses a method that just needs a BrowserContext, not a profile.  I've gone ahead
and added Profile::FromBrowserContext to the aforementioned horrifying line
(Prefer that to adding a method to SupervisedUserServiceFactory).

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (left):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_url_filter.cc:254:
DCHECK(CalledOnValidThread());
On 2017/03/28 10:22:58, Marc Treib wrote:
> Any reason for removing this?

This class inherits from NonThreadSafe, which has this check in its own
destructor.  A double-check seemed like overkill.  If you disagree, I can add it
back.

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/supervised_user_internals_message_handler.h
(right):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/supervised_user_internals_message_handler.h:64: bool
is_watching_filter_;
On 2017/03/28 10:22:58, Marc Treib wrote:
> Would a ScopedObserver work here?

I'd never heard of that class before.  Seems a bit like over-engineering to me,
but done.

[Marc Treib, 2017-03-29 08:07:56]

On 2017/03/28 17:53:27, mmenke wrote:
> On 2017/03/28 08:37:52, Marc Treib wrote:
> > Some early comments before starting the actual review.
> > 
> > On 2017/03/27 18:34:32, mmenke wrote:
> > > [treib]:  Since there are already browser tests, and they're all passing,
> I'm
> > > assuming this CL doesn't break anything, though the WebUI change does mean
> it
> > > will show more changes than it used to.
> > > 
> > > I don't see any browser tests for the WebUI stuff, and am not sure how to
> test
> > > that locally, other than to fool around with it a bit, but I'm not a
> > supervised
> > > user, which makes it a bit less meaningful.
> > 
> > You can create a supervised user: In the "create profile" dialog (from
> > chrome://settings, or via avatar menu -> Manage People -> Add Person)
there's
> a
> > checkbox "Supervise this user". (You have to be signed-in to Chrome for that
> to
> > work.)
> 
> This doesn't seem to work.  Adding a user who's supervised shows a message
> demanding I sign in to Chrome to supervise a user.  Signing in to Chrome
doesn't
> seem to work with chromium:  I go to settings, click sign in to chromium,
enter
> my credentials, click on...And nothing happens.  Tried both personal and corp
> credentials.
> 
> I don't get any visible error message, but looking at the network responses,
I'm
> getting a JSON file with the text "The OAuth client was not found." as a
> response to one request.  Other requests are being responded to with rather
less
> intelligible responses.

Ah yes, you need API keys for signin to work. See the Googler-specific
instructions here: https://goto.corp.google.com/building-chrome
Essentially, you just need a src-internal checkout.

[Marc Treib, 2017-03-29 08:08:41]

Thanks! LGTM, assuming that everything works out in the manual tests.

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (left):

https://codereview.chromium.org/2776493005/diff/200001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_url_filter.cc:254:
DCHECK(CalledOnValidThread());
On 2017/03/28 17:55:05, mmenke wrote:
> On 2017/03/28 10:22:58, Marc Treib wrote:
> > Any reason for removing this?
> 
> This class inherits from NonThreadSafe, which has this check in its own
> destructor.  A double-check seemed like overkill.  If you disagree, I can add
it
> back.

Ah okay, I wasn't aware of that. Carry on then :)

https://codereview.chromium.org/2776493005/diff/240001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
(right):

https://codereview.chromium.org/2776493005/diff/240001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:137:
->GetURLFilter()),
Ah, I see.
I guess you could make a helper like URLFilterFromNavigationHandle? (optional)

[Bernhard Bauer, 2017-03-29 11:52:20]

LGTM if you need it.

[mmenke, 2017-03-29 17:55:11]

On 2017/03/29 08:07:56, Marc Treib wrote:
> On 2017/03/28 17:53:27, mmenke wrote:
> > On 2017/03/28 08:37:52, Marc Treib wrote:
> > > Some early comments before starting the actual review.
> > > 
> > > On 2017/03/27 18:34:32, mmenke wrote:
> > > > [treib]:  Since there are already browser tests, and they're all
passing,
> > I'm
> > > > assuming this CL doesn't break anything, though the WebUI change does
mean
> > it
> > > > will show more changes than it used to.
> > > > 
> > > > I don't see any browser tests for the WebUI stuff, and am not sure how
to
> > test
> > > > that locally, other than to fool around with it a bit, but I'm not a
> > > supervised
> > > > user, which makes it a bit less meaningful.
> > > 
> > > You can create a supervised user: In the "create profile" dialog (from
> > > chrome://settings, or via avatar menu -> Manage People -> Add Person)
> there's
> > a
> > > checkbox "Supervise this user". (You have to be signed-in to Chrome for
that
> > to
> > > work.)
> > 
> > This doesn't seem to work.  Adding a user who's supervised shows a message
> > demanding I sign in to Chrome to supervise a user.  Signing in to Chrome
> doesn't
> > seem to work with chromium:  I go to settings, click sign in to chromium,
> enter
> > my credentials, click on...And nothing happens.  Tried both personal and
corp
> > credentials.
> > 
> > I don't get any visible error message, but looking at the network responses,
> I'm
> > getting a JSON file with the text "The OAuth client was not found." as a
> > response to one request.  Other requests are being responded to with rather
> less
> > intelligible responses.
> 
> Ah yes, you need API keys for signin to work. See the Googler-specific
> instructions here: https://goto.corp.google.com/building-chrome
> Essentially, you just need a src-internal checkout.

I still cannot get this to work.  I sign in to Chrome with my personal account,
add a person, enter an name, select my account as the person to control the
account, click save, and just get a spinner on the Add a person page that spins
eternally.

After a bunch of tries, when I try to create someone with the same name again, I
get told the supervised user already exists.  Trying to import gives me an error
"The supervised user couldn't be imported. Please check your hard drive space
and permissions and try again".  Given that I don't know what permission it
wants that it doesn't have, that doesn't help a whole lot.  Strangely, disabling
all sync options, I still can't create a person with the same name.

Trying to create yet another user results in the infinite spinner yet again.

Is it ok if I just manually add URLs to the blacklist, and test that way?  I
don't see much value in spending more time trying to get the settings UI for
supervised users working.

https://codereview.chromium.org/2776493005/diff/240001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_navigation_throttle.cc
(right):

https://codereview.chromium.org/2776493005/diff/240001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_navigation_throttle.cc:137:
->GetURLFilter()),
On 2017/03/29 08:08:41, Marc Treib wrote:
> Ah, I see.
> I guess you could make a helper like URLFilterFromNavigationHandle? (optional)

I'll leave it as-is.  This may be a common enough pattern that it makes sense to
add the call to ChromeContentBrowserClient instead.  I'll keep an eye out as  I
work more in this space.

[mmenke, 2017-03-29 19:43:02]

So creating a new supervised user seems to be working, it just takes a really,
really long time (As in on the order of minutes on a debug build, without a
debugger attached), though once or twice it was really fast (Maybe some sort of
server-side throttling?).  Importing a supervised user is now consistently
working, quickly.

Anyhow, the URL filters seem to be working correctly - I get the interstitial,
request permission buttons work, settings only affect the correct user,
about:supervised-user-internals shows queries (And just for the current user). 
Reloading an interstitial shows a flash of the previous page, but that's a
general interstitial issue, I assume.

I'm inclined to call the supervised user creation delay not-my-fault, and just
land the CL, thought I'd give you two a chance to say otherwise, though, out of
general paranoia.

On 2017/03/29 17:55:11, mmenke wrote:
> I still cannot get this to work.  I sign in to Chrome with my personal
account,
> add a person, enter an name, select my account as the person to control the
> account, click save, and just get a spinner on the Add a person page that
spins
> eternally.
> 
> After a bunch of tries, when I try to create someone with the same name again,
I
> get told the supervised user already exists.  Trying to import gives me an
error
> "The supervised user couldn't be imported. Please check your hard drive space
> and permissions and try again".  Given that I don't know what permission it
> wants that it doesn't have, that doesn't help a whole lot.  Strangely,
disabling
> all sync options, I still can't create a person with the same name.
> 
> Trying to create yet another user results in the infinite spinner yet again.
> 
> Is it ok if I just manually add URLs to the blacklist, and test that way?  I
> don't see much value in spending more time trying to get the settings UI for
> supervised users working.

[Marc Treib, 2017-03-30 08:48:57]

On 2017/03/29 19:43:02, mmenke wrote:
> So creating a new supervised user seems to be working, it just takes a really,
> really long time (As in on the order of minutes on a debug build, without a
> debugger attached), though once or twice it was really fast (Maybe some sort
of
> server-side throttling?).  Importing a supervised user is now consistently
> working, quickly.
> 
> Anyhow, the URL filters seem to be working correctly - I get the interstitial,
> request permission buttons work, settings only affect the correct user,
> about:supervised-user-internals shows queries (And just for the current user).

> Reloading an interstitial shows a flash of the previous page, but that's a
> general interstitial issue, I assume.
> 
> I'm inclined to call the supervised user creation delay not-my-fault, and just
> land the CL, thought I'd give you two a chance to say otherwise, though, out
of
> general paranoia.

Thanks for persisting with the manual test!
Yes, the long delay is almost certainly not your fault. It does involve the Sync
server, and its dev instances are sometimes quite slow, so this is not entirely
unexpected (though two minutes is still a bit excessive). So please feel free to
go ahead and land :)

[mmenke, 2017-03-30 14:39:22]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-30 14:39:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 14:50:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 14:50:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2017-03-30 14:55:46]

Oops, forgot the TBRs

[mmenke, 2017-03-30 15:05:45]

mmenke@chromium.org changed reviewers:
+ sky@chromium.org

[mmenke, 2017-03-30 15:05:46]

[+sky]:  Please review chrome/browser/ui/webui/*.  Already had a
c/b/supervised_user OWNERs signoff, though they don't look to own those webui
files, and the other file in that directory is just a minor API change.

Thanks!

[sky, 2017-03-30 20:09:48]

LGTM

[mmenke, 2017-03-30 20:30:23]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:31:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 20:42:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:42:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2017-03-30 20:46:09]

Description was changed from

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

BUG=704648
==========

to

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

TBR=calamity@chromium.org
BUG=704648
==========

[mmenke, 2017-03-30 20:46:09]

mmenke@chromium.org changed reviewers:
+ calamity@chromium.org

[mmenke, 2017-03-30 20:47:38]

[+calamity]:  TBRing you for browsing_history_handler for trivial API change in
a dependency.

[mmenke, 2017-03-30 20:47:47]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-30 20:48:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 23:59:57]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490906867371970,
"parent_rev": "6c378ee936e8504fb0617e35287634f8d860b384", "commit_rev":
"db2637ff32cb9a619f1e2920293eb2e68f20ac4c"}

[commit-bot: I haz the power, 2017-03-31 00:00:47]

Description was changed from

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

TBR=calamity@chromium.org
BUG=704648
==========

to

==========
Convert SupervisedUserResourceThrottle to a NavigationThrottle.

Also do a bit of incidental cleanup that enables - remove the second
SuprvisedUserURLFilter that lived on the IO thread, remove refcounting
of SuprvisedUserURLFilter, get rid of SupervisedUserContext, don't
duplicate std::maps when updating the filter, etc.

This is part of the effort to move content/browser/loader behind a
Mojo interface

TBR=calamity@chromium.org
BUG=704648

Review-Url: https://codereview.chromium.org/2776493005
Cr-Commit-Position: refs/heads/master@{#460940}
Committed:
https://chromium.googlesource.com/chromium/src/+/db2637ff32cb9a619f1e2920293e...
==========

[commit-bot: I haz the power, 2017-03-31 00:00:50]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/db2637ff32cb9a619f1e2920293e...

[calamity, 2017-04-03 04:42:02]

lgtm