Add the android partner bookmark shim. (issue 10834237)

[ted...@chromium.org]

Reviewers: Yaron, aruslan, John Abd-El-Malek,

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

Description:
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


Please review this at http://codereview.chromium.org/10834237/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
A chrome/browser/android/partner_bookmarks_shim.h
A chrome/browser/android/partner_bookmarks_shim.cc
A chrome/browser/android/partner_bookmarks_shim_unittest.cc
M chrome/chrome_browser.gypi
M chrome/chrome_tests.gypi

[aru...@chromium.org]

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

http://codereview.chromium.org/10834237/

[yfri...@chromium.org]

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_bookmarks_shim.h
File chrome/browser/android/partner_bookmarks_shim.h (right):

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

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

+sky - chrome OWNERS
-jam

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

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

http://codereview.chromium.org/10834237/diff/1/chrome/browser/android/partner_bookmarks_shim.h#newcode9
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.

http://codereview.chromium.org/10834237/

[s...@chromium.org]

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

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

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.

http://codereview.chromium.org/10834237/

[s...@chromium.org]

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/partner_bookmarks_shim.cc
File chrome/browser/android/partner_bookmarks_shim.cc (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.cc#newcode76
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/partner_bookmarks_shim.cc#newcode112
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/partner_bookmarks_shim.h
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.h#newcode25
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/partner_bookmarks_shim.h#newcode28
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/partner_bookmarks_shim.h#newcode34
chrome/browser/android/partner_bookmarks_shim.h:34: void Reset();
Add documentation.

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.h#newcode37
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/partner_bookmarks_shim.h#newcode45
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/partner_bookmarks_shim.h#newcode61
chrome/browser/android/partner_bookmarks_shim.h:61: const BookmarkNode*
GetAttachPoint() const { return attach_point_; }
Trivial accessors should be named used unix_hacker_style.

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

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/partner_bookmarks_shim.cc
File chrome/browser/android/partner_bookmarks_shim.cc (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.cc#newcode76
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/partner_bookmarks_shim.cc#newcode112
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/partner_bookmarks_shim.h
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.h#newcode25
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/partner_bookmarks_shim.h#newcode28
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/partner_bookmarks_shim.h#newcode34
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/partner_bookmarks_shim.h#newcode37
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/partner_bookmarks_shim.h#newcode45
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/partner_bookmarks_shim.h#newcode61
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.

http://codereview.chromium.org/10834237/

[aru...@chromium.org]

>> 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).

http://codereview.chromium.org/10834237/

[s...@chromium.org]

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/partner_bookmarks_shim.h
File chrome/browser/android/partner_bookmarks_shim.h (right):

http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.h#newcode25
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.

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

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/partner_bookmarks_shim.h
> File chrome/browser/android/partner_bookmarks_shim.h (right):


http://codereview.chromium.org/10834237/diff/4003/chrome/browser/android/partner_bookmarks_shim.h#newcode25
> 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.

http://codereview.chromium.org/10834237/

[ted...@chromium.org]

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/

[Scott Violet]

Yes. And if this is temporary, LGTM

[scoobysna...@gmail.com]

How can I get rid of this partner bookmark bug 138236

[decoy...@gmail.com]

What does this mean? U guys been watching me beat off all this time?

[freemanalex...@gmail.com]

What the fucking fuck