Mac: Update zoom bubble UI

Mac: Update zoom bubble UI

This CL updates the Mac zoom bubble UI. It makes the following change:
  - add a zoom in and zoom out button
  - if bubble is open don't hide the bubble or omnibox icon when zoom level is default
  - reset auto close timer
  - disable animation when showing bubble because user clicked on icon
  - hide the zoom omnibox icon when user types in the omnibox

Screenshot: http://i.imgur.com/PjGBQvv.png

BUG=177931, 177544, 178227
TEST=Opened a tab and zoomed in, verified that the bubble still opened. Clicked on the zoom icon. Pressed reset and verified that the bubble didn't close.

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

[sail, 2013-02-25 20:06:23]

Updated UI based on feedback from sgabriel.

[sail, 2013-02-26 00:16:27]

+dbeam: for chrome/browser/ui/zoom OWNERS review

[Dan Beam, 2013-02-26 00:17:49]

https://codereview.chromium.org/12315069/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.cc (left):

https://codereview.chromium.org/12315069/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.cc:59: return zoom >
default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
is it reasonable to return + or - when the zoom is 100%?

[sail, 2013-02-26 03:10:35]

https://codereview.chromium.org/12315069/diff/12001/chrome/browser/ui/zoom/zo...
File chrome/browser/ui/zoom/zoom_controller.cc (left):

https://codereview.chromium.org/12315069/diff/12001/chrome/browser/ui/zoom/zo...
chrome/browser/ui/zoom/zoom_controller.cc:59: return zoom >
default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
On 2013/02/26 00:17:49, Dan Beam wrote:
> is it reasonable to return + or - when the zoom is 100%?

Done.
Good point. Changed to use IDR_ZOOM_NORMAL.

[Dan Beam, 2013-02-26 04:03:16]

IDR_ZOOM_NORMAL doesn't seem to exist.  What would the icon for *default* zoom
be, and where would you use it in the UI?

