[Offline pages] Removing offline pages from Bookmarks UI

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

Index: chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
index dc6ca0f1cdd3b76dfa4faf01ca6d4a249438b641..c77b85e203ddf6c7bc1c51df2a5f8d8702793675 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
@@ -4,26 +4,17 @@
 
 package org.chromium.chrome.browser.bookmarks;
 
-import org.chromium.base.Callback;
 import org.chromium.base.ObserverList;
 import org.chromium.base.VisibleForTesting;
-import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.browser.offlinepages.ClientId;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
-import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.OfflinePageModelObserver;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge.SavePageCallback;
-import org.chromium.chrome.browser.offlinepages.OfflinePageItem;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkType;
 import org.chromium.components.dom_distiller.core.DomDistillerUrlUtils;
-import org.chromium.components.offlinepages.SavePageResult;
 import org.chromium.content_public.browser.WebContents;
 
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.HashSet;
 import java.util.List;
 
 /**
@@ -39,7 +30,6 @@ public class BookmarkModel extends BookmarkBridge {
      */
     public interface AddBookmarkCallback {
         static final int SAVED = 0;
-        static final int SKIPPED = 1;
         static final int ERROR = 2;
 
         /**
@@ -65,25 +55,7 @@ public class BookmarkModel extends BookmarkBridge {
         void onDeleteBookmarks(String[] titles, boolean isUndoable);
     }
 
-    /** A comparator to sort the offline pages according to the most recent access time. */
-    private static final Comparator<OfflinePageItem> sOfflinePageComparator =
-            new Comparator<OfflinePageItem>() {
-                @Override
-                public int compare(OfflinePageItem o1, OfflinePageItem o2) {
-                    if (o1.getLastAccessTimeMs() > o2.getLastAccessTimeMs()) {
-                        return -1;
-                    } else if (o1.getLastAccessTimeMs() < o2.getLastAccessTimeMs()) {
-                        return 1;
-                    } else {
-                        return 0;
-                    }
-                }
-            };
-
     private ObserverList<BookmarkDeleteObserver> mDeleteObservers = new ObserverList<>();
-    private OfflinePageBridge mOfflinePageBridge;
-    private boolean mIsOfflinePageModelLoaded;
-    private OfflinePageModelObserver mOfflinePageModelObserver;
 
     /**
      * Initialize bookmark model for last used non-incognito profile.
@@ -95,27 +67,6 @@ public class BookmarkModel extends BookmarkBridge {
     @VisibleForTesting
     public BookmarkModel(Profile profile) {
         super(profile);
-
-        // Note: we check if mOfflinePageBridge is null after this to determine if offline pages
-        // feature is enabled. When it is enabled by default, we should check all the places
-        // that checks for nullability of mOfflinePageBridge.
-        if (OfflinePageBridge.isEnabled()) {
-            mOfflinePageBridge = OfflinePageBridge.getForProfile(profile);
-            if (mOfflinePageBridge.isOfflinePageModelLoaded()) {
-                mIsOfflinePageModelLoaded = true;
-            } else {
-                mOfflinePageModelObserver = new OfflinePageModelObserver() {
-                    @Override
-                    public void offlinePageModelLoaded() {
-                        mIsOfflinePageModelLoaded = true;
-                        if (isBookmarkModelLoaded()) {
-                            notifyBookmarkModelLoaded();
-                        }
-                    }
-                };
-                mOfflinePageBridge.addObserver(mOfflinePageModelObserver);
-            }
-        }
     }
 
     /**
@@ -123,18 +74,12 @@ public class BookmarkModel extends BookmarkBridge {
      */
     @Override
     public void destroy() {
-        if (mOfflinePageBridge != null) {
-            mOfflinePageBridge.removeObserver(mOfflinePageModelObserver);
-            mOfflinePageBridge = null;
-        }
-
         super.destroy();
     }
 
     @Override
     public boolean isBookmarkModelLoaded() {
-        return super.isBookmarkModelLoaded()
-                && (mOfflinePageBridge == null || mIsOfflinePageModelLoaded);
+        return super.isBookmarkModelLoaded();
     }
 
     /**
@@ -209,23 +154,15 @@ public class BookmarkModel extends BookmarkBridge {
      * @param callback The callback to be invoked when the bookmark is added.
      */
     public void addBookmarkAsync(BookmarkId parent, int index, String title, String url,

[Ian Wen, 2016/04/18 18:33:14]

This method is no longer async. I would rename it to
addBookmarkWithOfflinePage(). 

Also why do we still need AddBookmarkCallback?

[fgorski, 2016/04/18 23:39:05]

If you want me to change it to a synchronous version I can do that, but I
wouldn't mention the offline page in the method name as this is something that
may or may not happen and is really a side channel action for user convenience.

done.

-                                 WebContents webContents, final AddBookmarkCallback callback) {
+            WebContents webContents, AddBookmarkCallback callback) {
         url = DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl(url);
-        final BookmarkId bookmarkId = addBookmark(parent, index, title, url);
+        BookmarkId bookmarkId = addBookmark(parent, index, title, url);
 
-        // If bookmark was not created return an error.
-        if (bookmarkId == null) {
-            callback.onBookmarkAdded(null, AddBookmarkCallback.ERROR);
-            return;
-        }
-
-        // If there is no need to save offline page, return now.
-        if (mOfflinePageBridge == null || webContents == null) {
-            callback.onBookmarkAdded(bookmarkId, AddBookmarkCallback.SKIPPED);
-            return;
-        }
+        // Invoke the callback with the result of bookmark creation.
+        callback.onBookmarkAdded(bookmarkId,
+                bookmarkId != null ? AddBookmarkCallback.SAVED : AddBookmarkCallback.ERROR);
 
-        saveOfflinePage(bookmarkId, webContents, callback);
+        saveOfflinePage(bookmarkId, webContents);
     }
 
     /**
@@ -235,110 +172,42 @@ public class BookmarkModel extends BookmarkBridge {
     * @param webContents A {@link WebContents} object.
     * @param callback The callback to be invoked when the offline copy is saved.
     */
-    public void saveOfflinePage(final BookmarkId bookmarkId, WebContents webContents,
-            final AddBookmarkCallback callback) {
-        if (mOfflinePageBridge != null) {
-            RecordHistogram.recordBooleanHistogram("OfflinePages.IncognitoSave",
-                    webContents.isIncognito());
-            ClientId clientId = ClientId.createClientIdForBookmarkId(bookmarkId);
-            mOfflinePageBridge.savePage(webContents, clientId, new SavePageCallback() {
-                @Override
-                public void onSavePageDone(int savePageResult, String url, long offlineId) {
-                    int saveResult;
-                    if (savePageResult == SavePageResult.SUCCESS) {
-                        saveResult = AddBookmarkCallback.SAVED;
-                    } else if (savePageResult == SavePageResult.SKIPPED) {
-                        saveResult = AddBookmarkCallback.SKIPPED;
-                    } else {
-                        saveResult = AddBookmarkCallback.ERROR;
-                    }
-                    callback.onBookmarkAdded(bookmarkId, saveResult);
-                }
-            });
-        }
-    }
+    private void saveOfflinePage(final BookmarkId bookmarkId, WebContents webContents) {
+        // If bookmark ID is missing there is nothing to save here.
+        if (bookmarkId == null) return;
 
-    /**
-     * @see org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem#getTitle()
-     */
-    public String getBookmarkTitle(BookmarkId bookmarkId) {
-        return getBookmarkById(bookmarkId).getTitle();
-    }
+        // Making sure the feature is enabled.
+        if (!OfflinePageBridge.isEnabled()) return;
 
-    /**
-     * Retrieves the url to launch a bookmark or saved page. If latter, also marks it as being
-     * accessed and reports the UMAs.
-     *
-     * @param bookmarkId ID of the bookmark to launch.
-     * @return The launch URL.
-     */
-    public String getLaunchUrlAndMarkAccessed(BookmarkId bookmarkId) {
-        String url = getBookmarkById(bookmarkId).getUrl();
-        if (mOfflinePageBridge == null) return url;
+        // Nothing to save or save explicitly not performed.
+        if (webContents == null || webContents.isDestroyed() || webContents.isIncognito()) return;
 
-        return mOfflinePageBridge.getLaunchUrlFromOnlineUrl(url);
-    }
-
-    /**
-     * @return The id of the default folder to add bookmarks/folders to.
-     */
-    public BookmarkId getDefaultFolder() {
-        return getMobileFolderId();
-    }
+        OfflinePageBridge offlinePageBridge = OfflinePageBridge.getForProfile(getProfile());

[Ian Wen, 2016/04/18 18:33:13]

You might want to use Profile#getLastUsedProfile().getOriginal()?

On Android there are only two profiles. A normal profile and its incognito
profile. Bookmarks are always stored in the normal profile.

[fgorski, 2016/04/18 23:39:05]

I am hoping getProfile() will actually give me incognito here. Is that the case?

We deal with the problem you mentioned on the factory side. There is a chance we
will not want to save things from incognito profile, in which case I want to
know where the request came from. Does that make sense?

+        if (offlinePageBridge == null) return;
 
-    /**
-     * Gets a list of bookmark IDs for all offline pages.
-     *
-     * @return A list of bookmark IDs of bookmarks matching the offline pages filter.
-     */
-    public void getBookmarkIDsByFilter(
-            BookmarkFilter filter, final Callback<List<BookmarkId>> callback) {
-        assert filter == BookmarkFilter.OFFLINE_PAGES;
-        assert mOfflinePageBridge != null;
+        ClientId clientId = ClientId.createClientIdForBookmarkId(bookmarkId);
 
-        mOfflinePageBridge.getAllPages(new OfflinePageBridge.MultipleOfflinePageItemCallback() {
+        // TODO(fgorski): Ensure that request is queued if the model is not loaded.
+        offlinePageBridge.savePage(webContents, clientId, new SavePageCallback() {
             @Override
-            public void onResult(List<OfflinePageItem> offlinePages) {
-                callback.onResult(filterBookmarkIdsByOfflinePages(offlinePages));
+            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.
             }
         });
     }
 
     /**
-     * Gets all bookmarks that correspond to the given list of offline pages, in MRU order.
-     * @see http://crbug.com/537806
-     */
-    private List<BookmarkId> filterBookmarkIdsByOfflinePages(List<OfflinePageItem> offlinePages) {
-        Collections.sort(offlinePages, sOfflinePageComparator);
-
-        HashSet<BookmarkId> existingBookmarks =
-                new HashSet<>(getAllBookmarkIDsOrderedByCreationDate());
-
-        List<BookmarkId> bookmarkIds = new ArrayList<>();
-        for (OfflinePageItem offlinePage : offlinePages) {
-            BookmarkId bookmarkId = getBookmarkIdForOfflineClientId(offlinePage.getClientId());
-            if (existingBookmarks.contains(bookmarkId)) {
-                bookmarkIds.add(bookmarkId);
-            }
-        }
-        return bookmarkIds;
-    }
-
-    /**
-     * @return Offline page bridge.
+     * @see org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem#getTitle()
      */
-    public OfflinePageBridge getOfflinePageBridge() {
-        return mOfflinePageBridge;
+    public String getBookmarkTitle(BookmarkId bookmarkId) {
+        return getBookmarkById(bookmarkId).getTitle();
     }
 
     /**
-     * @param id The client id to convert.
-     * @return The bookmark id contained in the specified client id.
+     * @return The id of the default folder to add bookmarks/folders to.
      */
-    public static BookmarkId getBookmarkIdForOfflineClientId(ClientId id) {
-        if (!id.getNamespace().equals(OfflinePageBridge.BOOKMARK_NAMESPACE)) {
-            return new BookmarkId(-1, BookmarkType.NORMAL);
-        }
-        return BookmarkId.getBookmarkIdFromString(id.getId());
+    public BookmarkId getDefaultFolder() {
+        return getMobileFolderId();
     }
 }