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

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

Initial patch by mad@chromium: https://codereview.chromium.org/12310109/

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=186232

[beaudoin, 2013-03-04 20:41:52]

Picking this up from mad who's on vacation this week, and we want to land that
quickly.

The initial patch-set should match patch-set 11 from
https://codereview.chromium.org/12310109/  (rebased on ToT).

I'll be working on answering sky's comments next.

[Alexei Svitkine (slow), 2013-03-04 20:56:04]

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1137:
UserMetricsAction("ClickedBookmarkBarAppsShortcutButton"));
Can you move this UMA action to bookmark_utils.cc as a function, similar to the
other ones? This will need to be called from the Mac code too.

[sky, 2013-03-04 22:16:50]

It doesn't seem like you addressed all my comments to MAD. Could you make sure
all those are addressed too?

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:8951: +      Apps
indentation is off here and for Opens Apps launcher page

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:8954: +      Opens apps launcher page
This seems rather wordy, maybe 'Show apps'?

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:248:
scoped_ptr<ui::SlideAnimation> show_animation_;
This isn't needed, right?

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:687:
bookmarks_separator_pref = bookmarks_separator_view_->GetPreferredSize();
spacing

[tfarina, 2013-03-04 22:19:47]

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1137:
UserMetricsAction("ClickedBookmarkBarAppsShortcutButton"));
On 2013/03/04 20:56:04, Alexei Svitkine wrote:
> Can you move this UMA action to bookmark_utils.cc as a function, similar to
the
> other ones?
if you do, can you try to put in c/b/ui/bookmark_utils rather than
c/b/bookmarks?

Thanks,

> This will need to be called from the Mac code too.

[tfarina, 2013-03-04 22:20:41]

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1137:
UserMetricsAction("ClickedBookmarkBarAppsShortcutButton"));
On 2013/03/04 22:19:47, tfarina wrote:
> On 2013/03/04 20:56:04, Alexei Svitkine wrote:
> > Can you move this UMA action to bookmark_utils.cc as a function, similar to
> the
> > other ones?
> if you do, can you try to put in c/b/ui/bookmark_utils 
oops, this should c/b/ui/bookmarks/bookmark_utils

[beaudoin, 2013-03-04 23:30:21]

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:8951: +      Apps
On 2013/03/04 22:16:50, sky wrote:
> indentation is off here and for Opens Apps launcher page

Done.

https://codereview.chromium.org/12386088/diff/2001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:8954: +      Opens apps launcher page
On 2013/03/04 22:16:50, sky wrote:
> This seems rather wordy, maybe 'Show apps'?

Done.

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:248:
scoped_ptr<ui::SlideAnimation> show_animation_;
On 2013/03/04 22:16:50, sky wrote:
> This isn't needed, right?

Done.

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:687:
bookmarks_separator_pref = bookmarks_separator_view_->GetPreferredSize();
On 2013/03/04 22:16:50, sky wrote:
> spacing

Done.

https://codereview.chromium.org/12386088/diff/2001/chrome/browser/ui/views/bo...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1137:
UserMetricsAction("ClickedBookmarkBarAppsShortcutButton"));
On 2013/03/04 22:20:41, tfarina wrote:
> On 2013/03/04 22:19:47, tfarina wrote:
> > On 2013/03/04 20:56:04, Alexei Svitkine wrote:
> > > Can you move this UMA action to bookmark_utils.cc as a function, similar
to
> > the
> > > other ones?
> > if you do, can you try to put in c/b/ui/bookmark_utils 
> oops, this should c/b/ui/bookmarks/bookmark_utils

Extracted to a method, but I did not move the bookmark_util::Record* functions
to c/b/ui/bookmarks/bookmark_utils because at least one is used in non-ui code,
specifically in
 c/b/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc

[beaudoin, 2013-03-04 23:32:03]

@sky  I wanted to push the baseline as identical as possible as Mad's last
patch, do make diffs clearer. All your comments on his last patchset should be
adressed in pathset 2 of this CR.

[sky, 2013-03-05 01:42:58]

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1668: // Ash does not
paint the bookmarks separator line because it looks odd on
This ifdef is wrong, it needs to be based on the HOST_DESKTOP_TYPE of the
browser, eg:

if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
  bookmark_separator_view_->SetVisible(false);
else
  // your code

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1828:
apps_page_shortcut_->SetVisible(ShouldShowAppsShortcut());
Don't you also need to invoke UpdateBookmarksSeparatorVisibility()?

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:21:
BookmarkBarView bookmark_bar_view(browser(), NULL);
You should make this tests block until the bookmark model has loaded.

https://codereview.chromium.org/12386088/diff/12001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:1452:
'browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc',
sort

[beaudoin, 2013-03-05 16:13:18]

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1668: // Ash does not
paint the bookmarks separator line because it looks odd on
On 2013/03/05 01:42:58, sky wrote:
> This ifdef is wrong, it needs to be based on the HOST_DESKTOP_TYPE of the
> browser, eg:
> 
> if (browser_->host_desktop_type() == chrome::HOST_DESKTOP_TYPE_ASH)
>   bookmark_separator_view_->SetVisible(false);
> else
>   // your code

Done, but note that this ifdef was used previously and I just moved it here.

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1828:
apps_page_shortcut_->SetVisible(ShouldShowAppsShortcut());
On 2013/03/05 01:42:58, sky wrote:
> Don't you also need to invoke UpdateBookmarksSeparatorVisibility()?

Done.

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/browser/ui/views/b...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:21:
BookmarkBarView bookmark_bar_view(browser(), NULL);
On 2013/03/05 01:42:58, sky wrote:
> You should make this tests block until the bookmark model has loaded.

Looks like the test does not really need a bookmark model. I added one anyway.

Done.

https://codereview.chromium.org/12386088/diff/12001/chrome/chrome_tests_unit....
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/12386088/diff/12001/chrome/chrome_tests_unit....
chrome/chrome_tests_unit.gypi:1452:
'browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc',
On 2013/03/05 01:42:58, sky wrote:
> sort

Done.

[sky, 2013-03-05 17:03:14]

LGTM

[commit-bot: I haz the power, 2013-03-05 17:41:38]

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

[Alexei Svitkine (slow), 2013-03-05 17:44:33]

LGTM

[commit-bot: I haz the power, 2013-03-05 17:45:02]

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

[commit-bot: I haz the power, 2013-03-05 19:40:42]

Change committed as 186232