Add Instrumental Test for BookmarksBridge

chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java

Index: chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
index 110c3e521b350a4a675e847cb959b944e07b02c0..f7fec30157efe290569b7c463118c63b05d8e2dc 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
@@ -6,8 +6,10 @@ package org.chromium.chrome.browser;
 
 import org.chromium.base.CalledByNative;
 import org.chromium.base.ObserverList;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.bookmarks.BookmarkId;
+import org.chromium.components.bookmarks.BookmarkType;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -162,6 +164,15 @@ public class BookmarksBridge {
     }
 
     /**
+     * Load an empty partner bookmark shim for testing. The root node for bookmark will be an
+     * empty node.

[Kibeom Kim (inactive), 2014/09/17 19:28:08]

Let's make the comment and the actual function consistent. i.e., mention that
this doesn't update the root node if it's already loaded, or just force set the
root node to be empty always.

+     */
+    @VisibleForTesting
+    public void loadEmptyPartnerBookmarkShimForTesting() {
+        nativeLoadEmptyPartnerBookmarkShimForTesting(mNativeBookmarksBridge);
+    }
+
+    /**
      * Add an observer to bookmark model changes.
      * @param observer The observer to be added.
      */
@@ -263,6 +274,24 @@ public class BookmarksBridge {
     }
 
     /**
+     * @return Id representing the special "other" folder from bookmark model.
+     */
+    @VisibleForTesting
+    public BookmarkId getOtherFolderId() {
+        assert mIsNativeBookmarkModelLoaded;
+        return nativeGetOtherFolderId(mNativeBookmarksBridge);
+    }
+
+    /**
+     * @return BokmarkId representing special "desktop" folder, namely "bookmark bar".
+     */
+    @VisibleForTesting
+    public BookmarkId getDesktopFolderId() {
+        assert mIsNativeBookmarkModelLoaded;
+        return nativeGetDesktopFolderId(mNativeBookmarksBridge);
+    }
+
+    /**
      * Reads sub-folder IDs, sub-bookmark IDs, or both of the given folder.
      *
      * @param getFolders   Whether sub-folders should be returned.
@@ -382,6 +411,46 @@ public class BookmarksBridge {
     }
 
     /**
+     * Add a new folder to the given parent folder
+     *
+     * @param parent Folder where to add. Must be a normal editable folder, instead of a partner
+     *        bookmark folder or a managed bookomark folder or root node of the entire bookmark
+     *        model.

[Kibeom Kim (inactive), 2014/09/17 19:28:08]

indent the second and the third line to the first line's "Folder where...", also
same for #435 - #422

[Ian Wen, 2014/09/17 20:41:22]

Done.

+     * @param index The position to locate the new folder
+     * @param title The title text of the new folder
+     * @return Id of the added node. If adding failed (index is invalid, string is null, parent is
+     *         not editable), returns null.
+     */
+    public BookmarkId addFolder(BookmarkId parent, int index, String title) {
+        assert parent.getType() == BookmarkType.BOOKMARK_TYPE_NORMAL;

[Kibeom Kim (inactive), 2014/09/17 19:28:08]

To my understanding, two cases we still handle after debug-only assert failure
are

1. there is a security risk.
2. potential data corruption.

So in this case, if partner bookmark was passed and if there is the same id on
BookmarkModel coincidentally, we'll be adding to the wrong folder. So I think we
should still check the type and don't call nativeAddFolder if it's not
BOOKMARK_TYPE_NORMAL. Same for the below function.

[Ian Wen, 2014/09/17 20:41:22]

There is no data corruption here even if we pass a partner bookmark as parent.
In native addBookmark(), we get the node from getNodeById, which takes type as
an input. It will never get a normal node if type is set to be partner. The only
possible issue is that we will add a bookmark to the partner folder then and
bookmark model actually allows that.

[Kibeom Kim (inactive), 2014/09/18 17:46:36]

I just looked up BookmarkModel::AddNode and ::AddURL. Seems like it doesn't
really check if the parent BookmarkNode argument actually belongs to it.

+        assert index >= 0;
+        assert title != null;
+
+        return nativeAddFolder(mNativeBookmarksBridge, parent, index, title);

[Kibeom Kim (inactive), 2014/09/17 19:28:08]

optional: It's simpler to pass parent.getId() and parent.getType(), then we
don't have to make two additional JNI binding on the c++ side. also below too.

+    }
+
+    /**
+     * Add a new bookmark to a specific position below parent
+     *
+     * @param parent Folder where to add. Must be a normal editable folder, instead of a partner
+     *        bookmark folder or a managed bookomark folder or root node of the entire bookmark
+     *        model.
+     * @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
+     * @return Id of the added node. If adding failed (index is invalid, string is null, parent is
+     *         not editable), returns null.
+     */
+    public BookmarkId addBookmark(BookmarkId parent, int index, String title, String url) {
+        assert parent.getType() == BookmarkType.BOOKMARK_TYPE_NORMAL;
+        assert index >= 0;
+        assert title != null;
+        assert url != null;
+
+        return nativeAddBookmark(mNativeBookmarksBridge, parent, index, title, url);
+    }
+
+    /**
      * A bridge function to BookmarkModelFactory::GetForProfile.
      */
     public static long getNativeBookmarkModel(Profile profile) {
@@ -525,6 +594,8 @@ public class BookmarksBridge {
     private native void nativeGetAllFoldersWithDepths(long nativeBookmarksBridge,
             List<BookmarkId> folderList, List<Integer> depthList);
     private native BookmarkId nativeGetMobileFolderId(long nativeBookmarksBridge);
+    private native BookmarkId nativeGetOtherFolderId(long nativeBookmarksBridge);
+    private native BookmarkId nativeGetDesktopFolderId(long nativeBookmarksBridge);
     private native void nativeGetChildIDs(long nativeBookmarksBridge, long id, int type,
             boolean getFolders, boolean getBookmarks, List<BookmarkId> bookmarksList);
     private native void nativeGetAllBookmarkIDsOrderedByCreationDate(long nativeBookmarksBridge,
@@ -540,11 +611,16 @@ public class BookmarksBridge {
     private native void nativeGetCurrentFolderHierarchy(long nativeBookmarksBridge,
             BookmarkId folderId, BookmarksCallback callback,
             List<BookmarkItem> bookmarksList);
+    private native BookmarkId nativeAddFolder(long nativeBookmarksBridge, BookmarkId parent,
+            int index, String title);
     private native void nativeDeleteBookmark(long nativeBookmarksBridge, BookmarkId bookmarkId);
     private native void nativeMoveBookmark(long nativeBookmarksBridge, BookmarkId bookmarkId,
             BookmarkId newParentId, int index);
+    private native BookmarkId nativeAddBookmark(long nativeBookmarksBridge, BookmarkId parent,
+            int index, String title, String url);
     private static native long nativeGetNativeBookmarkModel(Profile profile);
     private static native boolean nativeIsEnhancedBookmarksFeatureEnabled(Profile profile);
+    private native void nativeLoadEmptyPartnerBookmarkShimForTesting(long nativeBookmarksBridge);
     private native long nativeInit(Profile profile);
     private native boolean nativeIsDoingExtensiveChanges(long nativeBookmarksBridge);
     private native void nativeDestroy(long nativeBookmarksBridge);