Simplify FaviconDriverImpl by removing extra FaviconHandler member.

Simplify FaviconDriverImpl by removing extra FaviconHandler member.

This CL removes the large_icon_handler_ instance from the FaviconDriverImpl without changing the overall functionality.

This instance used to provide large icon handling that was identical to the already extant, but not always enabled, touch_icon_handler_ instance. If those two instances were enabled at the same time, they would step on each other's toes trying to update the history backend with multiple updates for a given touch icon.

If the large favicon handler was expanded to also be responsible for fetching large representation of FAVICO images, it would also step on the toes of the (always enabled) favicon handler.

The large icon handler does not need to exist, when large icons are needed, the already existing touch icon handler can be enabled and the already existing favicon handler can be instructed to download large images.

Design Doc:
https://docs.google.com/document/d/1rv4x3goTWFJzQu5a_h94xfNhSUG3sU98lfxa1Prys1w/edit

BUG=467712

[Roger McFarlane (Chromium), 2015-04-30 19:11:12]

rogerm@chromium.org changed reviewers:
+ beaudoin@chromium.org, huangs@chromium.org, pkotwicz@chromium.org

[Roger McFarlane (Chromium), 2015-04-30 19:11:12]

PTAL.

This CL gets rid of the problem where the favicon handlers can step on each
others toes (by making their updates disjoint).

[pkotwicz, 2015-04-30 20:11:14]

Doesn't this CL change which favicon/favicon bitmap is downloaded and saved to
history?

With this CL applied:
- The favicon no longer shows up in the tab strip
- The favicon for a different icon URL is selected for the bookmark for
https://x20web.corp.google.com/~pkotwicz/change_favicon/chameleon.html

[pkotwicz, 2015-04-30 20:12:50]

For the sake of clarity, I am describing the change in behavior w.r.t to running
with the --enable-icon-ntp flag with and without this patch applied

[huangs, 2015-04-30 20:23:28]

On 2015/04/30 20:12:50, pkotwicz wrote:
> For the sake of clarity, I am describing the change in behavior w.r.t to
running
> with the --enable-icon-ntp flag with and without this patch applied

Rather than removing the new handler, it merges it with touch_icon_handler_. In
particular, |touch_icon_handler_| is added to desktop if Icon NTP is enabled.

[huangs, 2015-04-30 20:23:41]

Some comments.

https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
File components/favicon/core/favicon_handler.cc (right):

https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
components/favicon/core/favicon_handler.cc:544: #if defined(OS_ANDROID)
Can we remove the OS_ANDROID case?

https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
File components/favicon/core/favicon_handler.h (right):

https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
components/favicon/core/favicon_handler.h:73: // favicon candidates, if
|download_largest_favicon_| is true, the largest
Update name |download_largest_favicon_|

https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
components/favicon/core/favicon_handler.h:81: enum Type { FAVICON, TOUCH, LARGE
};
Remove LARGE?

[Roger McFarlane (Chromium), 2015-04-30 22:42:03]

No, Peter is right. Upon further testing... this doesn't work. Not sure
what I was seeing when I first tested.  :(

The original approach worked better. I'll play with it more tonight to see
if I can get rid of the icon proliferation problem. (Add a size comparison
the the deletion matcher?)
On Apr 30, 2015 4:23 PM, <huangs@chromium.org> wrote:

> Some comments.
>
>
>
>
https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
> File components/favicon/core/favicon_handler.cc (right):
>
>
>
https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
> components/favicon/core/favicon_handler.cc:544: #if defined(OS_ANDROID)
> Can we remove the OS_ANDROID case?
>
>
>
https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
> File components/favicon/core/favicon_handler.h (right):
>
>
>
https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
> components/favicon/core/favicon_handler.h:73: // favicon candidates, if
> |download_largest_favicon_| is true, the largest
> Update name |download_largest_favicon_|
>
>
>
https://chromiumcodereview.appspot.com/1118073002/diff/1/components/favicon/c...
> components/favicon/core/favicon_handler.h:81: enum Type { FAVICON,
> TOUCH, LARGE };
> Remove LARGE?
>
> https://chromiumcodereview.appspot.com/1118073002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2015-04-30 23:07:03]

Adding a size comparison to the deletion matcher would still be suboptimal. It
would be possible for a site to store a favicon of each pixel size that we can
download a favicon at. 192*192=36864

[beaudoin, 2015-08-11 17:01:44]

Removing myself from reviewers to clean things up.

[beaudoin, 2015-08-11 17:02:54]

beaudoin@chromium.org changed reviewers:
- beaudoin@chromium.org

[pkotwicz, 2015-08-24 17:11:54]

pkotwicz@chromium.org changed reviewers:
- pkotwicz@chromium.org