[Offline pages] Wiring saving and opening offline page

chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java

Index: chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
index 8f145f1f19aedefb4703805707f1e16bd1b33e23..ffbb47e721ee560c7ab3694c5302c86192675ef8 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
@@ -17,11 +17,15 @@ import org.chromium.chrome.browser.BookmarksBridge;
 import org.chromium.chrome.browser.favicon.LargeIconBridge;
 import org.chromium.chrome.browser.favicon.LargeIconBridge.LargeIconCallback;
 import org.chromium.chrome.browser.offline_pages.OfflinePageBridge;
+import org.chromium.chrome.browser.offline_pages.OfflinePageBridge.OfflinePageModelObserver;
+import org.chromium.chrome.browser.offline_pages.OfflinePageBridge.SavePageCallback;
 import org.chromium.chrome.browser.offline_pages.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.offline_pages.SavePageResult;
+import org.chromium.content_public.browser.WebContents;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -35,6 +39,17 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
     private static final int FAVICON_MAX_CACHE_SIZE = 10 * 1024 * 1024; // 10MB
 
     /**
+     * Callback for use with addBookmark.
+     */
+    public interface AddBookmarkCallback {
+        /**
+         * Called when the bookmark has been added.
+         * @param bookmarkId ID of the bookmark that has been added.
+         */
+        void onBookmarkAdded(BookmarkId bookmarkId);
+    }
+
+    /**
      * Observer that listens to delete event. This interface is used by undo controllers to know
      * which bookmarks were deleted. Note this observer only listens to events that go through
      * enhanced bookmark model.
@@ -53,6 +68,18 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
     private ObserverList<EnhancedBookmarkDeleteObserver> mDeleteObservers = new ObserverList<>();
     private LruCache<String, Pair<Bitmap, Integer>> mFaviconCache;
     private OfflinePageBridge mOfflinePageBridge;
+    private boolean mIsOfflinePageModelLoaded;
+
+    private final OfflinePageModelObserver mOfflinePageModelObserver =

[fgorski, 2015/08/13 17:41:51]

Is there a reason we create the observer here, even thou offline pages feature
might be switched off completely? 

can this be moved to 112?

[jianli, 2015/08/13 22:37:18]

Done.

+            new OfflinePageModelObserver() {
+        @Override
+        public void offlinePageModelLoaded() {
+            mIsOfflinePageModelLoaded = true;
+            if (isBookmarkModelLoaded()) {
+                notifyBookmarkModelLoaded();
+            }
+        }
+    };
 
     /**
      * Initialize enhanced bookmark model for last used non-incognito profile.
@@ -82,8 +109,12 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
         };
 
         if (OfflinePageBridge.isEnabled()) {
-            // TODO(jianli): Make sure to wait until the bridge is loaded.
             mOfflinePageBridge = new OfflinePageBridge(profile);
+            if (mOfflinePageBridge.isOfflinePageModelLoaded()) {
+                mIsOfflinePageModelLoaded = true;
+            } else {
+                mOfflinePageBridge.addObserver(mOfflinePageModelObserver);
+            }
         }
     }
 
@@ -92,9 +123,32 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
      */
     @Override
     public void destroy() {
-        super.destroy();
+        if (mOfflinePageBridge != null) {
+            mOfflinePageBridge.destroy();
+            mOfflinePageBridge = null;
+        }
         mLargeIconBridge.destroy();
         mFaviconCache = null;
+
+        super.destroy();
+    }
+
+    @Override
+    public boolean isBookmarkModelLoaded() {
+        return super.isBookmarkModelLoaded()
+                && (mOfflinePageBridge == null || mIsOfflinePageModelLoaded);
+    }
+
+    @Override
+    public void notifyBookmarkModelLoaded() {

[fgorski, 2015/08/13 17:41:50]

1) as per comment in BookmarksBridge 571 this should be protected.

2) Alternatively I think it is worth changing the if to:
if (isBookmarkModelLoaded())...

3) Another alternative:
in BookmarksBridge:
private void notifyBookmarkModelLoaded() {
  if (isBookmarkModelLoaded()) {
    for loop over observers
  }
}

while this override is gone completely.

