Introduce dedicated enum value for icons from Web Manifests

Introduce dedicated enum value for icons from Web Manifests

This enum is used to cache icons in the local database and by multiple
API calls to either select or prioritize favicons of different kind. By
introducing a new value (WEB_MANIFEST_ICON) we could allow UIs to use
them or not.

This allows an improvement in the fetching logic which is now trivial to
implement: Manifest icons should compete with Touch Icons rather than
regular favicons (i.e. if manifest icons are available, there is no need
to fetch touch icons, because the first are usually a good replacement).

BUG=690383
Review-Url: https://codereview.chromium.org/2891333002
Cr-Commit-Position: refs/heads/master@{#475149}
Committed: https://chromium.googlesource.com/chromium/src/+/1267cdae719e984677103603b58af30c6bf166f7

[mastiz, 2017-05-19 10:33:15]

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

[mastiz, 2017-05-19 10:33:15]

I'm not entirely convinced this change is desirable, but I see some important
upsides so I'm following your proposal.

The fact that I didn't have to update the handler type in unit tests surfaces
something fishy, i.e. that tests pass regardless of database lookups for page
URL. We might want to do something about it.

[Eugene But (OOO till 7-30), 2017-05-19 14:21:30]

eugenebut@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-05-19 14:21:31]

lgtm for ios/web if you land this

https://codereview.chromium.org/2891333002/diff/1/ios/web/public/favicon_url.h
File ios/web/public/favicon_url.h (right):

https://codereview.chromium.org/2891333002/diff/1/ios/web/public/favicon_url....
ios/web/public/favicon_url.h:24: WEB_MANIFEST_ICON = 1 << 3
nit: Do you want to add comma after the last item, so next time we don't have to
touch it?

[mastiz, 2017-05-22 05:17:58]

pkotwicz@: friendly ping.

[pkotwicz, 2017-05-24 05:13:20]

I will take a look tomorrow sorry for the delay. (Monday was a holiday for us)

[pkotwicz, 2017-05-24 14:27:43]

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

[pkotwicz, 2017-05-24 14:27:44]

+sky@ for his opinion on the new icon type (because he probably has one). I
think the new icon type is the "right thing"

Mikel, sorry for the delay. I was under the weather yesterday. The CL looks good
overall. You forgot to change some bits in HistoryBackend though.

https://codereview.chromium.org/2891333002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/2891333002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:63:
icon_types.push_back(favicon_base::IconType::WEB_MANIFEST_ICON);
You also need to update:
- AddToHomescreenDataFetcher::FetchFavicon()
- FaviconHelper#getLocalFaviconImageForURL()
- It looks like there are places on the desktop Mac code which need to be
updated as well

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:397: const std::vector<FaviconURL>&
candidates) {
Shouldn't |candidates| be of type favicon_base::WEB_MANIFEST_ICON?

I am referring to ExtractManifestIcons() in content_favicon_driver.cc

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:533: :
favicon_base::WEB_MANIFEST_ICON);
You need to update HistoryBackend::SetFaviconMappingsForPage() so that saving an
icon of type WEB_MANIFEST_ICON deletes icons of type TOUCH_ICON and
TOUCH_PRECOMPOSED_ICON

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:298:
favicon_base::IconType::TOUCH_ICON,
Shouldn't this be IconType::WEB_MANIFEST_ICON

https://codereview.chromium.org/2891333002/diff/1/components/favicon_base/fav...
File components/favicon_base/favicon_types.h (left):

https://codereview.chromium.org/2891333002/diff/1/components/favicon_base/fav...
components/favicon_base/favicon_types.h:27: // largest IconType takes priority
if data for multiple IconTypes is available.
Thank you for cleaning up the comment!

[sky, 2017-05-24 18:08:53]

I agree that the new type seems right.

[mastiz, 2017-05-25 11:14:14]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[mastiz, 2017-05-25 11:14:17]

Thanks, PTAL.

https://codereview.chromium.org/2891333002/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc
(right):

https://codereview.chromium.org/2891333002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/search/suggestions/url_suggestion_result.cc:63:
icon_types.push_back(favicon_base::IconType::WEB_MANIFEST_ICON);
On 2017/05/24 14:27:44, pkotwicz wrote:
> You also need to update:
> - AddToHomescreenDataFetcher::FetchFavicon()
> - FaviconHelper#getLocalFaviconImageForURL()
> - It looks like there are places on the desktop Mac code which need to be
> updated as well

Done.

Mac can be addressed in follow-up CLs since we don't even expose the flag there,
and we want to experiment on mobile first.

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:397: const std::vector<FaviconURL>&
candidates) {
On 2017/05/24 14:27:44, pkotwicz wrote:
> Shouldn't |candidates| be of type favicon_base::WEB_MANIFEST_ICON?
> 
> I am referring to ExtractManifestIcons() in content_favicon_driver.cc

Done, changed. It's certainly cleaner this way, although the field is ignored
altogether.

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:533: :
favicon_base::WEB_MANIFEST_ICON);
On 2017/05/24 14:27:44, pkotwicz wrote:
> You need to update HistoryBackend::SetFaviconMappingsForPage() so that saving
an
> icon of type WEB_MANIFEST_ICON deletes icons of type TOUCH_ICON and
> TOUCH_PRECOMPOSED_ICON

