[Android] EnhancedBookmarksBridge for bookmark description read&write.

[Android] EnhancedBookmarksBridge for bookmark description read&write.

Also moved BookmarksBridge#BookmarkItem to bookmarks component.

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

[sky, 2014-08-12 16:32:01]

What files do you need me to review here?

On Tue, Aug 12, 2014 at 3:14 AM,  <kkimlabs@chromium.org> wrote:
> Reviewers: Yaron, Ted C, sky,
>
> Description:
> [Android] EnhancedBookmarksBridge for bookmark description read&write.
>
> Also moved BookmarksBridge#BookmarkItem to bookmarks component.
>
> BUG=386785
>
> Please review this at https://codereview.chromium.org/459973004/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+609, -154 lines):
>   M chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
>   M chrome/browser/DEPS
>   M chrome/browser/android/bookmarks/bookmarks_bridge.h
>   M chrome/browser/android/bookmarks/bookmarks_bridge.cc
>   M chrome/browser/android/chrome_jni_registrar.cc
>   M chrome/chrome.gyp
>   M components/bookmarks.gypi
>   M components/bookmarks/DEPS
>   A + components/bookmarks/common/android/OWNERS
>   A components/bookmarks/common/android/bookmark_id.h
>   A components/bookmarks/common/android/bookmark_id.cc
>   A components/bookmarks/common/android/component_jni_registrar.h
>   A components/bookmarks/common/android/component_jni_registrar.cc
>   A
>
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java
>   A
>
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkType.template
>   A components/bookmarks/common/bookmark_type.h
>   A + components/bookmarks/common/bookmark_type_list.h
>   M components/enhanced_bookmarks.gypi
>   M components/enhanced_bookmarks/DEPS
>   A + components/enhanced_bookmarks/android/OWNERS
>   A components/enhanced_bookmarks/android/component_jni_registrar.h
>   A components/enhanced_bookmarks/android/component_jni_registrar.cc
>   A components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h
>   A components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc
>   A
>
components/enhanced_bookmarks/android/java/src/org/chromium/components/enhancedbookmarks/EnhancedBookmarksBridge.java
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2014-08-12 17:49:11]

On 2014/08/12 16:32:01, sky wrote:
> What files do you need me to review here?
> 

I added you as the owner of these flies, (and the rest to Yaron)

chrome/browser/DEPS
chrome/chrome.gyp
components/bookmarks.gypi
components/bookmarks/DEPS
components/bookmarks/common/android/OWNERS
components/bookmarks/common/android/bookmark_id.cc
components/bookmarks/common/android/bookmark_id.h
components/bookmarks/common/android/component_jni_registrar.cc
components/bookmarks/common/android/component_jni_registrar.h
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkType.template
components/bookmarks/common/android/bookmark_type.h
components/bookmarks/common/android/bookmark_type_list.h

I think Yaron can review these files also as it's Android specific changes, but
it will require your lgtm.

