Avoid blocking partner bookmarks reading on favicon fetching.

Currently, partner bookmarks aren't submitted until all the favicons are
fetched which can potentially be quite slow on a bad connection. This
change submits the bookmarks after they're read, regardless of whether
the favicons have been fetched yet.

When all the asynchronous favicon fetching has completed, if any
favicons were retrieved from a server, we refresh the bookmarks to show
the new favicons.

Also adds a new enum value for FaviconFetchResult to capture the case
of retrieving a duplicate favicon that wasn't written to the favicon
image cache as a result of being duplicate. These were being treated as
the default FAILURE_SERVER_ERROR before, and blacklisting URLs
unnecessarily even when the favicon existed in the cache already.

Bug: 780042
Change-Id: I2409acc3c7c558106aae417d7e709fb0e7539f3f
Reviewed-on: https://chromium-review.googlesource.com/804655
Commit-Queue: Troy Hildebrandt <thildebr@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524508}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java

@@ -60,6 +60,7 @@ class BookmarkItemsAdapter
     private Context mContext;
     private BookmarkPromoHeader mPromoHeaderManager;
     private String mSearchText;
+    private BookmarkId mCurrentFolder;
 
     private BookmarkModelObserver mBookmarkModelObserver = new BookmarkModelObserver() {
         @Override
@@ -310,6 +311,7 @@ class BookmarkItemsAdapter
         assert mDelegate != null;
 
         mSearchText = EMPTY_QUERY;
+        mCurrentFolder = folder;
 
         if (folder.equals(mDelegate.getModel().getRootFolderId())) {
             setBookmarks(mTopLevelFolders, new ArrayList<BookmarkId>());
@@ -327,6 +329,14 @@ class BookmarkItemsAdapter
     @Override
     public void onSelectionStateChange(List<BookmarkId> selectedBookmarks) {}
 
+    /**
+     * Refresh the list of bookmarks within the currently visible folder.
+     */
+    public void refresh() {
+        if (mCurrentFolder == null) return;
+        onFolderStateSet(mCurrentFolder);
+    }
+
     /**
      * Synchronously searches for the given query.
      * @param query The query text to search for.

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkManager.java

@@ -23,6 +23,7 @@ import org.chromium.chrome.browser.BasicNativePage;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkModelObserver;
 import org.chromium.chrome.browser.favicon.LargeIconBridge;
+import org.chromium.chrome.browser.partnerbookmarks.PartnerBookmarksReader;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 import org.chromium.chrome.browser.util.FeatureUtilities;
@@ -41,7 +42,8 @@ import java.util.Stack;
  * {@link BookmarkActivity} (phone) and {@link BookmarkPage} (tablet).
  */
 public class BookmarkManager implements BookmarkDelegate, SearchDelegate,
-                                        SelectableBottomSheetContentManager<BookmarkId> {
+                                        SelectableBottomSheetContentManager<BookmarkId>,
+                                        PartnerBookmarksReader.FaviconUpdateObserver {
     private static final int FAVICON_MAX_CACHE_SIZE_BYTES = 10 * 1024 * 1024; // 10MB
 
     /**
@@ -66,6 +68,7 @@ public class BookmarkManager implements BookmarkDelegate, SearchDelegate,
     private SelectionDelegate<BookmarkId> mSelectionDelegate;
     private final Stack<BookmarkUIState> mStateStack = new Stack<>();
     private LargeIconBridge mLargeIconBridge;
+    private boolean mFaviconsNeedRefresh;
     private String mInitialUrl;
     private boolean mIsDialogUi;
     private boolean mIsDestroyed;
@@ -117,6 +120,8 @@ public class BookmarkManager implements BookmarkDelegate, SearchDelegate,
         mActivity = activity;
         mIsDialogUi = isDialogUi;
 
+        PartnerBookmarksReader.addFaviconUpdateObserver(this);
+
         mSelectionDelegate = new SelectionDelegate<BookmarkId>() {
             @Override
             public boolean toggleSelectionForItem(BookmarkId bookmark) {
@@ -183,6 +188,20 @@ public class BookmarkManager implements BookmarkDelegate, SearchDelegate,
         ContextUtils.getAppSharedPreferences().edit().remove(PREF_SEARCH_HISTORY).apply();
     }
 
+    @Override
+    public void onUpdateFavicon(String url) {
+        mLargeIconBridge.clearFavicon(url);
+        mFaviconsNeedRefresh = true;
+    }
+
+    @Override
+    public void onCompletedFaviconLoading() {
+        if (mFaviconsNeedRefresh) {
+            mAdapter.refresh();
+            mFaviconsNeedRefresh = false;
+        }
+    }
+
     /**
      * Destroys and cleans up itself. This must be called after done using this class.
      */
@@ -206,6 +225,7 @@ public class BookmarkManager implements BookmarkDelegate, SearchDelegate,
         mBookmarkModel = null;
         mLargeIconBridge.destroy();
         mLargeIconBridge = null;
+        PartnerBookmarksReader.removeFaviconUpdateObserver(this);
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java

@@ -150,6 +150,13 @@ public class LargeIconBridge {
         }
     }
 
+    /**
+     * Removes the favicon from the local cache for the given URL.
+     */
+    public void clearFavicon(String url) {
+        mFaviconCache.remove(url);
+    }
+
     private static native long nativeInit();
     private static native void nativeDestroy(long nativeLargeIconBridge);
     private static native boolean nativeGetLargeIconForURL(long nativeLargeIconBridge,

chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksFaviconThrottle.java

@@ -86,7 +86,7 @@ public class PartnerBookmarksFaviconThrottle {
 
         if (result == FaviconFetchResult.FAILURE_SERVER_ERROR) {
             mNewEntries.put(url, System.currentTimeMillis() + FAVICON_RETRIEVAL_TIMEOUT_MS);
-        } else if (result != FaviconFetchResult.SUCCESS && !shouldFetchFromServerIfNecessary(url)
+        } else if (!isSuccessfulFetchResult(result) && !shouldFetchFromServerIfNecessary(url)
                 && (System.currentTimeMillis() < mCurrentEntries.get(url))) {
             // Keep storing an entry if it hasn't yet expired and we get didn't just get a success
             // response.
@@ -94,6 +94,11 @@ public class PartnerBookmarksFaviconThrottle {
         }
     }
 
+    private boolean isSuccessfulFetchResult(@FaviconFetchResult int result) {
+        return result == FaviconFetchResult.SUCCESS_FROM_CACHE
+                || result == FaviconFetchResult.SUCCESS_FROM_SERVER;
+    }
+
     /**
      * Determines, based on the contents of our entry set, whether or not we should even attempt to
      * reach out to a server to retrieve a favicon that isn't currently in our favicon image cache.

chrome/android/java/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksReader.java

@@ -16,6 +16,7 @@ import org.chromium.chrome.browser.AppHooks;
 
 import java.util.HashSet;
 import java.util.LinkedHashMap;
+import java.util.Set;
 
 import javax.annotation.concurrent.GuardedBy;
 
@@ -24,6 +25,7 @@ import javax.annotation.concurrent.GuardedBy;
 */
 public class PartnerBookmarksReader {
     private static final String TAG = "PartnerBMReader";
+    private static Set<FaviconUpdateObserver> sFaviconUpdateObservers = new HashSet<>();
 
     static final String LAST_EMPTY_READ_PREFS_NAME = "PartnerBookmarksReader.last_empty_read";
 
@@ -50,11 +52,30 @@ public class PartnerBookmarksReader {
     // don't end up shutting the bookmark reader down prematurely.
     private final Object mProgressLock = new Object();
     @GuardedBy("mProgressLock")
-    private int mNumFaviconsInProgress = 0;
+    private int mNumFaviconsInProgress;
     @GuardedBy("mProgressLock")
-    private boolean mShutDown = false;
+    private boolean mShutDown;
+    @GuardedBy("mProgressLock")
+    private boolean mFaviconsFetchedFromServer;
     private boolean mFinishedReading;
 
+    /**
+     * Observer for listeners to receive updates when changes are made to the favicon cache.
+     */
+    public interface FaviconUpdateObserver {
+        /**
+         * Called when a favicon has been updated, so observers can update their view if necessary.
+         *
+         * @param url The URL of the page for the favicon being updated.
+         */
+        void onUpdateFavicon(String url);
+
+        /**
+         * Called when all favicon loading for the partner bookmarks has completed.
+         */
+        void onCompletedFaviconLoading();
+    }
+
     /**
      * A callback used to indicate success or failure of favicon fetching when retrieving favicons
      * from cache or server.
@@ -77,6 +98,26 @@ public class PartnerBookmarksReader {
         initializeAndDisableEditingIfNecessary();
     }
 
+    /**
+     * Adds an observer for favicon updates as a result of fetching favicons from server during
+     * partner bookmark loading.
+     *
+     * @param observer The observer to add to the static list of observers.
+     */
+    public static void addFaviconUpdateObserver(FaviconUpdateObserver observer) {
+        sFaviconUpdateObservers.add(observer);
+    }
+
+    /**
+     * Removes an observer for favicon updates as a result of fetching favicons from server during
+     * partner bookmark loading.
+     *
+     * @param observer The observer to remove from the static list of observers.
+     */
+    public static void removeFaviconUpdateObserver(FaviconUpdateObserver observer) {
+        sFaviconUpdateObservers.remove(observer);
+    }
+
     /**
      * Asynchronously read bookmarks from the partner content provider
      */
@@ -107,6 +148,14 @@ public class PartnerBookmarksReader {
                         "PartnerBookmark.FaviconThrottleFetchResult", result,
                         FaviconFetchResult.UMA_BOUNDARY);
                 synchronized (mProgressLock) {
+                    if (result == FaviconFetchResult.SUCCESS_FROM_SERVER) {
+                        // If we've fetched a new favicon from a server, store a flag to indicate
+                        // this so we can refresh bookmarks when all favicons are fetched.
+                        mFaviconsFetchedFromServer = true;
+                        for (FaviconUpdateObserver observer : sFaviconUpdateObservers) {
+                            observer.onUpdateFavicon(nativeGetNativeUrlString(url));
+                        }
+                    }
                     mFaviconThrottle.onFaviconFetched(url, result);
                     --mNumFaviconsInProgress;
                     if (mNumFaviconsInProgress == 0 && mFinishedReading) {
@@ -132,6 +181,7 @@ public class PartnerBookmarksReader {
      * down the bookmark reader.
      */
     protected void onBookmarksRead() {
+        nativePartnerBookmarksCreationComplete(mNativePartnerBookmarksReader);
         mFinishedReading = true;
         synchronized (mProgressLock) {
             if (mNumFaviconsInProgress == 0) {
@@ -140,17 +190,26 @@ public class PartnerBookmarksReader {
         }
     }
 
-    /** Notifies the reader is complete and partner bookmarks should be submitted to the shim. */
+    /**
+     * Notifies the reader is complete, refreshes the partner bookmarks if necessary, and kills the
+     * native object
+     */
     protected void shutDown() {
         synchronized (mProgressLock) {
             if (mShutDown) return;
 
-            nativePartnerBookmarksCreationComplete(mNativePartnerBookmarksReader);
-            nativeDestroy(mNativePartnerBookmarksReader);
-            mNativePartnerBookmarksReader = 0;
             if (mFaviconThrottle != null) {
                 mFaviconThrottle.commit();
             }
+            // Make sure we refresh the bookmarks if we were fetching favicons from server, now that
+            // we have them all.
+            if (mFaviconsFetchedFromServer) {
+                for (FaviconUpdateObserver observer : sFaviconUpdateObservers) {
+                    observer.onCompletedFaviconLoading();
+                }
+            }
+            nativeDestroy(mNativePartnerBookmarksReader);
+            mNativePartnerBookmarksReader = 0;
             mShutDown = true;
         }
     }
@@ -340,5 +399,6 @@ public class PartnerBookmarksReader {
             String title, boolean isFolder, long parentId, byte[] favicon, byte[] touchicon,
             boolean fetchUncachedFaviconsFromServer, FetchFaviconCallback callback);
     private native void nativePartnerBookmarksCreationComplete(long nativePartnerBookmarksReader);
+    private static native String nativeGetNativeUrlString(String url);
     private static native void nativeDisablePartnerBookmarksEditing();
 }

chrome/android/junit/src/org/chromium/chrome/browser/partnerbookmarks/PartnerBookmarksFaviconThrottleTest.java

@@ -81,7 +81,7 @@ public class PartnerBookmarksFaviconThrottleTest {
 
         mFaviconThrottle.init();
         Assert.assertFalse(mFaviconThrottle.shouldFetchFromServerIfNecessary("url1"));
-        mFaviconThrottle.onFaviconFetched("url1", FaviconFetchResult.SUCCESS);
+        mFaviconThrottle.onFaviconFetched("url1", FaviconFetchResult.SUCCESS_FROM_CACHE);
         mFaviconThrottle.commit();
 
         mFaviconThrottle.init();

chrome/browser/android/bookmarks/partner_bookmarks_reader.cc

@@ -28,7 +28,9 @@
 
 using base::android::AttachCurrentThread;
 using base::android::CheckException;
+using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
+using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
 using base::android::JavaRef;
 using base::android::ScopedJavaGlobalRef;
@@ -68,13 +70,11 @@ void SetFaviconCallback(Profile* profile,
     bookmark_added_event->Signal();
 }
 
-void JNI_PartnerBookmarksReader_PrepareAndSetFavicon(
-    JNIEnv* env,
-    jbyte* icon_bytes,
-    int icon_len,
-    BookmarkNode* node,
-    Profile* profile,
-    favicon_base::IconType icon_type) {
+void PrepareAndSetFavicon(jbyte* icon_bytes,
+                          int icon_len,
+                          BookmarkNode* node,
+                          Profile* profile,
+                          favicon_base::IconType icon_type) {
   SkBitmap icon_bitmap;
   if (!gfx::PNGCodec::Decode(
       reinterpret_cast<const unsigned char*>(icon_bytes),
@@ -177,8 +177,8 @@ jlong PartnerBookmarksReader::AddPartnerBookmark(
         const int icon_len = env->GetArrayLength(icon);
         jbyte* icon_bytes = env->GetByteArrayElements(icon, nullptr);
         if (icon_bytes)
-          JNI_PartnerBookmarksReader_PrepareAndSetFavicon(
-              env, icon_bytes, icon_len, node.get(), profile_, icon_type);
+          PrepareAndSetFavicon(icon_bytes, icon_len, node.get(), profile_,
+                               icon_type);
         env->ReleaseByteArrayElements(icon, icon_bytes, JNI_ABORT);
       } else {
         // We should attempt to read the favicon from cache or retrieve it from
@@ -234,7 +234,7 @@ void PartnerBookmarksReader::GetFaviconImpl(const GURL& page_url,
   }
 
   GetFaviconFromCacheOrServer(page_url, fallback_to_server,
-                              std::move(callback));
+                              false /* from_server */, std::move(callback));
 }
 
 favicon::LargeIconService* PartnerBookmarksReader::GetLargeIconService() {
@@ -248,12 +248,14 @@ favicon::LargeIconService* PartnerBookmarksReader::GetLargeIconService() {
 void PartnerBookmarksReader::GetFaviconFromCacheOrServer(
     const GURL& page_url,
     bool fallback_to_server,
+    bool from_server,
     FaviconFetchedCallback callback) {
   GetLargeIconService()->GetLargeIconOrFallbackStyle(
       page_url, kPartnerBookmarksMinimumFaviconSizePx, 0,
       base::Bind(&PartnerBookmarksReader::OnGetFaviconFromCacheFinished,
                  base::Unretained(this), page_url,
-                 base::Passed(std::move(callback)), fallback_to_server),
+                 base::Passed(std::move(callback)), fallback_to_server,
+                 from_server),
       &favicon_task_tracker_);
 }
 
@@ -261,10 +263,17 @@ void PartnerBookmarksReader::OnGetFaviconFromCacheFinished(
     const GURL& page_url,
     FaviconFetchedCallback callback,
     bool fallback_to_server,
+    bool from_server,
     const favicon_base::LargeIconResult& result) {
-  // We got an image from the cache.
+  // |from_server| tells us if we fetched the image from the cache after we went
+  // to server for it, so this successful cache retrieval should actually return
+  // SUCCESS_FROM_SERVER.
   if (result.bitmap.is_valid()) {
-    std::move(callback).Run(FaviconFetchResult::SUCCESS);
+    if (from_server) {
+      std::move(callback).Run(FaviconFetchResult::SUCCESS_FROM_SERVER);
+    } else {
+      std::move(callback).Run(FaviconFetchResult::SUCCESS_FROM_CACHE);
+    }
     return;
   }
 
@@ -309,18 +318,24 @@ void PartnerBookmarksReader::OnGetFaviconFromServerFinished(
     FaviconFetchedCallback callback,
     favicon_base::GoogleFaviconServerRequestStatus status) {
   if (status != favicon_base::GoogleFaviconServerRequestStatus::SUCCESS) {
-    std::move(callback).Run(
-        status == favicon_base::GoogleFaviconServerRequestStatus::
-                      FAILURE_CONNECTION_ERROR
-            ? FaviconFetchResult::FAILURE_CONNECTION_ERROR
-            : FaviconFetchResult::FAILURE_SERVER_ERROR);
+    FaviconFetchResult result;
+    if (status == favicon_base::GoogleFaviconServerRequestStatus::
+                      FAILURE_CONNECTION_ERROR) {
+      result = FaviconFetchResult::FAILURE_CONNECTION_ERROR;
+    } else if (status == favicon_base::GoogleFaviconServerRequestStatus::
+                             FAILURE_ON_WRITE) {
+      result = FaviconFetchResult::FAILURE_WRITING_FAVICON_CACHE;
+    } else {
+      result = FaviconFetchResult::FAILURE_SERVER_ERROR;
+    }
+    std::move(callback).Run(result);
     return;
   }
 
   // The icon was successfully retrieved from the server, now we just have to
   // retrieve it from the cache where it was stored.
   GetFaviconFromCacheOrServer(page_url, false /* fallback_to_server */,
-                              std::move(callback));
+                              true /* from_server */, std::move(callback));
 }
 
 void PartnerBookmarksReader::OnFaviconFetched(
@@ -331,15 +346,14 @@ void PartnerBookmarksReader::OnFaviconFetched(
                                              static_cast<int>(result));
 }
 
-// static
+// ----------------------------------------------------------------
+
 static void JNI_PartnerBookmarksReader_DisablePartnerBookmarksEditing(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz) {
   PartnerBookmarksShim::DisablePartnerBookmarksEditing();
 }
 
-// ----------------------------------------------------------------
-
 static jlong JNI_PartnerBookmarksReader_Init(JNIEnv* env,
                                              const JavaParamRef<jobject>& obj) {
   Profile* profile = ProfileManager::GetActiveUserProfile();
@@ -349,3 +363,12 @@ static jlong JNI_PartnerBookmarksReader_Init(JNIEnv* env,
       partner_bookmarks_shim, profile);
   return reinterpret_cast<intptr_t>(reader);
 }
+
+static base::android::ScopedJavaLocalRef<jstring>
+JNI_PartnerBookmarksReader_GetNativeUrlString(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jstring>& j_url) {
+  GURL url(ConvertJavaStringToUTF8(j_url));
+  return ConvertUTF8ToJavaString(env, url.spec());
+}

chrome/browser/android/bookmarks/partner_bookmarks_reader.h

@@ -53,17 +53,26 @@ class PartnerBookmarksReader {
   // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.partnerbookmarks
   enum class FaviconFetchResult {
     // Successfully fetched a favicon from cache or server.
-    SUCCESS = 0,
+    // Deprecated, SUCCESS_FROM_CACHE and SUCCESS_FROM_SERVER should be used.
+    DEPRECATED_SUCCESS = 0,
     // Received a server error fetching the favicon.
-    FAILURE_SERVER_ERROR = 1,
+    FAILURE_SERVER_ERROR,
     // The icon service was unavailable.
-    FAILURE_ICON_SERVICE_UNAVAILABLE = 2,
+    FAILURE_ICON_SERVICE_UNAVAILABLE,
     // There was nothing in the cache, but we opted out of retrieving from
     // server.
-    FAILURE_NOT_IN_CACHE = 3,
+    FAILURE_NOT_IN_CACHE,
     // Request sent out and a connection error occurred (no valid HTTP response
     // received).
-    FAILURE_CONNECTION_ERROR = 4,
+    FAILURE_CONNECTION_ERROR,
+    // Success fetching the favicon from the cache without reaching out to the
+    // server.
+    SUCCESS_FROM_CACHE,
+    // Success fetching the favicon from server.
+    SUCCESS_FROM_SERVER,
+    // Failed to write the favicon to cache, likely from attempting to add a
+    // duplicate.
+    FAILURE_WRITING_FAVICON_CACHE,
     // Boundary value for UMA.
     UMA_BOUNDARY,
   };
@@ -81,11 +90,13 @@ class PartnerBookmarksReader {
                       FaviconFetchedCallback callback);
   void GetFaviconFromCacheOrServer(const GURL& page_url,
                                    bool fallback_to_server,
+                                   bool from_server,
                                    FaviconFetchedCallback callback);
   void OnGetFaviconFromCacheFinished(
       const GURL& page_url,
       FaviconFetchedCallback callback,
       bool fallback_to_server,
+      bool from_server,
       const favicon_base::LargeIconResult& result);
   void OnGetFaviconFromServerFinished(
       const GURL& page_url,

tools/metrics/histograms/enums.xml

@@ -32323,6 +32323,17 @@ Called by update_net_trust_anchors.py.-->
     Request sent out and a connection error occurred (no valid HTTP response
     received).
   </int>
+  <int value="5" label="SUCCESS_FROM_CACHE">
+    Success fetching the favicon from the cache without reaching out to the
+    server.
+  </int>
+  <int value="6" label="SUCCESS_FROM_SERVER">
+    Success fetching the favicon from server.
+  </int>
+  <int value="7" label="FAILURE_WRITING_FAVICON_CACHE">
+    Failed to write the favicon to cache, likely from attempting to add a
+    duplicate.
+  </int>
 </enum>
 
 <enum name="PassiveForcedListenerResultType">