[Offline Pages] Duplicate pages when save/delete bookmarks.

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java

Index: chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
index 9e195bcd6030e0ed6282d4740642200a147ccbf3..3affcbad045008cf0134d5ef3f04c54c5405f972 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
@@ -10,6 +10,7 @@ import android.content.IntentFilter;
 import android.os.BatteryManager;
 import android.os.Environment;
 
+import org.chromium.base.Callback;
 import org.chromium.base.Log;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.base.metrics.RecordUserAction;
@@ -21,12 +22,15 @@ import org.chromium.chrome.browser.snackbar.SnackbarManager.SnackbarController;
 import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.components.bookmarks.BookmarkId;
+import org.chromium.components.offlinepages.DeletePageResult;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.net.ConnectionType;
 import org.chromium.net.NetworkChangeNotifier;
 import org.chromium.ui.base.PageTransition;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.TimeUnit;
 
 /**
@@ -79,18 +83,44 @@ public class OfflinePageUtils {
         // Making sure tab is worth keeping.
         if (shouldSkipSavingTabOffline(tab)) return;
 
-        OfflinePageBridge offlinePageBridge = OfflinePageBridge.getForProfile(tab.getProfile());
+        final OfflinePageBridge offlinePageBridge =
+                OfflinePageBridge.getForProfile(tab.getProfile());
         if (offlinePageBridge == null) return;
 
-        WebContents webContents = tab.getWebContents();
-        ClientId clientId = ClientId.createClientIdForBookmarkId(bookmarkId);
+        final WebContents webContents = tab.getWebContents();
+        final ClientId clientId = ClientId.createClientIdForBookmarkId(bookmarkId);
+        final String url = webContents.getUrl();
 
-        // TODO(fgorski): Ensure that request is queued if the model is not loaded.
-        offlinePageBridge.savePage(webContents, clientId, new OfflinePageBridge.SavePageCallback() {
+        offlinePageBridge.getPagesByOnlineUrl(url, new Callback<List<OfflinePageItem>>() {

[fgorski, 2016/06/15 20:19:01]

this solution does not cover the case where you fail to save the offline page
when hitting bookmark... you will end up with no saved pages in that case.

How about solving that at the model level, by deleting cached pages once a
successful "user initiated" save happens? (this call should be made based on
which page is the newest and whether deleted page will be outlived by the one
that will be shown or not)

[romax, 2016/06/16 01:24:26]

I think we should have some logic here but I'm not sure 'newest' would be the
best one. Since the user may visit a page from last-n and bookmark the same
page, there'll be 2 copies of the same page. However, if we only keep one page
per online url, it seems against the assumption we have already
(getPagesByOnlineURL is returning multiple pages). Also, say a user opens a url,
bookmark it, and continue browsing in the same tab, eventually the archive would
be deleted by the purging logic in recent_tab_helper, so the user would not be
able to access the bookmarked page. I think we should have a talk, but please
briefly let me know what you think.

             @Override
-            public void onSavePageDone(int savePageResult, String url, long offlineId) {
-                // TODO(fgorski): Decide if we need to do anything with result.
-                // Perhaps some UMA reporting, but that can really happen someplace else.
+            public void onResult(List<OfflinePageItem> items) {
+                List<ClientId> pages_to_delete = new ArrayList<ClientId>();
+                for (OfflinePageItem item : items) {
+                    if (item.getUrl().equals(url)
+                            && item.getClientId().getNamespace().equals(clientId.getNamespace())) {
+                        pages_to_delete.add(item.getClientId());
+                    }
+                }
+                offlinePageBridge.deletePagesByClientId(pages_to_delete, new Callback<Integer>() {
+                    @Override
+                    public void onResult(Integer deletePageResult) {
+                        if (deletePageResult != DeletePageResult.SUCCESS
+                                && deletePageResult != DeletePageResult.NOT_FOUND) {
+                            return;
+                        }
+                        offlinePageBridge.savePage(
+                                webContents, clientId, new OfflinePageBridge.SavePageCallback() {
+                                    @Override
+                                    public void onSavePageDone(
+                                            int savePageResult, String url, long offlineId) {
+                                        // TODO(fgorski): Decide if we need to do anything with
+                                        // result.
+                                        // Perhaps some UMA reporting, but that can really happen
+                                        // someplace else.
+                                    }
+                                });
+                    }
+                });
             }
         });
     }