Add the android partner bookmark shim.

Add the android partner bookmark shim.

This is used android's NTP bookmark page to inject partner
bookmarks on top of the user's bookmark model without modifying thiers.

BUG=138236
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151509

[Ted C, 2012-08-08 22:50:53]

yfriedman && aruslan - android/ code bits
yfriedman - android/ OWNERS
jam - OWNERS for chrome*.gypi files

[aruslan, 2012-08-08 22:52:32]

Thanks! Looks good to me (I'm not a committer).

[Yaron, 2012-08-08 22:54:54]

lgtm but I'd recommend brettw or sky as opposed to jam. Doesn't hurt to have a
bookmark OWNER take a look :)

http://codereview.chromium.org/10834237/diff/1/chrome/browser/android/partner...
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/1/chrome/browser/android/partner...
chrome/browser/android/partner_bookmarks_shim.h:9: // scoped_ptr requires
complete class BookmarkNode, so we don't forward declare
This comment seems unnecessary.

[Ted C, 2012-08-08 23:00:39]

+sky - chrome OWNERS
-jam

[Ted C, 2012-08-08 23:01:48]

http://codereview.chromium.org/10834237/diff/1/chrome/browser/android/partner...
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/1/chrome/browser/android/partner...
chrome/browser/android/partner_bookmarks_shim.h:9: // scoped_ptr requires
complete class BookmarkNode, so we don't forward declare
On 2012/08/08 22:54:55, Yaron wrote:
> This comment seems unnecessary.

Done.

[sky, 2012-08-09 00:26:44]

gyp files LGTM. Are you asking for a review of the android files too?

[Ted C, 2012-08-09 00:45:52]

On 2012/08/09 00:26:44, sky wrote:
> gyp files LGTM. Are you asking for a review of the android files too?

A glance for glaring issues would be appreciated, but not necessary if you have
a huge backlog.

[sky, 2012-08-09 15:48:19]