[sky, 2014-08-12 19:49:29]

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_id.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: you've got an extra space here.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:4: 
Do we typically not use header guards for these files?

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:8: namespace android {
Do we typically introduce namespaces for android specific things?

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_type_list.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_type_list.h:13:
DEFINE_BOOKMARK_TYPE(PARTNER, 1)
Can you elaborate on why we need this type and what it's for? Especially why it
needs to be in the components directory.

[Yaron, 2014-08-12 20:15:50]

https://codereview.chromium.org/459973004/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/459973004/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:306:
public long getNativeBookmarkModel() {
Why do you need to expose this? Ideally we don't pass these around as ownership
becomes unlear.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_id.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:4: 
On 2014/08/12 19:49:28, sky wrote:
> Do we typically not use header guards for these files?

It should be there

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:8: namespace android {
On 2014/08/12 19:49:28, sky wrote:
> Do we typically introduce namespaces for android specific things?

I think it's fairly inconsistent :(

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java
(right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:17:
public static final int BOOKMARK_TYPE_NORMAL = 0;
Why generate the c++/native bindings if you're duplicating this here?

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:50:
private static boolean isValidBookmarkTypeFromChar(char c) {
What char? Is the char some encoding of the bookmark?

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:59:
public static BookmarkId getBookmarkIdFromString(String s) {
This and the above method seem like they have a very specific use in mind and
it's not clear that any string/char will suffice. Can you clarify what this is
for?

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right):

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:32:
DCHECK(type == BookmarkType::NORMAL);
DCHECK_EQ

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:49:
DCHECK(type == BookmarkType::NORMAL);
DCHECK_EQ

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:9: 
unused

[Kibeom Kim (inactive), 2014-08-12 22:33:49]

https://codereview.chromium.org/459973004/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/459973004/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:306:
public long getNativeBookmarkModel() {
On 2014/08/12 20:15:48, Yaron wrote:
> Why do you need to expose this? Ideally we don't pass these around as
ownership
> becomes unlear.

Native BookmarkModel instance was needed by EnhancedBookmarksBridge.java because
components/enhanced_bookmarks/metadata_accessor.h requires it.

I guess this signature "public static long getNativeBookmarkModel(Profile
profile);" is conceptually better.

Done.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_id.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/08/12 19:49:29, sky wrote:
> nit: you've got an extra space here.

Done.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_id.h:4: 
On 2014/08/12 20:15:49, Yaron wrote:
> On 2014/08/12 19:49:28, sky wrote:
> > Do we typically not use header guards for these files?
> 
> It should be there

Done.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_type_list.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_type_list.h:13:
DEFINE_BOOKMARK_TYPE(PARTNER, 1)
On 2014/08/12 19:49:29, sky wrote:
> Can you elaborate on why we need this type and what it's for? Especially why
it
> needs to be in the components directory.

First I needed to make BoomkarkId.java component because
EnhancedBookmarksBridge.java will be using that, and these constants were a part
of BookmarkId class.

In this CL, I'm using these constants in BookmarkId.java as
BookmarkType.BOOKMARK_TYPE_* , and also in enhanced_bookmarks_bridge.cc,
DCHECK_EQ(type, BookmarkType::NORMAL); .

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java
(right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:17:
public static final int BOOKMARK_TYPE_NORMAL = 0;
On 2014/08/12 20:15:49, Yaron wrote:
> Why generate the c++/native bindings if you're duplicating this here?

Done. (forgot to update here, also moved BookmarksBridge#INVALID_FOLDER_ID to
here.)

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:50:
private static boolean isValidBookmarkTypeFromChar(char c) {
On 2014/08/12 20:15:49, Yaron wrote:
> What char? Is the char some encoding of the bookmark?

commented below.

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/java/src/org/chromium/components/bookmarks/BookmarkId.java:59:
public static BookmarkId getBookmarkIdFromString(String s) {
On 2014/08/12 20:15:49, Yaron wrote:
> This and the above method seem like they have a very specific use in mind and
> it's not clear that any string/char will suffice. Can you clarify what this is
> for?

I just looked up. It's used in BookmarksPage.java and this string is for
bookmark page url, "chrome-native://bookmarks/#[[here]]".

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right):

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:32:
DCHECK(type == BookmarkType::NORMAL);
On 2014/08/12 20:15:50, Yaron wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:49:
DCHECK(type == BookmarkType::NORMAL);
On 2014/08/12 20:15:49, Yaron wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:9: 
On 2014/08/12 20:15:50, Yaron wrote:
> unused

Done.

[sky, 2014-08-12 23:14:52]

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
File components/bookmarks/common/android/bookmark_type_list.h (right):

https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
components/bookmarks/common/android/bookmark_type_list.h:13:
DEFINE_BOOKMARK_TYPE(PARTNER, 1)
On 2014/08/12 22:33:48, Kibeom Kim wrote:
> On 2014/08/12 19:49:29, sky wrote:
> > Can you elaborate on why we need this type and what it's for? Especially why
> it
> > needs to be in the components directory.
> 
> First I needed to make BoomkarkId.java component because
> EnhancedBookmarksBridge.java will be using that, and these constants were a
part
> of BookmarkId class.
> 
> In this CL, I'm using these constants in BookmarkId.java as
> BookmarkType.BOOKMARK_TYPE_* , and also in enhanced_bookmarks_bridge.cc,
> DCHECK_EQ(type, BookmarkType::NORMAL); .

You're describing where they are used. I'm asking what they are for.

[Kibeom Kim (inactive), 2014-08-12 23:28:31]

On 2014/08/12 23:14:52, sky wrote:
>
https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
> File components/bookmarks/common/android/bookmark_type_list.h (right):
> 
>
https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
> components/bookmarks/common/android/bookmark_type_list.h:13:
> DEFINE_BOOKMARK_TYPE(PARTNER, 1)
> On 2014/08/12 22:33:48, Kibeom Kim wrote:
> > On 2014/08/12 19:49:29, sky wrote:
> > > Can you elaborate on why we need this type and what it's for? Especially
why
> > it
> > > needs to be in the components directory.
> > 
> > First I needed to make BoomkarkId.java component because
> > EnhancedBookmarksBridge.java will be using that, and these constants were a
> part
> > of BookmarkId class.
> > 
> > In this CL, I'm using these constants in BookmarkId.java as
> > BookmarkType.BOOKMARK_TYPE_* , and also in enhanced_bookmarks_bridge.cc,
> > DCHECK_EQ(type, BookmarkType::NORMAL); .
> 
> You're describing where they are used. I'm asking what they are for.

This is to distinguish BookmarkModel's node id and PartnerBookmarkShim's node id
when interpreting BookmarkId#mId.

[sky, 2014-08-12 23:43:02]

Why do we need the two of them? And does this code really need to live
in enhanced_bookmarks? Is it going to remain android specific?

On Tue, Aug 12, 2014 at 4:28 PM,  <kkimlabs@chromium.org> wrote:
> On 2014/08/12 23:14:52, sky wrote:
>
>
https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
>>
>> File components/bookmarks/common/android/bookmark_type_list.h (right):
>
>
>
>
https://codereview.chromium.org/459973004/diff/40001/components/bookmarks/com...
>>
>> components/bookmarks/common/android/bookmark_type_list.h:13:
>> DEFINE_BOOKMARK_TYPE(PARTNER, 1)
>> On 2014/08/12 22:33:48, Kibeom Kim wrote:
>> > On 2014/08/12 19:49:29, sky wrote:
>> > > Can you elaborate on why we need this type and what it's for?
>> > > Especially
>
> why
>>
>> > it
>> > > needs to be in the components directory.
>> >
>> > First I needed to make BoomkarkId.java component because
>> > EnhancedBookmarksBridge.java will be using that, and these constants
>> > were a
>> part
>> > of BookmarkId class.
>> >
>> > In this CL, I'm using these constants in BookmarkId.java as
>> > BookmarkType.BOOKMARK_TYPE_* , and also in enhanced_bookmarks_bridge.cc,
>> > DCHECK_EQ(type, BookmarkType::NORMAL); .
>
>
>> You're describing where they are used. I'm asking what they are for.
>
>
> This is to distinguish BookmarkModel's node id and PartnerBookmarkShim's
> node id
> when interpreting BookmarkId#mId.
>
> https://codereview.chromium.org/459973004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Kibeom Kim (inactive), 2014-08-13 01:13:34]

On 2014/08/12 23:43:02, sky wrote:
> Why do we need the two of them? And does this code really need to live
> in enhanced_bookmarks? Is it going to remain android specific?
> 

Yes, this Java binding will remain Android specific, so technically it's not
necessary to make it component. Previous discussion is here
https://codereview.chromium.org/431843002/ . I'm OK with either ways.

I think the reason BookmarkId class represents two different types of ids is
that we want to treat them equally on Java UI code, as we simply put partner
bookmark root node under mobile node on Android. Instead, we have many type
branches in bookmarks_bridge.cc, and it uses BookmarkModel or
PartnerBookmarksShim depending on its type.

[Yaron, 2014-08-13 01:42:07]

On 2014/08/13 01:13:34, Kibeom Kim wrote:
> On 2014/08/12 23:43:02, sky wrote:
> > Why do we need the two of them? And does this code really need to live
> > in enhanced_bookmarks? Is it going to remain android specific?
> > 
Can you explain why it shouldn't be in enhanced_bookmarks even if it's
android-specific? Given that enhanced_bookmarks has several features being added
that are over and above what's in bookmarks, and that we've been asked to
minimize changes to the existing bookmarks code, I think isolating as much
functionality into the enhanced_bookmark component makes sense. Note that the
underlying model code (i.e. below the bridge) will be cross-platform. If and
when the code makes use of chrome services we could move it to
chrome/browser/enhanced_bookmarks, but I would think we'd componentize to the
extent possible, no?
> 
> Yes, this Java binding will remain Android specific, so technically it's not
> necessary to make it component. Previous discussion is here
> https://codereview.chromium.org/431843002/ . I'm OK with either ways.
> 
> I think the reason BookmarkId class represents two different types of ids is
> that we want to treat them equally on Java UI code, as we simply put partner
> bookmark root node under mobile node on Android. Instead, we have many type
> branches in bookmarks_bridge.cc, and it uses BookmarkModel or
> PartnerBookmarksShim depending on its type.

[sky, 2014-08-13 14:25:57]

On Tue, Aug 12, 2014 at 6:42 PM,  <yfriedman@chromium.org> wrote:
> On 2014/08/13 01:13:34, Kibeom Kim wrote:
>>
>> On 2014/08/12 23:43:02, sky wrote:
>> > Why do we need the two of them? And does this code really need to live
>> > in enhanced_bookmarks? Is it going to remain android specific?
>> >
>
> Can you explain why it shouldn't be in enhanced_bookmarks even if it's
> android-specific?

No reason. I'm just trying to understand the dependencies and where
this code is going to be used.

> Given that enhanced_bookmarks has several features being
> added
> that are over and above what's in bookmarks, and that we've been asked to
> minimize changes to the existing bookmarks code, I think isolating as much
> functionality into the enhanced_bookmark component makes sense. Note that
> the
> underlying model code (i.e. below the bridge) will be cross-platform. If and
> when the code makes use of chrome services we could move it to
> chrome/browser/enhanced_bookmarks, but I would think we'd componentize to
> the
> extent possible, no?

Yes, I agree.

  -Scott
>
>
>> Yes, this Java binding will remain Android specific, so technically it's
>> not
>> necessary to make it component. Previous discussion is here
>> https://codereview.chromium.org/431843002/ . I'm OK with either ways.
>
>
>> I think the reason BookmarkId class represents two different types of ids
>> is
>> that we want to treat them equally on Java UI code, as we simply put
>> partner
>> bookmark root node under mobile node on Android. Instead, we have many
>> type
>> branches in bookmarks_bridge.cc, and it uses BookmarkModel or
>> PartnerBookmarksShim depending on its type.
>
>
>
>
> https://codereview.chromium.org/459973004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-13 14:28:25]

On Tue, Aug 12, 2014 at 6:13 PM,  <kkimlabs@chromium.org> wrote:
> On 2014/08/12 23:43:02, sky wrote:
>>
>> Why do we need the two of them? And does this code really need to live
>> in enhanced_bookmarks? Is it going to remain android specific?
>
>
>
> Yes, this Java binding will remain Android specific, so technically it's not
> necessary to make it component. Previous discussion is here
> https://codereview.chromium.org/431843002/ . I'm OK with either ways.
>
> I think the reason BookmarkId class represents two different types of ids is
> that we want to treat them equally on Java UI code, as we simply put partner
> bookmark root node under mobile node on Android. Instead, we have many type
> branches in bookmarks_bridge.cc, and it uses BookmarkModel or
> PartnerBookmarksShim depending on its type.

I'm curious why the two types live in a different places. Seems
beneficial if they could be unified.

  -Scott
>
>
> https://codereview.chromium.org/459973004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-13 14:29:40]

I'm LGTMing, I still have a question (just sent), but that shouldn't hold up
landing this.

[Kibeom Kim (inactive), 2014-08-13 18:13:36]

On 2014/08/13 14:28:25, sky wrote:
> On Tue, Aug 12, 2014 at 6:13 PM, 
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=kkimlabs@chromium.org>
wrote:
> > On 2014/08/12 23:43:02, sky wrote:
> >>
> >> Why do we need the two of them? And does this code really need to live
> >> in enhanced_bookmarks? Is it going to remain android specific?
> >
> >
> >
> > Yes, this Java binding will remain Android specific, so technically it's not
> > necessary to make it component. Previous discussion is here
> > https://codereview.chromium.org/431843002/ . I'm OK with either ways.
> >
> > I think the reason BookmarkId class represents two different types of ids is
> > that we want to treat them equally on Java UI code, as we simply put partner
> > bookmark root node under mobile node on Android. Instead, we have many type
> > branches in bookmarks_bridge.cc, and it uses BookmarkModel or
> > PartnerBookmarksShim depending on its type.
> 
> I'm curious why the two types live in a different places. Seems
> beneficial if they could be unified.
> 
>   -Scott

Yes I think so too, it will be nice. I suspect that partner bookmark can take
advantage of ChromeBookmarkClient, but I haven't looked onto those two in detail
myself.

+cc aruslan (partner bookmark feature owner)
+cc joaodasilva

[Yaron, 2014-08-13 20:39:48]

lgtm

https://codereview.chromium.org/459973004/diff/60001/components/bookmarks/com...
File components/bookmarks/common/android/OWNERS (right):

https://codereview.chromium.org/459973004/diff/60001/components/bookmarks/com...
components/bookmarks/common/android/OWNERS:2: tedchoc@chromium.org
you can add yourself

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/OWNERS (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/OWNERS:1: yfriedman@chromium.org
here too

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:34: const
BookmarkNode *node = bookmarks::GetBookmarkNodeByID(
nit: const BookmarkNode* node (below too)

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:12: using
base::android::ScopedJavaLocalRef;
nit: inline

[Kibeom Kim (inactive), 2014-08-14 01:04:12]

https://codereview.chromium.org/459973004/diff/60001/components/bookmarks/com...
File components/bookmarks/common/android/OWNERS (right):

https://codereview.chromium.org/459973004/diff/60001/components/bookmarks/com...
components/bookmarks/common/android/OWNERS:2: tedchoc@chromium.org
On 2014/08/13 20:39:48, Yaron wrote:
> you can add yourself

Done.

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/OWNERS (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/OWNERS:1: yfriedman@chromium.org
On 2014/08/13 20:39:48, Yaron wrote:
> here too

Done.

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:34: const
BookmarkNode *node = bookmarks::GetBookmarkNodeByID(
On 2014/08/13 20:39:48, Yaron wrote:
> nit: const BookmarkNode* node (below too)

Done.

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h (right):

https://codereview.chromium.org/459973004/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/android/enhanced_bookmarks_bridge.h:12: using
base::android::ScopedJavaLocalRef;
On 2014/08/13 20:39:48, Yaron wrote:
> nit: inline

Done.

[Kibeom Kim (inactive), 2014-08-14 01:04:25]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-08-14 01:06:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/459973004/120001

[Kibeom Kim (inactive), 2014-08-14 06:23:32]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2014-08-14 07:24:20]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-08-14 07:25:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/459973004/140001

[Kibeom Kim (inactive), 2014-08-14 07:30:46]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2014-08-14 17:39:54]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:42:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/459973004/140001

[Kibeom Kim (inactive), 2014-08-14 18:50:32]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2014-08-14 20:52:40]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-08-14 20:56:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/459973004/140001

[commit-bot: I haz the power, 2014-08-14 20:59:27]

Committed patchset #8 (140001) as 289695