Treat touch icons just like non-touch icons on mobile

Treat touch icons just like non-touch icons on mobile

The goal is to avoid any special-handling of touch icons: they should be
treated just like non-touch icons when it comes to fetching them.

One benefit is that mobile data consumption would be reduced (~halved)
for sites that publish high resolution icons for both touch and
non-touch variants. Both would be downloaded prior to this patch.

This introduces a potential regression affecting bookmarks to sites that
publish:
- A low-resolution favicon (e.g. 16x16).
- A high-resolution touch icon (e.g. 192x192).

Prior to this patch, both would be downloaded. The first would be used
for bookmark sync purposes (e.g. to be later displayed on desktop), and
the second for display purposes.

BUG=698671, 681875

[mastiz, 2017-04-04 12:39:40]

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

[mastiz, 2017-04-04 12:39:41]

This would likely require UI review, which I'd like to bundle with other
special-casing in place for iOS, and possibly the sync quirk to mitigate the
desktop<->mobile favicon conflicts via sync.

Anyhow, I think we can get the code review started.

[mastiz, 2017-04-04 12:39:59]

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

[commit-bot: I haz the power, 2017-04-04 12:40:07]

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-04-04 13:52:00]

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

[commit-bot: I haz the power, 2017-04-04 13:52:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pkotwicz, 2017-04-04 13:58:09]

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

[pkotwicz, 2017-04-04 13:58:10]

+michaelbai@ for Android stuff. My recollection is that Android prefers non
touch favicons over favicons provided that the favicons are large enough. You
will need to update the sorting in FaviconHandler::NotifyFaviconUpdated() to do
this. Currently this preference is kind of expressed in callers to
FaviconService::GetLargestRawFaviconForPageURL()

- You need to update the callers which request touch icons e.g.
BookmarkModel::GetFavicon()

I don't foresee anything major which will break. However, I have not gone
through all of the callers

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

https://codereview.chromium.org/2795763004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:245: service_->SetFavicons(url_,
icon_url, icon_type, image);
In a separate CL you will need to update HistoryBackend::SetFaviconsImpl() so
that it deletes icons which are mapped to the page URL but which are of the
wrong type

[mastiz, 2017-04-05 13:19:21]

On 2017/04/04 13:58:10, pkotwicz wrote:
> +michaelbai@ for Android stuff. My recollection is that Android prefers non
> touch favicons over favicons provided that the favicons are large enough. You
> will need to update the sorting in FaviconHandler::NotifyFaviconUpdated() to
do
> this. Currently this preference is kind of expressed in callers to
> FaviconService::GetLargestRawFaviconForPageURL()

Actually, don't we need to get rid of such preferences before landing this
patch?

Clients currently can define arbitrary type preferences and arbitrary minimum
sizes, so I don't think it's possible/realistic to honor those needs if we merge
the two FaviconHandlers into one.

For this, I've started https://codereview.chromium.org/2796343003. Please let me
know if we should first focus on reviewing that one.

WDYT?

> 
> - You need to update the callers which request touch icons e.g.
> BookmarkModel::GetFavicon()
> 
> I don't foresee anything major which will break. However, I have not gone
> through all of the callers
> 
>
https://codereview.chromium.org/2795763004/diff/1/components/favicon/core/fav...
> File components/favicon/core/favicon_handler.cc (right):
> 
>
https://codereview.chromium.org/2795763004/diff/1/components/favicon/core/fav...
> components/favicon/core/favicon_handler.cc:245: service_->SetFavicons(url_,
> icon_url, icon_type, image);
> In a separate CL you will need to update HistoryBackend::SetFaviconsImpl() so
> that it deletes icons which are mapped to the page URL but which are of the
> wrong type

[pkotwicz, 2017-04-05 14:33:36]

We don't need to get rid of the preferences but we need to make all callers have
the same preference. If all callers have the same preference, then we can adjust
the sorting logic in FaviconService::GetLargestRawFaviconForPageURL()
appropriately

[mastiz, 2017-04-05 14:52:51]

On 2017/04/05 14:33:36, pkotwicz wrote:
> We don't need to get rid of the preferences but we need to make all callers
have
> the same preference. If all callers have the same preference, then we can
adjust
> the sorting logic in FaviconService::GetLargestRawFaviconForPageURL()
> appropriately

That essentially means adjusting the min required size. 

Why is that preferrable to removing the whole preference logic?

We'll need to involve the same people due to effects in the different UIs.

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

michaelbai@ can comment on why we have the preference logic on Android.
crbug.com/298446 has some details

It would be nice if we can make the preference logic on iOS identical to the
preference logic on Android

[mastiz, 2017-04-05 19:48:08]

On 2017/04/05 15:30:30, pkotwicz wrote:
> michaelbai@ can comment on why we have the preference logic on Android.
> crbug.com/298446 has some details
> 
> It would be nice if we can make the preference logic on iOS identical to the
> preference logic on Android

I would certainly be interested in the original rationale and precise UX
guidelines. Reading through the bug, it looks like the initial idea was to break
ties using the type, then evolved into encouraging favicons.

I believe we should aim at simplifying this logic. In the current form, it's too
easy for UIs to screw up (by passing a low min_size, not realizing that this
could choose 16x16 icons, which I believe is the case in different UIs on
Android). Besides, min_sizes are different enough (16 vs 48, IIUC) to prevent
optimizing the fetching logic. 

michaelbai@: we're appreciate some more details here and possibly who else
should be looped in.

[mastiz, 2017-04-18 08:06:01]

On 2017/04/05 19:48:08, mastiz wrote:
> On 2017/04/05 15:30:30, pkotwicz wrote:
> > michaelbai@ can comment on why we have the preference logic on Android.
> > crbug.com/298446 has some details
> > 
> > It would be nice if we can make the preference logic on iOS identical to the
> > preference logic on Android
> 
> I would certainly be interested in the original rationale and precise UX
> guidelines. Reading through the bug, it looks like the initial idea was to
break
> ties using the type, then evolved into encouraging favicons.
> 
> I believe we should aim at simplifying this logic. In the current form, it's
too
> easy for UIs to screw up (by passing a low min_size, not realizing that this
> could choose 16x16 icons, which I believe is the case in different UIs on
> Android). Besides, min_sizes are different enough (16 vs 48, IIUC) to prevent
> optimizing the fetching logic. 
> 
> michaelbai@: we're appreciate some more details here and possibly who else
> should be looped in.

Friendly ping for michaelbai@, thanks!

[michaelbai, 2017-04-20 16:20:30]

On 2017/04/18 08:06:01, mastiz wrote:
> On 2017/04/05 19:48:08, mastiz wrote:
> > On 2017/04/05 15:30:30, pkotwicz wrote:
> > > michaelbai@ can comment on why we have the preference logic on Android.
> > > crbug.com/298446 has some details
> > > 
> > > It would be nice if we can make the preference logic on iOS identical to
the
> > > preference logic on Android
> > 
> > I would certainly be interested in the original rationale and precise UX
> > guidelines. Reading through the bug, it looks like the initial idea was to
> break
> > ties using the type, then evolved into encouraging favicons.
> > 
> > I believe we should aim at simplifying this logic. In the current form, it's
> too
> > easy for UIs to screw up (by passing a low min_size, not realizing that this
> > could choose 16x16 icons, which I believe is the case in different UIs on
> > Android). Besides, min_sizes are different enough (16 vs 48, IIUC) to
prevent
> > optimizing the fetching logic. 
> > 
> > michaelbai@: we're appreciate some more details here and possibly who else
> > should be looped in.
> 
> Friendly ping for michaelbai@, thanks!

The reason is that we will deprecate apple touch icon finally. 

What's the regression of 16x16 standard icon?

[mastiz, 2017-04-20 16:46:55]

On 2017/04/20 16:20:30, michaelbai - ooo wrote:
> On 2017/04/18 08:06:01, mastiz wrote:
> > On 2017/04/05 19:48:08, mastiz wrote:
> > > On 2017/04/05 15:30:30, pkotwicz wrote:
> > > > michaelbai@ can comment on why we have the preference logic on Android.
> > > > crbug.com/298446 has some details
> > > > 
> > > > It would be nice if we can make the preference logic on iOS identical to
> the
> > > > preference logic on Android
> > > 
> > > I would certainly be interested in the original rationale and precise UX
> > > guidelines. Reading through the bug, it looks like the initial idea was to
> > break
> > > ties using the type, then evolved into encouraging favicons.
> > > 
> > > I believe we should aim at simplifying this logic. In the current form,
it's
> > too
> > > easy for UIs to screw up (by passing a low min_size, not realizing that
this
> > > could choose 16x16 icons, which I believe is the case in different UIs on
> > > Android). Besides, min_sizes are different enough (16 vs 48, IIUC) to
> prevent
> > > optimizing the fetching logic. 
> > > 
> > > michaelbai@: we're appreciate some more details here and possibly who else
> > > should be looped in.
> > 
> > Friendly ping for michaelbai@, thanks!
> 
> The reason is that we will deprecate apple touch icon finally. 

Who is the main sponsor/lead of this idea? I mean, I've seen this mentioned in
multiple places, but at some point it's worth reconsidering if this is ever
going to happen?

> 
> What's the regression of 16x16 standard icon?

The regression is that sync would now use a downscaled icon (say,
192x192-->16x16) instead of the other available 16x16 icon which doesn't need
resampling for sync.

[michaelbai, 2017-04-20 17:46:54]

On 2017/04/20 16:46:55, mastiz wrote:
> On 2017/04/20 16:20:30, michaelbai - ooo wrote:
> > On 2017/04/18 08:06:01, mastiz wrote:
> > > On 2017/04/05 19:48:08, mastiz wrote:
> > > > On 2017/04/05 15:30:30, pkotwicz wrote:
> > > > > michaelbai@ can comment on why we have the preference logic on
Android.
> > > > > crbug.com/298446 has some details
> > > > > 
> > > > > It would be nice if we can make the preference logic on iOS identical
to
> > the
> > > > > preference logic on Android
> > > > 
> > > > I would certainly be interested in the original rationale and precise UX
> > > > guidelines. Reading through the bug, it looks like the initial idea was
to
> > > break
> > > > ties using the type, then evolved into encouraging favicons.
> > > > 
> > > > I believe we should aim at simplifying this logic. In the current form,
> it's
> > > too
> > > > easy for UIs to screw up (by passing a low min_size, not realizing that
> this
> > > > could choose 16x16 icons, which I believe is the case in different UIs
on
> > > > Android). Besides, min_sizes are different enough (16 vs 48, IIUC) to
> > prevent
> > > > optimizing the fetching logic. 
> > > > 
> > > > michaelbai@: we're appreciate some more details here and possibly who
else
> > > > should be looped in.
> > > 
> > > Friendly ping for michaelbai@, thanks!
> > 
> > The reason is that we will deprecate apple touch icon finally. 
> 
> Who is the main sponsor/lead of this idea? I mean, I've seen this mentioned in
> multiple places, but at some point it's worth reconsidering if this is ever
> going to happen?
> 

Grace

> > 
> > What's the regression of 16x16 standard icon?
> 
> The regression is that sync would now use a downscaled icon (say,
> 192x192-->16x16) instead of the other available 16x16 icon which doesn't need
> resampling for sync.