Extend LargeIconService to fetch missing favicons from a Google server

components/history/core/browser/history_backend.cc

Index: components/history/core/browser/history_backend.cc
diff --git a/components/history/core/browser/history_backend.cc b/components/history/core/browser/history_backend.cc
index 62187120cff3ae3d97a93a60b1fdcd8759dff2e5..2db1a7696a81eb9e1cb4748047ca290b2188c854 100644
--- a/components/history/core/browser/history_backend.cc
+++ b/components/history/core/browser/history_backend.cc
@@ -1699,12 +1699,13 @@ void HistoryBackend::MergeFavicon(
   ScheduleCommit();
 }
 
-void HistoryBackend::SetFavicons(const GURL& page_url,
-                                 favicon_base::IconType icon_type,
-                                 const GURL& icon_url,
-                                 const std::vector<SkBitmap>& bitmaps) {
+favicon_base::FaviconID HistoryBackend::SetFavicons(
+    const GURL& page_url,
+    favicon_base::IconType icon_type,
+    const GURL& icon_url,
+    const std::vector<SkBitmap>& bitmaps) {
   if (!thumbnail_db_ || !db_)
-    return;
+    return favicon_base::FaviconID(0);
 
   DCHECK_GE(kMaxFaviconBitmapsPerIconURL, bitmaps.size());
 
@@ -1735,6 +1736,23 @@ void HistoryBackend::SetFavicons(const GURL& page_url,
     SendFaviconChangedNotificationForIconURL(icon_url);
   }
   ScheduleCommit();
+  return icon_id;
+}
+
+void HistoryBackend::SetExpiredFaviconsIfNoneKnown(

[pkotwicz, 2017/03/08 05:38:16]

You should add a test testing that this method does in fact not overwrite
existing data

[mastiz, 2017/03/08 16:37:33]

Will do, I'll add the tests once the rest looks OK, to avoid back and forth with
signature changes.

+    const GURL& page_url,
+    favicon_base::IconType icon_type,
+    const GURL& icon_url,
+    const std::vector<SkBitmap>& bitmaps) {
+  if (!thumbnail_db_ || !db_)
+    return;
+

[pkotwicz, 2017/03/03 22:33:25]

Given that |page_url| is currently a substring of |icon_url| I would get rid of
the |icon_url| parameter and use |page_url| as the |icon_url|.

If you keep the |icon_url| parameter you need to check that there is nothing in
the database for |icon_url| too

[pkotwicz, 2017/03/08 05:38:16]

Ping on this comment

[mastiz, 2017/03/08 16:37:33]

I think it's rather hacky to make such assumption at this layer, conditioned to
the fact that this function is only used by LargeIconService. There's also the
inconsistency of WasUnableToDownload, which we use the icon URL for.

Wrt to checking |icon_url| in the DB, I'm not convinced this is needed. Do you
suggest the function name might be misleading, and that future clients would
assume |icon_url| is also checked? We could also rename to ...IfNoMappingKnown.
WDYT?

[pkotwicz, 2017/03/09 02:25:57]

The sync code uses |page_url| if |icon_url| is missing as well.
(BookmarkChangeProcessor::SetBookmarkFavicon()). I think that the simpler API
makes using |page_url| as the |icon_url| worth it.

Although, I expect that you will only write one or two tests for this function,
a simpler API also means a smaller test surface.

Yes, I am concerned that future clients would assume that |icon_url| is also
checked.

[mastiz, 2017/03/15 16:58:10]

The server-side guys have implemented including the icon URL as part of the
response. How do you think we should proceed with this?

I suggest we do the parsing of this field in a follow-up CL (after the latest
version of the server has been rolled out) WDYT?

Even if you're fine with this, I assume we might need to find a better name for
this function?

[pkotwicz, 2017/03/16 00:50:45]

In this case, this function should check that there isn't any data for both the
page URL and the icon URL

The function name is reasonable. sky@ might have a suggestion for a better name.
He is the OWNER of history/

[mastiz, 2017/03/21 12:09:17]

This change has landed in the form of a dedicated patch:
https://codereview.chromium.org/2750313002

+  if (!thumbnail_db_->GetIconMappingsForPageURL(page_url,
+                                                /*mapping_data*/ nullptr)) {
+    favicon_base::FaviconID icon_id =
+        SetFavicons(page_url, icon_type, icon_url, bitmaps);
+    thumbnail_db_->SetFaviconOutOfDate(icon_id);

[pkotwicz, 2017/03/03 22:33:25]

Don't you need to run ScheduleCommit()?

The ScheduleCommit() call will be a no-op because SetFavicons() already calls
it. However, it is weird for us to count on the ScheduleCommit() called by
SetFavicons() to schedule the commit for
ThumbnailDatabase::SetFaviconOutOfDate() as well

[mastiz, 2017/03/08 16:37:33]

Done, added the redundant ScheduleCommit() for clarity (will be a no-op).

+  }
 }
 
 void HistoryBackend::SetFaviconsOutOfDateForPage(const GURL& page_url) {