[jianli, 2015/08/13 22:37:18]

Took the alternative you suggest. We still need to define
notifyBookmarkModelLoaded as protected since EnhancedBookmarksModel needs to
call it.

[fgorski, 2015/08/13 22:58:52]

Acknowledged.

+        if (mOfflinePageBridge == null || mIsOfflinePageModelLoaded) {
+            super.notifyBookmarkModelLoaded();
+            return;
+        }
+
+        // If offline pages feature is enabled and the loading is not done yet, wait.
+        // offlinePageModelLoaded will be called and super.notifyBookmarkModelLoaded() will then
+        // be called.
     }
 
     /**
@@ -156,10 +210,37 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
         }
     }
 
-    @Override
-    public BookmarkId addBookmark(BookmarkId parent, int index, String title, String url) {
+    /**
+     * Add a new bookmark asynchronously.

[Kibeom Kim (inactive), 2015/08/13 20:43:48]

Can't this also add a bookmark synchronously at #230?

[jianli, 2015/08/13 22:37:18]

Yes. But since we're calling a callback, it is still a async pattern.

+     *
+     * @param parent Folder where to add.
+     * @param index The position where the bookmark will be placed in parent folder
+     * @param title Title of the new bookmark.
+     * @param url Url of the new bookmark
+     * @param webContents A {@link WebContents} object.
+     * @param callback The callback to be invoked when the bookmark is added.
+     */
+    public void addBookmarkAsync(BookmarkId parent, int index, String title, String url,
+                                 WebContents webContents, final AddBookmarkCallback callback) {
         url = DomDistillerUrlUtils.getOriginalUrlFromDistillerUrl(url);
-        return super.addBookmark(parent, index, title, url);
+        final BookmarkId enhancedId = addBookmark(parent, index, title, url);
+
+        // If there is no need to save offline page, return now.
+        if (mOfflinePageBridge == null) {
+            callback.onBookmarkAdded(enhancedId);
+            return;
+        }
+
+        mOfflinePageBridge.savePage(webContents, enhancedId,
+                new SavePageCallback() {
+                    @Override
+                    public void onSavePageDone(int savePageResult, String url) {
+                        // TODO(jianli): Error handling.
+                        if (savePageResult == SavePageResult.SUCCESS) {
+                            callback.onBookmarkAdded(enhancedId);
+                        }
+                    }
+                });
     }
 
     /**
@@ -170,6 +251,22 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
     }
 
     /**
+     * Returns the url used to launch a bookmark.
+     *
+     * @param bookmarkId ID of the bookmark to launch.
+     */
+    public String getBookmarkLaunchUrl(BookmarkId bookmarkId) {
+        String url = getBookmarkById(bookmarkId).getUrl();

[Kibeom Kim (inactive), 2015/08/13 20:43:48]

suggestion:

Since this doesn't need to be called if (mOfflinePageBridge != null), I'd do

if (mOfflinePageBridge == null) {
    return getBookmarkById(bookmarkId).getUrl();
} else {
    return ....;
}

[jianli, 2015/08/13 22:37:18]

Done.

+
+        // If offline page is enabled, use the offline url.
+        if (mOfflinePageBridge != null) {
+            url = mOfflinePageBridge.getPageByBookmarkId(bookmarkId).getOfflineUrl();

[fgorski, 2015/08/13 17:41:51]

what if mOfflinePageBridge.getPageByBookmarkId(bookmarkId) returns null, because
the offline page was deleted... worth adding test if possible...
Code for delete is already in.

[jianli, 2015/08/13 22:37:18]

Done.

Will add more tests latet.

+        }
+
+        return url;
+    }
+
+    /**
      * Retrieves a favicon and fallback color for the given |url|. An LRU cache is used to store the
      * favicons. If the favicon is not already present in the cache, it is retrieved using
      * LargeIconBridge#getLargeIconForUrl().
@@ -221,4 +318,11 @@ public class EnhancedBookmarksModel extends BookmarksBridge {
         }
         return bookmarkIds;
     }
+
+    /**
+     * @return Offline page bridge.
+     */
+    public OfflinePageBridge getOfflinePageBridge() {
+        return mOfflinePageBridge;
+    }
 }