Commit e17a15e4 authored by Scott Violet's avatar Scott Violet Committed by Commit Bot

favicon: makes FaviconHandler::FetchFavicon() early out

Specifically if the url is the same as the last page and the
navigation is a same-document navigation, then FaviconHandler need
not do anything. This scenario happens when using
history.replaceState() and the like (which is used on the SRP).

BUG=none
TEST=covered by test

Change-Id: Ida3aabeefb1cef033b68b667049fa724efe7b4f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2335024Reviewed-by: default avatarPeter Kotwicz <pkotwicz@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794559}
parent 22e26ade
...@@ -190,6 +190,12 @@ favicon_base::IconTypeSet FaviconHandler::GetIconTypesFromHandlerType( ...@@ -190,6 +190,12 @@ favicon_base::IconTypeSet FaviconHandler::GetIconTypesFromHandlerType(
} }
void FaviconHandler::FetchFavicon(const GURL& page_url, bool is_same_document) { void FaviconHandler::FetchFavicon(const GURL& page_url, bool is_same_document) {
// Some same document navigations (such as those done by
// history.replaceState) do not change the url. No need to start over in this
// case.
if (is_same_document && page_url == last_page_url_)
return;
cancelable_task_tracker_for_page_url_.TryCancelAll(); cancelable_task_tracker_for_page_url_.TryCancelAll();
cancelable_task_tracker_for_candidates_.TryCancelAll(); cancelable_task_tracker_for_candidates_.TryCancelAll();
......
...@@ -579,6 +579,18 @@ TEST_F(FaviconHandlerTest, UpdateFaviconMappingsAndFetch) { ...@@ -579,6 +579,18 @@ TEST_F(FaviconHandlerTest, UpdateFaviconMappingsAndFetch) {
RunHandlerWithSimpleFaviconCandidates({kIconURL16x16}); RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
} }
// Verifies same document navigation to the last page does not trigger fetch.
TEST_F(FaviconHandlerTest, SameDocumentNavigationToLastUrlDoesNotFetchAgain) {
EXPECT_CALL(favicon_service_,
UpdateFaviconMappingsAndFetch(base::flat_set<GURL>{kPageURL},
kIconURL16x16, kFavicon,
/*desired_size_in_dip=*/16, _, _));
EXPECT_CALL(favicon_service_, GetFaviconForPageURL(_, _, _, _, _)).Times(1);
auto handler = RunHandlerWithSimpleFaviconCandidates({kIconURL16x16});
handler->FetchFavicon(kPageURL, true);
}
// Test that we don't try to delete favicon mappings when a page URL is not in // Test that we don't try to delete favicon mappings when a page URL is not in
// history even if the page lists no favicons. // history even if the page lists no favicons.
TEST_F(FaviconHandlerTest, DoNotDeleteFaviconMappingsIfNotInHistory) { TEST_F(FaviconHandlerTest, DoNotDeleteFaviconMappingsIfNotInHistory) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment