Change order of sync/local favicon queries in recent tabs menu

Prefactoring step for making FaviconSource and RecentTabsSubMenuModel
share the same code for favicon retrieval. We request first from local
storage and then from sync in case of failure. We no longer rely on the
fact that the tab is local or not. This order of operations is the one
currently used in FaviconSource.

Bug: 955475
Change-Id: I02ed5615ae462b916a09b3d0dd85dea39d88a680
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609849
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660077}

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

@@ -563,29 +563,9 @@ void RecentTabsSubMenuModel::AddDeviceFavicon(
 }
 
 void RecentTabsSubMenuModel::AddTabFavicon(int command_id, const GURL& url) {
-  bool is_local_tab = command_id < kFirstOtherDevicesTabCommandId;
   int index_in_menu = GetIndexOfCommandId(command_id);
 
-  if (!is_local_tab) {
-    // If tab has synced favicon, use it.
-    // Note that currently, other devices' tabs only have favicons if
-    // --sync-tab-favicons switch is on; according to zea@, this flag is now
-    // automatically enabled for iOS and android, and they're looking into
-    // enabling it for other platforms.
-    sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate();
-    scoped_refptr<base::RefCountedMemory> favicon_png;
-    if (open_tabs &&
-        open_tabs->GetSyncedFaviconForPageURL(url.spec(), &favicon_png)) {
-      favicon::RecordFaviconRequestMetric(
-          favicon::FaviconRequestOrigin::RECENTLY_CLOSED_TABS,
-          favicon::FaviconAvailability::kSync);
-      gfx::Image image = gfx::Image::CreateFrom1xPNGBytes(favicon_png);
-      SetIcon(index_in_menu, image);
-      return;
-    }
-  }
-
-  // Otherwise, start to fetch the favicon from local history asynchronously.
+  // Start to fetch the favicon from local history asynchronously.
   // Set default icon first.
   SetIcon(index_in_menu, favicon::GetDefaultFavicon());
   // Start request to fetch actual icon if possible.
@@ -595,34 +575,52 @@ void RecentTabsSubMenuModel::AddTabFavicon(int command_id, const GURL& url) {
   if (!favicon_service)
     return;
 
+  bool is_local_tab = command_id < kFirstOtherDevicesTabCommandId;
   favicon_service->GetFaviconImageForPageURL(
       url,
       base::Bind(&RecentTabsSubMenuModel::OnFaviconDataAvailable,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 command_id),
+                 weak_ptr_factory_.GetWeakPtr(), url, command_id),
       is_local_tab ? &local_tab_cancelable_task_tracker_
                    : &other_devices_tab_cancelable_task_tracker_);
 }
 
 void RecentTabsSubMenuModel::OnFaviconDataAvailable(
+    const GURL& page_url,
     int command_id,
     const favicon_base::FaviconImageResult& image_result) {
-  if (image_result.image.IsEmpty()) {
+  int index_in_menu = GetIndexOfCommandId(command_id);
+  if (!image_result.image.IsEmpty()) {
     favicon::RecordFaviconRequestMetric(
         favicon::FaviconRequestOrigin::RECENTLY_CLOSED_TABS,
-        favicon::FaviconAvailability::kNotAvailable);
+        favicon::FaviconAvailability::kLocal);
+    DCHECK_GT(index_in_menu, -1);
+    SetIcon(index_in_menu, image_result.image);
+    ui::MenuModelDelegate* delegate = menu_model_delegate();
+    if (delegate)
+      delegate->OnIconChanged(index_in_menu);
+    return;
+  }
+
+  // If tab has synced favicon, use it.
+  // Note that currently, other devices' tabs only have favicons if
+  // --sync-tab-favicons switch is on; according to zea@, this flag is now
+  // automatically enabled for iOS and android, and they're looking into
+  // enabling it for other platforms.
+  sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate();
+  scoped_refptr<base::RefCountedMemory> favicon_png;
+  if (open_tabs &&
+      open_tabs->GetSyncedFaviconForPageURL(page_url.spec(), &favicon_png)) {
+    favicon::RecordFaviconRequestMetric(
+        favicon::FaviconRequestOrigin::RECENTLY_CLOSED_TABS,
+        favicon::FaviconAvailability::kSync);
+    gfx::Image image = gfx::Image::CreateFrom1xPNGBytes(favicon_png);
+    SetIcon(index_in_menu, image);
     return;
   }
 
   favicon::RecordFaviconRequestMetric(
       favicon::FaviconRequestOrigin::RECENTLY_CLOSED_TABS,
-      favicon::FaviconAvailability::kLocal);
-  int index_in_menu = GetIndexOfCommandId(command_id);
-  DCHECK_GT(index_in_menu, -1);
-  SetIcon(index_in_menu, image_result.image);
-  ui::MenuModelDelegate* delegate = menu_model_delegate();
-  if (delegate)
-    delegate->OnIconChanged(index_in_menu);
+      favicon::FaviconAvailability::kNotAvailable);
 }
 
 int RecentTabsSubMenuModel::CommandIdToTabVectorIndex(

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

@@ -119,6 +119,7 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   // OnFaviconDataAvailable() will be invoked when the favicon is ready.
   void AddTabFavicon(int command_id, const GURL& url);
   void OnFaviconDataAvailable(
+      const GURL& page_url,
       int command_id,
       const favicon_base::FaviconImageResult& image_result);