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

Issue 817348 link

Starred by 1 user
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Mar 2018
Cc:
mastiz@chromium.org
ios-bugs@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug



Sign in to add a comment

sync_sessions::FaviconCache does not always update visit times

Project Member Reported by mastiz@chromium.org, Feb 28 2018 
sync_sessions::FaviconCache::OnReceivedSyncFavicon() takes a parameter named |icon_bytes| which is not actually set, except in tests. This cases the function to return early and *not* update the visit timestamps, which the caller RefreshFaviconVisitTimesFromForeignTab() is trying to do.

I don't fully understand the exact consequences of this, but it's possible that the issue contributes to some missing favicons in the UI.
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 1 2018 

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

commit 805ee322c2df23bef9108594cf71c8ace6afdac7
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Mar 01 22:23:25 2018

Fix FaviconCache not updating visits times from sync

The previous API, OnReceivedSyncFavicon(), received a parameter
|icon_bytes| that was actually only used in tests. This prevented the
visit time from being updated (in-memory).

While fixing this, we move some code (the loop that iterates all
navigations of a sync_pb::SessionTab) to favicon_cache.cc, to make
SessionsSyncManager smaller and couple FaviconCache's API more with
how it is actually used (single caller).

Bug:  817348 
Change-Id: I8fb7457177e298759f40e0bcf8d576ffc9fb7f9a
Reviewed-on: https://chromium-review.googlesource.com/940136
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Nicolas Zea (slow) <zea@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540304}
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/BUILD.gn
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/favicon_cache.cc
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/favicon_cache.h
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/favicon_cache_unittest.cc
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/805ee322c2df23bef9108594cf71c8ace6afdac7/components/sync_sessions/sessions_sync_manager.h

Comment 2 by mastiz@chromium.org, Mar 2 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment