Bring up of the metadata accessors for enhanced bookmarks.

Bring up of the metadata accessors for enhanced bookmarks.

BUG=None

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

[Kibeom Kim (inactive), 2014-06-18 17:44:58]

thanks! (but I'm not OWNER)

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
components/components_tests.gyp:103:
'enhanced_bookmarks/metadata_accessor_unittest.cc',
below image_store_unittest.cc?

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
components/components_tests.gyp:264: 'components.gyp:enhanced_bookmarks_proto',
isn't this already included as 'components.gyp:enhanced_bookmarks' 's
dependency?

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks.gypi (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks.gypi:22: 'enhanced_bookmarks/metadata_accessor.h',
both lines below image_store_util.h?

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor.cc (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor.cc:136: // This method consumes
the raw pointer.
I think mentioning passing the ownership is more clear.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor.h (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor.h:33: const BookmarkNode* node);
This and other function's |node| argument isn't expected to be null so I think
|const BookmarkNode&| type is preferable according to google styleguide.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor_unittest.cc (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor_unittest.cc:18: const
std::string bookmark_url("http://example.com/index.html");
BOOKMARK_URL

[noyau (Ping after 24h), 2014-06-19 09:07:00]

It appear I am owner of this! Did you made me owner of this?

Adding blundell@ for the component gyp changes

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
components/components_tests.gyp:103:
'enhanced_bookmarks/metadata_accessor_unittest.cc',
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> below image_store_unittest.cc?

Ha, I renamed the class and forgot to reorder everywhere. Done.

https://codereview.chromium.org/336263003/diff/40001/components/components_te...
components/components_tests.gyp:264: 'components.gyp:enhanced_bookmarks_proto',
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> isn't this already included as 'components.gyp:enhanced_bookmarks' 's
> dependency?

Done.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks.gypi (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks.gypi:22: 'enhanced_bookmarks/metadata_accessor.h',
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> both lines below image_store_util.h?

Done.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor.cc (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor.cc:136: // This method consumes
the raw pointer.
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> I think mentioning passing the ownership is more clear.
Removed the confusing comments and made the passing of ownership explicit using
a scoped_ptr.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor.h (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor.h:33: const BookmarkNode* node);
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> This and other function's |node| argument isn't expected to be null so I think
> |const BookmarkNode&| type is preferable according to google styleguide.

This mimic the BookmarkModel API where the bookmark nodes are always passed by
pointer. I went for consistency as this API is an add-on to the bookmark API.

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
File components/enhanced_bookmarks/metadata_accessor_unittest.cc (right):

https://codereview.chromium.org/336263003/diff/40001/components/enhanced_book...
components/enhanced_bookmarks/metadata_accessor_unittest.cc:18: const
std::string bookmark_url("http://example.com/index.html");
On 2014/06/18 17:44:57, Kibeom Kim wrote:
> BOOKMARK_URL

Done.

[blundell, 2014-06-19 15:54:03]

lgtm

[Kibeom Kim (inactive), 2014-06-19 17:02:24]

LGTM

[noyau (Ping after 24h), 2014-06-19 22:18:13]

The CQ bit was checked by noyau@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:18:46]

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

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

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 09:47:42]

Change committed as 278656