Added Pin/Unpin button to the App Info Dialog on ChromeOS

Added Pin/Unpin button to the App Info Dialog on ChromeOS

Since the "Create Shortcuts" button is disabled on ChromeOS, add a
pin/unpin button that allows pinning and unpinning the app from the
shelf. This is the functional equivalent of creating shortcuts on
ChromeOS.

Based off https://codereview.chromium.org/335523002/.

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

[sashab, 2014-06-17 08:03:56]

Low priority :)

[benwells, 2014-06-18 01:52:55]

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:56:
ash::Shell::GetInstance()->GetShelfDelegate();
Why do you get the shelf delegate here? It isn't used in this function.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:90: bool
can_unpin =
Super nit: I'd call this variable is_pinned.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:142: void
AppInfoFooterPanel::SetPinnedToShelf(bool pinned) {
Nit: this function reads better if this is value not pinned. (Pinned imples to
me that it was pinned).

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:157: #if
defined(OS_CHROMEOS)
As discussed this #if (and the one above for shortcuts) should be changed. It
should instead check the host desktop type for the window. See host_desktop.h
for the type.

[sashab, 2014-06-19 04:15:40]

I can't see a better way to get HostDesktopType before the AppListContentsView
is created other than to plumb the AppListWindow through AppInfoDialog.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc
(right):

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:56:
ash::Shell::GetInstance()->GetShelfDelegate();
On 2014/06/18 01:52:55, benwells wrote:
> Why do you get the shelf delegate here? It isn't used in this function.

Oops. Fixed. Done.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:90: bool
can_unpin =
On 2014/06/18 01:52:55, benwells wrote:
> Super nit: I'd call this variable is_pinned.

I actually called it 'is_pinned' in the first place, and then changed it to
this. If it's called is_pinned the following lines become:

unpin_from_shelf_button_->SetVisible(!is_pinned);
pin_to_shelf_button_->SetVisible(is_pinned);

which is pretty confusing (why is the unpin button's visibility true if the app
is not pinned?)

I've now called the variable 'show_pin_button'. Let me know what you think.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:142: void
AppInfoFooterPanel::SetPinnedToShelf(bool pinned) {
On 2014/06/18 01:52:55, benwells wrote:
> Nit: this function reads better if this is value not pinned. (Pinned imples to
> me that it was pinned).

I renamed it 'pin_to_shelf' and called the function 'SetPinToShelf'.

https://codereview.chromium.org/340453004/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/app_info_dialog/app_info_footer_panel.cc:157: #if
defined(OS_CHROMEOS)
On 2014/06/18 01:52:54, benwells wrote:
> As discussed this #if (and the one above for shortcuts) should be changed. It
> should instead check the host desktop type for the window. See host_desktop.h
> for the type.

Done.

[benwells, 2014-06-19 04:45:23]

lgtm

[sashab, 2014-06-19 04:54:47]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-06-19 04:57:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/340453004/80001

[commit-bot: I haz the power, 2014-06-19 13:08:20]

Change committed as 278351