Cache task for MostVisitedSitesHost.

Bug: 1071503
Change-Id: Idf2067f16cbdb4970965ddea980b5679621613cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2149514Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Cathy Li <chili@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Commit-Queue: Hao Dong <spdonghao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#762043}

chrome/android/java/src/org/chromium/chrome/browser/suggestions/mostvisited/MostVisitedSitesFaviconHelper.java

@@ -59,18 +59,23 @@ public class MostVisitedSitesFaviconHelper {
      * Save the favicon to the disk.
      * @param topSitesInfo SiteSuggestions data updated.
      * @param urlsToUpdate The set of urls which need to fetch and save the favicon.
+     * @param callback The callback function after skipping the existing favicon or saving favicon.
      */
-    public void saveFaviconsToFile(List<SiteSuggestion> topSitesInfo, Set<String> urlsToUpdate) {
+    public void saveFaviconsToFile(
+            List<SiteSuggestion> topSitesInfo, Set<String> urlsToUpdate, Runnable callback) {
         for (SiteSuggestion siteData : topSitesInfo) {
             String url = siteData.url;
             if (!urlsToUpdate.contains(url)) {
+                if (callback != null) {
+                    callback.run();
+                }
                 continue;
             }
             LargeIconBridge.LargeIconCallback iconCallback =
                     (icon, fallbackColor, isFallbackColorDefault, iconType) -> {
                 saveFaviconToFile(String.valueOf(siteData.faviconId),
                         MostVisitedSitesMetadataUtils.getOrCreateTopSitesDirectory(), url,
-                        fallbackColor, icon);
+                        fallbackColor, icon, callback);
             };
             fetchIcon(siteData, iconCallback);
         }
@@ -120,32 +125,44 @@ public class MostVisitedSitesFaviconHelper {
      * @param url The url which the favicon corresponds to.
      * @param fallbackColor The color for generating a new icon when favicon is null from native.
      * @param icon The favicon fetched from native.
+     * @param callback The callback function after saving each favicon.
      */
-    private void saveFaviconToFile(
-            String fileName, File directory, String url, int fallbackColor, Bitmap icon) {
-        AsyncTask.THREAD_POOL_EXECUTOR.execute(() -> {
-            Bitmap newIcon = icon;
-            // If icon is null, we need to generate a favicon.
-            if (newIcon == null) {
-                Log.i(TAG, "Favicon is null for " + url + ". Generating an icon for it.");
-                mIconGenerator.setBackgroundColor(fallbackColor);
-                newIcon = mIconGenerator.generateIconForUrl(url);
+    private void saveFaviconToFile(String fileName, File directory, String url, int fallbackColor,
+            Bitmap icon, Runnable callback) {
+        new AsyncTask<Void>() {
+            @Override
+            protected Void doInBackground() {
+                Bitmap newIcon = icon;
+                // If icon is null, we need to generate a favicon.
+                if (newIcon == null) {
+                    Log.i(TAG, "Favicon is null for " + url + ". Generating an icon for it.");
+                    mIconGenerator.setBackgroundColor(fallbackColor);
+                    newIcon = mIconGenerator.generateIconForUrl(url);
+                }
+                // Save icon to file.
+                File metadataFile = new File(directory, fileName);
+                AtomicFile file = new AtomicFile(metadataFile);
+                FileOutputStream stream;
+                try {
+                    stream = file.startWrite();
+                    assert newIcon != null;
+                    newIcon.compress(Bitmap.CompressFormat.PNG, 100, stream);
+                    file.finishWrite(stream);
+                    Log.i(TAG,
+                            "Finished saving top sites favicons to file: "
+                                    + metadataFile.getAbsolutePath());
+                } catch (IOException e) {
+                    Log.e(TAG, "Fail to write file: " + metadataFile.getAbsolutePath());
+                }
+                return null;
             }
-            // Save icon to file.
-            File metadataFile = new File(directory, fileName);
-            AtomicFile file = new AtomicFile(metadataFile);
-            FileOutputStream stream;
-            try {
-                stream = file.startWrite();
-                assert newIcon != null;
-                newIcon.compress(Bitmap.CompressFormat.PNG, 100, stream);
-                file.finishWrite(stream);
-                Log.i(TAG,
-                        "Finished saving top sites favicons to file: "
-                                + metadataFile.getAbsolutePath());
-            } catch (IOException e) {
-                Log.e(TAG, "Fail to write file: " + metadataFile.getAbsolutePath());
+
+            @Override
+            protected void onPostExecute(Void aVoid) {
+                if (callback != null) {
+                    callback.run();
+                }
             }
-        });
+        }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/suggestions/mostvisited/MostVisitedSitesHost.java

@@ -9,6 +9,7 @@ import android.content.Context;
 import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
 import org.chromium.base.task.AsyncTask;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.browser.suggestions.SiteSuggestion;
@@ -50,6 +51,16 @@ public class MostVisitedSitesHost implements MostVisitedSites.Observer {
 
     private static boolean sSkipRestoreFromDiskForTests;
 
+    private Runnable mCurrentTask;
+    private Runnable mPendingTask;
+    // Whether restoreFromDisk() is finished.
+    private boolean mIsSynced;
+    // Records how many remaining files the current task needs to save.
+    private int mCurrentFilesNeedToSaveCount;
+    // Records how many remaining files the pending task needs to save. This value is used to
+    // replace mCurrentFilesNeedToSaveCount when updating pending to current task.
+    private int mPendingFilesNeedToSaveCount;
+
     public MostVisitedSitesHost(Context context, Profile profile) {
         LargeIconBridge largeIconBridge =
                 SuggestionsDependencyFactory.getInstance().createLargeIconBridge(profile);
@@ -61,6 +72,56 @@ public class MostVisitedSitesHost implements MostVisitedSites.Observer {
         }
     }
 
+    /**
+     * Once new siteSuggestions info is available, call this function to update map and set and save
+     * data to the disk. If syncing with disk hasn't finished or there is a current task running,
+     * make this new task the pending task. Otherwise, make this new task the current task and run
+     * it.
+     * @param siteSuggestions The new SiteSuggestions.
+     */
+    public void saveMostVisitedSitesInfo(List<SiteSuggestion> siteSuggestions) {
+        // Ensure that saving happens after map and set are updated. Use finishOneFileSaving() as
+        // callback to record when this current task has been finished.
+        Runnable newTask = () -> updateMapAndSetForNewSites(siteSuggestions, () -> {
+            MostVisitedSitesMetadataUtils.saveSuggestionListsToFile(
+                    siteSuggestions, this::finishOneFileSaving);
+            mFaviconSaver.saveFaviconsToFile(
+                    siteSuggestions, mUrlsToUpdateFavicon, this::finishOneFileSaving);
+        });
+
+        if (!mIsSynced || mCurrentTask != null) {
+            // Skip last mPendingTask which is not necessary to run.
+            mPendingTask = newTask;
+            mPendingFilesNeedToSaveCount = siteSuggestions.size() + 1;
+        } else {
+            // Assign newTask to mCurrentTask and run this task.
+            mCurrentTask = newTask;
+            mCurrentFilesNeedToSaveCount = siteSuggestions.size() + 1;
+            // Skip any pending task.
+            mPendingTask = null;
+            mPendingFilesNeedToSaveCount = 0;
+
+            Log.d(TAG, "Start a new task.");
+            mCurrentTask.run();
+        }
+    }
+
+    @Override
+    public void onSiteSuggestionsAvailable(List<SiteSuggestion> siteSuggestions) {
+        saveMostVisitedSitesInfo(siteSuggestions);
+    }
+
+    @Override
+    public void onIconMadeAvailable(String siteUrl) {}
+
+    /**
+     * Start the observer.
+     * @param maxResults The max number of sites to observe.
+     */
+    public void startObserving(int maxResults) {
+        mMostVisitedSites.setObserver(this, maxResults);
+    }
+
     /**
      * Restore disk info to the mUrlToIDMap.
      */
@@ -85,32 +146,12 @@ public class MostVisitedSitesHost implements MostVisitedSites.Observer {
                     mUrlToIdMap.put(site.url, site.faviconId);
                 }
                 buildIdToUrlMap();
+                mIsSynced = true;
+                updatePendingToCurrent();
             }
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
-    // TODO(spdonghao): Add a queue to cache the new SiteSuggestions task when there is a pending
-    // task that is updating the favicon files and metadata on disk. Reschedule the cached task when
-    // the pending task is completed.
-    @Override
-    public void onSiteSuggestionsAvailable(List<SiteSuggestion> siteSuggestions) {
-        updateMapAndSetForNewSites(siteSuggestions, () -> {
-            MostVisitedSitesMetadataUtils.saveSuggestionListsToFile(siteSuggestions, null);
-            mFaviconSaver.saveFaviconsToFile(siteSuggestions, mUrlsToUpdateFavicon);
-        });
-    }
-
-    @Override
-    public void onIconMadeAvailable(String siteUrl) {}
-
-    /**
-     * Start the observer.
-     * @param maxResults The max number of sites to observe.
-     */
-    public void startObserving(int maxResults) {
-        mMostVisitedSites.setObserver(this, maxResults);
-    }
-
     /**
      * Update mUrlToIDMap and mUrlsToUpdateFavicon based on the new SiteSuggestions passed in.
      * @param newSuggestions The new SiteSuggestions.
@@ -280,6 +321,27 @@ public class MostVisitedSitesHost implements MostVisitedSites.Observer {
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
+    @VisibleForTesting
+    protected void updatePendingToCurrent() {
+        mCurrentTask = mPendingTask;
+        mCurrentFilesNeedToSaveCount = mPendingFilesNeedToSaveCount;
+        mPendingTask = null;
+        if (mCurrentTask != null) {
+            Log.d(TAG, "Start a new task.");
+            mCurrentTask.run();
+        }
+    }
+
+    private void finishOneFileSaving() {
+        ThreadUtils.assertOnUiThread();
+        mCurrentFilesNeedToSaveCount--;
+
+        // If there is no file needed to save for current task, update pending task to current.
+        if (mCurrentFilesNeedToSaveCount == 0) {
+            updatePendingToCurrent();
+        }
+    }
+
     @VisibleForTesting
     protected Map<String, Integer> getUrlToIDMapForTesting() {
         return mUrlToIdMap;
@@ -299,4 +361,29 @@ public class MostVisitedSitesHost implements MostVisitedSites.Observer {
     protected static void setSkipRestoreFromDiskForTesting() {
         sSkipRestoreFromDiskForTests = true;
     }
+
+    @VisibleForTesting
+    public void setIsSyncedForTesting(boolean isSynced) {
+        mIsSynced = isSynced;
+    }
+
+    @VisibleForTesting
+    public int getCurrentFilesNeedToSaveCountForTesting() {
+        return mCurrentFilesNeedToSaveCount;
+    }
+
+    @VisibleForTesting
+    public int getPendingFilesNeedToSaveCountForTesting() {
+        return mPendingFilesNeedToSaveCount;
+    }
+
+    @VisibleForTesting
+    public void setCurrentTaskForTesting(Runnable currentTask) {
+        mCurrentTask = currentTask;
+    }
+
+    @VisibleForTesting
+    public void setPendingTaskForTesting(Runnable pendingTask) {
+        mPendingTask = pendingTask;
+    }
 }

chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/mostvisited/MostVisitedSitesFaviconHelperTest.java

@@ -80,7 +80,7 @@ public class MostVisitedSitesFaviconHelperTest {
         TestThreadUtils.runOnUiThreadBlocking(
                 ()
                         -> mMostVisitedSitesFaviconHelper.saveFaviconsToFile(
-                                mExpectedSiteSuggestions, urlsToUpdate));
+                                mExpectedSiteSuggestions, urlsToUpdate, null));
 
         // Wait util the file number equals to expected one.
         CriteriaHelper.pollInstrumentationThread(Criteria.equals(

chrome/android/javatests/src/org/chromium/chrome/browser/suggestions/mostvisited/MostVisitedSitesHostTest.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.suggestions.mostvisited;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 
 import android.graphics.Bitmap;
 import android.support.test.filters.SmallTest;
@@ -149,6 +150,71 @@ public class MostVisitedSitesHostTest {
                 (int) mMostVisitedSitesHost.getUrlToIDMapForTesting().get("https://www.2.com"));
     }
 
+    /**
+     * Test when synchronization of metadata stored on disk hasn't finished yet, all coming tasks
+     * will be set as the pending task. Besides, the latest task will override the old one.
+     */
+    @Test
+    @SmallTest
+    public void testSyncNotFinished() {
+        List<SiteSuggestion> newTopSites1 = createFakeSiteSuggestions1();
+        List<SiteSuggestion> newTopSites2 = createFakeSiteSuggestions2();
+
+        mMostVisitedSitesHost.setIsSyncedForTesting(false);
+
+        // If restoring from disk is not finished, all coming tasks should be set as the pending
+        // task.
+        mMostVisitedSitesHost.saveMostVisitedSitesInfo(newTopSites1);
+        mMostVisitedSitesHost.saveMostVisitedSitesInfo(newTopSites2);
+
+        // newTopSites1 should be skipped and newTopSites2 should be the pending task.
+        assertEquals(newTopSites2.size(),
+                mMostVisitedSitesHost.getPendingFilesNeedToSaveCountForTesting() - 1);
+    }
+
+    /**
+     * Test when current task is not finished, all coming tasks will be set as the pending task.
+     * Besides, the latest task will override the old one.
+     */
+    @Test
+    @SmallTest
+    public void testCurrentNotNull() {
+        List<SiteSuggestion> newTopSites1 = createFakeSiteSuggestions1();
+        List<SiteSuggestion> newTopSites2 = createFakeSiteSuggestions2();
+
+        mMostVisitedSitesHost.setIsSyncedForTesting(true);
+        mMostVisitedSitesHost.setCurrentTaskForTesting(() -> {});
+
+        // If current task is not null, all saving tasks should be set as pending task.
+        mMostVisitedSitesHost.saveMostVisitedSitesInfo(newTopSites1);
+        mMostVisitedSitesHost.saveMostVisitedSitesInfo(newTopSites2);
+
+        // newTopSites1 should be skipped and newTopSites2 should be the pending task.
+        assertEquals(newTopSites2.size(),
+                mMostVisitedSitesHost.getPendingFilesNeedToSaveCountForTesting() - 1);
+    }
+
+    /**
+     * Test when current task is finished, the pending task should be set as current task and run.
+     */
+    @Test
+    @SmallTest
+    public void testTasksContinuity() {
+        AtomicBoolean isPendingRun = new AtomicBoolean(false);
+
+        // Set and run current task.
+        mMostVisitedSitesHost.setIsSyncedForTesting(true);
+        mMostVisitedSitesHost.setCurrentTaskForTesting(null);
+        mMostVisitedSitesHost.saveMostVisitedSitesInfo(createFakeSiteSuggestions1());
+
+        // When current task is not finished, set pending task.
+        assertTrue(mMostVisitedSitesHost.getCurrentFilesNeedToSaveCountForTesting() > 0);
+        mMostVisitedSitesHost.setPendingTaskForTesting(() -> isPendingRun.set(true));
+
+        // isPendingRun should eventually become true.
+        CriteriaHelper.pollInstrumentationThread(isPendingRun::get);
+    }
+
     private void checkMapAndSet(Set<String> expectedUrlsToFetchIcon, Set<String> expectedUrlsInMap,
             Set<Integer> expectedIdsInMap, List<SiteSuggestion> newSiteSuggestions) {
         // Check whether mExpectedSiteSuggestions' faviconIDs have been updated.