Refactor FaviconDriver::OnFaviconAvailable()

Refactor FaviconDriver::OnFaviconAvailable()

This CL:
- Removes weird OnFaviconAvailable() behavior if gfx::Image is empty.
- Merges: FaviconDriver::SetActiveFaviconValidity()
          FaviconDriver::SetActiveFaviconURL()
          FaviconDriver::SetActiveFaviconImage()
- Merges FaviconDriverObserver::OnFaviconAvailable() and
         FaviconDriverObserver::OnFaviconUpdated().

BUG=542057
Committed: https://crrev.com/7e8e893275dd310e13ae77de88ebec260d61994a
Cr-Commit-Position: refs/heads/master@{#361132}

[pkotwicz, 2015-11-10 01:46:33]

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

[pkotwicz, 2015-11-10 01:46:33]

Scott, can you please take a look?

I am not a big fan of introducing yet another icon type "NotificationIconType"
but I can't think of a better solution

[pkotwicz, 2015-11-10 01:46:49]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2015-11-10 01:46:57]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-11-10 01:47:16]

Description was changed from

==========
Refactor FaviconDriver::OnFaviconAvailable()

This CL:
- Removes weird OnFaviconAvailable() behavior if gfx::Image is empty.
- Merges: FaviconDriver::SetActiveFaviconValidity()
          FaviconDriver::SetActiveFaviconURL()
          FaviconDriver::SetActiveFaviconImage()
- Merges FaviconDriverObserver::OnFaviconAvailable() and
         FaviconDriverObserver::OnFaviconUpdated().

BUG=542057
==========

to

==========
Refactor FaviconDriver::OnFaviconAvailable()

This CL:
- Removes weird OnFaviconAvailable() behavior if gfx::Image is empty.
- Merges: FaviconDriver::SetActiveFaviconValidity()
          FaviconDriver::SetActiveFaviconURL()
          FaviconDriver::SetActiveFaviconImage()
- Merges FaviconDriverObserver::OnFaviconAvailable() and
         FaviconDriverObserver::OnFaviconUpdated().

BUG=542057
==========

[pkotwicz, 2015-11-16 16:42:01]

Scott, Ping!

[sky, 2015-11-19 17:57:37]

Sorry, been busy and this doesn't seem like a high priority. Will find
time for it today.

On Mon, Nov 16, 2015 at 8:42 AM,  <pkotwicz@chromium.org> wrote:
> Scott, Ping!
>
> https://codereview.chromium.org/1407353012/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2015-11-20 00:44:26]

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:148: // This should never
occur.
DCHECK

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:41: virtual GURL GetFaviconURL() const
= 0;
I'm worried about the timing of this. Why is it needed?

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver_observer.h:9: #include "url/gurl.h"
You should be able to forward declare GURL.

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver_observer.h:28: NON_TOUCH_16_DIP,
I'm not a fan of NON_TOUCH_ here. I prefer DEFAULT, LARGEST and TOUCH_LARGEST.

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.cc:350: if (image.IsEmpty())
Are you sure no one needs to distinguish if only an empty icon was provided?

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.h:84:
FaviconDriverObserver::NotificationIconType handler_type);
I find it weird that this takes a NotificationIconType. I think the type should
be called FaviconType and declared in its own header that both places consume.

[pkotwicz, 2015-11-20 03:13:29]

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:41: virtual GURL GetFaviconURL() const
= 0;
I think that it is reasonable for a caller to be able to both get the current
favicon gfx::Image and the current favicon GURL.

This is used in tab_contents_tag_browsertest.cc

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.h:84:
FaviconDriverObserver::NotificationIconType handler_type);
I wanted to avoid confusion with favicon_base::IconType in
components/favicon_base/favicon_types.h
What do you suggest?

[sky, 2015-11-20 16:03:59]

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:41: virtual GURL GetFaviconURL() const
= 0;
On 2015/11/20 03:13:29, pkotwicz wrote:
> I think that it is reasonable for a caller to be able to both get the current
> favicon gfx::Image and the current favicon GURL.
> 
> This is used in tab_contents_tag_browsertest.cc

I don't like adding code just for a test, especially tricky code like this.

[pkotwicz, 2015-11-20 16:06:11]

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.h:84:
FaviconDriverObserver::NotificationIconType handler_type);
I wanted to avoid confusion with favicon_base::IconType in
components/favicon_base/favicon_types.h
What do you suggest?

[sky, 2015-11-20 16:34:41]

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.h:84:
FaviconDriverObserver::NotificationIconType handler_type);
On 2015/11/20 16:06:11, pkotwicz wrote:
> I wanted to avoid confusion with favicon_base::IconType in
> components/favicon_base/favicon_types.h
> What do you suggest?

Ok, fair enough.

[pkotwicz, 2015-11-21 23:02:40]

Patchset #2 (id:60001) has been deleted

[pkotwicz, 2015-11-21 23:04:08]

Scott, can you please take another look?

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:148: // This should never
occur.
On 2015/11/20 00:44:26, sky wrote:
> DCHECK

Done.

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver.h:41: virtual GURL GetFaviconURL() const
= 0;
On 2015/11/20 16:03:59, sky wrote:
> On 2015/11/20 03:13:29, pkotwicz wrote:
> > I think that it is reasonable for a caller to be able to both get the
current
> > favicon gfx::Image and the current favicon GURL.
> > 
> > This is used in tab_contents_tag_browsertest.cc
> 
> I don't like adding code just for a test, especially tricky code like this.

Done.

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

https://codereview.chromium.org/1407353012/diff/40001/components/favicon/core...
components/favicon/core/favicon_driver_observer.h:28: NON_TOUCH_16_DIP,
I think that the names that I have are more descriptive of what the constants
are for.
In particular, I can foresee people thinking that LARGEST refers to the largest
icon available. LARGEST refers to the largest favicon excluding touch icons.

It would be nice if we could eliminate NON_TOUCH_LARGEST and TOUCH_LARGEST and
just have LARGEST. However, we can't do that because of super bad Android code

[sky, 2015-11-22 15:48:36]

LGTM

[pkotwicz, 2015-11-23 16:36:30]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-11-23 16:37:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407353012/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407353012/80001

[commit-bot: I haz the power, 2015-11-23 17:35:54]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2015-11-23 17:36:49]

Patchset 2 (id:??) landed as
https://crrev.com/7e8e893275dd310e13ae77de88ebec260d61994a
Cr-Commit-Position: refs/heads/master@{#361132}