Add a SupervisedUserServiceObserver.

Add a SupervisedUserServiceObserver.

For now, this is used in the SupervisedUserInterstitial instead of listening to pref changes manually.
More clients will follow in later CLs.

BUG=

Committed: https://crrev.com/ab0a39ec734e1f3e397993462dc61141c4f53c69
Cr-Commit-Position: refs/heads/master@{#296412}

[Marc Treib, 2014-09-23 15:02:28]

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

[Marc Treib, 2014-09-23 15:02:28]

I fixed your TODO - PTAL!
(I do have an ulterior motive though: I plan to add an "OnCustodianInfoChanged"
method to the observer, which is needed for the avatar menu.)

[Bernhard Bauer, 2014-09-24 09:34:10]

LGTM, thanks!

[Marc Treib, 2014-09-24 11:44:34]

Ping, in case you want to take another look: Turns out the interstitial may
outlive the WebContents that owns it. I've fixed the problem I introduced (using
the WebContents in the interstitial dtor), and also tried to fix a pre-existing
case where this might occur (OnURLFilterChanged->ShouldProceed).

[Bernhard Bauer, 2014-09-24 11:52:00]

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:280: if
(web_contents_ && ShouldProceed())
Why do we need the |web_contents_| check now? Just because we need the
|web_contents_| to get the profile from it, to unregister ourselves from the
SUService?

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:310:
web_contents_ = NULL;
Is it guaranteed that this will be called, even if e.g. the tab/window is
closed?

Also, under what circumstances does the interstitial actually outlive the
WebContents?

[Marc Treib, 2014-09-24 12:02:27]

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:280: if
(web_contents_ && ShouldProceed())
On 2014/09/24 11:51:59, Bernhard Bauer wrote:
> Why do we need the |web_contents_| check now? Just because we need the
> |web_contents_| to get the profile from it, to unregister ourselves from the
> SUService?

Because ShouldProceed uses the WebContents to get the Profile->SUService to
check the URLFilter. If we outlive the WebContents, that won't work. I guess we
could also cache a Profile*, but in this case it doesn't matter anyway.

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:310:
web_contents_ = NULL;
On 2014/09/24 11:51:59, Bernhard Bauer wrote:
> Is it guaranteed that this will be called, even if e.g. the tab/window is
> closed?

Yes, InterstitialPageImpl calls OnDontProceed when the tab/window is closed.

> Also, under what circumstances does the interstitial actually outlive the
> WebContents?

Apparently, we (almost?) always outlive the WebContents - the order of
destruction is a bit weird here. See InterstitialPageImpl::WebContentsDestroyed,
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

[Bernhard Bauer, 2014-09-24 12:58:43]

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:310:
web_contents_ = NULL;
On 2014/09/24 12:02:27, Marc Treib wrote:
> On 2014/09/24 11:51:59, Bernhard Bauer wrote:
> > Is it guaranteed that this will be called, even if e.g. the tab/window is
> > closed?
> 
> Yes, InterstitialPageImpl calls OnDontProceed when the tab/window is closed.
> 
> > Also, under what circumstances does the interstitial actually outlive the
> > WebContents?
> 
> Apparently, we (almost?) always outlive the WebContents - the order of
> destruction is a bit weird here. See
InterstitialPageImpl::WebContentsDestroyed,
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

Hm, accessing |web_contents_| above already worries me... WebContentsDestroyed()
is called from the WebContentsImpl destructor, so we're calling into a partially
destroyed WebContents.

I think caching the Profile instead would be much easier, as it's guaranteed
that that will outlive us. That would also get rid of the additional
|web_contents_| check in OnURLFilterChanged().

[Marc Treib, 2014-09-24 13:25:26]

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
File chrome/browser/supervised_user/supervised_user_interstitial.cc (right):

https://codereview.chromium.org/597573002/diff/40001/chrome/browser/supervise...
chrome/browser/supervised_user/supervised_user_interstitial.cc:310:
web_contents_ = NULL;
On 2014/09/24 12:58:43, Bernhard Bauer wrote:
> On 2014/09/24 12:02:27, Marc Treib wrote:
> > On 2014/09/24 11:51:59, Bernhard Bauer wrote:
> > > Is it guaranteed that this will be called, even if e.g. the tab/window is
> > > closed?
> > 
> > Yes, InterstitialPageImpl calls OnDontProceed when the tab/window is closed.
> > 
> > > Also, under what circumstances does the interstitial actually outlive the
> > > WebContents?
> > 
> > Apparently, we (almost?) always outlive the WebContents - the order of
> > destruction is a bit weird here. See
> InterstitialPageImpl::WebContentsDestroyed,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> Hm, accessing |web_contents_| above already worries me...
WebContentsDestroyed()
> is called from the WebContentsImpl destructor, so we're calling into a
partially
> destroyed WebContents.

That's a good point.

> I think caching the Profile instead would be much easier, as it's guaranteed
> that that will outlive us. That would also get rid of the additional
> |web_contents_| check in OnURLFilterChanged().

Done.

[Bernhard Bauer, 2014-09-24 14:00:54]

LGTM!

[Marc Treib, 2014-09-24 14:02:55]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-09-24 14:03:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/597573002/60001

[commit-bot: I haz the power, 2014-09-24 14:48:35]

Committed patchset #4 (id:60001) as ffa8762417cfb7b76ed974cbce10b399004b3af0

[commit-bot: I haz the power, 2014-09-24 14:49:04]

Patchset 4 (id:??) landed as
https://crrev.com/ab0a39ec734e1f3e397993462dc61141c4f53c69
Cr-Commit-Position: refs/heads/master@{#296412}