Make FaviconService::GetRawFaviconForPageURL() select the best candidate among all the icon types

Make FaviconService::GetRawFaviconForPageURL() select the best candidate among all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL changes the behavior FaviconHelper#getLocalFaviconImageForURL()

BUG=712466
Review-Url: https://codereview.chromium.org/2823093002
Cr-Commit-Position: refs/heads/master@{#471674}
Committed: https://chromium.googlesource.com/chromium/src/+/1fb2030a6e961c668f3636d6fb65ca4effee0341

[pkotwicz, 2017-04-18 00:00:58]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-04-18 00:17:40]

Description was changed from

==========
ABCD
==========

to

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes FaviconService::GetRawFaviconForPageURL(*, FAVICON |
TOUCH_ICON, *, *) to return the icon url which is a better match.

BUG=712466
==========

[pkotwicz, 2017-04-18 00:17:41]

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

[pkotwicz, 2017-04-18 00:17:51]

mastiz@ can you please take a look?

[pkotwicz, 2017-04-18 00:18:22]

Description was changed from

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes FaviconService::GetRawFaviconForPageURL(*, FAVICON |
TOUCH_ICON, *, *) to return the icon url which is a better match.

BUG=712466
==========

to

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

BUG=712466
==========

[mastiz, 2017-04-18 08:04:35]

Thanks!

Can you please clarify (in the UI description) which UIs would be affected by
this change, if any?

If there's at least one UI affected, we should block this on getting an approval
from UI review, bundled with https://codereview.chromium.org/2795763004/

It looks to me that many clients specify one icon type so that's fine. Then
there's at least WebUI which supports both, but I'm not aware of any UI actually
making use of it (desktop code is a moot point since we don't fetch touch icons
anyway).

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:265: favicon_base::IconType
icon_type,
This signature change is orthogonal. Can you please split it to a dedicated CL?
That part should be trivial so we can have a smaller diff for your CL that
actually changed behavior.

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_service.h (left):

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
components/favicon/core/favicon_service.h:93: // TOUCH_ICON and FAVICON. If
there is no favicon bitmap of size
Update?

[pkotwicz, 2017-04-20 03:57:02]

Description was changed from

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

BUG=712466
==========

to

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL affects FaviconHelper#getLocalFaviconImageForURL()

BUG=712466
==========

[pkotwicz, 2017-04-20 03:59:10]

Description was changed from

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL affects FaviconHelper#getLocalFaviconImageForURL()

BUG=712466
==========

to

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL changes the behavior FaviconHelper#getLocalFaviconImageForURL()

BUG=712466
==========

[pkotwicz, 2017-04-20 04:06:19]

FaviconHelper#getLocalFaviconImageForURL() is the only caller of
FaviconService::GetRawFaviconForPageURL().
FaviconHelper#getLocalFaviconImageForURL() is used on various UIs on Android
such as "Recent Tabs"

I don't think that this CL needs to go through UI review because it "fixes"
FaviconHelper#getLocalFaviconImageForURL()
On Android, we prefer non touch-icons over touch-icons

This CL is different from patch set #1 because it keeps the sorting in
ThumbnailDatabase::GetIconMappingsForPageURL(). The rational for keeping the
sorting is to make the result of ThumbnailDatabase::GetIconMappingsForPageURL()
predictable when there are touch icons and non touch icons which are equally
good

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler_unittest.cc:265: favicon_base::IconType
icon_type,
On 2017/04/18 08:04:34, mastiz wrote:
> This signature change is orthogonal. Can you please split it to a dedicated
CL?
> That part should be trivial so we can have a smaller diff for your CL that
> actually changed behavior.

Done.

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
File components/favicon/core/favicon_service.h (left):

https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
components/favicon/core/favicon_service.h:93: // TOUCH_ICON and FAVICON. If
there is no favicon bitmap of size
On 2017/04/18 08:04:34, mastiz wrote:
> Update?

Done.

[mastiz, 2017-04-20 08:55:50]

LGTM modulo optional comments, thanks!

On 2017/04/20 04:06:19, pkotwicz wrote:
> FaviconHelper#getLocalFaviconImageForURL() is the only caller of
> FaviconService::GetRawFaviconForPageURL().
> FaviconHelper#getLocalFaviconImageForURL() is used on various UIs on Android
> such as "Recent Tabs"

Can you please mention this in the CL description? That is, affected UIs. This
will facilitate search if behavioral changes are reported.

> 
> I don't think that this CL needs to go through UI review because it "fixes"
> FaviconHelper#getLocalFaviconImageForURL()

Fair enough, your call. I have no knowledge of why this logic was introduced in
the first place.

> On Android, we prefer non touch-icons over touch-icons
> 
> This CL is different from patch set #1 because it keeps the sorting in
> ThumbnailDatabase::GetIconMappingsForPageURL(). The rational for keeping the
> sorting is to make the result of
ThumbnailDatabase::GetIconMappingsForPageURL()
> predictable when there are touch icons and non touch icons which are equally
> good
> 
>
https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
> File components/favicon/core/favicon_handler_unittest.cc (right):
> 
>
https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
> components/favicon/core/favicon_handler_unittest.cc:265:
favicon_base::IconType
> icon_type,
> On 2017/04/18 08:04:34, mastiz wrote:
> > This signature change is orthogonal. Can you please split it to a dedicated
> CL?
> > That part should be trivial so we can have a smaller diff for your CL that
> > actually changed behavior.
> 
> Done.
> 
>
https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
> File components/favicon/core/favicon_service.h (left):
> 
>
https://codereview.chromium.org/2823093002/diff/20001/components/favicon/core...
> components/favicon/core/favicon_service.h:93: // TOUCH_ICON and FAVICON. If
> there is no favicon bitmap of size
> On 2017/04/18 08:04:34, mastiz wrote:
> > Update?
> 
> Done.

