Implements a zoom icon in the Omnibox for Views.

Implements a zoom icon in the Omnibox for Views.

BUG=128816
TEST=none


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

[Ben Goodger (Google), 2012-06-04 15:16:49]

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
File chrome/browser/ui/gtk/zoom_bubble_gtk.cc (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.cc:37: int zoomPercent, bool autoclose) {
leave

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.cc:40: g_bubble = new
ZoomBubbleGtk(anchor, profile, zoomPercent, autoclose);
leave this parameter name the way it was

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
File chrome/browser/ui/gtk/zoom_bubble_gtk.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.h:40: ZoomBubbleGtk(GtkWidget* anchor,
Profile* profile,
leave it the way it was

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_dialogs.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_dialogs.h:58: int zoomPercent,
zoom_percent

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_dialogs.h:59: bool autoclose);
document autoclose. also, call it auto_close

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

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:474:
browser::ShowZoomBubbleView(zoom_view_, zoom_percent, true);
is this function called from anywhere outside the views frontend? if not, then
you should just instantiate the bubble here directly.

the browser_dialogs file exists only for functions that need to be called from
cross-platform code.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/zoom_view.h:20: explicit ZoomView();
-explicit

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/zoom_view.h:42: };
DISALLOW_COPY_AND

[Kyle Horimoto, 2012-06-05 05:03:59]

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
File chrome/browser/ui/gtk/zoom_bubble_gtk.cc (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.cc:37: int zoomPercent, bool autoclose) {
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> leave

This was changed in my other CL and must have gotten tracked here because I
rebased off of that CL. Someone told me to change this in the other CL so I'll
just leave it as-is.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.cc:40: g_bubble = new
ZoomBubbleGtk(anchor, profile, zoomPercent, autoclose);
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> leave this parameter name the way it was

Same.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
File chrome/browser/ui/gtk/zoom_bubble_gtk.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/gtk/zoom_bub...
chrome/browser/ui/gtk/zoom_bubble_gtk.h:40: ZoomBubbleGtk(GtkWidget* anchor,
Profile* profile,
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> leave it the way it was

Oops - this was changed in my other CL and must have gotten tracked here because
I rebased off of that CL. Someone told me to change this in the other CL so I'll
just leave it as-is.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
File chrome/browser/ui/views/browser_dialogs.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_dialogs.h:58: int zoomPercent,
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> zoom_percent

Done.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/browse...
chrome/browser/ui/views/browser_dialogs.h:59: bool autoclose);
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> document autoclose. also, call it auto_close

Done.

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

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_view.cc:474:
browser::ShowZoomBubbleView(zoom_view_, zoom_percent, true);
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> is this function called from anywhere outside the views frontend? if not, then
> you should just instantiate the bubble here directly.
> 
> the browser_dialogs file exists only for functions that need to be called from
> cross-platform code.

Done.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/zoom_view.h:20: explicit ZoomView();
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> -explicit

Done.

http://codereview.chromium.org/10494004/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/zoom_view.h:42: };
On 2012/06/04 15:16:49, Ben Goodger (Google) wrote:
> DISALLOW_COPY_AND

Done.

[Kyle Horimoto, 2012-06-07 03:49:20]

bump - could I get a review on this, please? Thanks.

[Kyle Horimoto, 2012-06-18 17:24:28]

bump - could I please get a review for this? Thanks!

[Kyle Horimoto, 2012-06-21 20:54:54]

Hi Peter,

Ben was previously reviewing this, but I think he's too busy at the moment to
finish. Would you mind reviewing this since you are also an owner? Thanks!

Kyle

[Ben Goodger (Google), 2012-06-22 19:16:05]

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:21: // Singleton
instance.
// static

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:58: if (auto_close_)
braces around this one

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:68: :
BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
4-space indent to the :

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:79: void
ZoomBubbleView::Close() {
this doesn't look to be used?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:47: };
DISALLOW_COPY_AND_ASSIGN(..

[Peter Kasting, 2012-06-22 20:16:20]

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:252: zoom_view_ = new
ZoomView();
This should appear in the same cases as the content settings icons.  I think
those appear in popup and app windows show I think we should create this
unconditionally.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:362:
zoom_view_->SetVisible(false);
Doesn't this need to be:

  zoom_view_->SetVisible(!model_->input_in_progress());

...so that it reappears once editing is done?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:465: if (zoom_view_)
NULL-check no longer needed once this is created unconditionally (see above).

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:473:
UpdateContentSettingsIcons();
Why are we calling this?

Did you instead mean to just do Layout(); SchedulePaint(); ?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:610: if (zoom_view_ &&
zoom_view_->visible())
Nit: Remove NULL-check and move to appropriate location (2 places)

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:52: class ZoomView;
Nit: This should be at the end of the list (alphabetical order)

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:480: // The zoom icon.
Shouldn't this icon be placed next to the content settings icons, since it
functions very similarly (in being a state indicator that appears when the state
of a page differs from the default)?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:22: ZoomBubbleView*
ZoomBubbleView::zoom_bubble_ = NULL;
Using a singleton for this is wrong.  For example, if the user opens the same
page in two browser windows, then changes zoom, we'll want to show a bubble in
both windows (in part because we don't know which one the user might be looking
at; one might be obscured).  So it seems like we need at least one bubble per
browser window.

Rather than using a singleton, you should look at the content settings bubbles
or the star bubble to see how they do ownership.

Also, be very careful with auto-showing bubbles; take my example two-window case
above, and minimize one window.  Now you probably don't want to try and show a
bubble in that window (because where precisely are you going to parent it?). 
Make sure you have thought through and handled scary edge case scenarios like
this.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:54: zoom_percent_label_
= new views::Label(
Nit: We don't need a member for this, since we never use it directly outside of
here.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:79: void
ZoomBubbleView::Close() {
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> this doesn't look to be used?

CloseBubble() above?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:25: //
views::WidgetDelegate method.
Nit: Since this doesn't inherit directly from views::WidgetDelegate, I'd comment
this as coming from views::BubbleDelegateView.  (I think it's also slightly more
common to just say "// <base class name>:" or "// <base class name>", but your
comment is fine too.)

Can this be made private?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:30: virtual void Init()
OVERRIDE;
Nit: No one overrides this class, right?  So this can be private.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:19: // width needed to display
it.
? I don't understand why this is important.

Wouldn't it be better to set the state to NONE here and not bother to call
Update() yet, and say that callers need to set our state correctly before doing
layout?

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:50: SetVisible(false);
I don't like that both the location bar and this code call SetVisible() on this
object.  They can fight each other.  Make one place (probably this object)
responsible for doing all the SetVisible() calls and tracking the necessary
state.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:99: if (event.key_code() ==
ui::VKEY_SPACE ||
Nit: If you reverse this conditional you can avoid {}

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:27: void Update();
Nit: Add comments

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:28: private:
Nit: Add blank line above

[Kyle Horimoto, 2012-06-26 21:55:53]

Addressed comments - please take another look :)

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:252: zoom_view_ = new
ZoomView();
On 2012/06/22 20:16:20, Peter Kasting wrote:
> This should appear in the same cases as the content settings icons.  I think
> those appear in popup and app windows show I think we should create this
> unconditionally.

Done. Moved below the code adding the Star, etc. in order to preserve the
correct order.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:362:
zoom_view_->SetVisible(false);
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Doesn't this need to be:
> 
>   zoom_view_->SetVisible(!model_->input_in_progress());
> 
> ...so that it reappears once editing is done?

Moved this check to ZoomView::Update() :)

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:465: if (zoom_view_)
On 2012/06/22 20:16:20, Peter Kasting wrote:
> NULL-check no longer needed once this is created unconditionally (see above).

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:473:
UpdateContentSettingsIcons();
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Why are we calling this?
> 
> Did you instead mean to just do Layout(); SchedulePaint(); ?

Hmm, I think this is part of some code I ended up removing. Sorry about that :/

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:610: if (zoom_view_ &&
zoom_view_->visible())
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: Remove NULL-check and move to appropriate location (2 places)

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:52: class ZoomView;
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: This should be at the end of the list (alphabetical order)

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:480: // The zoom icon.
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Shouldn't this icon be placed next to the content settings icons, since it
> functions very similarly (in being a state indicator that appears when the
state
> of a page differs from the default)?

Done. Makes sense - I had just put it by the StarView previously since they are
laid out next to each other.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:21: // Singleton
instance.
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> // static

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:22: ZoomBubbleView*
ZoomBubbleView::zoom_bubble_ = NULL;
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Using a singleton for this is wrong.  For example, if the user opens the same
> page in two browser windows, then changes zoom, we'll want to show a bubble in
> both windows (in part because we don't know which one the user might be
looking
> at; one might be obscured).  So it seems like we need at least one bubble per
> browser window.
> 
> Rather than using a singleton, you should look at the content settings bubbles
> or the star bubble to see how they do ownership.
> 
> Also, be very careful with auto-showing bubbles; take my example two-window
case
> above, and minimize one window.  Now you probably don't want to try and show a
> bubble in that window (because where precisely are you going to parent it?). 
> Make sure you have thought through and handled scary edge case scenarios like
> this.

As discussed offline, the bubble is purposely not shown in any window but the
active window, so a singleton is acceptable here.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:54: zoom_percent_label_
= new views::Label(
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: We don't need a member for this, since we never use it directly outside
of
> here.

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:58: if (auto_close_)
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> braces around this one

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:68: :
BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> 4-space indent to the :

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:79: void
ZoomBubbleView::Close() {
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> this doesn't look to be used?

It's used in CloseBubble() above.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:79: void
ZoomBubbleView::Close() {
On 2012/06/22 20:16:20, Peter Kasting wrote:
> On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> > this doesn't look to be used?
> 
> CloseBubble() above?

Yep.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:25: //
views::WidgetDelegate method.
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: Since this doesn't inherit directly from views::WidgetDelegate, I'd
comment
> this as coming from views::BubbleDelegateView.  (I think it's also slightly
more
> common to just say "// <base class name>:" or "// <base class name>", but your
> comment is fine too.)
> 
> Can this be made private?

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:30: virtual void Init()
OVERRIDE;
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: No one overrides this class, right?  So this can be private.

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:47: };
On 2012/06/22 19:16:06, Ben Goodger (Google) wrote:
> DISALLOW_COPY_AND_ASSIGN(..

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:19: // width needed to display
it.
On 2012/06/22 20:16:20, Peter Kasting wrote:
> ? I don't understand why this is important.
> 
> Wouldn't it be better to set the state to NONE here and not bother to call
> Update() yet, and say that callers need to set our state correctly before
doing
> layout?

I set it to NONE, and directly called SetImageSize() instead.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:50: SetVisible(false);
On 2012/06/22 20:16:20, Peter Kasting wrote:
> I don't like that both the location bar and this code call SetVisible() on
this
> object.  They can fight each other.  Make one place (probably this object)
> responsible for doing all the SetVisible() calls and tracking the necessary
> state.

Done. Good idea - this is far more elegant.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:99: if (event.key_code() ==
ui::VKEY_SPACE ||
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: If you reverse this conditional you can avoid {}

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.h (right):

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:27: void Update();
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: Add comments

Done.

http://codereview.chromium.org/10494004/diff/26002/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:28: private:
On 2012/06/22 20:16:20, Peter Kasting wrote:
> Nit: Add blank line above

Done.

[Peter Kasting, 2012-06-26 23:23:39]

Most of the "move this" nits are just for ordering consistency, but the layout
one will actually move where the icon is displayed (to be adjacent to the
content settings icons).

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:256: zoom_view_ = new
ZoomView(model_);
Nit: Move this to just below the content settings loop

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:351:
zoom_view_->Update();
Nit: Move the RefreshXXX() calls below to the top of this function and then put
this between them

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:591: entry_width -=
zoom_view_->GetPreferredSize().width() + GetItemPadding();
Nit: Move this to just above the content settings loop

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:684: int zoom_width =
zoom_view_->GetPreferredSize().width();
Nit: Move this to just above the content settings loop

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:176: void
SetPreviewEnabledPageAction(ExtensionAction *page_action,
Nit: While you're here, can you fix the "*" location?

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:188: // Sets the
tooltip for the zoom icon.
Tiny nit: For ordering consistency, let's move these declarations/definitions
above SetPreviewEnabledPageAction(), since the actual zoom icon is listed before
the page action icons.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:194: // Show the zoom
bubble.
Nit: Show -> Shows

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:30: // be updated.
Not necessarily true.  The bubble could be shown with a different anchor than
before.  For example, we hit ctrl-+ on one window and then quickly hit ctrl-+ on
a different window.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:31: SetImageSize(size);
Nit: Is it possible to omit this, calculate it dynamically, etc.?

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:62: int resource_id =
zoom_icon_state_ == ZoomController::ZOOM_PLUS_ICON ?
Nit: You could also inline this into the next statement

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:76: if
(ZoomBubbleView::IsShowing())
Nit: Shorter:

  return !ZoomBubbleView::IsShowing() && ImageView::GetTooltipText(p, tooltip);

[Kyle Horimoto, 2012-06-30 00:22:49]

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:256: zoom_view_ = new
ZoomView(model_);
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Move this to just below the content settings loop

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:351:
zoom_view_->Update();
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Move the RefreshXXX() calls below to the top of this function and then
put
> this between them

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:591: entry_width -=
zoom_view_->GetPreferredSize().width() + GetItemPadding();
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Move this to just above the content settings loop

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:684: int zoom_width =
zoom_view_->GetPreferredSize().width();
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Move this to just above the content settings loop

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:176: void
SetPreviewEnabledPageAction(ExtensionAction *page_action,
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: While you're here, can you fix the "*" location?

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:188: // Sets the
tooltip for the zoom icon.
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Tiny nit: For ordering consistency, let's move these declarations/definitions
> above SetPreviewEnabledPageAction(), since the actual zoom icon is listed
before
> the page action icons.

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:194: // Show the zoom
bubble.
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Show -> Shows

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:30: // be updated.
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Not necessarily true.  The bubble could be shown with a different anchor than
> before.  For example, we hit ctrl-+ on one window and then quickly hit ctrl-+
on
> a different window.

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:31: SetImageSize(size);
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Is it possible to omit this, calculate it dynamically, etc.?

This cannot be ommitted because the image size must be set so that the location
bar has a proper width to allocate to the icon. There are two ways of going
about this: calling SetImageSize() as I did, or actually assigning an image to
the ImageView (in which case it will be calculated dynamically). That's what I
was doing before in the initializer list when I had initialized zoom_icon_state_
to ZoomController::ZOOM_PLUS_ICON. However, since you said that it was hacky to
do this when it should be set to ZoomController::NONE, there doesn't seem to be
a better way to do this.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:62: int resource_id =
zoom_icon_state_ == ZoomController::ZOOM_PLUS_ICON ?
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: You could also inline this into the next statement

Done.

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:76: if
(ZoomBubbleView::IsShowing())
On 2012/06/26 23:23:39, Peter Kasting wrote:
> Nit: Shorter:
> 
>   return !ZoomBubbleView::IsShowing() && ImageView::GetTooltipText(p,
tooltip);

Done.

[Peter Kasting, 2012-07-02 19:21:27]

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

http://codereview.chromium.org/10494004/diff/44001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.cc:31: SetImageSize(size);
On 2012/06/30 00:22:50, Kyle Horimoto wrote:
> On 2012/06/26 23:23:39, Peter Kasting wrote:
> > Nit: Is it possible to omit this, calculate it dynamically, etc.?
> 
> This cannot be ommitted because the image size must be set so that the
location
> bar has a proper width to allocate to the icon.

I don't think this is true once you fix the comments in location_bar_view.cc,
because then the location bar won't check the size of the zoom view until it's
made visible via Update(), which will also set an image.

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:669: entry_width -=
zoom_view_->GetPreferredSize().width() + GetItemPadding();
This should only happen if the zoom icon is visible, lest we leave an empty gap.

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:765: int zoom_width =
zoom_view_->GetPreferredSize().width();
This block should only happen if the zoom icon is visible, lest we leave an
empty gap.

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:28: // If the bubble is
already showing and its |auto_close_| value is equal to
Nit: Add "in this window" after "already showing".  I also suggest checking
anchor_view equality before auto_close equality below.

[Kyle Horimoto, 2012-07-02 21:09:02]

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:669: entry_width -=
zoom_view_->GetPreferredSize().width() + GetItemPadding();
On 2012/07/02 19:21:27, Peter Kasting wrote:
> This should only happen if the zoom icon is visible, lest we leave an empty
gap.

Done.

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:765: int zoom_width =
zoom_view_->GetPreferredSize().width();
On 2012/07/02 19:21:27, Peter Kasting wrote:
> This block should only happen if the zoom icon is visible, lest we leave an
> empty gap.

Done.

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10494004/diff/53008/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:28: // If the bubble is
already showing and its |auto_close_| value is equal to
On 2012/07/02 19:21:27, Peter Kasting wrote:
> Nit: Add "in this window" after "already showing".  I also suggest checking
> anchor_view equality before auto_close equality below.

Done.

[Peter Kasting, 2012-07-02 23:25:09]

LGTM

[commit-bot: I haz the power, 2012-07-02 23:30:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10494004/61002

[commit-bot: I haz the power, 2012-07-03 00:13:37]

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

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/khorimoto@chromium.org/10494004/61002

[commit-bot: I haz the power, 2012-07-03 04:52:31]

Change committed as 145259