Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
550543003:
Add Instrumental Test for BookmarksBridge (Closed)
DescriptionInstrumental test for BookmarksBridge. It currently tests these functionalities: addBookmark, addFolder and getAllFoldersWithDepths.
BUG=411545
Committed: https://crrev.com/efe56eafb8e74f07c009bc384d15675ac2d50c6b
Cr-Commit-Position: refs/heads/master@{#295627}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Removed some mistakes #
Total comments: 37
Patch Set 4 : Add AddFolder/Bookmark unit test #
Total comments: 1
Patch Set 5 : correct typo #
Total comments: 18
Patch Set 6 : make tests look better #
Total comments: 15
Patch Set 7 : add verify function #
Total comments: 11
Patch Set 8 : make test more sccinct #
Total comments: 1
Patch Set 9 : Made tests more readable #Patch Set 10 : update DEPS #
Messages
Total messages: 29 (6 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org, tedchoc@chromium.org, yfriedman@chromium.org
yfriedman@chromium.org: Please review changes in tedchoc@chromium.org: Please review changes in
On 2014/09/06 01:06:02, Ian Wen wrote: > mailto:yfriedman@chromium.org: Please review changes in > > mailto:tedchoc@chromium.org: Please review changes in so now this can have a test. There's already a "getBookmarkById" function on the bridge. Additionally, if the purpose of this is to create an enhanced bookmark it's better to use BM::AddURLWithCreationTimeAndMetaInfo directly
ianwen@chromium.org changed reviewers: + newt@chromium.org
ianwen@chromium.org changed reviewers: - yfriedman@chromium.org
PTAL. :)
https://codereview.chromium.org/550543003/diff/20001/build/android/developer_... File build/android/developer_recommended_flags.gypi (right): https://codereview.chromium.org/550543003/diff/20001/build/android/developer_... build/android/developer_recommended_flags.gypi:47: 'create_standalone_apk%': 0, mistake? :) https://codereview.chromium.org/550543003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/550543003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:26: friend class BookmarksBridgeTestHelper; where is this class?
https://codereview.chromium.org/550543003/diff/20001/build/android/developer_... File build/android/developer_recommended_flags.gypi (right): https://codereview.chromium.org/550543003/diff/20001/build/android/developer_... build/android/developer_recommended_flags.gypi:47: 'create_standalone_apk%': 0, On 2014/09/16 01:11:01, Kibeom Kim wrote: > mistake? :) Yeah. Removed. https://codereview.chromium.org/550543003/diff/20001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/550543003/diff/20001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:26: friend class BookmarksBridgeTestHelper; On 2014/09/16 01:11:01, Kibeom Kim wrote: > where is this class? Mistake. Removed this line...
https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:165: * Load missing description https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:167: public void loadEmptyPartnerBookmarkShimForTesting() { Add @VisibleForTesting https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:273: * @return The BookmarkId for Mobile folder node invalid javadoc ... same for below https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:275: BookmarkId getOtherFolderId() { Add @VisibleForTesting to both of these as well. Otherwise proguard will remove them and the tests will fail (since they are only called in tests at the moment). I would also just make them public since they are perfectly fine to be exposed in the API. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:416: public BookmarkId addFolder(BookmarkId parent, int index, String title) { How are you going to find out the index to add nodes to? By default, they usually go to the end of the folder, and there is currently no way to get child count for a folder (without doing something like getChildIDs and then using .size(), which is doing a bunch more jni marshaling than we need). Same below. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:423: * @param index Position the bookmark will be placed in parent folder The position where the bookmark will be placed in the parent folder https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:424: * @param title Title string of the new bookmark string is unnecessary here and below https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:25: * Dummy bookmark model is structured as below: I think this is overkill. I don't think you need this structure for all tests, so I would just try to isolate the logic you want to test in particular methods. You might find that you eventually want to have a initializeTestBookmarks() or something that does something along these lines, but at this point it complicates this file too much. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:86: addFolders(); per my comment above, I would drop this and the next line. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:126: ua = mBookmarksBridge.addBookmark(mMobileNode, 0, "ua", "http://www.google.com"); add separate unit tests for adding bookmarks and another different one for adding folders. Test error conditions, etc... where you would expect null as a return. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:132: @Feature({"Bookmark Bridge"}) I would just use Bookmarks instead of "Bookmark Bridge" https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:151: if (partner_bookmarks_shim_->IsLoaded()) return; in c++, the return goes on the next line. It is fine to have no parens in the c++ case for single line conditional/statement combinations. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:367: const BookmarkNode* mobileNode = bookmark_model_->mobile_node(); since you're here, can you change this to mobile_node like the other comments below? https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:376: const BookmarkNode* otherNode = bookmark_model_->other_node(); s/otherNode/other_node https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:385: const BookmarkNode* desktopNode = bookmark_model_->bookmark_bar_node(); s/desktopNode/desktop_node https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:592: const BookmarkNode* newNode = bookmark_model_->AddFolder( s/newNode/new_node https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:592: const BookmarkNode* newNode = bookmark_model_->AddFolder( I would add a blank line above this line as well. Also, this can return null if passed an invalid ID. Then it will just crash on the next line as you call ->id() on it. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:658: const BookmarkNode* newNode = bookmark_model_->AddURL( s/newNode/new_node Blank line here too https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:117: void DeleteBookmark(JNIEnv* env, jobject obj, jobject j_bookmark_id_obj); This doesn't seem to have a java counterpart...nor is it implemented in the .cc file.
https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:165: * Load On 2014/09/16 02:03:51, Ted C wrote: > missing description Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:167: public void loadEmptyPartnerBookmarkShimForTesting() { On 2014/09/16 02:03:52, Ted C wrote: > Add @VisibleForTesting Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:273: * @return The BookmarkId for Mobile folder node On 2014/09/16 02:03:51, Ted C wrote: > invalid javadoc ... same for below Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:275: BookmarkId getOtherFolderId() { On 2014/09/16 02:03:51, Ted C wrote: > Add @VisibleForTesting to both of these as well. Otherwise proguard will remove > them and the tests will fail (since they are only called in tests at the > moment). > > I would also just make them public since they are perfectly fine to be exposed > in the API. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:416: public BookmarkId addFolder(BookmarkId parent, int index, String title) { On 2014/09/16 02:03:52, Ted C wrote: > How are you going to find out the index to add nodes to? By default, they > usually go to the end of the folder, and there is currently no way to get child > count for a folder (without doing something like getChildIDs and then using > .size(), which is doing a bunch more jni marshaling than we need). > > Same below. Currently every folder/ bookmark is inserted at the front of parent (index 0). When we list all folders to user, we sort according to folder name; when we show bookmarks, we sort them according to creation date. Therefore it might not matter where the new node is inserted. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:423: * @param index Position the bookmark will be placed in parent folder On 2014/09/16 02:03:51, Ted C wrote: > The position where the bookmark will be placed in the parent folder Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:424: * @param title Title string of the new bookmark On 2014/09/16 02:03:52, Ted C wrote: > string is unnecessary here and below Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:25: * Dummy bookmark model is structured as below: On 2014/09/16 02:03:52, Ted C wrote: > I think this is overkill. I don't think you need this structure for all tests, > so I would just try to isolate the logic you want to test in particular methods. > > You might find that you eventually want to have a initializeTestBookmarks() or > something that does something along these lines, but at this point it > complicates this file too much. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:86: addFolders(); On 2014/09/16 02:03:52, Ted C wrote: > per my comment above, I would drop this and the next line. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:126: ua = mBookmarksBridge.addBookmark(mMobileNode, 0, "ua", "http://www.google.com"); On 2014/09/16 02:03:52, Ted C wrote: > add separate unit tests for adding bookmarks and another different one for > adding folders. > > Test error conditions, etc... where you would expect null as a return. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:132: @Feature({"Bookmark Bridge"}) On 2014/09/16 02:03:52, Ted C wrote: > I would just use Bookmarks instead of "Bookmark Bridge" Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:151: if (partner_bookmarks_shim_->IsLoaded()) return; On 2014/09/16 02:03:52, Ted C wrote: > in c++, the return goes on the next line. > > It is fine to have no parens in the c++ case for single line > conditional/statement combinations. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:367: const BookmarkNode* mobileNode = bookmark_model_->mobile_node(); On 2014/09/16 02:03:52, Ted C wrote: > since you're here, can you change this to mobile_node like the other comments > below? Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:376: const BookmarkNode* otherNode = bookmark_model_->other_node(); On 2014/09/16 02:03:52, Ted C wrote: > s/otherNode/other_node Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:385: const BookmarkNode* desktopNode = bookmark_model_->bookmark_bar_node(); On 2014/09/16 02:03:52, Ted C wrote: > s/desktopNode/desktop_node Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:592: const BookmarkNode* newNode = bookmark_model_->AddFolder( On 2014/09/16 02:03:53, Ted C wrote: > I would add a blank line above this line as well. > > Also, this can return null if passed an invalid ID. Then it will just crash on > the next line as you call ->id() on it. Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.cc:658: const BookmarkNode* newNode = bookmark_model_->AddURL( On 2014/09/16 02:03:52, Ted C wrote: > s/newNode/new_node > > Blank line here too Done. https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... File chrome/browser/android/bookmarks/bookmarks_bridge.h (right): https://codereview.chromium.org/550543003/diff/40001/chrome/browser/android/b... chrome/browser/android/bookmarks/bookmarks_bridge.h:117: void DeleteBookmark(JNIEnv* env, jobject obj, jobject j_bookmark_id_obj); On 2014/09/16 02:03:53, Ted C wrote: > This doesn't seem to have a java counterpart...nor is it implemented in the .cc > file. This is not a newly created function. It has been in both cc and java files since long time ago.
https://codereview.chromium.org/550543003/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:438: * @param title Titl of the new bookmark s/Titl/Title
https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:277: * Get the special folder "other" from bookmark model. instead of Get, use @return https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:417: * bookmark folder or a managed bookomark folder or root node of the entire align these lines of javadoc with the "Folder" above. Same for the next function. https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:425: if (parent.getType() != BookmarkType.BOOKMARK_TYPE_NORMAL || index < 0 || title == null) { We shouldn't silently accept bad arguments. They are covering up programming errors in other parts of the code. We should either just add asserts for this assert parent.getType() != BookmarkType.BOOKMARK_TYPE_NORMAL; assert index >= 0; assert title != null; I think it is sad that the native bookmark_model returns null for Folder or Node. I wouldn't go about special handling these (even if it allows you to null check them in tests). https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:54: ThreadUtils.runOnUiThreadBlocking(new Runnable() { If you're running the entire test content on the UI thread, I believe you can just add the @UiThreadTest annotation https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:58: mBookmarksBridge.addBookmark(mDesktopNode, 0, "dumm", "http://yahoo.com")); Save the returned BookmarkId and use that to get the bookmark via getBookmarkById. Verify that the title and URL are as expected. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:85: BookmarkId a; I would just define these where they are first used. And folderA, folderB, ... is more descriptive. Same for bookmarkA, bookmarkB, etc... https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:107: mBookmarksBridge.addFolder(a, 0, "ac"); This seems like more folders than you need. I think you really only need to test siblings, and descendants, and repeating the siblings so often just seems overly verbose. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:136: // assertEquals(folderList.size(), 18); remove this https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:157: assertTrue(depthList.get(i) >= 0); You should be verifying the values of the depth list is correct and not all some default value.
Rewrote the tests entirely to make them better and more clear. Thanks for the great suggestions. https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:277: * Get the special folder "other" from bookmark model. On 2014/09/16 21:53:55, Ted C wrote: > instead of Get, use @return Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:417: * bookmark folder or a managed bookomark folder or root node of the entire On 2014/09/16 21:53:54, Ted C wrote: > align these lines of javadoc with the "Folder" above. Same for the next > function. Done. Eclipse formatting error. https://codereview.chromium.org/550543003/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:425: if (parent.getType() != BookmarkType.BOOKMARK_TYPE_NORMAL || index < 0 || title == null) { On 2014/09/16 21:53:54, Ted C wrote: > We shouldn't silently accept bad arguments. They are covering up programming > errors in other parts of the code. > > We should either just add asserts for this > > assert parent.getType() != BookmarkType.BOOKMARK_TYPE_NORMAL; > assert index >= 0; > assert title != null; > > I think it is sad that the native bookmark_model returns null for Folder or > Node. I wouldn't go about special handling these (even if it allows you to null > check them in tests). Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:54: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2014/09/16 21:53:55, Ted C wrote: > If you're running the entire test content on the UI thread, I believe you can > just add the @UiThreadTest annotation Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:58: mBookmarksBridge.addBookmark(mDesktopNode, 0, "dumm", "http://yahoo.com")); On 2014/09/16 21:53:55, Ted C wrote: > Save the returned BookmarkId and use that to get the bookmark via > getBookmarkById. Verify that the title and URL are as expected. Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:85: BookmarkId a; On 2014/09/16 21:53:55, Ted C wrote: > I would just define these where they are first used. > > And folderA, folderB, ... is more descriptive. > > Same for bookmarkA, bookmarkB, etc... Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:107: mBookmarksBridge.addFolder(a, 0, "ac"); On 2014/09/16 21:53:55, Ted C wrote: > This seems like more folders than you need. I think you really only need to > test siblings, and descendants, and repeating the siblings so often just seems > overly verbose. Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:136: // assertEquals(folderList.size(), 18); On 2014/09/16 21:53:55, Ted C wrote: > remove this Done. https://codereview.chromium.org/550543003/diff/80001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:157: assertTrue(depthList.get(i) >= 0); On 2014/09/16 21:53:55, Ted C wrote: > You should be verifying the values of the depth list is correct and not all some > default value. Done.
Let's update the commit message. Optional: I think this test can be a unit test(inheriting from AndroidTestCase), because only profile initialization is needed and we can use a fake profile. But I'm also fine with this. https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:168: * empty node. 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. https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:418: * model. indent the second and the third line to the first line's "Folder where...", also same for #435 - #422 https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:425: assert parent.getType() == BookmarkType.BOOKMARK_TYPE_NORMAL; 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. https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:429: return nativeAddFolder(mNativeBookmarksBridge, parent, index, title); 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. https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:101: idToDepth.put(folderC, 0); how about just creating ground-truth expectedFolderList and expectedDepthList, and compare that by == below. I think that would make the code much simpler. https://codereview.chromium.org/550543003/diff/100001/chrome/browser/android/... File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right): https://codereview.chromium.org/550543003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:653: long bookmark_id = JavaBookmarkIdGetId(env, j_parent_id_obj); maybe parent_id? https://codereview.chromium.org/550543003/diff/100001/chrome/browser/android/... chrome/browser/android/bookmarks/bookmarks_bridge.cc:661: GURL(base::android::ConvertJavaStringToUTF16(env, j_url))); Existing code calls ParseAndMaybeAppendScheme in chrome_browser_provider.cc . Do you think we need that here?
https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:69: assertEquals(mBookmarksBridge.getBookmarkById(bookmarks[i]).getTitle(), I think these verification steps are still a bit more complicated than necessary. In my opinion, it would be clearer to have a separate verification function (or two if you want to have one for bookmarks and one for folders): verifyBookmarkEntry(BookmarkId id, String title, String url, boolean isFolder, BookmarkId parentId) { BookmarkItem item = mBookmarksBridge.getBookmarkById(id); assertEquals(title, item.getTitle()); ... } Then it would just be: BookmarkId id = mBookmarksBridge.addBookmark(mDesktopNode, 0, "a", "http://a.com"); verifyBookmarkEntry(id, "a", "http://a.com", false, mDesktopNode); ... ... ... That should make the test really easy to understand and doesn't require jumping around. https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:101: idToDepth.put(folderC, 0); On 2014/09/17 19:28:08, Kibeom Kim wrote: > how about just creating ground-truth expectedFolderList and expectedDepthList, > and compare that by == below. I think that would make the code much simpler. I think you'd want to use: http://developer.android.com/reference/android/test/MoreAsserts.html#assertCo..., java.lang.Object...) To compare equality, but I think that should work.
https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:418: * model. On 2014/09/17 19:28:08, Kibeom Kim wrote: > indent the second and the third line to the first line's "Folder where...", also > same for #435 - #422 Done. https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:425: assert parent.getType() == BookmarkType.BOOKMARK_TYPE_NORMAL; On 2014/09/17 19:28:08, Kibeom Kim wrote: > 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. 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. https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:69: assertEquals(mBookmarksBridge.getBookmarkById(bookmarks[i]).getTitle(), On 2014/09/17 19:51:31, Ted C wrote: > I think these verification steps are still a bit more complicated than > necessary. > > In my opinion, it would be clearer to have a separate verification function (or > two if you want to have one for bookmarks and one for folders): > > verifyBookmarkEntry(BookmarkId id, String title, String url, boolean isFolder, > BookmarkId parentId) { > BookmarkItem item = mBookmarksBridge.getBookmarkById(id); > assertEquals(title, item.getTitle()); > ... > } > > Then it would just be: > BookmarkId id = mBookmarksBridge.addBookmark(mDesktopNode, 0, "a", > "http://a.com"); > verifyBookmarkEntry(id, "a", "http://a.com", false, mDesktopNode); > > ... > ... > ... > > That should make the test really easy to understand and doesn't require jumping > around. Done. https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:101: idToDepth.put(folderC, 0); On 2014/09/17 19:51:31, Ted C wrote: > On 2014/09/17 19:28:08, Kibeom Kim wrote: > > how about just creating ground-truth expectedFolderList and expectedDepthList, > > and compare that by == below. I think that would make the code much simpler. > > I think you'd want to use: > http://developer.android.com/reference/android/test/MoreAsserts.html#assertCo..., > java.lang.Object...) > To compare equality, but I think that should work. My major concern about comparing lists directly was that in other languages, the position of "Desktop Folder" will change, so if we launch a bot in other languages, this instrumental test is going to fail. Should I care about multi-language here?
https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:425: assert parent.getType() == BookmarkType.BOOKMARK_TYPE_NORMAL; On 2014/09/17 20:41:22, Ian Wen wrote: > > 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. I just looked up BookmarkModel::AddNode and ::AddURL. Seems like it doesn't really check if the parent BookmarkNode argument actually belongs to it. https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/100001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:101: idToDepth.put(folderC, 0); On 2014/09/17 20:41:22, Ian Wen wrote: > On 2014/09/17 19:51:31, Ted C wrote: > > On 2014/09/17 19:28:08, Kibeom Kim wrote: > > > how about just creating ground-truth expectedFolderList and > expectedDepthList, > > > and compare that by == below. I think that would make the code much simpler. > > > > I think you'd want to use: > > > http://developer.android.com/reference/android/test/MoreAsserts.html#assertCo..., > > java.lang.Object...) > > To compare equality, but I think that should work. > > My major concern about comparing lists directly was that in other languages, the > position of "Desktop Folder" will change, so if we launch a bot in other > languages, this instrumental test is going to fail. Should I care about > multi-language here? Good point! I'm not sure if there is any bot running other than English. But I guess handling non-English mDesktopNode shouldn't hurt, maybe exclude the node from the result and expected arrays before you compare?
https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:68: verifyBookmark(bookmarkA, "a", "http://a.com/", false, mDesktopNode); I would put these directly under the corresponding adds (requires less looking up and down). https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:83: if (!isFolder) assertEquals(expectedUrl, item.getUrl()); the bookmarkitem also has isFolder() I believe, so we should assert that matches the expected value. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:109: BookmarkId[] bookmarks = new BookmarkId[] { based on my comment below, you wouldn't need to track these bookmarkIds as the folder checks would ensure they are not in the list. Just add a comment that you're adding bookmarks to ensure they do not show up in the final list. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:136: assertTrue(depthList.get(i) >= 0); You don't need this line as you are comparing the depths to the expected values below. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:137: assertTrue(idToDepth.containsKey(folderList.get(i))); For your assertTrue statements, include a statement that can be used for debugging. i.e. assertTrue("Returned list contained unexpected key: " + folderList.get(i), <assertion>); https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:138: assertEquals(depthList.get(i), idToDepth.get(folderList.get(i))); I think you can simplify your number of assertions by removing items from the idToDepth list. Then right after this block, you should assert that the idToDepth map is empty. Then you should be able to remove all the asserts above this loop until: assertEquals(folderList.size(), depthList.size()); All the other asserts would then be covered by this loop.
https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:68: verifyBookmark(bookmarkA, "a", "http://a.com/", false, mDesktopNode); On 2014/09/18 17:56:32, Ted C wrote: > I would put these directly under the corresponding adds (requires less looking > up and down). Done. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:83: if (!isFolder) assertEquals(expectedUrl, item.getUrl()); On 2014/09/18 17:56:31, Ted C wrote: > the bookmarkitem also has isFolder() I believe, so we should assert that matches > the expected value. Done. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:136: assertTrue(depthList.get(i) >= 0); On 2014/09/18 17:56:31, Ted C wrote: > You don't need this line as you are comparing the depths to the expected values > below. Done. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:137: assertTrue(idToDepth.containsKey(folderList.get(i))); On 2014/09/18 17:56:31, Ted C wrote: > For your assertTrue statements, include a statement that can be used for > debugging. > > i.e. > > assertTrue("Returned list contained unexpected key: " + folderList.get(i), > <assertion>); Done. https://codereview.chromium.org/550543003/diff/120001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:138: assertEquals(depthList.get(i), idToDepth.get(folderList.get(i))); On 2014/09/18 17:56:31, Ted C wrote: > I think you can simplify your number of assertions by removing items from the > idToDepth list. Then right after this block, you should assert that the > idToDepth map is empty. > > Then you should be able to remove all the asserts above this loop until: > > assertEquals(folderList.size(), depthList.size()); > > All the other asserts would then be covered by this loop. Done.
https://codereview.chromium.org/550543003/diff/140001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/550543003/diff/140001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:137: assertTrue("Returned list contained less elements than expected", idToDepth.isEmpty()); Change this line to show numbers instead of just a boolean.
lgtm
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/550543003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by ianwen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/550543003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 8fb43f49ebb0ed09692a41dd4cc5a5edaefc8cb4
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/efe56eafb8e74f07c009bc384d15675ac2d50c6b Cr-Commit-Position: refs/heads/master@{#295627} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
