New issue
Advanced search Search tips

Issue 741340 link

Starred by 2 users
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Oct 2017
Cc:
pkotw...@chromium.org
treib@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 498436
issue 736254
issue 754386



Sign in to add a comment

Propagate favicon through client-side redirects

Project Member Reported by mastiz@chromium.org, Jul 12 2017 
In the current code, when favicons are found for a page, they are cached for the page's URL and the server-side redirect chain that led to that page.

For client-side redirects, no equivalent exists, except a workaround that strips the fragment/ref of the URL.

Ideally, anything that is perceived as a redirect by the user (e.g. meta refresh tag, Javascript location overrides, history API) should be treated similarly to server-side redirects for favicon purposes. Not doing so results in NTP tiles without icons (or with less icons).

(Desktop thumbnails might be affected by the same issue)

Known examples of gray (or colored) NTP tiles include:
www.hrs.de (redirects to touch.hrs.de)
maps.google.com (redirects to e.g. https://www.google.com/maps/@48.142803,11.5411858,17z)
mail.google.com (a series of server-side redirects followed by client-side ones)
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 13 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eccf32f192883a884c16575ac7c5c2d733747a61

commit eccf32f192883a884c16575ac7c5c2d733747a61
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Jul 13 11:54:15 2017

Add browser tests for redirect-like cases that currently work well

The goal is to verify that such pages would display a nice icon in UIs
like the NTP, which typically use the page URL prior to client-side
redirects.

Examples that couldn't be added because they are not supported well:
- Client-side redirect with meta refresh tags.
- history.pushState().
- history.replaceState().

Bug:  741340 
Change-Id: Ia37ccf59c5da3322f9fb34b3f8e05b75d1d51414
Reviewed-on: https://chromium-review.googlesource.com/567502
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486349}
[modify] https://crrev.com/eccf32f192883a884c16575ac7c5c2d733747a61/chrome/browser/favicon/content_favicon_driver_browsertest.cc
[add] https://crrev.com/eccf32f192883a884c16575ac7c5c2d733747a61/chrome/test/data/favicon/page_with_hash_override.html
[add] https://crrev.com/eccf32f192883a884c16575ac7c5c2d733747a61/chrome/test/data/favicon/page_with_location_override_within_page.html
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c9435e4568fdebea953fdf0f1d864fc73002b081

commit c9435e4568fdebea953fdf0f1d864fc73002b081
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jul 26 02:24:33 2017

Fix meta refresh tags not propagating back favicons

In the presence of a client-side redirect via meta refresh tags (or
similar, as reflected in new tests), if the landing page lists favicons,
they should be properly associated to the whole redirect chain.

HistoryBackend sometimes strips the first element of the chain because
the redirecting URL must have been added previously. For the purpose of
favicon propagation, we'd like to set the favicon for that URL too, in
fact to all involved URLs if it happened to be a redirect chain.

The change intends to improve icons (mobile), thumbnails (desktop)
and titles (both) in the NTP.

Bug:  741340 ,498436
Change-Id: I9865f24f0b84032e072dfa53a76e5abef349ea4e
Reviewed-on: https://chromium-review.googlesource.com/569760
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489534}
[modify] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/chrome/browser/favicon/content_favicon_driver_browsertest.cc
[add] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/chrome/test/data/favicon/page_with_location_override_to_other_page.html
[add] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/chrome/test/data/favicon/page_with_meta_refresh_tag.html
[add] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/chrome/test/data/favicon/page_with_meta_refresh_tag_to_server_redirect.html
[modify] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/components/history/core/browser/history_backend.h
[modify] https://crrev.com/c9435e4568fdebea953fdf0f1d864fc73002b081/components/history/core/browser/history_backend_unittest.cc

Comment 3 by mastiz@chromium.org, Aug 10 2017

Blocking: 754386
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 15 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/047511efacd04f9849483a1b9060927e97d05b95

commit 047511efacd04f9849483a1b9060927e97d05b95
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Aug 15 22:23:32 2017

Propagate favicons across redirect-like pushState/replaceState

Many pages use Javascript to change the URL in the omnibox with the
history API, namely pushState() and replaceState(), which are considered
in-page navigations because the actual content of the page doesn't
change.

For pages that trigger such functions early, before the page has loaded,
no favicon candidates are received (because content reports favicons in
a late stage) and hence no favicons are associated to the URL prior to
the URL update. This leads to gray tiles in the New Tab Page, because
they point to the first.

We fix this by special-handling this particular scenario, by introducing
a state variable (FaviconHandler::candidates_received_) to distinguish
whether the previous page was fully loaded (or at least candidates were
provided).

Bug:  741340 , 736254 , 754386 
Change-Id: I2c11a3238bf1e14652bb20abc8cfbb3c46dc848a
Reviewed-on: https://chromium-review.googlesource.com/595977
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494574}
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/chrome/browser/favicon/content_favicon_driver_browsertest.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/chrome/browser/sessions/session_restore_delegate.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/chrome/browser/sessions/tab_loader.cc
[add] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/chrome/test/data/favicon/pushstate_with_favicon.html
[add] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/chrome/test/data/favicon/replacestate_with_favicon.html
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/content/content_favicon_driver.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_driver.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_driver_impl.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_driver_impl.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_handler.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_handler.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_handler_unittest.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_service.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_service_impl.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/favicon_service_impl.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/core/test/mock_favicon_service.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/ios/web_favicon_driver.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/favicon/ios/web_favicon_driver.mm
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/history/core/browser/history_backend.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/history/core/browser/history_backend_unittest.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/history/core/browser/history_service.cc
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/components/history/core/browser/history_service.h
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/ios/chrome/browser/reading_list/reading_list_distiller_page.mm
[modify] https://crrev.com/047511efacd04f9849483a1b9060927e97d05b95/ios/chrome/browser/tabs/tab.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d

commit 7b244fb7f5e5e1a455f35925abcfc3d80ebe391d
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Sep 20 07:41:20 2017

Propagate all page URLs at once via SetFavicons()

This is a minor performance improvement as a follow up to
https://chromium-review.googlesource.com/595977, where rather advanced
cases (redirect-like pushState/replaceState) cause a favicon to be
associated to multiple page URLs.

In the first implementation, prior to this patch, there is a loop in
FaviconHandler::SetFavicon() to process each page URL individually and
update the history database. This has the downside that
HistoryBackend::SetFaviconsImpl() processes the same icon multiple
times and needs to compare bitmaps to realize only the mappings need to
be changed.

As an alternative, the FaviconService/HistoryService signature for
SetFavicons() can be extended to support multiple page URLs.

Bug:  741340 
Change-Id: Id2c3ff0379c8305e86701cd9966d3e4c654da7d4
Reviewed-on: https://chromium-review.googlesource.com/649788
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503078}
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/chrome/browser/bookmarks/bookmark_html_writer_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/chrome/browser/history/android/android_provider_backend_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/chrome/browser/sync/test/integration/bookmarks_helper.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/content/content_favicon_driver.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_handler.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_handler.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_handler_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_service.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_service_impl.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/favicon_service_impl.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/favicon/core/test/mock_favicon_service.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/history/core/browser/history_backend.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/history/core/browser/history_backend_unittest.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/history/core/browser/history_service.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/history/core/browser/history_service.h
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/ntp_tiles/icon_cacher_impl.cc
[modify] https://crrev.com/7b244fb7f5e5e1a455f35925abcfc3d80ebe391d/components/ntp_tiles/icon_cacher_impl_unittest.cc

Comment 6 by mastiz@chromium.org, Oct 30 2017

Status: Fixed (was: Untriaged)
►

Sign in to add a comment