Remove dependency on Profile's IsOffTheRecord in favicon_handler.

Remove dependency on Profile's IsOffTheRecord in favicon_handler.

Have FaviconTabHelper supports IsOffTheRecord to move knowledge of Profile out of favicon_handler.

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

[jif, 2014-04-08 09:53:59]

PTAL

[blundell, 2014-04-08 10:01:19]

LGTM with nits

nice!

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:20: #include
"components/favicon/content/content_favicon_driver.h"
This should be the interface class, not the content implementation.

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:291: // The driver which implements
platform-specific PasswordManager operations.
Favicon

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler_unittest.cc:171: class MockFaviconDriver
: public FaviconDriver {
s/Mock/Test

https://codereview.chromium.org/227153007/diff/60001/components/favicon/DEPS
File components/favicon/DEPS (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/DEPS#...
components/favicon/DEPS:1: include_rules = [
This should have "-content".

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
File components/favicon/content/DEPS (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/DEPS:1: include_rules = [
This should have +content/public/common.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/DEPS:2: "+content/public/browser",
This should be under ./browser.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
File components/favicon/content/content_favicon_driver.h (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/content_favicon_driver.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: kill (c).

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/content_favicon_driver.h:18: class
ContentFaviconDriver : public FaviconDriver,
Add a class-level comment.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: kill (c).

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:19: // If this browsing session should
not be persisted.
Returns whether the user is operating in an off-the-record context.

[jif, 2014-04-08 11:55:46]

Thanks.

+stevenjb for an other pair of eyes.

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:291: // The driver which implements
platform-specific PasswordManager operations.
On 2014/04/08 10:01:19, blundell wrote:
> Favicon

Done.

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/227153007/diff/60001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler_unittest.cc:171: class MockFaviconDriver
: public FaviconDriver {
On 2014/04/08 10:01:19, blundell wrote:
> s/Mock/Test

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/DEPS
File components/favicon/DEPS (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/DEPS#...
components/favicon/DEPS:1: include_rules = [
On 2014/04/08 10:01:19, blundell wrote:
> This should have "-content".

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
File components/favicon/content/DEPS (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/DEPS:1: include_rules = [
On 2014/04/08 10:01:19, blundell wrote:
> This should have +content/public/common.

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/DEPS:2: "+content/public/browser",
On 2014/04/08 10:01:19, blundell wrote:
> This should be under ./browser.

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
File components/favicon/content/content_favicon_driver.h (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/content_favicon_driver.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/04/08 10:01:19, blundell wrote:
> nit: kill (c).

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/conte...
components/favicon/content/content_favicon_driver.h:18: class
ContentFaviconDriver : public FaviconDriver,
On 2014/04/08 10:01:19, blundell wrote:
> Add a class-level comment.

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/04/08 10:01:19, blundell wrote:
> nit: kill (c).

Done.

https://codereview.chromium.org/227153007/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:19: // If this browsing session should
not be persisted.
On 2014/04/08 10:01:19, blundell wrote:
> Returns whether the user is operating in an off-the-record context.

Done.

[stevenjb, 2014-04-08 15:55:57]

https://codereview.chromium.org/227153007/diff/90001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/227153007/diff/90001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:292: FaviconDriver* driver_;  // weak
I haven't seen "Driver" used much in Chrome code, and the meaning was not
immediately clear to me. Maybe use "PlatformDelegate" instead? Or would it be
possible just to integrate this with |delegate_|? That seems like it would be
simpler since FaviconHandlerDelegate already has a dependency on src/content for
GetActiveEntry.

[blundell, 2014-04-08 16:01:56]

https://codereview.chromium.org/227153007/diff/90001/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/227153007/diff/90001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:292: FaviconDriver* driver_;  // weak
FooDriver is the naming convention for the interface that abstracts the
dependencies that the core code of a layered component Foo has on the content
API.

In the medium term the FaviconHandlerDelegate will be integrated into the
FaviconDriver interface and content-based impl, yes.

On 2014/04/08 15:55:57, stevenjb wrote:
> I haven't seen "Driver" used much in Chrome code, and the meaning was not
> immediately clear to me. Maybe use "PlatformDelegate" instead? Or would it be
> possible just to integrate this with |delegate_|? That seems like it would be
> simpler since FaviconHandlerDelegate already has a dependency on src/content
for
> GetActiveEntry.
> 
>

[stevenjb, 2014-04-08 16:15:08]

I see. Does it make sense then to put IsOffTheRecord() into
FaviconHandlerDelegate
now and change that class to FaviconDriver when the time comes? Having two
delegate/driver classes in the interim is confusing and for now at least
doesn't seem necessary.



On Tue, Apr 8, 2014 at 10:01 AM, <blundell@chromium.org> wrote:

>
> https://codereview.chromium.org/227153007/diff/90001/
> chrome/browser/favicon/favicon_handler.h
> File chrome/browser/favicon/favicon_handler.h (right):
>
> https://codereview.chromium.org/227153007/diff/90001/
> chrome/browser/favicon/favicon_handler.h#newcode292
> chrome/browser/favicon/favicon_handler.h:292: FaviconDriver* driver_;
> // weak
> FooDriver is the naming convention for the interface that abstracts the
> dependencies that the core code of a layered component Foo has on the
> content API.
>
> In the medium term the FaviconHandlerDelegate will be integrated into
> the FaviconDriver interface and content-based impl, yes.
>
>
> On 2014/04/08 15:55:57, stevenjb wrote:
>
>> I haven't seen "Driver" used much in Chrome code, and the meaning was
>>
> not
>
>> immediately clear to me. Maybe use "PlatformDelegate" instead? Or
>>
> would it be
>
>> possible just to integrate this with |delegate_|? That seems like it
>>
> would be
>
>> simpler since FaviconHandlerDelegate already has a dependency on
>>
> src/content for
>
>> GetActiveEntry.
>>
>
>
>
> https://codereview.chromium.org/227153007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blundell, 2014-04-08 20:06:58]

On 2014/04/08 16:15:08, stevenjb wrote:
> I see. Does it make sense then to put IsOffTheRecord() into
> FaviconHandlerDelegate
> now and change that class to FaviconDriver when the time comes? Having two
> delegate/driver classes in the interim is confusing and for now at least
> doesn't seem necessary.
> 
> 
> 
> On Tue, Apr 8, 2014 at 10:01 AM, <mailto:blundell@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/227153007/diff/90001/
> > chrome/browser/favicon/favicon_handler.h
> > File chrome/browser/favicon/favicon_handler.h (right):
> >
> > https://codereview.chromium.org/227153007/diff/90001/
> > chrome/browser/favicon/favicon_handler.h#newcode292
> > chrome/browser/favicon/favicon_handler.h:292: FaviconDriver* driver_;
> > // weak
> > FooDriver is the naming convention for the interface that abstracts the
> > dependencies that the core code of a layered component Foo has on the
> > content API.
> >
> > In the medium term the FaviconHandlerDelegate will be integrated into
> > the FaviconDriver interface and content-based impl, yes.
> >
> >
> > On 2014/04/08 15:55:57, stevenjb wrote:
> >
> >> I haven't seen "Driver" used much in Chrome code, and the meaning was
> >>
> > not
> >
> >> immediately clear to me. Maybe use "PlatformDelegate" instead? Or
> >>
> > would it be
> >
> >> possible just to integrate this with |delegate_|? That seems like it
> >>
> > would be
> >
> >> simpler since FaviconHandlerDelegate already has a dependency on
> >>
> > src/content for
> >
> >> GetActiveEntry.
> >>
> >
> >
> >
> > https://codereview.chromium.org/227153007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, SGTM. In fact, FaviconHandlerDelegate can be renamed to FaviconDriver in an
immediate followup to this CL -- there's nothing blocking that.

[jif, 2014-04-09 09:57:33]

On 2014/04/08 20:06:58, blundell wrote:
> On 2014/04/08 16:15:08, stevenjb wrote:
> > I see. Does it make sense then to put IsOffTheRecord() into
> > FaviconHandlerDelegate
> > now and change that class to FaviconDriver when the time comes? Having two
> > delegate/driver classes in the interim is confusing and for now at least
> > doesn't seem necessary.
> > 
> > 
> > 
> > On Tue, Apr 8, 2014 at 10:01 AM, <mailto:blundell@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/227153007/diff/90001/
> > > chrome/browser/favicon/favicon_handler.h
> > > File chrome/browser/favicon/favicon_handler.h (right):
> > >
> > > https://codereview.chromium.org/227153007/diff/90001/
> > > chrome/browser/favicon/favicon_handler.h#newcode292
> > > chrome/browser/favicon/favicon_handler.h:292: FaviconDriver* driver_;
> > > // weak
> > > FooDriver is the naming convention for the interface that abstracts the
> > > dependencies that the core code of a layered component Foo has on the
> > > content API.
> > >
> > > In the medium term the FaviconHandlerDelegate will be integrated into
> > > the FaviconDriver interface and content-based impl, yes.
> > >
> > >
> > > On 2014/04/08 15:55:57, stevenjb wrote:
> > >
> > >> I haven't seen "Driver" used much in Chrome code, and the meaning was
> > >>
> > > not
> > >
> > >> immediately clear to me. Maybe use "PlatformDelegate" instead? Or
> > >>
> > > would it be
> > >
> > >> possible just to integrate this with |delegate_|? That seems like it
> > >>
> > > would be
> > >
> > >> simpler since FaviconHandlerDelegate already has a dependency on
> > >>
> > > src/content for
> > >
> > >> GetActiveEntry.
> > >>
> > >
> > >
> > >
> > > https://codereview.chromium.org/227153007/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Yes, SGTM. In fact, FaviconHandlerDelegate can be renamed to FaviconDriver in
an
> immediate followup to this CL -- there's nothing blocking that.

OK, PTAL.

[blundell, 2014-04-09 10:58:23]

LGTM

[jif, 2014-04-09 13:03:25]

The CQ bit was checked by jif@chromium.org

[commit-bot: I haz the power, 2014-04-09 13:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jif@chromium.org/227153007/170001

[stevenjb, 2014-04-09 16:32:39]

lgtm

[commit-bot: I haz the power, 2014-04-09 16:41:55]

Change committed as 262709