[mastiz, 2017-04-20 08:55:59]

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
File components/history/core/browser/thumbnail_database.h (right):

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database.h:176: // mapping_data is not
NULL.
Does anybody use the sorting per IconType? If not, update comment and simplify
SQL? This would otherwise remain is historic artifact.

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
File components/history/core/browser/thumbnail_database_unittest.cc (right):

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database_unittest.cc:82: void
AddFaviconAndMapSimple(ThumbnailDatabase* db,
Nit: s/Map/Mapping/?

'Map' can otherwise interpreted as verb, i.e. AddAndMap.

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database_unittest.cc:539:
EXPECT_EQ(kIconUrl3, icon_mappings[2].icon_url);
A more natural way to do this would be to get rid of SortMappingsByIconUrl and
instead introduce MATCHER_P(HasIconUrl) and then do:
EXPECT_THAT(icon_mappings, UnorderedElementsAre(HasIconUrl(kIconUrl1),
HasIconUrl(kIconUrl2), HasIconUrl(kIconUrl3)));

Not feeling strongly though.

[pkotwicz, 2017-04-30 03:49:56]

Scott, can you please take a look?

This CL makes calling FaviconService::GetRawFaviconForPageURL() with multiple
icon types have a more intuitive and a more useful behavior.

This should improve the behavior of FaviconHelper::GetLocalFaviconImageForURL()

This deficiency came up in the code review for
LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache()

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
File components/history/core/browser/thumbnail_database.h (right):

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database.h:176: // mapping_data is not
NULL.
No, no one uses the sorting per IconType. It is useful though. For instance, if
there is a favicon of type FAVICON and a favicon of type TOUCH_ICON stored in
the database for the same page URL the sorting affects which of the two bitmaps
is returned. This makes the favicon logic more predictable which is good

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
File components/history/core/browser/thumbnail_database_unittest.cc (right):

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database_unittest.cc:82: void
AddFaviconAndMapSimple(ThumbnailDatabase* db,
On 2017/04/20 08:55:59, mastiz wrote:
> Nit: s/Map/Mapping/?
> 
> 'Map' can otherwise interpreted as verb, i.e. AddAndMap.

Done.

https://codereview.chromium.org/2823093002/diff/40001/components/history/core...
components/history/core/browser/thumbnail_database_unittest.cc:539:
EXPECT_EQ(kIconUrl3, icon_mappings[2].icon_url);
I chose to not add a GMock dependency to this test for the sake of a smooth
review. Some people like GMock others don't. No clue about sky@'s opinion. I
believe that Ben Goodger dislikes it it

[pkotwicz, 2017-04-30 03:50:08]

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

[pkotwicz, 2017-05-10 20:44:04]

Scott, Ping!

[sky, 2017-05-12 13:23:02]

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

[sky, 2017-05-12 13:23:03]

This LGTM, but the current logic was specifically added by Tao. Tao, is this
change OK for your needs?

https://codereview.chromium.org/2823093002/diff/80001/components/history/core...
File components/history/core/browser/thumbnail_database_unittest.cc (right):

https://codereview.chromium.org/2823093002/diff/80001/components/history/core...
components/history/core/browser/thumbnail_database_unittest.cc:535:
EXPECT_EQ(3u, icon_mappings.size());
ASSERT

[michaelbai, 2017-05-12 16:16:56]

Thanks for pinging me, LGTM

[pkotwicz, 2017-05-14 20:15:00]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-05-14 20:15:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mastiz@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2823093002/#ps100001
(title: "Merge branch 'master' into icon_type")

[commit-bot: I haz the power, 2017-05-14 20:15:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-14 21:06:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-14 21:06:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pkotwicz, 2017-05-15 04:54:05]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-05-15 04:54:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 05:29:45]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494824045244770,
"parent_rev": "7455b06b8b993933393900c6624534335d412e4b", "commit_rev":
"1fb2030a6e961c668f3636d6fb65ca4effee0341"}

[commit-bot: I haz the power, 2017-05-15 05:29:56]

Description was changed from

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL changes the behavior FaviconHelper#getLocalFaviconImageForURL()

BUG=712466
==========

to

==========
Make FaviconService::GetRawFaviconForPageURL() select the best candidate among
all the icon types

This CL changes the behavior of FaviconService::GetRawFaviconForPageURL() when
multiple icon types are passed in.
Scenario:
A page_url is mapped to:
icon_url1 of type FAVICON
icon_url2 of type TOUCH_ICON

Previously
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) returned
the TOUCH_ICON regardless of which icon url was a better match because
TOUCH_ICONs had precence over FAVICONs.

This CL changes
FaviconService::GetRawFaviconForPageURL(*, FAVICON | TOUCH_ICON, *, *) 
to return the icon url which is a better match.

This CL changes the behavior FaviconHelper#getLocalFaviconImageForURL()

BUG=712466

Review-Url: https://codereview.chromium.org/2823093002
Cr-Commit-Position: refs/heads/master@{#471674}
Committed:
https://chromium.googlesource.com/chromium/src/+/1fb2030a6e961c668f3636d6fb65...
==========

[commit-bot: I haz the power, 2017-05-15 05:29:57]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1fb2030a6e961c668f3636d6fb65...