New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 754386 link

Starred by 1 user
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Sep 2017
Cc:
█ ramyan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 741340


Sign in to add a comment

maps.google.com displays gray icon on the NTP

Project Member Reported by mastiz@chromium.org, Aug 10 2017 
Chrome Version: All
OS: Android

What steps will reproduce the problem?
(1) Clear browsing history
(2) Visit maps.google.com
(3) Open NTP

What is the expected result?
A Most Visited tile should be displayed for Google Maps with an appropriate (non-gray) icon.

What happens instead?
The displayed icon is gray.
Clicking on the tile doesn't solve the problem.

Reason:
A series of client-side redirects and Javascript history replaceState() causes Chrome to not associate the favicon to the original URL in the chain, where the NTP tile is pointing to. Hence, all lookups in the favicon database fail, leading to a gray tile.

Comment 1 by fhorschig@chromium.org, Aug 11 2017

Labels: zine-triaged
Project Member

Comment 2 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

Comment 3 by mastiz@chromium.org, Sep 4 2017

Status: Fixed (was: Started)
Verified on dev.
►

Sign in to add a comment