Fix BookmarkNode MetaInfoMap copypasting for Mac

Fix BookmarkNode MetaInfoMap copypasting for Mac

Bookmark node meta info APIs is exposed Chrome to the Bookmark
Manager extension for storing extra meta data for bookmarks.

This API is also used by
//components/enhanced_bookmarks/metadata_accessor.cc to
store/access extra meta data such as associated image URL for
a bookmark.

Without this patch the meta data stored along with the bookmark
will be lost when it has been cut/pasted.

The same functionality works perfectly fine on non-Mac platforms,
here we add the missing implementation for Mac.

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

[Jiang Jiang, 2014-07-30 13:00:33]

Can any of you take a look at this? Since avi is the one who last touched this
code and sky is in the OWNERS of //components/bookmarks.

This is surprisingly omitted for Mac (implemented for other platforms already).

[sky, 2014-07-30 17:17:48]

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm (right):

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm:160:
NSDictionary* DictionaryFromBookmarkMetaInfo(
I'm surprised we don't have some general function that takes a
std::map<string,string> and does this for you already. Similarly for
MetaInfoMapFromDictionary.

[Avi (use Gerrit), 2014-07-30 17:37:40]

Is there a bug for this? Can you put that in the "BUG=" line? I'm not sure what
this fixes. (I'm still reading the CL.)

[Avi (use Gerrit), 2014-07-30 17:45:10]

This LGTM. You should totally link in a bug to explain why.

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm (right):

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm:163:
[[NSMutableDictionary alloc] initWithCapacity:meta_info_map.size()];
You can do

NSMutableDictionary* dictionary = [NSMutableDictionary dictionary];

(like line 176 below) and avoid autoreleasing manually on line 171. A bit
cleaner.

[Jiang Jiang, 2014-07-30 19:37:43]

On 2014/07/30 17:37:40, Avi wrote:
> Is there a bug for this? Can you put that in the "BUG=" line? I'm not sure
what
> this fixes. (I'm still reading the CL.)

Good question, we are using bookmark node meta info here in Opera to store image
thumbnails, this SetMetaInfo API is also exposed (by both Chrome and Opera) to
Bookmark Manager extension for storing extra meta data.

This API is also used by
https://chromium.googlesource.com/chromium/src/+/master/components/enhanced_b...
to store/access extra meta data such as associated image URL for a bookmark.
(This enhanced_bookmarks component is not used by Opera but I suppose Chrome has
use for it.)

Without this patch the meta data stored along with the bookmark will be lost
when it has been cut/pasted.

Since it works perfectly fine on non-Mac platforms I see no reason not to
support it and upstream this patch.

Is the above reason sufficient to make a bug?

[Avi (use Gerrit), 2014-07-30 19:41:02]

On 2014/07/30 19:37:43, Jiang wrote:
> On 2014/07/30 17:37:40, Avi wrote:
> > Is there a bug for this? Can you put that in the "BUG=" line? I'm not sure
> what
> > this fixes. (I'm still reading the CL.)
> 
> Good question, we are using bookmark node meta info here in Opera to store
image
> thumbnails, this SetMetaInfo API is also exposed (by both Chrome and Opera) to
> Bookmark Manager extension for storing extra meta data.
> 
> This API is also used by
>
https://chromium.googlesource.com/chromium/src/+/master/components/enhanced_b...
> to store/access extra meta data such as associated image URL for a bookmark.
> (This enhanced_bookmarks component is not used by Opera but I suppose Chrome
has
> use for it.)
> 
> Without this patch the meta data stored along with the bookmark will be lost
> when it has been cut/pasted.
> 
> Since it works perfectly fine on non-Mac platforms I see no reason not to
> support it and upstream this patch.
> 
> Is the above reason sufficient to make a bug?

Yes. It can be exactly what you wrote here:

Title: Chrome Mac drops metadata on cut/paste
Body: (what you wrote as to why it matters)

Bug titles say what the change does. _Why_ goes in a bug. When, years from now,
people want to know why you made a change, they go to the bug. Any CL with
complexity more than fixing a typo in a comment needs a corresponding bug.

[Jiang Jiang, 2014-07-30 19:51:50]

On 2014/07/30 19:41:02, Avi wrote:
> On 2014/07/30 19:37:43, Jiang wrote:
> > On 2014/07/30 17:37:40, Avi wrote:
> > > Is there a bug for this? Can you put that in the "BUG=" line? I'm not sure
> > what
> > > this fixes. (I'm still reading the CL.)
> > 
> > Good question, we are using bookmark node meta info here in Opera to store
> image
> > thumbnails, this SetMetaInfo API is also exposed (by both Chrome and Opera)
to
> > Bookmark Manager extension for storing extra meta data.
> > 
> > This API is also used by
> >
>
https://chromium.googlesource.com/chromium/src/+/master/components/enhanced_b...
> > to store/access extra meta data such as associated image URL for a bookmark.
> > (This enhanced_bookmarks component is not used by Opera but I suppose Chrome
> has
> > use for it.)
> > 
> > Without this patch the meta data stored along with the bookmark will be lost
> > when it has been cut/pasted.
> > 
> > Since it works perfectly fine on non-Mac platforms I see no reason not to
> > support it and upstream this patch.
> > 
> > Is the above reason sufficient to make a bug?
> 
> Yes. It can be exactly what you wrote here:
> 
> Title: Chrome Mac drops metadata on cut/paste
> Body: (what you wrote as to why it matters)
> 
> Bug titles say what the change does. _Why_ goes in a bug. When, years from
now,
> people want to know why you made a change, they go to the bug. Any CL with
> complexity more than fixing a typo in a comment needs a corresponding bug.

Done, CL description also updated.

[Avi (use Gerrit), 2014-07-30 19:55:28]

SLGTM

[Jiang Jiang, 2014-07-31 10:05:56]

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
File components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm (right):

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm:160:
NSDictionary* DictionaryFromBookmarkMetaInfo(
On 2014/07/30 17:17:48, sky wrote:
> I'm surprised we don't have some general function that takes a
> std::map<string,string> and does this for you already. Similarly for
> MetaInfoMapFromDictionary.

Can't find anything in base at least.

https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm:163:
[[NSMutableDictionary alloc] initWithCapacity:meta_info_map.size()];
On 2014/07/30 17:45:10, Avi wrote:
> You can do
> 
> NSMutableDictionary* dictionary = [NSMutableDictionary dictionary];
> 
> (like line 176 below) and avoid autoreleasing manually on line 171. A bit
> cleaner.

Done.

[Jiang Jiang, 2014-07-31 10:06:03]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-07-31 10:08:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/428183004/40001

[commit-bot: I haz the power, 2014-07-31 13:27:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/35850)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[commit-bot: I haz the power, 2014-07-31 13:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-31 13:30:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Jiang Jiang, 2014-07-31 13:33:31]

The CQ bit was checked by jiangj@opera.com

[Jiang Jiang, 2014-07-31 13:33:51]

The CQ bit was unchecked by jiangj@opera.com

[Jiang Jiang, 2014-07-31 15:26:05]

On 2014/07/30 17:17:48, sky wrote:
>
https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
> File components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm (right):
> 
>
https://codereview.chromium.org/428183004/diff/1/components/bookmarks/browser...
> components/bookmarks/browser/bookmark_pasteboard_helper_mac.mm:160:
> NSDictionary* DictionaryFromBookmarkMetaInfo(
> I'm surprised we don't have some general function that takes a
> std::map<string,string> and does this for you already. Similarly for
> MetaInfoMapFromDictionary.

Looks like I still need LGTM from OWNER, can you please take another look?

[sky, 2014-07-31 15:45:34]

LGTM

[Jiang Jiang, 2014-07-31 15:47:51]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-07-31 15:49:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/428183004/80001

[commit-bot: I haz the power, 2014-07-31 21:31:35]

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

[commit-bot: I haz the power, 2014-08-01 10:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 10:24:59]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jiang Jiang, 2014-08-01 10:32:31]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2014-08-01 10:35:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jiangj@opera.com/428183004/80001

[commit-bot: I haz the power, 2014-08-01 17:07:53]

Change committed as 287024