Add JNI bridges for EnhancedBookmark Create/AddFolder/Move

Add JNI bridges for EnhancedBookmark Create/AddFolder/Move

Enhanced bookmarks should be calling the the EnhancedBookmarkModel
to do create/modify actions rather than the regular BookmarkModel
so that we are sure that fields get populated stars-related
metadata.

BUG=415817
Committed: https://crrev.com/a4dbf3c94b6e8fbb9e435644691f877f9c2f6b98
Cr-Commit-Position: refs/heads/master@{#302019}

[danduong, 2014-10-29 01:43:17]

danduong@chromium.org changed reviewers:
+ ianwen@chromium.org, kkimlabs@chromium.org

[Ian Wen, 2014-10-29 05:39:24]

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java
(right):

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:48:
return nativeAddFolder(mNativeEnhancedBookmarksBridge, parent, index, title);
Javadoc for these three functions?

[Kibeom Kim (inactive), 2014-10-29 06:02:39]

lgtm with Ian's comment

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java
(right):

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:115:
}
Let's move this function to BookmarkId.java and expose through bookmark_id.h,
and also delete BookmarksBridge#createBookmarkId to avoid code duplication.

[danduong, 2014-10-29 19:50:45]

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
File
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java
(right):

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:48:
return nativeAddFolder(mNativeEnhancedBookmarksBridge, parent, index, title);
On 2014/10/29 05:39:24, Ian Wen wrote:
> Javadoc for these three functions?

Done.

https://codereview.chromium.org/685953002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/EnhancedBookmarksBridge.java:115:
}
On 2014/10/29 06:02:39, Kibeom Kim wrote:
> Let's move this function to BookmarkId.java and expose through bookmark_id.h,
> and also delete BookmarksBridge#createBookmarkId to avoid code duplication.

Done.

[danduong, 2014-10-29 19:51:05]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2014-10-29 19:51:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685953002/20001

[commit-bot: I haz the power, 2014-10-29 19:58:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-29 19:59:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[danduong, 2014-10-29 20:31:59]

danduong@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Yaron, 2014-10-29 21:50:44]

It's a little sad to be duplicating lots of the BookmarksBridge into EHB but I"m
not sure of a better way without making invasive changes to BookmarksModel

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
(right):

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:190: bool
EnhancedBookmarksBridge::IsEditable(const BookmarkNode* node) const {
BookmarksBridge::IsEditable has additional safety checks. I assume this is
because you are handling partner bookmarks checks in the internal repo but why?
Seems like the right layer would be here.

[danduong, 2014-10-29 22:13:40]

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
(right):

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:190: bool
EnhancedBookmarksBridge::IsEditable(const BookmarkNode* node) const {
On 2014/10/29 21:50:43, Yaron (on leave) wrote:
> BookmarksBridge::IsEditable has additional safety checks. I assume this is
> because you are handling partner bookmarks checks in the internal repo but
why?
> Seems like the right layer would be here.

That's still the right layer. We haven't added any partner bookmark support to
Enhanced bookmark model yet.  This is currently in progress.

[Yaron, 2014-10-29 23:02:31]

On 2014/10/29 22:13:40, danduong wrote:
>
https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
> File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
> (right):
> 
>
https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
> chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:190:
bool
> EnhancedBookmarksBridge::IsEditable(const BookmarkNode* node) const {
> On 2014/10/29 21:50:43, Yaron (on leave) wrote:
> > BookmarksBridge::IsEditable has additional safety checks. I assume this is
> > because you are handling partner bookmarks checks in the internal repo but
> why?
> > Seems like the right layer would be here.
> 
> That's still the right layer. We haven't added any partner bookmark support to
> Enhanced bookmark model yet.  This is currently in progress.

What's still the right layer? Internal?

[danduong, 2014-10-29 23:54:26]

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
File chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc
(right):

https://codereview.chromium.org/685953002/diff/20001/chrome/browser/android/e...
chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:190: bool
EnhancedBookmarksBridge::IsEditable(const BookmarkNode* node) const {
On 2014/10/29 22:13:40, danduong wrote:
> On 2014/10/29 21:50:43, Yaron (on leave) wrote:
> > BookmarksBridge::IsEditable has additional safety checks. I assume this is
> > because you are handling partner bookmarks checks in the internal repo but
> why?
> > Seems like the right layer would be here.
> 
> That's still the right layer. We haven't added any partner bookmark support to
> Enhanced bookmark model yet.  This is currently in progress.

This layer.

[Kibeom Kim (inactive), 2014-10-29 23:57:40]

On 2014/10/29 21:50:44, Yaron (on leave) wrote:
> It's a little sad to be duplicating lots of the BookmarksBridge into EHB but
I"m
> not sure of a better way without making invasive changes to BookmarksModel
> 

Yeah it's really sad to have these duplicates, but since we're targeting to
replace the old bookmark UI in a mid-term, I think it's OK to just wait and
merge BookmarksBridge to EnhancedBookmarksBridge. If it doesn't happen, we
definitely need to reorganize.


On 2014/10/29 23:02:31, Yaron (on leave) wrote:
> On 2014/10/29 22:13:40, danduong wrote:
> > That's still the right layer. We haven't added any partner bookmark support
to
> > Enhanced bookmark model yet.  This is currently in progress.
> 
> What's still the right layer? Internal?

We can keep PartnerBookmarksShim inside EnhancedBookmarksBridge just like we did
for BookmarksBridge, but since partner bookmarks are no longer under the Mobile
node in enhanced bookmark, I think it's cleaner to have another layer on the top
of EnhancedBookmarksBridge and PartnerBookmarksBridge(new), and that can be in
the public repo.

[Yaron, 2014-10-30 01:55:38]

On 2014/10/29 23:57:40, Kibeom Kim wrote:
> On 2014/10/29 21:50:44, Yaron (on leave) wrote:
> > It's a little sad to be duplicating lots of the BookmarksBridge into EHB but
> I"m
> > not sure of a better way without making invasive changes to BookmarksModel
> > 
> 
> Yeah it's really sad to have these duplicates, but since we're targeting to
> replace the old bookmark UI in a mid-term, I think it's OK to just wait and
> merge BookmarksBridge to EnhancedBookmarksBridge. If it doesn't happen, we
> definitely need to reorganize.
> 
> 
> On 2014/10/29 23:02:31, Yaron (on leave) wrote:
> > On 2014/10/29 22:13:40, danduong wrote:
> > > That's still the right layer. We haven't added any partner bookmark
support
> to
> > > Enhanced bookmark model yet.  This is currently in progress.
> > 
> > What's still the right layer? Internal?
> 
> We can keep PartnerBookmarksShim inside EnhancedBookmarksBridge just like we
did
> for BookmarksBridge, but since partner bookmarks are no longer under the
Mobile
> node in enhanced bookmark, I think it's cleaner to have another layer on the
top
> of EnhancedBookmarksBridge and PartnerBookmarksBridge(new), and that can be in
> the public repo.

lgtm

[danduong, 2014-10-30 01:57:03]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2014-10-30 01:57:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685953002/20001

[commit-bot: I haz the power, 2014-10-30 02:10:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-30 02:11:33]

Patchset 2 (id:??) landed as
https://crrev.com/a4dbf3c94b6e8fbb9e435644691f877f9c2f6b98
Cr-Commit-Position: refs/heads/master@{#302019}