Refactor browser window zoom handling and enable zoom icon on all platforms.

Refactor browser window zoom handling and enable zoom icon on all platforms.

On all platforms:
After this CL there should be zoom icon in the omnibox when on a page with non-default zoom (- for less than default, + for more than default). If the zoom is reset with Ctrl+0, no bubble should show up and the icon should disappear. If tabs with different zooms are switched between, the zoom icon should update itself for each tab without showing a bubble.

On Views and GTK:
While changing zoom with Ctrl +/-, a zoom bubble should show up if you zoomed *to* a non-default level. If you quickly change to a different tab after changing the zoom level, the bubble should disappear.
 
BUG=86817, 128817
TEST=Compiles, passes tests, doesn't regress current zoom functionality.
TODO=Zoom *bubble* on Mac.

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

[Evan Stade, 2012-07-13 05:31:00]

this looks ok to me. Ben should be the reviewer though.

http://codereview.chromium.org/10736028/diff/4001/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/10736028/diff/4001/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:126: // Called when the zoom in the active
tab is changed.
docs for can_show_bubble.

http://codereview.chromium.org/10736028/diff/4001/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10736028/diff/4001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1266:
TabContents::FromWebContents(GetWebContents())->zoom_controller();
why not GetTabContents?

[Dan Beam, 2012-07-24 03:00:49]

+khorimoto before I bring in the platform specific reviewers. also, obviously
I'll wait to see what comes of your CL with the views bubble and rebase that
onto this.

https://chromiumcodereview.appspot.com/10736028/diff/4001/chrome/browser/ui/b...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/4001/chrome/browser/ui/b...
chrome/browser/ui/browser_window.h:126: // Called when the zoom in the active
tab is changed.
On 2012/07/13 05:31:00, Evan Stade wrote:
> docs for can_show_bubble.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/4001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/4001/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1266:
TabContents::FromWebContents(GetWebContents())->zoom_controller();
On 2012/07/13 05:31:00, Evan Stade wrote:
> why not GetTabContents?

Done.

[Kyle Horimoto, 2012-07-24 03:22:28]

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:99: // change (e.g.
switching tabs, creating a new tab, creating a new browser).
Nit: Get rid of the ambiguous "interesting" vs. "not interesting" wording and
simply state when |can_show_bubble| is true or false.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1233: UpdateZoomIcon();
OnZoomButtonPress() should display the zoom bubble. Unless I'm reading this
incorrectly, this does not display the zoom bubble.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1300: int zoom_resource =
zoom_percent > 100 ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
This is incorrect: zoom_resource should be IDR_ZOOM_PLUS when zoom_percent >
default_zoom_percent, which we cannot assume is 100.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/zoom_view.cc:37: zoom_percent > 100 ?
IDR_ZOOM_PLUS : IDR_ZOOM_MINUS));
Same thing with 100 vs. default zoom percent.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/zoom_view.h (left):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/zoom_view.h:36: virtual bool
OnMousePressed(const views::MouseEvent& event) OVERRIDE;
Why are we getting rid of all these mouse event handlers? These are used to show
the zoom bubble when the ZoomView is clicked.

[Kyle Horimoto, 2012-07-24 20:08:11]

http://codereview.chromium.org/10736028/diff/31002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10736028/diff/31002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1737:
window_->ZoomChangedForActiveTab(can_show_bubble && window_->IsActive());
Also, is there a better method name we can think of? Because I wrote a lot of
this code, I understand that "ActiveTab" means the whatever is the current tab
being displayed in the browser window, regardless of whether that window is
active or not. However, I can definitely see someone unfamiliar with the code
not realizing that this method will also be called on windows that are not the
active window.

[Evan Stade, 2012-07-24 20:12:49]

gtk lgtm

http://codereview.chromium.org/10736028/diff/31002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10736028/diff/31002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1737:
window_->ZoomChangedForActiveTab(can_show_bubble && window_->IsActive());
On 2012/07/24 20:08:11, Kyle Horimoto wrote:
> Also, is there a better method name we can think of? Because I wrote a lot of
> this code, I understand that "ActiveTab" means the whatever is the current tab
> being displayed in the browser window, regardless of whether that window is
> active or not. However, I can definitely see someone unfamiliar with the code
> not realizing that this method will also be called on windows that are not the
> active window.