Doesn't this mean you can't really iterate through the bookmark tree to find the
partner bookmarks? And similarly doesn't it mean if you have a partner bookmark
you can't walk up the tree? This will have a lot of ramifications for existing
code that uses the bookmarkmodel.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
File chrome/browser/android/partner_bookmarks_shim.cc (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.cc:76: attach_point_ =
attach_point;
Is it possible for someone to delete this bookmark after created?

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.cc:112: for (int i= 0, child_count
= parent->child_count(); i < child_count; ++i) {
'i=' -> 'i ='

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
kPartnerBookmarkIdBits;
How do you know that a valid bookmark can't end up with this id?

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:28: static PartnerBookmarksShim*
GetInstance();
constructor/destructor before other methods.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:34: void Reset();
Add documentation.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:37: void AttachTo(BookmarkModel*
bookmark_model,
I think this class should be a profilekeyedservice and be attached to a specific
bookmarkmodel. Much less error prone that way.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:45: struct Observer {
This should be a class.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:61: const BookmarkNode*
GetAttachPoint() const { return attach_point_; }
Trivial accessors should be named used unix_hacker_style.

[Ted C, 2012-08-09 16:47:01]

I guess I should have been more clear about the scope of this class.

Partner bookmarks should only appear if the consumer is explicitly aware of
their existence.  I strongly suspect the only place this will get used is in
android's bookmark NTP handler that needs to expose these partner bookmarks.

If you request the model for a profile, we do not want you to have to worry
about these partner bookmarks at all.

As for traversing, it is only needs to be possible in combination with the shim.
 We expose get_attach_point for consumers to be aware of when they should inject
these partner bookmarks to the user (i.e. we do this as we are exporting the
model to json for the webui).

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
File chrome/browser/android/partner_bookmarks_shim.cc (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.cc:76: attach_point_ =
attach_point;
On 2012/08/09 15:48:19, sky wrote:
> Is it possible for someone to delete this bookmark after created?

Really the only use case we should support is to allow attaching to permanent
nodes, I'll add a DCHECK for that.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.cc:112: for (int i= 0, child_count
= parent->child_count(); i < child_count; ++i) {
On 2012/08/09 15:48:19, sky wrote:
> 'i=' -> 'i ='

Done.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
kPartnerBookmarkIdBits;
On 2012/08/09 15:48:19, sky wrote:
> How do you know that a valid bookmark can't end up with this id?

@Ruslan, any reason you chose this number originally?

If not, is there any part of the ID space that is assured to never have
bookmarks?  Does the model use any negative IDs?

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:28: static PartnerBookmarksShim*
GetInstance();
On 2012/08/09 15:48:19, sky wrote:
> constructor/destructor before other methods.

Done.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:34: void Reset();
On 2012/08/09 15:48:19, sky wrote:
> Add documentation.

Done.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:37: void AttachTo(BookmarkModel*
bookmark_model,
On 2012/08/09 15:48:19, sky wrote:
> I think this class should be a profilekeyedservice and be attached to a
specific
> bookmarkmodel. Much less error prone that way.

will look into that next.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:45: struct Observer {
On 2012/08/09 15:48:19, sky wrote:
> This should be a class.

Done.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:61: const BookmarkNode*
GetAttachPoint() const { return attach_point_; }
On 2012/08/09 15:48:19, sky wrote:
> Trivial accessors should be named used unix_hacker_style.

Done.

[aruslan, 2012-08-09 17:31:44]

>> How do you know that a valid bookmark can't end up with this id?
> @Ruslan, any reason you chose this number originally?
> If not, is there any part of the ID space that is
> assured to never have bookmarks?
> Does the model use any negative IDs?

Model starts with ID 1 and increment the ID on any AddFolder/URL; similarly for
the codec without checking if the ID space is exhausted.  The ID space gets
defragmented (ie all IDs are reassigned to be sequential) on every load.
Negative IDs are not used within the bookmarks model.
Note that the ID space is 64-bit.

The shim (being the only entity layered on top of the bookmarks model) limits
the ID space to 32-bit and grabs the upper 64K of it.
Currently, it is possible for the bookmarks model to exhaust the limited ID
space under stress load that doesn't incur re-loading.  In this case, the shim
will effectively hide and disable editing of nodes with the conflicting IDs.

We can move the limit definition from the shim into the bookmarks model and
attempt to enforce it, and/or we can shift the grabbed ID space to the upper
limit of 52-bit ID space (instead of 32-bit).

[sky, 2012-08-09 19:41:11]

Why not inject these bookmarks directly into the model?
The code that encodes the bookmarks could be made aware of them so they aren't
written to disk.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
kPartnerBookmarkIdBits;
On 2012/08/09 16:47:01, Ted C wrote:
> On 2012/08/09 15:48:19, sky wrote:
> > How do you know that a valid bookmark can't end up with this id?
> 
> @Ruslan, any reason you chose this number originally?
> 
> If not, is there any part of the ID space that is assured to never have
> bookmarks?  Does the model use any negative IDs?

We don't restrict the ids in anyway.

[Ted C, 2012-08-09 22:54:58]

On 2012/08/09 19:41:11, sky wrote:
> Why not inject these bookmarks directly into the model?
> The code that encodes the bookmarks could be made aware of them so they aren't
> written to disk.
>

Adding these bookmarks to the model seemed like a more involved process than we
wanted for something that will probably soon be moved to java code anyway (as we
are rewriting all the NTP in java in the "near" future).

The use cases I was concerned about dealing with were:
- ensuring they didn't get persisted to disk
- ensuring they didn't get synced
- ensuring they couldn't be edited by other sources
- ensuring a slow loading partner bookmarks provider (that in theory could be
making long running network requests) did not block the model from finishing
loading and notifying all observers

The shim approach isolated the pain to just the NTP as it was the only portion
of the system that needed to be aware of these bookmarks.

>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
> File chrome/browser/android/partner_bookmarks_shim.h (right):
> 
>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
> chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
> kPartnerBookmarkIdBits;
> On 2012/08/09 16:47:01, Ted C wrote:
> > On 2012/08/09 15:48:19, sky wrote:
> > > How do you know that a valid bookmark can't end up with this id?
> > 
> > @Ruslan, any reason you chose this number originally?
> > 
> > If not, is there any part of the ID space that is assured to never have
> > bookmarks?  Does the model use any negative IDs?
> 
> We don't restrict the ids in anyway.

I think I can isolate the ID namespaces by changing our handling of these in the
NTP, so maybe this will be less of an issue.  Since we know a BookmarkNode is a
partner bookmark because it's ancestor is the partner bookmark root node, we can
check by following it's parent chain to determine if it ever hits the node root.

Then when we serialize the nodes to json, we can just mark them as partner
bookmarks and know that the ID space is separate.

[Ted C, 2012-08-09 23:30:08]

On 2012/08/09 22:54:58, Ted C wrote:
> On 2012/08/09 19:41:11, sky wrote:
> > Why not inject these bookmarks directly into the model?
> > The code that encodes the bookmarks could be made aware of them so they
aren't
> > written to disk.
> >
> 
> Adding these bookmarks to the model seemed like a more involved process than
we
> wanted for something that will probably soon be moved to java code anyway (as
we
> are rewriting all the NTP in java in the "near" future).
> 
> The use cases I was concerned about dealing with were:
> - ensuring they didn't get persisted to disk
> - ensuring they didn't get synced
> - ensuring they couldn't be edited by other sources
> - ensuring a slow loading partner bookmarks provider (that in theory could be
> making long running network requests) did not block the model from finishing
> loading and notifying all observers
> 
> The shim approach isolated the pain to just the NTP as it was the only portion
> of the system that needed to be aware of these bookmarks.
> 
> >
>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
> > File chrome/browser/android/partner_bookmarks_shim.h (right):
> > 
> >
>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
> > chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
> > kPartnerBookmarkIdBits;
> > On 2012/08/09 16:47:01, Ted C wrote:
> > > On 2012/08/09 15:48:19, sky wrote:
> > > > How do you know that a valid bookmark can't end up with this id?
> > > 
> > > @Ruslan, any reason you chose this number originally?
> > > 
> > > If not, is there any part of the ID space that is assured to never have
> > > bookmarks?  Does the model use any negative IDs?
> > 
> > We don't restrict the ids in anyway.
> 
> I think I can isolate the ID namespaces by changing our handling of these in
the
> NTP, so maybe this will be less of an issue.  Since we know a BookmarkNode is
a
> partner bookmark because it's ancestor is the partner bookmark root node, we
can
> check by following it's parent chain to determine if it ever hits the node
root.
> 
> Then when we serialize the nodes to json, we can just mark them as partner
> bookmarks and know that the ID space is separate.

Maybe I should move this to the soon to be created ntp_android directory so it
is clearer to only apply to that?

[sky, 2012-08-09 23:42:21]

Yes. And if this is temporary, LGTM

On Thu, Aug 9, 2012 at 4:30 PM,  <tedchoc@chromium.org> wrote:
> On 2012/08/09 22:54:58, Ted C wrote:
>>
>> On 2012/08/09 19:41:11, sky wrote:
>> > Why not inject these bookmarks directly into the model?
>> > The code that encodes the bookmarks could be made aware of them so they
>
> aren't
>>
>> > written to disk.
>> >
>
>
>> Adding these bookmarks to the model seemed like a more involved process
>> than
>
> we
>>
>> wanted for something that will probably soon be moved to java code anyway
>> (as
>
> we
>>
>> are rewriting all the NTP in java in the "near" future).
>
>
>> The use cases I was concerned about dealing with were:
>> - ensuring they didn't get persisted to disk
>> - ensuring they didn't get synced
>> - ensuring they couldn't be edited by other sources
>> - ensuring a slow loading partner bookmarks provider (that in theory could
>> be
>> making long running network requests) did not block the model from
>> finishing
>> loading and notifying all observers
>
>
>> The shim approach isolated the pain to just the NTP as it was the only
>> portion
>> of the system that needed to be aware of these bookmarks.
>
>
>> >
>
>
>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
>>
>> > File chrome/browser/android/partner_bookmarks_shim.h (right):
>> >
>> >
>
>
>
http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/part...
>>
>> > chrome/browser/android/partner_bookmarks_shim.h:25: static const int64
>> > kPartnerBookmarkIdBits;
>> > On 2012/08/09 16:47:01, Ted C wrote:
>> > > On 2012/08/09 15:48:19, sky wrote:
>> > > > How do you know that a valid bookmark can't end up with this id?
>> > >
>> > > @Ruslan, any reason you chose this number originally?
>> > >
>> > > If not, is there any part of the ID space that is assured to never
>> > > have
>> > > bookmarks?  Does the model use any negative IDs?
>> >
>> > We don't restrict the ids in anyway.
>
>
>> I think I can isolate the ID namespaces by changing our handling of these
>> in
>
> the
>>
>> NTP, so maybe this will be less of an issue.  Since we know a BookmarkNode
>> is
>
> a
>>
>> partner bookmark because it's ancestor is the partner bookmark root node,
>> we
>
> can
>>
>> check by following it's parent chain to determine if it ever hits the node
>
> root.
>
>> Then when we serialize the nodes to json, we can just mark them as partner
>> bookmarks and know that the ID space is separate.
>
>
> Maybe I should move this to the soon to be created ntp_android directory so
> it
> is clearer to only apply to that?
>
> http://codereview.chromium.org/10834237/

[Ted C, 2012-08-14 00:04:16]

Cleaned it up and moved to the ntp/android directory.  I removed the need for
the ID mask and I pushed the logic of keeping the logical separation of the two
trees to our NTP handler that I will be upstreaming after this.

[commit-bot: I haz the power, 2012-08-14 15:45:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/10834237/3005

[commit-bot: I haz the power, 2012-08-14 18:02:44]

Change committed as 151509