https://codereview.chromium.org/12315069/diff/9004/chrome/browser/ui/zoom/zoo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12315069/diff/9004/chrome/browser/ui/zoom/zoo...
chrome/browser/ui/zoom/zoom_controller.cc:56: int
ZoomController::GetResourceForZoomLevel() const {
if (IsAtDefaultZoom())
  return IDR_ZOOM_NORMAL;

[sail, 2013-02-26 04:53:21]

On 2013/02/26 04:03:16, Dan Beam wrote:
> IDR_ZOOM_NORMAL doesn't seem to exist. 
I'm updating all the zoom icons in a separate CL. See
https://codereview.chromium.org/12310089/

> What would the icon for *default* zoom
> be, 
I've uploaded a copy here so you can see: http://i.imgur.com/AR3R3c3.png

> and where would you use it in the UI?
This is in the CL description: "- if bubble is open don't hide the bubble or
omnibox icon when zoom level is default"

https://codereview.chromium.org/12315069/diff/9004/chrome/browser/ui/zoom/zoo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12315069/diff/9004/chrome/browser/ui/zoom/zoo...
chrome/browser/ui/zoom/zoom_controller.cc:56: int
ZoomController::GetResourceForZoomLevel() const {
On 2013/02/26 04:03:16, Dan Beam wrote:
> if (IsAtDefaultZoom())
>   return IDR_ZOOM_NORMAL;

Done.

[Robert Sesek, 2013-02-26 16:08:24]

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:136: -
(void)performLayout {
This method is a bit too long now.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:137: ui::NativeTheme
*nativeTheme = ui::NativeTheme::instance();
nit: asterisk placement

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:142:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
nit: space after ->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:194:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
nit: space after ->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:271:
[gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
nit: space after ->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:115:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
nit: space after ->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:175:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:409:
nativeTheme-> GetSystemColor(
->

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:72: SetVisible(false);
Is this necessary? Won't Update() be called when the zoom changes?

[Dan Beam, 2013-02-26 18:36:31]

lgtm w/nit

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/zoom/zoo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/zoom/zoo...
chrome/browser/ui/zoom/zoom_controller.cc:59: return IDR_ZOOM_NORMAL;
  if (IsAtDefaultZoom())
    return IDR_ZOOM_NORMAL;
  double zoom = web_contents()->GetZoomLevel();
  return zoom > default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;

(declare locals as close as possible -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...)

[sail, 2013-02-26 22:04:15]

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:136: -
(void)performLayout {
On 2013/02/26 16:08:24, rsesek wrote:
> This method is a bit too long now.

Done.
Refactored the common button creation code to its own method.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:137: ui::NativeTheme
*nativeTheme = ui::NativeTheme::instance();
On 2013/02/26 16:08:24, rsesek wrote:
> nit: asterisk placement

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:142:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> nit: space after ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:194:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> nit: space after ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:271:
[gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> nit: space after ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:115:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> nit: space after ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:175:
gfx::SkColorToCalibratedNSColor(nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:409:
nativeTheme-> GetSystemColor(
On 2013/02/26 16:08:24, rsesek wrote:
> ->

Done.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:72: SetVisible(false);
On 2013/02/26 16:08:24, rsesek wrote:
> Is this necessary? Won't Update() be called when the zoom changes?

This is only for default zoom case. For example:
  - zoom is at 90%, use opens the zoom bubble
  - user presses "+"
  - update is called and we don't hide the zoom omnibox icon because bubble is
open
  - bubble closes
At this point Update has already been called so we have to explicitly hide the
zoom icon.

I've added a comment explaining this.

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/zoom/zoo...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/12315069/diff/1017/chrome/browser/ui/zoom/zoo...
chrome/browser/ui/zoom/zoom_controller.cc:59: return IDR_ZOOM_NORMAL;
On 2013/02/26 18:36:31, Dan Beam wrote:
>   if (IsAtDefaultZoom())
>     return IDR_ZOOM_NORMAL;
>   double zoom = web_contents()->GetZoomLevel();
>   return zoom > default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS :
IDR_ZOOM_MINUS;
> 
> (declare locals as close as possible -
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_...)

Done.

[Robert Sesek, 2013-02-26 23:15:16]

Scott should probably also review some of those location_bar changes.

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:35: @interface
ZoomHoverButton : HoverButton {
nit: {} are not necessary

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:151:
action:@selector(zoomOut:)];
nit: align :

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/info_bubble_window.h (right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/info_bubble_window.h:28: BOOL enableAnimations_;
It's kind of weird how this interacts with delayOnClose_. It seems like this
would be better suited to an enum? info_bubble::kShowAnimation |
info_bubble::kHideAnimation = info_bubble::kAllAnimations (the default). To keep
this CL simpler, maybe split this part out into a new CL?

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:113:
ui::NativeTheme *nativeTheme = ui::NativeTheme::instance();
nit: asterisk placement

[sail, 2013-02-27 00:35:34]

shess: could you take a look at the chrome/browser/ui/cocoa/location_bar
changes? Thanks!

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:35: @interface
ZoomHoverButton : HoverButton {
On 2013/02/26 23:15:16, rsesek wrote:
> nit: {} are not necessary

Done.

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:151:
action:@selector(zoomOut:)];
On 2013/02/26 23:15:16, rsesek wrote:
> nit: align :

Done.

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/in...
File chrome/browser/ui/cocoa/info_bubble_window.h (right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/in...
chrome/browser/ui/cocoa/info_bubble_window.h:28: BOOL enableAnimations_;
On 2013/02/26 23:15:16, rsesek wrote:
> It's kind of weird how this interacts with delayOnClose_. It seems like this
> would be better suited to an enum? info_bubble::kShowAnimation |
> info_bubble::kHideAnimation = info_bubble::kAllAnimations (the default). To
keep
> this CL simpler, maybe split this part out into a new CL?

Done.

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/2023/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:113:
ui::NativeTheme *nativeTheme = ui::NativeTheme::instance();
On 2013/02/26 23:15:16, rsesek wrote:
> nit: asterisk placement

Done.

[sail, 2013-02-27 01:41:39]

+thakis for src/ui/* OWNERS review

[Nico, 2013-02-27 12:46:46]

Don't we have a theme service below chrome/ already? Since this is all chrome
code, why do we need to add another theme service below ui/?

[Scott Hess - ex-Googler, 2013-02-27 15:22:10]

LGTM, though, just style sorts of nits.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:165:
const CGFloat kSeparatorHeight = 1.0;
Is this the same on hidpi?  I'd say "probably", but sometimes it's not.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:409:
ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor));
This wad of code seems like it's just dying for a helper function.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/mock_toolbar_model.h (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/mock_toolbar_model.h:12: //
|WouldReplaceSearchURLWithSearchTerms| to be customized.
Since you're pulling this out to be shared ... is there any namespacing or
anything that belongs in here?

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:51: }
This maybe calls for a change to the method name, as "ShowBubble" no longer
seems descriptive.

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
File ui/native_theme/native_theme_mac.h (right):

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
ui/native_theme/native_theme_mac.h:15: static NativeThemeMac* instance();
This seems odd to expose.  Wouldn't it only be necessary if the native-theme
abstraction were broken?

[Robert Sesek, 2013-02-27 17:14:40]

LGTM

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:99: ?
info_bubble::kAnimateOrderIn | info_bubble::kAnimateOrderOut
nit: should only be indented 4

[Elliot Glaysher, 2013-02-27 17:39:46]

On 2013/02/27 12:46:46, Nico wrote:
> Don't we have a theme service below chrome/ already? Since this is all chrome
> code, why do we need to add another theme service below ui/?

ThemeService replaces IDR image resources with ones that come from an extension.
NativeTheme is how you inject platform native colors into views. (I am currently
working on writing one for the linux_aura port that reads GTK colors.)

[sail, 2013-02-27 17:57:42]

On 2013/02/27 17:39:46, Elliot Glaysher wrote:
> On 2013/02/27 12:46:46, Nico wrote:
> > Don't we have a theme service below chrome/ already? Since this is all
chrome
> > code, why do we need to add another theme service below ui/?
> 
> ThemeService replaces IDR image resources with ones that come from an
extension.
> NativeTheme is how you inject platform native colors into views. (I am
currently
> working on writing one for the linux_aura port that reads GTK colors.)

What Elliot said.
Also, examples of where we'll use this outside of chrome:
  - tab modal dialogs (wittman@ is moving this outside of chrome/*)
  - apps launcher (tapted@ is implementing this in ui/app_list/cocoa/)

[sail, 2013-02-27 18:48:17]

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:99: ?
info_bubble::kAnimateOrderIn | info_bubble::kAnimateOrderOut
On 2013/02/27 17:14:40, rsesek wrote:
> nit: should only be indented 4

Done.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm
(right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:165:
const CGFloat kSeparatorHeight = 1.0;
On 2013/02/27 15:22:10, shess wrote:
> Is this the same on hidpi?  I'd say "probably", but sometimes it's not.

Yea this is correct in hidpi mode. For example, the system menu item separators
are 2 device pixels high in hidpi mdoe. The same for chrome menu item
separators.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.mm:409:
ui::NativeTheme::kColorId_FocusedMenuItemBackgroundColor));
On 2013/02/27 15:22:10, shess wrote:
> This wad of code seems like it's just dying for a helper function.

Done.

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/mock_toolbar_model.h (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/mock_toolbar_model.h:12: //
|WouldReplaceSearchURLWithSearchTerms| to be customized.
On 2013/02/27 15:22:10, shess wrote:
> Since you're pulling this out to be shared ... is there any namespacing or
> anything that belongs in here?

Done.
Added chrome::testing:: to match run_loop_testing.h

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12315069/diff/14006/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:51: }
On 2013/02/27 15:22:10, shess wrote:
> This maybe calls for a change to the method name, as "ShowBubble" no longer
> seems descriptive.

Done.
Changed to ToggleBubble()

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
File ui/native_theme/native_theme_mac.h (right):

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
ui/native_theme/native_theme_mac.h:15: static NativeThemeMac* instance();
On 2013/02/27 15:22:10, shess wrote:
> This seems odd to expose.  Wouldn't it only be necessary if the native-theme
> abstraction were broken?

This is consistent with the other platforms. From what I understand this is used
to conditionally change the UI based on the theme used. Here's an example:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Scott Hess - ex-Googler, 2013-02-27 19:01:58]

still lgtm for location/bar stuff.

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
File ui/native_theme/native_theme_mac.h (right):

https://codereview.chromium.org/12315069/diff/14006/ui/native_theme/native_th...
ui/native_theme/native_theme_mac.h:15: static NativeThemeMac* instance();
On 2013/02/27 18:48:18, sail wrote:
> On 2013/02/27 15:22:10, shess wrote:
> > This seems odd to expose.  Wouldn't it only be necessary if the native-theme
> > abstraction were broken?
> 
> This is consistent with the other platforms. From what I understand this is
used
> to conditionally change the UI based on the theme used.

Say no more, say no more.  Please, say no more, I think this is going somewhere
I don't want to know about!

[Nico, 2013-02-28 12:53:17]

ui lgtm from an owners perspective.

[commit-bot: I haz the power, 2013-02-28 16:30:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12315069/9007

[commit-bot: I haz the power, 2013-02-28 16:56:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-28 18:06:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12315069/9007

[commit-bot: I haz the power, 2013-02-28 19:52:37]

Change committed as 185301

[rohitrao (ping after 24h), 2013-03-05 19:53:06]

We added a TestToolbarModel class in
https://chromiumcodereview.appspot.com/11040055/.  Can that replace
MockToolbarModel?