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

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

 // Copyright 2015 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.
 
 package org.chromium.chrome.browser.offlinepages;
 
 import android.content.Context;
 import android.content.Intent;
 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;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.snackbar.Snackbar;
 import org.chromium.chrome.browser.snackbar.SnackbarManager;
 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;
 
 /**
  * A class holding static util functions for offline pages.
  */
 public class OfflinePageUtils {
     private static final String TAG = "OfflinePageUtils";
     /** Background task tag to differentiate from other task types */
     public static final String TASK_TAG = "OfflinePageUtils";
 
@@ skipping 32 matching lines @@
     public static void saveBookmarkOffline(BookmarkId bookmarkId, Tab tab) {
         // If bookmark ID is missing there is nothing to save here.
         if (bookmarkId == null) return;
 
         // Making sure the feature is enabled.
         if (!OfflinePageBridge.isOfflineBookmarksEnabled()) return;
 
         // 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.
+                                    }
+                                });
+                    }
+                });
             }
         });
     }
 
     /**
      * Indicates whether we should skip saving the given tab as an offline page.
      * A tab shouldn't be saved offline if it shows an error page or a sad tab page.
      */
     private static boolean shouldSkipSavingTabOffline(Tab tab) {
         WebContents webContents = tab.getWebContents();
@@ skipping 145 matching lines @@
     private static int batteryPercentage(Intent batteryStatus) {
         int level = batteryStatus.getIntExtra(BatteryManager.EXTRA_LEVEL, -1);
         int scale = batteryStatus.getIntExtra(BatteryManager.EXTRA_SCALE, -1);
         if (scale == 0) return 0;
 
         int percentage = Math.round(100 * level / (float) scale);
         Log.d(TAG, "Battery Percentage is " + percentage);
         return percentage;
     }
 }