Set metainfo ID for enhanced bookmarks extension

Set metainfo ID for enhanced bookmarks extension

This is a temporary workaround to allow for reliable creation of a
durable unique ID for the extension. It will go away once we are able to
provide control of all bookmark creation paths via the extension API.

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

[Mike Wittman, 2014-06-13 17:56:05]

PTAL

kalman: chrome/browser/extensions
sky: components/bookmarks

[not at google - send to devlin, 2014-06-13 18:00:12]

me --> yoz

[Yoyo Zhou, 2014-06-13 23:54:00]

I've read the bug and I'm not sure how this addresses the issue that different
Chrome instances might use different IDs for the same bookmark. The code as
described here LGTM though, with a few nits.

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:105: static const char
*enhanced_extension_hashes[] = {
Comment what extensions these represent.

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:144: // Use
[0..range_mid) for bookmarks, [range_mid..2*range_mid) for folders.
Why is this necessary? They have different prefixes already.

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
File components/bookmarks/browser/bookmark_model_observer.h (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
components/bookmarks/browser/bookmark_model_observer.h:35: virtual void
BookmarkNodeAdding(BookmarkModel* model,
OnWillAddBookmarkNode is more consistent with the others in this interface.

[Mike Wittman, 2014-06-14 00:49:10]

On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> I've read the bug and I'm not sure how this addresses the issue that different
> Chrome instances might use different IDs for the same bookmark. The code as
> described here LGTM though, with a few nits.

The key part is assigning the metainfo ID before the bookmark is synced. When
the bookmark is synced to other Chrome instances, the enhanced bookmark
extensions in those instances are notified via OnBookmarkCreated and can check
for the existence of the metainfo ID and, if present, not attempt to assign a
new ID.


https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:105: static const char
*enhanced_extension_hashes[] = {
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> Comment what extensions these represent.

Done.

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:144: // Use
[0..range_mid) for bookmarks, [range_mid..2*range_mid) for folders.
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> Why is this necessary? They have different prefixes already.

To match the existing extension and server-side behavior. Not sure why it was
done this way in the first place.

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
File components/bookmarks/browser/bookmark_model_observer.h (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
components/bookmarks/browser/bookmark_model_observer.h:35: virtual void
BookmarkNodeAdding(BookmarkModel* model,
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> OnWillAddBookmarkNode is more consistent with the others in this interface.

Changed to OnWillAddBookmarkNode.

[Mike Wittman, 2014-06-14 00:49:55]

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:105: static const char
*enhanced_extension_hashes[] = {
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> Comment what extensions these represent.

Done.

https://chromiumcodereview.appspot.com/338593002/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:144: // Use
[0..range_mid) for bookmarks, [range_mid..2*range_mid) for folders.
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> Why is this necessary? They have different prefixes already.

To match the existing extension and server-side behavior. Not sure why it was
done this way in the first place.

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
File components/bookmarks/browser/bookmark_model_observer.h (right):

https://chromiumcodereview.appspot.com/338593002/diff/40001/components/bookma...
components/bookmarks/browser/bookmark_model_observer.h:35: virtual void
BookmarkNodeAdding(BookmarkModel* model,
On 2014/06/13 23:54:00, Yoyo Zhou wrote:
> OnWillAddBookmarkNode is more consistent with the others in this interface.

Changed to OnWillAddBookmarkNode.

[Yoyo Zhou, 2014-06-14 00:52:25]

LGTM

https://chromiumcodereview.appspot.com/338593002/diff/60001/chrome/browser/ex...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://chromiumcodereview.appspot.com/338593002/diff/60001/chrome/browser/ex...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:108:
"D5736E4B5CF695CB93A2FB57E4FDC6E5AFAB6FE2",
What I mean is inline comments like

"D57..." // Release version
(e.g. see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...)

[sky, 2014-06-16 14:57:22]

https://codereview.chromium.org/338593002/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model_observer.h (right):

https://codereview.chromium.org/338593002/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_observer.h:35: virtual void
OnWillAddBookmarkNode(BookmarkModel* model,
Add test coverage of this.

[Mike Wittman, 2014-06-16 18:47:59]

https://codereview.chromium.org/338593002/diff/60001/components/bookmarks/bro...
File components/bookmarks/browser/bookmark_model_observer.h (right):

https://codereview.chromium.org/338593002/diff/60001/components/bookmarks/bro...
components/bookmarks/browser/bookmark_model_observer.h:35: virtual void
OnWillAddBookmarkNode(BookmarkModel* model,
On 2014/06/16 14:57:22, sky wrote:
> Add test coverage of this.

Done.

[sky, 2014-06-16 19:54:55]

LGTM

[Mike Wittman, 2014-06-16 21:16:18]

https://codereview.chromium.org/338593002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/bookmarks/bookmarks_api.cc (right):

https://codereview.chromium.org/338593002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/bookmarks/bookmarks_api.cc:108:
"D5736E4B5CF695CB93A2FB57E4FDC6E5AFAB6FE2",
On 2014/06/14 00:52:24, Yoyo Zhou wrote:
> What I mean is inline comments like
> 
> "D57..." // Release version
> (e.g. see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...)

Done.

[Mike Wittman, 2014-06-16 21:16:23]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:17:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wittman@chromium.org/338593002/100001

[commit-bot: I haz the power, 2014-06-17 03:03:09]

Change committed as 277635