Mac: Add a shortcut to open the Apps page from the bookmark bar.

Mac: Add a shortcut to open the Apps page from the bookmark bar.

BUG=177377
TEST=Make sure that an app shortcut is available in the bookmark bar when instant extended is enabled, and that this button can be hidden/shown from the bookmark bar context menu.


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

[beaudoin, 2013-03-07 01:47:11]

Here is my take on the "apps" menu in the bookmark bar for mac. Still need to do
the context menu, but let me know what you think.

[Alexei Svitkine (slow), 2013-03-07 15:56:59]

LGTM with a few nits

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:229:
scoped_nsobject<BookmarkButton> appsPageShortcutButton_;
Nit: Needs a comment.

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:930:
setBookmarkNode:bookmarkModel_->bookmark_bar_node()];
Nit: Remove this from the diff.

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1409: -
(BookmarkButton*)createCustomBookmarksButton:(const BookmarkNode*)node {
Nit: Add a comment.

[beaudoin, 2013-03-07 21:03:54]

If you could take a quick look, Alexei, just to make sure I did the
preference-listening correctly in the bridge. Thanks!

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:229:
scoped_nsobject<BookmarkButton> appsPageShortcutButton_;
On 2013/03/07 15:56:59, Alexei Svitkine wrote:
> Nit: Needs a comment.

Done.

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:930:
setBookmarkNode:bookmarkModel_->bookmark_bar_node()];
On 2013/03/07 15:56:59, Alexei Svitkine wrote:
> Nit: Remove this from the diff.

Done.