Done.

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:298:
favicon_base::IconType::TOUCH_ICON,
On 2017/05/24 14:27:44, pkotwicz wrote:
> Shouldn't this be IconType::WEB_MANIFEST_ICON

Why? Both are hacky, but WEB_MANIFEST_ICON seems even worse to me unless there
are practical implications I'm missing.

https://codereview.chromium.org/2891333002/diff/1/ios/web/public/favicon_url.h
File ios/web/public/favicon_url.h (right):

https://codereview.chromium.org/2891333002/diff/1/ios/web/public/favicon_url....
ios/web/public/favicon_url.h:24: WEB_MANIFEST_ICON = 1 << 3
On 2017/05/19 14:21:31, Eugene But wrote:
> nit: Do you want to add comma after the last item, so next time we don't have
to
> touch it?

Done.

[commit-bot: I haz the power, 2017-05-25 11:14:28]

Dry run: 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-25 11:24:02]

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

[commit-bot: I haz the power, 2017-05-25 11:24:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-05-25 20:18:19]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 20:19:13]

Dry run: 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-25 21:01:46]

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

[commit-bot: I haz the power, 2017-05-25 21:01:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pkotwicz, 2017-05-26 06:15:42]

I'll take a look at the CL tomorrow morning

[pkotwicz, 2017-05-26 14:49:56]

LGTM with nits

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2891333002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:298:
favicon_base::IconType::TOUCH_ICON,
I see now. Sorry for the error on my part.

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
components/favicon/core/favicon_driver_impl.cc:107: // override inline
favicons).
You need to update this comment

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
components/favicon/core/favicon_driver_impl.cc:109: handler->icon_types() &
favicon_base::TOUCH_ICON
Might as well make this WEB_MANIFEST_ICON

[mastiz, 2017-05-26 18:30:25]

Comments addressed, thanks.

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
File components/favicon/core/favicon_driver_impl.cc (right):

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
components/favicon/core/favicon_driver_impl.cc:107: // override inline
favicons).
On 2017/05/26 14:49:56, pkotwicz wrote:
> You need to update this comment

Done.

https://codereview.chromium.org/2891333002/diff/80001/components/favicon/core...
components/favicon/core/favicon_driver_impl.cc:109: handler->icon_types() &
favicon_base::TOUCH_ICON
On 2017/05/26 14:49:56, pkotwicz wrote:
> Might as well make this WEB_MANIFEST_ICON

Done.

[mastiz, 2017-05-26 18:31:33]

sky@: would you mind taking a look, at history component and possibly beyond?
Thx!

[sky, 2017-05-26 19:50:36]

I only looked at history, LGTM

[mastiz, 2017-05-26 20:05:16]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 20:05:49]

Dry run: CQ is trying da patch.

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

[mastiz, 2017-05-26 20:31:43]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2017-05-26 20:31:54]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-05-26 20:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, pkotwicz@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2891333002/#ps120001
(title: "Put browsertest behind #if.")

[commit-bot: I haz the power, 2017-05-26 20:33:12]

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-26 21:57:13]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495830714846430,
"parent_rev": "74545c81cc8998d6ea435a79330e4c5be506b36e", "commit_rev":
"1267cdae719e984677103603b58af30c6bf166f7"}

[commit-bot: I haz the power, 2017-05-26 21:57:31]

Description was changed from

==========
Introduce dedicated enum value for icons from Web Manifests

This enum is used to cache icons in the local database and by multiple
API calls to either select or prioritize favicons of different kind. By
introducing a new value (WEB_MANIFEST_ICON) we could allow UIs to use
them or not.

This allows an improvement in the fetching logic which is now trivial to
implement: Manifest icons should compete with Touch Icons rather than
regular favicons (i.e. if manifest icons are available, there is no need
to fetch touch icons, because the first are usually a good replacement).

BUG=690383
==========

to

==========
Introduce dedicated enum value for icons from Web Manifests

This enum is used to cache icons in the local database and by multiple
API calls to either select or prioritize favicons of different kind. By
introducing a new value (WEB_MANIFEST_ICON) we could allow UIs to use
them or not.

This allows an improvement in the fetching logic which is now trivial to
implement: Manifest icons should compete with Touch Icons rather than
regular favicons (i.e. if manifest icons are available, there is no need
to fetch touch icons, because the first are usually a good replacement).

BUG=690383

Review-Url: https://codereview.chromium.org/2891333002
Cr-Commit-Position: refs/heads/master@{#475149}
Committed:
https://chromium.googlesource.com/chromium/src/+/1267cdae719e984677103603b58a...
==========

[commit-bot: I haz the power, 2017-05-26 21:57:33]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/1267cdae719e984677103603b58a...