I think it's fine because "active" is the term that's used throughout Browser
and BrowserWindow code to refer to "foreground". (Although foreground is even
better.)

[Dan Beam, 2012-08-04 03:24:20]

I will be resuming work on this now that downstream CLs have landed, sorry for
the delay (and will be adding all kind of fun reviewers soon!).

[Dan Beam, 2012-08-15 09:04:58]

so this has changed a bit, might want to take another glance (estade +
khorimoto).  I'll add mac reviewers (shess@, rohitrao@) back now that I've been
able to do the refactor they (and others) wanted in the first place.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:99: // change (e.g.
switching tabs, creating a new tab, creating a new browser).
On 2012/07/24 03:22:28, Kyle Horimoto wrote:
> Nit: Get rid of the ambiguous "interesting" vs. "not interesting" wording and
> simply state when |can_show_bubble| is true or false.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1233: UpdateZoomIcon();
On 2012/07/24 03:22:28, Kyle Horimoto wrote:
> OnZoomButtonPress() should display the zoom bubble. Unless I'm reading this
> incorrectly, this does not display the zoom bubble.

This works again.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1300: int zoom_resource =
zoom_percent > 100 ? IDR_ZOOM_PLUS : IDR_ZOOM_MINUS;
On 2012/07/24 03:22:28, Kyle Horimoto wrote:
> This is incorrect: zoom_resource should be IDR_ZOOM_PLUS when zoom_percent >
> default_zoom_percent, which we cannot assume is 100.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/zoom_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/zoom_view.cc:37: zoom_percent > 100 ?
IDR_ZOOM_PLUS : IDR_ZOOM_MINUS));
On 2012/07/24 03:22:28, Kyle Horimoto wrote:
> Same thing with 100 vs. default zoom percent.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/zoom_view.h (left):

https://chromiumcodereview.appspot.com/10736028/diff/31002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/zoom_view.h:36: virtual bool
OnMousePressed(const views::MouseEvent& event) OVERRIDE;
On 2012/07/24 03:22:28, Kyle Horimoto wrote:
> Why are we getting rid of all these mouse event handlers? These are used to
show
> the zoom bubble when the ZoomView is clicked.

I've added them back now that the code compiles with the new API, ;).

[Dan Beam, 2012-08-15 09:04:59]

so this has changed a bit, might want to take another glance (estade +
khorimoto).  I'll add mac reviewers (shess@, rohitrao@) back now that I've been
able to do the refactor they (and others) wanted in the first place.

[Evan Stade, 2012-08-15 22:01:34]

https://chromiumcodereview.appspot.com/10736028/diff/47001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (left):

https://chromiumcodereview.appspot.com/10736028/diff/47001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1396:
ZoomBubbleGtk::Show(zoom_.get(), GetTabContents(), false);
false is important

[Kyle Horimoto, 2012-08-15 22:16:21]

non-mac lgtm

[Dan Beam, 2012-08-15 23:13:37]

https://chromiumcodereview.appspot.com/10736028/diff/47001/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (left):

https://chromiumcodereview.appspot.com/10736028/diff/47001/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1396:
ZoomBubbleGtk::Show(zoom_.get(), GetTabContents(), false);
On 2012/08/15 22:01:34, Evan Stade wrote:
> false is important

Done. (reverted this change)

[Dan Beam, 2012-08-15 23:26:01]

