Extend LargeIconService to fetch missing favicons from a Google server

components/history/core/browser/history_backend.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/history/core/browser/history_backend.h"
 
 #include <algorithm>
 #include <functional>
 #include <list>
 #include <map>
@@ skipping 1681 matching lines @@
   if (!favicon_created && (!bitmap_identical || favicon_bitmaps_copied)) {
     // If there was a favicon at |icon_url| prior to MergeFavicon() being
     // called, there may be page URLs which also use the favicon at |icon_url|.
     // Notify the UI that the favicon has changed for |icon_url|.
     SendFaviconChangedNotificationForIconURL(icon_url);
   }
 
   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());
 
   favicon_base::FaviconID icon_id =
       thumbnail_db_->GetFaviconIDForFaviconURL(icon_url, icon_type, nullptr);
 
   bool favicon_created = false;
   if (!icon_id) {
     icon_id = thumbnail_db_->AddFavicon(icon_url, icon_type);
     favicon_created = true;
@@ skipping 10 matching lines @@
     SendFaviconChangedNotificationForPageAndRedirects(page_url);
   }
 
   if (favicon_data_modified && !favicon_created) {
     // If there was a favicon at |icon_url| prior to SetFavicons() being called,
     // there may be page URLs which also use the favicon at |icon_url|. Notify
     // the UI that the favicon has changed for |icon_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) {
   std::vector<IconMapping> icon_mappings;
 
   if (!thumbnail_db_ ||
       !thumbnail_db_->GetIconMappingsForPageURL(page_url, &icon_mappings))
     return;
 
   for (std::vector<IconMapping>::iterator m = icon_mappings.begin();
@@ skipping 853 matching lines @@
   // transaction is currently open.
   db_->CommitTransaction();
   db_->Vacuum();
   db_->BeginTransaction();
   db_->GetStartDate(&first_recorded_time_);
 
   return true;
 }
 
 }  // namespace history