Add pin icon to the omnibar in windows 8 metro mode.

Add pin icon to the omnibar in windows 8 metro mode.

The pin icon allows pinning and unpinning the start screen. The icon used changes depending on whether the page is currently pinned or not.

BUG=129598
TEST=Test pages can be pinned and unpinned from metro mode


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148511

[sky, 2012-07-20 16:46:40]

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser.h#ne...
chrome/browser/ui/browser.h:103: public MetroPinTabHelper::Delegate,
I don't like browser implementing an interface that is solely for windows. Can
this move to browserview?

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser_wind...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser_wind...
chrome/browser/ui/browser_window.h:128: virtual void SetMetroPinnedState(bool
is_pinned) = 0;
Move this with other Metro methods.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/metro_pin_ta...
File chrome/browser/ui/metro_pin_tab_helper.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/metro_pin_ta...
chrome/browser/ui/metro_pin_tab_helper.h:15: class Delegate {
Move this into its own file.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:299:
metro_pin_view_->SetVisible(true);
Default visibility is true.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1161: View*
right_anchor = chrome_to_mobile_view_;
1161-1168 shouldn't be in this if block.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/metro_pin_view.cc (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/metro_pin_view.cc:20:
set_id(VIEW_ID_STAR_BUTTON);
This isn't the right id.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/metro_pin_view.cc:46: // We want to show
the bubble on mouse release; that is the standard behavior
The reason StarView isn't a button is that it shows a bubble. That isn't the
case with this class. You should be able to use an ImageButton.

http://codereview.chromium.org/10800054/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10800054/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:3740:
'browser/ui/views/location_bar/metro_pin_view.cc',
sort

[benwells, 2012-07-23 08:03:43]

Thanks for the feedback Scott, I like this CL more now...

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser.h#ne...
chrome/browser/ui/browser.h:103: public MetroPinTabHelper::Delegate,
On 2012/07/20 16:46:41, sky wrote:
> I don't like browser implementing an interface that is solely for windows. Can
> this move to browserview?

Yep, thanks for the suggestion! I didn't like all the #iffery....

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser_wind...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/browser_wind...
chrome/browser/ui/browser_window.h:128: virtual void SetMetroPinnedState(bool
is_pinned) = 0;
On 2012/07/20 16:46:41, sky wrote:
> Move this with other Metro methods.

It's gone now :)

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/metro_pin_ta...
File chrome/browser/ui/metro_pin_tab_helper.h (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/metro_pin_ta...
chrome/browser/ui/metro_pin_tab_helper.h:15: class Delegate {
On 2012/07/20 16:46:41, sky wrote:
> Move this into its own file.

Done.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:299:
metro_pin_view_->SetVisible(true);
On 2012/07/20 16:46:41, sky wrote:
> Default visibility is true.

Done. I updated some others as well.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1161: View*
right_anchor = chrome_to_mobile_view_;
On 2012/07/20 16:46:41, sky wrote:
> 1161-1168 shouldn't be in this if block.

Sorry, I don't understand. My understanding is this code is re-inserting the
page action views. Before it does that it works out which view to put them next
to, in right-anchor. right_anchor is only used in this if block (1176).

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/metro_pin_view.cc (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/metro_pin_view.cc:20:
set_id(VIEW_ID_STAR_BUTTON);
On 2012/07/20 16:46:41, sky wrote:
> This isn't the right id.

Done.

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/metro_pin_view.cc:46: // We want to show
the bubble on mouse release; that is the standard behavior
On 2012/07/20 16:46:41, sky wrote:
> The reason StarView isn't a button is that it shows a bubble. That isn't the
> case with this class. You should be able to use an ImageButton.

Cool, that's much better!

http://codereview.chromium.org/10800054/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10800054/diff/1/chrome/chrome_browser.gypi#new...
chrome/chrome_browser.gypi:3740:
'browser/ui/views/location_bar/metro_pin_view.cc',
On 2012/07/20 16:46:41, sky wrote:
> sort

Done.

[sky, 2012-07-23 16:02:17]

LGTM

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10800054/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1161: View*
right_anchor = chrome_to_mobile_view_;
On 2012/07/23 08:03:43, benwells wrote:
> On 2012/07/20 16:46:41, sky wrote:
> > 1161-1168 shouldn't be in this if block.
> 
> Sorry, I don't understand. My understanding is this code is re-inserting the
> page action views. Before it does that it works out which view to put them
next
> to, in right-anchor. right_anchor is only used in this if block (1176).

You're right, my mistake.

http://codereview.chromium.org/10800054/diff/14001/chrome/browser/ui/metro_pi...
File chrome/browser/ui/metro_pin_tab_helper.h (right):

http://codereview.chromium.org/10800054/diff/14001/chrome/browser/ui/metro_pi...
chrome/browser/ui/metro_pin_tab_helper.h:41: // The observer that we inform when
the is_pinned_ state changes.
Use |s around references to members and arguments.

[commit-bot: I haz the power, 2012-07-25 03:47:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800054/22001

[commit-bot: I haz the power, 2012-07-25 04:17:59]

Try job failure for 10800054-22001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-25 23:23:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800054/23020

[commit-bot: I haz the power, 2012-07-26 00:48:21]

Try job failure for 10800054-23020 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-26 01:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800054/21006

[commit-bot: I haz the power, 2012-07-26 02:05:12]

Try job failure for 10800054-21006 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-26 02:29:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10800054/17006

[commit-bot: I haz the power, 2012-07-26 03:58:58]

Change committed as 148511