Add Observer in FaviconTabHelper

This patch added Observer in FaviconTabHelper, so the favicon retrieved by FaviconHandler could be used right away.

BUG=428218
Committed: https://crrev.com/498547a3a74cbd304593305a5166a114000ef9b6
Cr-Commit-Position: refs/heads/master@{#302700}

[michaelbai, 2014-10-29 04:20:45]

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

[michaelbai, 2014-10-29 04:30:33]

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:596: }
I just simply added code here because this is touch icon specific and will be
deleted when we remove the icon type concept.

[sky, 2014-10-29 15:50:55]

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:295: NotifyFaviconUpdated(icon_url);
Sorry, this is wrong. We shouldn't need the invalidate anything if the favicon
hasn't changed. Who is listening for the changes that needs that you need to
update? Seems like they should instead be listening for history change, if that
is what they care about, right?

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/684983003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.h:151: // Notifies the driver that the
icon was updated and could be retreived
retrieved

[michaelbai, 2014-10-29 20:54:02]

Added observer, PTAL, if you are fine with current design, I will add unit
tests.

[sky, 2014-10-30 03:34:31]

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

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:163: inline GURL GetIconURL(const
std::vector<
Is it really necessary to inline here?

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:163: inline GURL GetIconURL(const
std::vector<
GetFirstIconURL

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:305: SetFaviconOnActivePage(icon_url,
image);
nit: spacing is off now (use git cl format).

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

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:229: // Sets the favicon's data.
update comment.

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

https://codereview.chromium.org/684983003/diff/40001/components/favicon/core/...
components/favicon/core/favicon_driver.h:66: virtual void
OnFaviconAvailable(const gfx::Image& image) = 0;
Can we leave the FaviconHandler as it was, meaning calling into
SetActiveFaviconImage and instead make FaviconTabHelper take a boolean as to
whether it should push to NavigationEntry or not? In either can it notifies the
observer.

[michaelbai, 2014-10-30 19:22:51]

PTAL, I didn't added unit tests yet because there is more FaviconDriver change.

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

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:163: inline GURL GetIconURL(const
std::vector<
On 2014/10/30 03:34:30, sky wrote:
> GetFirstIconURL

Function was removed

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:163: inline GURL GetIconURL(const
std::vector<
On 2014/10/30 03:34:31, sky wrote:
> Is it really necessary to inline here?

Function was removed

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.cc:305: SetFaviconOnActivePage(icon_url,
image);
On 2014/10/30 03:34:30, sky wrote:
> nit: spacing is off now (use git cl format).

Acknowledged.

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

https://codereview.chromium.org/684983003/diff/40001/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_handler.h:229: // Sets the favicon's data.
On 2014/10/30 03:34:31, sky wrote:
> update comment.

Done.

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

https://codereview.chromium.org/684983003/diff/40001/components/favicon/core/...
components/favicon/core/favicon_driver.h:66: virtual void
OnFaviconAvailable(const gfx::Image& image) = 0;
On 2014/10/30 03:34:31, sky wrote:
> Can we leave the FaviconHandler as it was, meaning calling into
> SetActiveFaviconImage and instead make FaviconTabHelper take a boolean as to
> whether it should push to NavigationEntry or not? In either can it notifies
the
> observer.

Let me know if you are happy with new patch.

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:63: virtual void
SetActiveFaviconValidity(bool validity) = 0;
Hey Scott, I am not sure these methods still needed or not, who should I add as
reviewer?

[sky, 2014-10-30 21:30:51]

sky@chromium.org changed reviewers:
+ blundell@chromium.org, jif@chromium.org

[sky, 2014-10-30 21:31:28]

+jif@chromium.org, blundell@chromium.org, 

Hopefully one of them can answer your question here:
https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...

[sky, 2014-10-30 21:33:49]

This approach is fine by me.

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:52: virtual void
OnFaviconAvailable(const gfx::Image& image,
You need to make it clear image is not necessarily the 16x16 image. Also, you
need to make it clear what update_active_favicon means.

[blundell, 2014-10-31 06:53:42]

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:63: virtual void
SetActiveFaviconValidity(bool validity) = 0;
On 2014/10/30 19:22:51, michaelbai wrote:
> Hey Scott, I am not sure these methods still needed or not, who should I add
as
> reviewer?

jif@ is the right person to answer this question.

[jif-google, 2014-10-31 12:05:17]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2014-10-31 12:05:18]

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:63: virtual void
SetActiveFaviconValidity(bool validity) = 0;
On 2014/10/30 19:22:51, michaelbai wrote:
> Hey Scott, I am not sure these methods still needed or not, who should I add
as
> reviewer?

michaelbai>
Since GetActiveFaviconValidity is used in conditions, and that
SetActiveFaviconValidity is used to change the validity from false (the default
value) to true, I would say that they are.

What makes you think that they aren't needed anymore?

[michaelbai, 2014-10-31 15:57:57]

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:63: virtual void
SetActiveFaviconValidity(bool validity) = 0;
On 2014/10/31 12:05:18, jif-google wrote:
> On 2014/10/30 19:22:51, michaelbai wrote:
> > Hey Scott, I am not sure these methods still needed or not, who should I add
> as
> > reviewer?
> 
> michaelbai>
> Since GetActiveFaviconValidity is used in conditions, and that
> SetActiveFaviconValidity is used to change the validity from false (the
default
> value) to true, I would say that they are.
> 
> What makes you think that they aren't needed anymore?


I don't know how you use the driver, this patch added OnFaviconAvailable()
method, and it seemed that SetActiveFaviconValidity() could be done in it,  see
the FaviconTabHelper::OnFaviconAvailable as example, also the other 2 methods
could be removed.

Did you implement FaviconDriver or use FaviconTableHelper in your code?

[jif-google, 2014-10-31 16:51:30]

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:63: virtual void
SetActiveFaviconValidity(bool validity) = 0;
On 2014/10/31 15:57:56, michaelbai wrote:
> On 2014/10/31 12:05:18, jif-google wrote:
> > On 2014/10/30 19:22:51, michaelbai wrote:
> > > Hey Scott, I am not sure these methods still needed or not, who should I
add
> > as
> > > reviewer?
> > 
> > michaelbai>
> > Since GetActiveFaviconValidity is used in conditions, and that
> > SetActiveFaviconValidity is used to change the validity from false (the
> default
> > value) to true, I would say that they are.
> > 
> > What makes you think that they aren't needed anymore?
> 
> 
> I don't know how you use the driver, this patch added OnFaviconAvailable()
> method, and it seemed that SetActiveFaviconValidity() could be done in it, 
see
> the FaviconTabHelper::OnFaviconAvailable as example, also the other 2 methods
> could be removed.
> 
> Did you implement FaviconDriver or use FaviconTableHelper in your code?

Resolved offline.
The conclusion is: we can remove the methods.

[michaelbai, 2014-10-31 23:21:27]

PTAL

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

https://codereview.chromium.org/684983003/diff/60001/components/favicon/core/...
components/favicon/core/favicon_driver.h:52: virtual void
OnFaviconAvailable(const gfx::Image& image,
On 2014/10/30 21:33:49, sky wrote:
> You need to make it clear image is not necessarily the 16x16 image. Also, you
> need to make it clear what update_active_favicon means.

Done.

[michaelbai, 2014-11-04 05:24:46]

ping

[jif, 2014-11-04 16:21:47]

On 2014/11/04 05:24:46, michaelbai wrote:
> ping

lgtm

[sky, 2014-11-04 18:11:43]

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:224: // Notifies driver favicon
available.
driver->|driver_|

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:228: bool update_active_favicon);
Document what update_active_favicon. It's ok to say see
FaviconDriver::OnFaviconAvailable for details.

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

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:42: class Observer {
Move to it's own file and add protected virtual destructor.

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

https://codereview.chromium.org/684983003/diff/120001/components/favicon/core...
components/favicon/core/favicon_driver.h:49: // Notifies the driver favicon
|image| is available to use. |image| might
update_active_image -> is_active_favicon.

Notifies the driver a favicon image is available. |image| is not necessarily
16x16. |icon_url| is the url the image is from. If |is_active_favicon| is true
the image corresponds to the favicon (possibly empty) of the page.

[michaelbai, 2014-11-04 19:26:57]

PTAL

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:224: // Notifies driver favicon
available.
On 2014/11/04 18:11:43, sky wrote:
> driver->|driver_|

Done.

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:228: bool update_active_favicon);
On 2014/11/04 18:11:43, sky wrote:
> Document what update_active_favicon. It's ok to say see
> FaviconDriver::OnFaviconAvailable for details.

Done.

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

https://codereview.chromium.org/684983003/diff/120001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:42: class Observer {
On 2014/11/04 18:11:43, sky wrote:
> Move to it's own file and add protected virtual destructor.

Done.

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

https://codereview.chromium.org/684983003/diff/120001/components/favicon/core...
components/favicon/core/favicon_driver.h:49: // Notifies the driver favicon
|image| is available to use. |image| might
On 2014/11/04 18:11:43, sky wrote:
> update_active_image -> is_active_favicon.
> 
> Notifies the driver a favicon image is available. |image| is not necessarily
> 16x16. |icon_url| is the url the image is from. If |is_active_favicon| is true
> the image corresponds to the favicon (possibly empty) of the page.

Done.

[sky, 2014-11-04 21:06:31]

LGTM

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_tab_helper_observer.h (right):

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_tab_helper_observer.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
style guide says no more (c).

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_tab_helper_observer.h:21: virtual
~FaviconTabHelperObserver();
You can inline the destructor and nuke the .cc.

[michaelbai, 2014-11-04 21:51:27]

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
File chrome/browser/favicon/favicon_tab_helper_observer.h (right):

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_tab_helper_observer.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/11/04 21:06:31, sky wrote:
> style guide says no more (c).

Done.

https://codereview.chromium.org/684983003/diff/10004/chrome/browser/favicon/f...
chrome/browser/favicon/favicon_tab_helper_observer.h:21: virtual
~FaviconTabHelperObserver();
On 2014/11/04 21:06:31, sky wrote:
> You can inline the destructor and nuke the .cc.

Done.

[michaelbai, 2014-11-04 21:51:37]

The CQ bit was checked by michaelbai@chromium.org

[commit-bot: I haz the power, 2014-11-04 21:52:32]

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

[commit-bot: I haz the power, 2014-11-04 23:10:19]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2014-11-04 23:11:16]

Patchset 10 (id:??) landed as
https://crrev.com/498547a3a74cbd304593305a5166a114000ef9b6
Cr-Commit-Position: refs/heads/master@{#302700}