Fix a small bug in bookmarks_bridge.cc

Fix a small bug in bookmarks_bridge.cc

While fetching the node_id and type from java bookmark object, we fallback
to the mobile bookmarks folder in case the value in the invalid -1. We need
to update the java bookmark object to reflect this change before passing it
back.

BUG=316405
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235499

[Ramya, 2013-11-08 00:16:47]

tedchoc@/newt@ PTAL

[Ted C, 2013-11-08 01:28:26]

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:294:
private static BookmarkId getBookmarkId(long id, int type) {
I would call this createBookmarkId

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:209: const BookmarkNode*
BookmarksBridge::GetFolderWithFallback(
I wouldn't add new methods for this or GetNodeByID.  I think keep the awareness
of the bookmarkID java object isolated to only the JNI methods and use the
previous versions internally.

[newt (away), 2013-11-08 01:54:05]

looks good. a few ideas inline, take them or leave them :)

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:294:
private static BookmarkId getBookmarkId(long id, int type) {
maybe call it "createBookmarkId()"?

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:88: // Recreate the java
bookmarkId object due to fallback.
An idea: you could modify GetFolderWithFallback() to "return" the fallen back
Java BookmarkId as well as the BookmarkNode (using out parameters). Then you
could move lines 88-91 into GetFolderWithFallback() and call
Java_BookmarksBridge_getBookmarkId() only if fallback was required. e.g.:

    const BookmarkNode* GetFolderWithFallback(ScopedJavaLocalRef<jobject>
j_folder_id) {
        ...
        // if fallback:
        j_folder_id.Reset(Java_BookmarksBridge_getBookmarkId(env, folder->id())
    }

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:194: const BookmarkNode*
BookmarksBridge::GetNodeByID(jobject j_node_id_obj) {
I'm not sure how much work AttachCurrentThread() is, but it's probably worth
passing in to GetNodeByID() and GetFolderWithFallback() the JNIEnv since we
already know it.

[Ramya, 2013-11-11 18:34:16]

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:294:
private static BookmarkId getBookmarkId(long id, int type) {
On 2013/11/08 01:28:27, Ted C wrote:
> I would call this createBookmarkId

Done.

https://codereview.chromium.org/61873005/diff/30001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:294:
private static BookmarkId getBookmarkId(long id, int type) {
On 2013/11/08 01:54:06, newt wrote:
> maybe call it "createBookmarkId()"?

Done.

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
File chrome/browser/android/bookmarks/bookmarks_bridge.cc (right):

https://codereview.chromium.org/61873005/diff/30001/chrome/browser/android/bo...
chrome/browser/android/bookmarks/bookmarks_bridge.cc:209: const BookmarkNode*
BookmarksBridge::GetFolderWithFallback(
On 2013/11/08 01:28:27, Ted C wrote:
> I wouldn't add new methods for this or GetNodeByID.  I think keep the
awareness
> of the bookmarkID java object isolated to only the JNI methods and use the
> previous versions internally.

Done.

[Ted C, 2013-11-14 22:26:50]

lgtm

[commit-bot: I haz the power, 2013-11-14 23:42:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-15 01:33:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-15 03:56:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-15 07:08:25]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 18:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-15 21:55:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-16 00:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cramya@chromium.org/61873005/150001

[commit-bot: I haz the power, 2013-11-16 02:37:24]

Change committed as 235499