Componentize FaviconTabHelper

Componentize FaviconTabHelper

Split FaviconTabHelper into three classes depending on what can be
shared with iOS (FaviconDriverImpl), what only depends on //content
(ContentFaviconDriver) and what depends on //chrome (FaviconTabHelper).

FaviconTabHelper still exists to provide an easier factory (as
ContentFaviconDriver cannot access KeyedService factories defined in
//chrome) as FaviconTabHelper::CreateForWebContents() is called from
multiple locations in //chrome code and to provide an implementation for
FaviconTabHelper::ShouldDisplayFavicon (this method will be turned into
a free function in a followup CL as it only depends on WebContents
public interface, not on ContentFaviconDriver).

Introduce WebFaviconDriver, an iOS implementation of FaviconDriver that
uses FaviconDriverImpl to share as much code as possible with the
ContentFaviconDriver.

BUG=370877, 472117
Committed: https://crrev.com/70a04802728cbdc0c3da3c5a43fd2aa6feaf1ca8
Cr-Commit-Position: refs/heads/master@{#324430}

[sdefresne, 2015-04-07 14:30:45]

Patchset #1 (id:1) has been deleted

[sdefresne, 2015-04-07 14:30:52]

Patchset #1 (id:20001) has been deleted

[sdefresne, 2015-04-07 14:32:31]

sdefresne@chromium.org changed reviewers:
+ droger@chromium.org, miu@chromium.org, sky@chromium.org

[sdefresne, 2015-04-07 14:32:32]

droger: please take a look

miu: please review as OWNERS of
  chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm 
  chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

sky: please review new DEPS on ui/base from components/favicon

[droger, 2015-04-07 15:33:24]

Looks good from a high level. Mostly nits below.

It's a bit hard to review, because it's not easy to see what moved and what is
new.
Is it correct that the new code is:
- the web implementation of the driver
- FaviconDriverImpl:: HasPendingTasksForTest
?

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.h (right):

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:16: static FaviconTabHelper*
FromWebContents(content::WebContents* web_contents);
Could this be moved to ContentFaviconDriver?
Since FaviconTabHelper has no function, I don't see why we would need to get
one.

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper_browsertest.cc (right):

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper_browsertest.cc:191:
->HasPendingTasksForTest();
Why do we need the method on FaviconTabHelper? The existing code is using only
FaviconHandler, could the method be on FaviconHandler only?

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.h (right):

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.h:21: // ContentFaviconDriver
is an implementation of FaviconDriver that listen to
s/listen/listens/

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.h:29: // Returns the current
tab's favicon urls. If this is empty,
s/urls/URLs/

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

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:24: // implementation must be provided
by the driver.
Is this comment up to date?
It seems that the driver must NOT provide an implementation of this, but rather
provide an implementation of FaviconDriverImpl.

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:39: // entry. This will return an empty
bitmap if there are no navigation
s/This will return/Returns/

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
File components/favicon/core/favicon_driver_impl.h (right):

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver_impl.h:39: // FaviconDriverImpl works
with FaviconHandlers to fetch the favicons. It fethes
s/fethes/fetches/

[sdefresne, 2015-04-07 16:50:06]

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.h (right):

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:16: static FaviconTabHelper*
FromWebContents(content::WebContents* web_contents);
On 2015/04/07 15:33:24, droger wrote:
> Could this be moved to ContentFaviconDriver?
> Since FaviconTabHelper has no function, I don't see why we would need to get
> one.

All the client code use the following pattern:

FaviconTabHelper* favicon_tab_helper =
FaviconTabHelper::FromWebContents(web_contents);
favicon_tab_helper->SomeMethod(...);

Providing this method on FaviconTabHelper allow me to limit the size of the CL
by not touching the client code. I'll remove it in a followup CL.

I've added a comment.

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper_browsertest.cc (right):

https://codereview.chromium.org/1064823002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper_browsertest.cc:191:
->HasPendingTasksForTest();
On 2015/04/07 15:33:24, droger wrote:
> Why do we need the method on FaviconTabHelper? The existing code is using only
> FaviconHandler, could the method be on FaviconHandler only?

There is no public method to get the FaviconHandler owned by the
FaviconDriverImpl. Exposing the method on FaviconDriverImpl appeared to be the
cleanest way to fix this test.

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.h (right):

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.h:21: // ContentFaviconDriver
is an implementation of FaviconDriver that listen to
On 2015/04/07 15:33:24, droger wrote:
> s/listen/listens/

Done.

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.h:29: // Returns the current
tab's favicon urls. If this is empty,
On 2015/04/07 15:33:24, droger wrote:
> s/urls/URLs/

Done.

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

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:24: // implementation must be provided
by the driver.
On 2015/04/07 15:33:24, droger wrote:
> Is this comment up to date?
> It seems that the driver must NOT provide an implementation of this, but
rather
> provide an implementation of FaviconDriverImpl.

Acknowledged.

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:39: // entry. This will return an empty
bitmap if there are no navigation
On 2015/04/07 15:33:24, droger wrote:
> s/This will return/Returns/

Done.

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
File components/favicon/core/favicon_driver_impl.h (right):

https://codereview.chromium.org/1064823002/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver_impl.h:39: // FaviconDriverImpl works
with FaviconHandlers to fetch the favicons. It fethes
On 2015/04/07 15:33:24, droger wrote:
> s/fethes/fetches/

Done.

[sdefresne, 2015-04-07 16:52:49]

On 2015/04/07 15:33:24, droger wrote:
> Looks good from a high level. Mostly nits below.
> 
> It's a bit hard to review, because it's not easy to see what moved and what is
> new.
> Is it correct that the new code is:
> - the web implementation of the driver
> - FaviconDriverImpl:: HasPendingTasksForTest
> ?

High-level view is:
- FaviconTabHelper implementation was split between FaviconDriverImpl and
ContentFaviconDriver
- WebFaviconDriver was implemented (new code)

FaviconDriverImpl::HasPendingTasksForTest is an helper method to avoid declaring
FaviconTabHelperTest friend will FaviconDriver, FaviconDriverImpl,
ContentFaviconDriver. It is just de-inlining the test that was in
FaviconTabHelperTest::HasPendingTasks().

[droger, 2015-04-07 17:01:56]

It looks like the patch set where the comments are adressed has not been
uploaded. Did you forget to upload?

[sdefresne, 2015-04-08 07:57:12]

On 2015/04/07 17:01:56, droger wrote:
> It looks like the patch set where the comments are adressed has not been
> uploaded. Did you forget to upload?

Sorry forgot to upload. Done. PTAL.

[sdefresne, 2015-04-08 08:08:35]

Patchset #2 (id:60001) has been deleted

[sdefresne, 2015-04-08 08:08:41]

Patchset #2 (id:80001) has been deleted

[droger, 2015-04-08 09:31:59]

lgtm

https://codereview.chromium.org/1064823002/diff/140001/components/favicon.gypi
File components/favicon.gypi (right):

https://codereview.chromium.org/1064823002/diff/140001/components/favicon.gyp...
components/favicon.gypi:78: 'favicon_core',
Your new file depends on base and ui/gfx.

[sdefresne, 2015-04-08 16:50:08]

Patchset #2 (id:100001) has been deleted

[sdefresne, 2015-04-08 16:50:15]

Patchset #2 (id:120001) has been deleted

[sdefresne, 2015-04-08 17:07:36]

On 2015/04/08 09:31:59, droger wrote:
> lgtm
> 
> https://codereview.chromium.org/1064823002/diff/140001/components/favicon.gypi
> File components/favicon.gypi (right):
> 
>
https://codereview.chromium.org/1064823002/diff/140001/components/favicon.gyp...
> components/favicon.gypi:78: 'favicon_core',
> Your new file depends on base and ui/gfx.

Thanks, done.

miu/sky: could you review as OWNERS

- miu: please review as OWNERS of
  chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm 
  chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

- sky: please review new DEPS on ui/base from components/favicon

[miu, 2015-04-08 20:24:24]

*tab_strip_controller.* lgtm

[sdefresne, 2015-04-09 08:35:59]

sdefresne@chromium.org changed reviewers:
+ sadrul@chromium.org
- sky@chromium.org

[sdefresne, 2015-04-09 08:40:45]

sadrul: please review the new DEPS on //ui/base

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+ui/base',

[sadrul, 2015-04-09 13:15:03]

On 2015/04/09 08:40:45, sdefresne wrote:
> sadrul: please review the new DEPS on //ui/base
> 
> ** Presubmit ERRORS **
> Missing LGTM from OWNERS of dependencies added to DEPS:
>     '+ui/base',

lgtm

[sdefresne, 2015-04-09 13:20:40]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-04-09 13:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1064823002/#ps200001
(title: "Fix BUILD.gn")

[commit-bot: I haz the power, 2015-04-09 13:21:07]

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

[commit-bot: I haz the power, 2015-04-09 14:00:28]

Committed patchset #5 (id:200001)

[commit-bot: I haz the power, 2015-04-09 14:01:19]

Patchset 5 (id:??) landed as
https://crrev.com/70a04802728cbdc0c3da3c5a43fd2aa6feaf1ca8
Cr-Commit-Position: refs/heads/master@{#324430}