+pkasting for chrome/browser/ui/views and chrome/browser/ui/*.(h|cc)

https://chromiumcodereview.appspot.com/10736028/diff/35070/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/35070/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:401:
ZoomBubbleView::CloseBubble();
NOTE: yes, this is just a patch in of
https://chromiumcodereview.appspot.com/10854169/, will resolve when that lands

[Evan Stade, 2012-08-16 00:24:33]

gtk lgtm

[Peter Kasting, 2012-08-16 04:04:47]

LGTM

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:122: // Called when the zoom in the active
tab is changed. |can_show_bubble| will
Nit: Technically this is more like "Called to force the zoom state for the
active tab to be recalculated", since the call to this in browser_commands.cc is
basically a "set your baseline state correctly" call rather than a true zoom
change.  But up to you whether you think calling this out is important.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:124: // based on a user action, not switching
tabs nor from the zoom widget in the
Nit: a user action -> pressing the zoom up/zoom down keyboard shortcuts

(since the other things you mention are also user actions)

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:517: if
(can_show_bubble && zoom_view_ && zoom_view_->visible())
Nit: You don't need to check |zoom_view_| unless this can somehow be called
before the location bar view is actually in a view hierarchy.  (In which case,
we should probably fix it to not be called.)

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:523: if (!zoom_view_
|| !tab_contents)
Nit: Shorter:

  zoom_view_->Update(GetTabContents()->zoom_controller());

zoom_view_ is non-NULL for the reason given above.  GetTabContents() is also
expected to be non-NULL most places in this file.  (I don't know why we're not
consistent, unless there are actually a few codepaths that are somehow
legitimately reachable with no TabContents... maybe that's some sort of testing
state?)

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.h:190: // and there are
no other things showing in the bubble's way.
Nit: TBH I'd scrap this whole comment and just say "See comment in
browser_window.h.", since I think the comment there is better.

[Scott Hess - ex-Googler, 2012-08-16 04:52:58]

LGTM for ui/cocoa, minor suggestions.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:474: void
LocationBarViewMac::OnImageDecorationsChanged() {
Suggest OnDecorationsChanged(), I think the fact that the spots you're replacing
are all image decorations is probably coincidental.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: if
(!zoom_decoration_.get() || !tab_contents)
zoom_decoration_ can't be NULL, AFAICT.

I'm neutral on checking or not checking tab_contents.  I seem to recall past
arguments with Peter about whether it's really necessary, but nobody ever seemed
to conclusively demonstrate necessary or not-necessary.  So poke around the file
and choose whichever convention seems reasonable.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.h:17: // non-standard zoom
level. TODO(dbeam): add mouse event handlers to show a zoom
TODO() should start a new line, so that it's easy to delete it when you do it.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:36: IDS_TOOLTIP_ZOOM,
base::IntToString16(zoom_percent)) retain]);
There's a lot going on in this line, I think it might benefit from some
temporary variables to help document things.

[Dan Beam, 2012-08-16 06:52:28]

addressing pkasting@ review feedback

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:122: // Called when the zoom in the active
tab is changed. |can_show_bubble| will
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: Technically this is more like "Called to force the zoom state for the
> active tab to be recalculated", since the call to this in browser_commands.cc
is
> basically a "set your baseline state correctly" call rather than a true zoom
> change.  But up to you whether you think calling this out is important.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:124: // based on a user action, not switching
tabs nor from the zoom widget in the
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: a user action -> pressing the zoom up/zoom down keyboard shortcuts
> 
> (since the other things you mention are also user actions)

Done.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:517: if
(can_show_bubble && zoom_view_ && zoom_view_->visible())
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: You don't need to check |zoom_view_| unless this can somehow be called
> before the location bar view is actually in a view hierarchy.  (In which case,
> we should probably fix it to not be called.)

Changed these to DCHECK()s instead as you're right that it'd be better to only
call this when there is a zoom_view_ and assert it's non-NULL.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:523: if (!zoom_view_
|| !tab_contents)
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: Shorter:
> 
>   zoom_view_->Update(GetTabContents()->zoom_controller());
> 
> zoom_view_ is non-NULL for the reason given above.  GetTabContents() is also
> expected to be non-NULL most places in this file.  (I don't know why we're not
> consistent, unless there are actually a few codepaths that are somehow
> legitimately reachable with no TabContents... maybe that's some sort of
testing
> state?)

I think khorimoto@ mentioned that the browser gets created at first without
tabcontents' initialized.  If this isn't too hard to fix/doesn't make the code
ugly, I can update this to only get called when we expect and change to
DCHECK(), but I have a feeling there's a reason for this behavior...

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.h:190: // and there are
no other things showing in the bubble's way.
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: TBH I'd scrap this whole comment and just say "See comment in
> browser_window.h.", since I think the comment there is better.

Agreed, was tired of updating these in like 12 places anyways, :).  Done.

[Dan Beam, 2012-08-16 06:52:29]

addressing pkasting@ review feedback

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:122: // Called when the zoom in the active
tab is changed. |can_show_bubble| will
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: Technically this is more like "Called to force the zoom state for the
> active tab to be recalculated", since the call to this in browser_commands.cc
is
> basically a "set your baseline state correctly" call rather than a true zoom
> change.  But up to you whether you think calling this out is important.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/browser_window.h:124: // based on a user action, not switching
tabs nor from the zoom widget in the
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: a user action -> pressing the zoom up/zoom down keyboard shortcuts
> 
> (since the other things you mention are also user actions)

Done.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:517: if
(can_show_bubble && zoom_view_ && zoom_view_->visible())
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: You don't need to check |zoom_view_| unless this can somehow be called
> before the location bar view is actually in a view hierarchy.  (In which case,
> we should probably fix it to not be called.)

Changed these to DCHECK()s instead as you're right that it'd be better to only
call this when there is a zoom_view_ and assert it's non-NULL.

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:523: if (!zoom_view_
|| !tab_contents)
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: Shorter:
> 
>   zoom_view_->Update(GetTabContents()->zoom_controller());
> 
> zoom_view_ is non-NULL for the reason given above.  GetTabContents() is also
> expected to be non-NULL most places in this file.  (I don't know why we're not
> consistent, unless there are actually a few codepaths that are somehow
> legitimately reachable with no TabContents... maybe that's some sort of
testing
> state?)

I think khorimoto@ mentioned that the browser gets created at first without
tabcontents' initialized.  If this isn't too hard to fix/doesn't make the code
ugly, I can update this to only get called when we expect and change to
DCHECK(), but I have a feeling there's a reason for this behavior...

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/44032/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.h:190: // and there are
no other things showing in the bubble's way.
On 2012/08/16 04:04:48, Peter Kasting wrote:
> Nit: TBH I'd scrap this whole comment and just say "See comment in
> browser_window.h.", since I think the comment there is better.

Agreed, was tired of updating these in like 12 places anyways, :).  Done.

[Dan Beam, 2012-08-16 08:55:03]

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:474: void
LocationBarViewMac::OnImageDecorationsChanged() {
On 2012/08/16 04:52:58, shess wrote:
> Suggest OnDecorationsChanged(), I think the fact that the spots you're
replacing
> are all image decorations is probably coincidental.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: if
(!zoom_decoration_.get() || !tab_contents)
On 2012/08/16 04:52:58, shess wrote:
> zoom_decoration_ can't be NULL, AFAICT.

Done.

> I'm neutral on checking or not checking tab_contents.  I seem to recall past
> arguments with Peter about whether it's really necessary, but nobody ever
seemed
> to conclusively demonstrate necessary or not-necessary.  So poke around the
file
> and choose whichever convention seems reasonable.

khorimoto@ mentioned that he got segfaults sometimes when he just assumed the
tabcontents was there, I'll try to figure out how this happens.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.h (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.h:17: // non-standard zoom
level. TODO(dbeam): add mouse event handlers to show a zoom
On 2012/08/16 04:52:58, shess wrote:
> TODO() should start a new line, so that it's easy to delete it when you do it.

Done.

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:36: IDS_TOOLTIP_ZOOM,
base::IntToString16(zoom_percent)) retain]);
On 2012/08/16 04:52:58, shess wrote:
> There's a lot going on in this line, I think it might benefit from some
> temporary variables to help document things.

Done.

[Dan Beam, 2012-08-16 15:46:12]

+sky for chrome/test/* and chrome/*.gypi

[sky, 2012-08-16 16:44:56]

LGTM

[Scott Hess - ex-Googler, 2012-08-16 20:09:31]

still LGTM for ui/cocoa/

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10736028/diff/34031/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:751: if
(!zoom_decoration_.get() || !tab_contents)
On 2012/08/16 08:55:03, Dan Beam wrote:
> On 2012/08/16 04:52:58, shess wrote:
> > I'm neutral on checking or not checking tab_contents.  I seem to recall past
> > arguments with Peter about whether it's really necessary, but nobody ever
> seemed
> > to conclusively demonstrate necessary or not-necessary.  So poke around the
> file
> > and choose whichever convention seems reasonable.
> 
> khorimoto@ mentioned that he got segfaults sometimes when he just assumed the
> tabcontents was there, I'll try to figure out how this happens.

Seems reasonable.  This class is a mess.

[commit-bot: I haz the power, 2012-08-17 06:33:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10736028/37106

[commit-bot: I haz the power, 2012-08-17 13:46:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/10736028/37106

[commit-bot: I haz the power, 2012-08-17 13:47:22]

Change committed as 152080