https://codereview.chromium.org/12550006/diff/2001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1409: -
(BookmarkButton*)createCustomBookmarksButton:(const BookmarkNode*)node {
On 2013/03/07 15:56:59, Alexei Svitkine wrote:
> Nit: Add a comment.

Done.

[beaudoin, 2013-03-07 21:05:46]

+jrg : For c/b/ui/cocoa/bookmarks/*
+sky: For c/b/bookmarks

[beaudoin, 2013-03-07 21:06:40]

sky: Please also check c/b/ui/views/*

[Alexei Svitkine (slow), 2013-03-07 21:17:07]

LGTM

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (right):

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:28:
profile_pref_registrar_.Init(browser->profile()->GetPrefs());
Are you only using |browser| to get a profile? If so, pass in a profile.

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:32:
base::Unretained(this)));
Indent this line 4 more.

[John Grabowski, 2013-03-07 21:35:14]

LGTM

Have you got a link to some mocks, or design doc?

Please wait for an LGTM from rsesek@ or another team member currently active on
Mac Chrome.

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (right):

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:28:
profile_pref_registrar_.Init(browser->profile()->GetPrefs());
On 2013/03/07 21:17:07, Alexei Svitkine wrote:
> Are you only using |browser| to get a profile? If so, pass in a profile.

+1

[beaudoin, 2013-03-08 00:02:50]

I think asvitkine@ can be considered "active on Mac chrome". Alexei, don't
hesitate to add another reviewer if you think it would be useful.

@gideon : Do you have a link to mocks for this?

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (right):

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:28:
profile_pref_registrar_.Init(browser->profile()->GetPrefs());
On 2013/03/07 21:35:14, John Grabowski wrote:
> On 2013/03/07 21:17:07, Alexei Svitkine wrote:
> > Are you only using |browser| to get a profile? If so, pass in a profile.
> 
> +1

Done.

https://codereview.chromium.org/12550006/diff/7001/chrome/browser/ui/cocoa/bo...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:32:
base::Unretained(this)));
On 2013/03/07 21:17:07, Alexei Svitkine wrote:
> Indent this line 4 more.

Done.

[sky, 2013-03-08 00:52:05]

It's my understanding the 'apps' item on the bookmark bar is purely a button.
It's not a place you can add bookmarks to. So, why are we changing the model for
this?

[beaudoin, 2013-03-08 03:15:56]

On 2013/03/08 00:52:05, sky wrote:
> It's my understanding the 'apps' item on the bookmark bar is purely a button.
> It's not a place you can add bookmarks to. So, why are we changing the model
for
> this?

You're right, and I tried to keep it out, but the Mac bookmark bar buttons are
very tightly linked to bookmark nodes (they keep pointers to it). I'll try again
tomorrow to untangle this.

[beaudoin, 2013-03-08 19:33:34]

R: -sky

[beaudoin, 2013-03-08 19:33:34]

R: -sky

[beaudoin, 2013-03-08 19:35:25]

I made the BookmarkButtonCell a bit more flexible so it supports custom buttons
that are not attached to a node. This means this CL no longer need to touch the
model or the views implementation.

@Alexei, John: PTAL.

[Alexei Svitkine (slow), 2013-03-08 19:45:47]

https://codereview.chromium.org/12550006/diff/23001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/23001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:293: // button is
"apps page" shortcut: flash it
Remove the comment about the apps page button here, since it can't be flashed.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1421: -
(BookmarkButton*)createCustomBookmarksButton:(NSCell*)cell {
Don't pass the cell in and just call -setCell: from the callsite on the returned
button.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1422: 
Remove empty line.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1854:
[BookmarkButtonCell buttonCellForContextMenu:buttonFolderContextMenu_
-buttonCellForContextMenu: doesn't make sense.

I suggest switching the order of params around and making this one
-buttonCellWithText:image:contextMenu: and changing the param names and order on
the other method too, to be consistent.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:81:
cellText:(NSString*)cellText
Align the :'s

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:195: // anything.
Is this how the Windows implementation works? No context menu for the Apps
button?

[beaudoin, 2013-03-08 22:07:17]

Answered Alexei's comments.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1421: -
(BookmarkButton*)createCustomBookmarksButton:(NSCell*)cell {
On 2013/03/08 19:45:48, Alexei Svitkine wrote:
> Don't pass the cell in and just call -setCell: from the callsite on the
returned
> button.

Looks like -setCell must be called before -setDelegate (or -setTarget?) so I
left it here.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1422: 
On 2013/03/08 19:45:48, Alexei Svitkine wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/12550006/diff/36001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1854:
[BookmarkButtonCell buttonCellForContextMenu:buttonFolderContextMenu_
On 2013/03/08 19:45:48, Alexei Svitkine wrote:
> -buttonCellForContextMenu: doesn't make sense.
> 
> I suggest switching the order of params around and making this one
> -buttonCellWithText:image:contextMenu: and changing the param names and order
on
> the other method too, to be consistent.

Done.

[beaudoin, 2013-03-09 01:57:06]

John, Alexei: Can you PTAL this is getting close to the final version, rebased
on top of Alexei's changes.

[Alexei Svitkine (slow), 2013-03-11 14:31:04]

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:9: #include
"base/prefs/pref_service.h"
Remove double include.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (left):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:217: -
(void)addNonBookmarkButtonsToView;
Nit: Can you add all the new private methods you're introducing to this section?

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1156: -
(BookmarkButton*)createCustomBookmarkButtonForCell:(NSCell*)cell {
Nit: This doesn't actually follow the Cocoa create naming convention (caller
doesn't get a retained object), so name it instead
-customBookmarkButtonForCell:.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1213: -
(IBAction)openAppsPage:(id)sender {
Nit: This doesn't need to be an IBAction, change the return value to void.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1572: -
(BookmarkButtonCell*)cellForCustomButton:(NSString*)text
Nit: rename to -cellForCustomButtonWithText:image:

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:63: -
(id)initWithMenuController:
As discussed on a previous patchset, we wanted |text| to be the first param, so
that it's:

-initWithText:image:menuController:
-buttonCellWithText:image:menuController:

and

-buttonCellForNode:text:image:menuController:

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:190: // anything.
Hmm, my other refactoring CL didn't actually remove this whole method, as I told
you offline.

I don't think this does what the current Windows implementation does. We still
want to have some kind of context menu here, so that the user can at least
uncheck App shortcut item in it.

[John Grabowski, 2013-03-11 16:03:38]

Nothing to add other that what Alexei already pointed out.

@alexei: please send me a CL adding you as OWNER for this bookmark directory.

[beaudoin, 2013-03-11 21:02:14]

Answered Alexei's comments, fixed the tests.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_bridge.mm:9: #include
"base/prefs/pref_service.h"
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> Remove double include.

Done.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (left):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:217: -
(void)addNonBookmarkButtonsToView;
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> Nit: Can you add all the new private methods you're introducing to this
section?

This section misses quite a few of the private methods already in there. The
order seems also a bit off. Do you want me to thoroughly clean it up? (The order
is also off in the .h for public methods.) If yes, I'd suggest doing it in a
follow-up CL.

Done.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1156: -
(BookmarkButton*)createCustomBookmarkButtonForCell:(NSCell*)cell {
Done.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1213: -
(IBAction)openAppsPage:(id)sender {
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> Nit: This doesn't need to be an IBAction, change the return value to void.

Done.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:1572: -
(BookmarkButtonCell*)cellForCustomButton:(NSString*)text
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> Nit: rename to -cellForCustomButtonWithText:image:

Done.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.h:63: -
(id)initWithMenuController:
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> As discussed on a previous patchset, we wanted |text| to be the first param,
so
> that it's:
> 
> -initWithText:image:menuController:
> -buttonCellWithText:image:menuController:
> 
> and
> 
> -buttonCellForNode:text:image:menuController:
> 

Done.

[beaudoin, 2013-03-11 21:02:51]

Done that too, forgot to mark it.

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm (right):

https://codereview.chromium.org/12550006/diff/62001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_button_cell.mm:190: // anything.
On 2013/03/11 14:31:04, Alexei Svitkine wrote:
> Hmm, my other refactoring CL didn't actually remove this whole method, as I
told
> you offline.
> 
> I don't think this does what the current Windows implementation does. We still
> want to have some kind of context menu here, so that the user can at least
> uncheck App shortcut item in it.

Done.

[Alexei Svitkine (slow), 2013-03-11 21:06:43]

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1523:
prefs::kShowAppsShortcutInBookmarkBar, false);
Please clean up after the test - don't just set a pref and leave like that for
the rest of the tests that run after this!

Alternatively, fix the test to not rely on this assumption.

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:44:
text:@"Testing"
Align the :'s.

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:425: [button
setTarget:barController_];
That shouldn't be showing up the diff, is your base set correctly?

[beaudoin, 2013-03-11 21:29:56]

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller_unittest.mm:1523:
prefs::kShowAppsShortcutInBookmarkBar, false);
On 2013/03/11 21:06:43, Alexei Svitkine wrote:
> Please clean up after the test - don't just set a pref and leave like that for
> the rest of the tests that run after this!
> 
> Alternatively, fix the test to not rely on this assumption.

Looks like this was not needed after all.

Done.

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_button_cell_unittest.mm:44:
text:@"Testing"
On 2013/03/11 21:06:43, Alexei Svitkine wrote:
> Align the :'s.

Done.

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:425: [button
setTarget:barController_];
On 2013/03/11 21:06:43, Alexei Svitkine wrote:
> That shouldn't be showing up the diff, is your base set correctly?

Hmmm, it seems ok, and no diff show up here for me. Bug on your side? Or are you
comparing to a previous patchset?

[Alexei Svitkine (slow), 2013-03-11 21:39:54]

LGTM, thanks.

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm
(right):

https://codereview.chromium.org/12550006/diff/69001/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_controller.mm:425: [button
setTarget:barController_];
On 2013/03/11 21:29:56, beaudoin wrote:
> On 2013/03/11 21:06:43, Alexei Svitkine wrote:
> > That shouldn't be showing up the diff, is your base set correctly?
> 
> Hmmm, it seems ok, and no diff show up here for me. Bug on your side? Or are
you
> comparing to a previous patchset?

Ah, that was it, my bad!

[commit-bot: I haz the power, 2013-03-11 22:01:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/12550006/67003

[commit-bot: I haz the power, 2013-03-12 00:41:25]

Change committed as 187428