Tracks changes to zoom icon and zoom percentage. Notifies browser window if the icon should be chan…

Tracks changes to zoom icon and zoom percentage. Notifies browser window if the icon should be changed or if the zoom bubble should be shown.

BUG=none
TEST=none


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

[tfarina, 2012-05-26 02:15:35]

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
File chrome/browser/ui/zoom/zoom_tab_helper_delegate.h (right):

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:23: protected:
nit: add a blank line above.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:24: virtual
~ZoomTabHelperDelegate();
nit: we implement this virtual dtor inline here, so no need to implement it in
the source file.

[Ben Goodger (Google), 2012-05-29 18:04:40]

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

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/browser.h#ne...
chrome/browser/ui/browser.h:95: public ZoomTabHelperDelegate,
rather than make Browser be the observer, and then have to wire the notification
through to the platform specific frontend bit, why not just have the specific
piece of platform frontend be the observer?

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

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/browser_wind...
chrome/browser/ui/browser_window.h:129: int zoomPercent) = 0;
zoom_percent, here and everywhere else.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
File chrome/browser/ui/zoom/zoom_tab_helper.h (right):

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:18: class ZoomTabHelper : public
content::NotificationObserver,
ZoomController

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:64: TabContentsWrapper*
tab_contents_wrapper_;
you don't appear to use this as a TabContentsWrapper, just as a
content::WebContents. So just store as a WebContents.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
File chrome/browser/ui/zoom/zoom_tab_helper_delegate.h (right):

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:14: class
ZoomTabHelperDelegate {
ZoomObserver

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:17: virtual void
ZoomIconStateChanged(TabContentsWrapper *source,
OnZoomIcon..

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:22: virtual void
ZoomChanged(TabContentsWrapper *source, int zoomPercent) = 0;
OnZoomChanged...

[tfarina, 2012-05-29 18:09:12]

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
File chrome/browser/ui/zoom/zoom_tab_helper.h (right):

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:24: enum ZoomIconState {
nit: this should goes first in this section (i.e, before the ctor and dtor).

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:30: ZoomIconState zoom_icon_status()
const { return zoom_icon_state_; }
nit: zoom_icon_state()

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:37: virtual void DidNavigateMainFrame(
I bet you can make this and Observe private.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:39: const content::FrameNavigateParams&
params) OVERRIDE;
include compiler_specific.h for OVERRIDE

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:57: // Registers and unregisters
notifications.
nit: I'd remove this comment, it doesn't add much, as the intention of
NotificationRegistrar is pretty obvious.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:63: // Owning TabContentsWrapper.
I guess you don't own TabContentsWrapper.

http://codereview.chromium.org/10444040/diff/1/chrome/browser/ui/zoom/zoom_ta...
chrome/browser/ui/zoom/zoom_tab_helper.h:69:
DISALLOW_COPY_AND_ASSIGN(ZoomTabHelper);
include basictypes.h for this.

[Kyle Horimoto, 2012-05-31 23:13:52]

Addressed comments. Please take another look, thanks!

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/brow...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser.h:95: public ZoomTabHelperDelegate,
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> rather than make Browser be the observer, and then have to wire the
notification
> through to the platform specific frontend bit, why not just have the specific
> piece of platform frontend be the observer?

As discussed offline, if the Browser is not the observer, there doesn't seem to
be a way to change the icon when 1) A tab is pulled out of a window into it's
own window, 2) A tab is popped into an existing window, or 3) The active tab on
a single window changes. I'll keep this implementation unless we can find a
better way to do this.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:129: int zoomPercent) = 0;
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> zoom_percent, here and everywhere else.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
File chrome/browser/ui/zoom/zoom_tab_helper.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:18: class ZoomTabHelper : public
content::NotificationObserver,
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> ZoomController

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:24: enum ZoomIconState {
On 2012/05/29 18:09:12, tfarina wrote:
> nit: this should goes first in this section (i.e, before the ctor and dtor).

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:30: ZoomIconState zoom_icon_status()
const { return zoom_icon_state_; }
On 2012/05/29 18:09:12, tfarina wrote:
> nit: zoom_icon_state()

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:37: virtual void DidNavigateMainFrame(
On 2012/05/29 18:09:12, tfarina wrote:
> I bet you can make this and Observe private.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:39: const content::FrameNavigateParams&
params) OVERRIDE;
On 2012/05/29 18:09:12, tfarina wrote:
> include compiler_specific.h for OVERRIDE

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:57: // Registers and unregisters
notifications.
On 2012/05/29 18:09:12, tfarina wrote:
> nit: I'd remove this comment, it doesn't add much, as the intention of
> NotificationRegistrar is pretty obvious.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:63: // Owning TabContentsWrapper.
On 2012/05/29 18:09:12, tfarina wrote:
> I guess you don't own TabContentsWrapper.

No, it owns this.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:64: TabContentsWrapper*
tab_contents_wrapper_;
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> you don't appear to use this as a TabContentsWrapper, just as a
> content::WebContents. So just store as a WebContents.

Actually, I do use it as a TabContentsWrapper. It gets passed back to Browser
through the delegate methods to ensure that the proper tab is being updated.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:69:
DISALLOW_COPY_AND_ASSIGN(ZoomTabHelper);
On 2012/05/29 18:09:12, tfarina wrote:
> include basictypes.h for this.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
File chrome/browser/ui/zoom/zoom_tab_helper_delegate.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:14: class
ZoomTabHelperDelegate {
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> ZoomObserver

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:17: virtual void
ZoomIconStateChanged(TabContentsWrapper *source,
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> OnZoomIcon..

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:22: virtual void
ZoomChanged(TabContentsWrapper *source, int zoomPercent) = 0;
On 2012/05/29 18:04:41, Ben Goodger (Google) wrote:
> OnZoomChanged...

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:23: protected:
On 2012/05/26 02:15:35, tfarina wrote:
> nit: add a blank line above.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper_delegate.h:24: virtual
~ZoomTabHelperDelegate();
On 2012/05/26 02:15:35, tfarina wrote:
> nit: we implement this virtual dtor inline here, so no need to implement it in
> the source file.

Done.

[tfarina, 2012-05-31 23:20:58]

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
File chrome/browser/ui/zoom/zoom_tab_helper.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:63: // Owning TabContentsWrapper.
On 2012/05/31 23:13:52, Kyle Horimoto wrote:
> On 2012/05/29 18:09:12, tfarina wrote:
> > I guess you don't own TabContentsWrapper.
> 
> No, it owns this.

You don't own TabContentsWrapper. This is a weak pointer. You don't manage the
lifetime of this pointer, do you? Otherwise this would be TabContentsWrapper
wrapper_; or scoped_ptr<TabContentsWrapper> wrapper_;

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.cc:61: double currentZoomLevel =
nit: current_zoom_level. Please fix the other occurrences below. We use
unix_hacker style for naming variables.

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.cc:85: if (delegate())
nit: use direct access, i.e delegate_, this is the preferred way, instead of
calling the functions accessor.

[Kyle Horimoto, 2012-05-31 23:37:20]

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
File chrome/browser/ui/zoom/zoom_tab_helper.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/1/chrome/browser/ui/zoom...
chrome/browser/ui/zoom/zoom_tab_helper.h:63: // Owning TabContentsWrapper.
On 2012/05/31 23:21:00, tfarina wrote:
> On 2012/05/31 23:13:52, Kyle Horimoto wrote:
> > On 2012/05/29 18:09:12, tfarina wrote:
> > > I guess you don't own TabContentsWrapper.
> > 
> > No, it owns this.
> 
> You don't own TabContentsWrapper. This is a weak pointer. You don't manage the
> lifetime of this pointer, do you? Otherwise this would be TabContentsWrapper
> wrapper_; or scoped_ptr<TabContentsWrapper> wrapper_;

Not quite sure what you're trying to say here. This TabContentsWrapper owns the
current object (i.e., has an instance of this class as an ivar). It allocates an
instance of this class in its constructor and destroys it in the destructor, so
it is always created before this object is created and destroyed after this
object is destroyed. Don't think we need to worry about the lifetime of the
pointer.

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.cc:61: double currentZoomLevel =
On 2012/05/31 23:21:00, tfarina wrote:
> nit: current_zoom_level. Please fix the other occurrences below. We use
> unix_hacker style for naming variables.

Done.

https://chromiumcodereview.appspot.com/10444040/diff/8002/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.cc:85: if (delegate())
On 2012/05/31 23:21:00, tfarina wrote:
> nit: use direct access, i.e delegate_, this is the preferred way, instead of
> calling the functions accessor.

Done.

[tfarina, 2012-05-31 23:52:17]

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_controller.h:36: void set_delegate(ZoomObserver* d)
{ delegate_ = d; }
nit: set_observer, also please avoid abbreviations s/d/observer

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_controller.h:67: // Delegate receiving notifications
on state changes.
nit: s/Delegate/Observer

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_controller.h:68: ZoomObserver* delegate_;
nit: s/delegate_/observer_

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
File chrome/browser/ui/zoom/zoom_observer.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_observer.h:17: virtual void
OnZoomIconChanged(TabContentsWrapper *source,
nit: TabContentsWrapper* source, below too.

[Kyle Horimoto, 2012-06-01 00:33:08]

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_controller.h:36: void set_delegate(ZoomObserver* d)
{ delegate_ = d; }
On 2012/05/31 23:52:17, tfarina wrote:
> nit: set_observer, also please avoid abbreviations s/d/observer

Done.

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_controller.h:68: ZoomObserver* delegate_;
On 2012/05/31 23:52:17, tfarina wrote:
> nit: s/delegate_/observer_

Done.

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
File chrome/browser/ui/zoom/zoom_observer.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/11002/chrome/browser/ui/...
chrome/browser/ui/zoom/zoom_observer.h:17: virtual void
OnZoomIconChanged(TabContentsWrapper *source,
On 2012/05/31 23:52:17, tfarina wrote:
> nit: TabContentsWrapper* source, below too.

Done.

[tfarina, 2012-06-01 00:47:53]

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/b...
chrome/browser/ui/browser.h:98: public ZoomObserver,
To be honest I think ZoomTabHelper and ZoomTabHelperDelegate is a more much
consistent naming scheme given the above. Ben, thoughts? I know you asked him to
rename, I wonder why ;)

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_cocoa.h:62: virtual void
ShowZoomBubble(int zoomPercent) OVERRIDE;
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:256: int zoomPercent) {
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/c...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:264: void
BrowserWindowCocoa::ShowZoomBubble(int zoomPercent) {
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_window_gtk.cc:897: void
BrowserWindowGtk::ShowZoomBubble(int zoomPercent) {
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_window_gtk.h:92: virtual void ShowZoomBubble(int
zoomPercent) OVERRIDE;
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/v...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/v...
chrome/browser/ui/views/frame/browser_view.cc:735: void
BrowserView::ShowZoomBubble(int zoomPercent) {
nit: zoom_percent

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.cc:51: // Fall through.
nit: fix indentation here.

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.h:35: ZoomObserver* observer() const {
return observer_; }
do you need to expose the observer?

[tfarina, 2012-06-01 00:52:37]

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
File chrome/browser/ui/zoom/zoom_controller.h (right):

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.h:54: ZoomIconState zoom_icon_state_;
nit: initialize this in the constructor initializer list.

https://chromiumcodereview.appspot.com/10444040/diff/7003/chrome/browser/ui/z...
chrome/browser/ui/zoom/zoom_controller.h:57: int zoom_percent_;
nit: initialize this in the constructor initializer list.

[Kyle Horimoto, 2012-06-01 18:04:49]

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

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:98: public ZoomObserver,
On 2012/06/01 00:47:53, tfarina wrote:
> To be honest I think ZoomTabHelper and ZoomTabHelperDelegate is a more much
> consistent naming scheme given the above. Ben, thoughts? I know you asked him
to
> rename, I wonder why ;)

I'd also be interested to hear the reasoning. Ben?

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.h:62: virtual void
ShowZoomBubble(int zoomPercent) OVERRIDE;
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:256: int zoomPercent) {
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Oops, that wasn't even supposed to be there at all. Guess I didn't catch that
since I didn't compile on Mac.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:264: void
BrowserWindowCocoa::ShowZoomBubble(int zoomPercent) {
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.cc:897: void
BrowserWindowGtk::ShowZoomBubble(int zoomPercent) {
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.h:92: virtual void ShowZoomBubble(int
zoomPercent) OVERRIDE;
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:735: void
BrowserView::ShowZoomBubble(int zoomPercent) {
On 2012/06/01 00:47:53, tfarina wrote:
> nit: zoom_percent

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
chrome/browser/ui/zoom/zoom_controller.cc:51: // Fall through.
On 2012/06/01 00:47:53, tfarina wrote:
> nit: fix indentation here.

Done.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
File chrome/browser/ui/zoom/zoom_controller.h (right):

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
chrome/browser/ui/zoom/zoom_controller.h:35: ZoomObserver* observer() const {
return observer_; }
On 2012/06/01 00:47:53, tfarina wrote:
> do you need to expose the observer?

Nope - deleted that. Thanks.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
chrome/browser/ui/zoom/zoom_controller.h:54: ZoomIconState zoom_icon_state_;
On 2012/06/01 00:52:37, tfarina wrote:
> nit: initialize this in the constructor initializer list.

Done. Although it really doesn't matter since UpdateState() is called from the
constructor and immediately changes the value if needed anyway.

http://codereview.chromium.org/10444040/diff/7003/chrome/browser/ui/zoom/zoom...
chrome/browser/ui/zoom/zoom_controller.h:57: int zoom_percent_;
On 2012/06/01 00:52:37, tfarina wrote:
> nit: initialize this in the constructor initializer list.

Done. Although it really doesn't matter since UpdateState() is called from the
constructor and immediately changes the value if needed anyway.

[Ben Goodger (Google), 2012-06-04 15:11:05]

OK I thought about this over the weekend, and I've not been able to come up with
a better way of doing this other than a larger refactoring of the way state is
updated from Browser, which I'm not going to request here.

LGTM as-is.

[commit-bot: I haz the power, 2012-06-04 17:20:20]

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

[commit-bot: I haz the power, 2012-06-04 19:00:12]

Change committed as 140339