Implements the "Set to default" button on the zoom bubble.

Implements the "Set to default" button on the zoom bubble.

BUG=128816
TEST=


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

[tfarina, 2012-07-17 05:57:51]

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:38: // ButtonListener
method.
nit: views::ButtonListener

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:39: virtual void
ButtonPressed(views::Button* sender,
can you move this declaration right after the Close()? So we keep virtuals ones
separated from the instance methods.

[Peter Kasting, 2012-07-17 06:45:06]

There are no mocks available on the bug.  Can you post some screenshots, and if
possible any mocks you got from the UI designers?

[Kyle Horimoto, 2012-07-17 18:26:23]

Oops, sorry pkasting. Uploaded the screenshot & mock to the bug report.

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (right):

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:38: // ButtonListener
method.
On 2012/07/17 05:57:56, tfarina wrote:
> nit: views::ButtonListener

Done.

http://codereview.chromium.org/10792020/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:39: virtual void
ButtonPressed(views::Button* sender,
On 2012/07/17 05:57:56, tfarina wrote:
> can you move this declaration right after the Close()? So we keep virtuals
ones
> separated from the instance methods.

Done.

[Peter Kasting, 2012-07-17 23:14:08]

I added a few comments on the bug.  I will also review this in a bit as I don't
think any of my comments there impact this review.

[Peter Kasting, 2012-07-18 03:26:30]

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:98: const TabContents*
tab_contents = delegate->GetTabContents();
Nit: While you're touching this file, can you maybe make it so we're consistent
about using either GetTabContents() or delegate_->GetTabContents()?

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:276: // for the
appropriate TabContents.
Nit: This seems a little sketchy since apparently this "assigning" action
happens completely outside our control or visibility -- later we just happily
call GetWebContentsFromDelegate() on the delegate as well as
delegate_->GetTabContents().  Is it possible to make the tab contents be
non-NULL at the time the delegate is passed in to this object so there isn't a
window like this?  If not, is there a specific time where we know the tab
contents will exist so we can e.g. DCHECK that in GetTabContents() and refer to
it in the comments here?

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:521: void
LocationBarView::ShowZoomBubble(int zoom_percent) {
Nit: If this argument is no longer used, can we un-plumb it?

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:29: const int
kPercentageSeparatorSpacing = 7;
These should be using standardized values from layout_constants.h, e.g.
kRelatedControlVerticalSpacing.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:35: const int
kPercentageFontSize = 20;
This should not be hard-coded, because otherwise it won't scale as the user
scales up/down their system font size.  Instead you should use DeriveFont() to
offset the default font size by some amount.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:110:
tab_contents_->web_contents()->GetBrowserContext())->
Nit: Just use tab_contents_->profile()

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:127: if (auto_close_) {
Wait, so in the auto-close case, no button is available?  Why not?  Seems like
not only would it be potentially useful as-is but would also inform users that
they can reopen the bubble to access that functionality.

Also, if we do this, then so long as the user is hovering the text button, we
shouldn't run the timer (and we should reset it when the mouse moves off the
text button so the bubble doesn't instantly disappear).

Also, even if we don't do this, if the auto-close variant bubble is showing and
I click on the icon in the omnibox, do I then get the non-auto-close variant?

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:135:
container->SetLayoutManager(new views::BoxLayout(
Why do you need a child container with its own layout manager?  Why can't the
separator and text button be added as direct children?

[Kyle Horimoto, 2012-07-18 23:49:38]

Addressed all comments. However, there is one thing that is working incorrectly:
when you mouse over the percentage label, padding, or separator, everything
works as expected. However, mousing over the button causes OnMouseExited() to be
fired, even though it is still within the bubble view. Any ideas as to why this
would be? Thanks!

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:98: const TabContents*
tab_contents = delegate->GetTabContents();
On 2012/07/18 03:26:30, Peter Kasting wrote:
> Nit: While you're touching this file, can you maybe make it so we're
consistent
> about using either GetTabContents() or delegate_->GetTabContents()?

Done. But FYI, this specific case is not a member function of LocationBarView,
so we shouldn't be doing that here.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:276: // for the
appropriate TabContents.
On 2012/07/18 03:26:30, Peter Kasting wrote:
> Nit: This seems a little sketchy since apparently this "assigning" action
> happens completely outside our control or visibility -- later we just happily
> call GetWebContentsFromDelegate() on the delegate as well as
> delegate_->GetTabContents().  Is it possible to make the tab contents be
> non-NULL at the time the delegate is passed in to this object so there isn't a
> window like this?  If not, is there a specific time where we know the tab
> contents will exist so we can e.g. DCHECK that in GetTabContents() and refer
to
> it in the comments here?

Hmm, not exactly. ToolbarView creates a LocationBarView when the window is
created. At this point, we have only created the container (window) for a
TabContents, not the actual TabContents (which is why we can't just call
GetTabContents() here). After the window is set up,
TabStripModel::AppendTabContents() is called with the current tab contents
(e.g., the website being viewed or the NTP). At this point, |delegate_| (which
is the ToolbarView that created the LocationBarView) can get a reference to the
TabContents. Unfortunately, there doesn't seem to be a way to check this except
to DCHECK() that the pointer returned by GetTabContents() is non-NULL :/. Should
I just do that?

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:521: void
LocationBarView::ShowZoomBubble(int zoom_percent) {
On 2012/07/18 03:26:30, Peter Kasting wrote:
> Nit: If this argument is no longer used, can we un-plumb it?

This is already being done: http://codereview.chromium.org/10736028/.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:29: const int
kPercentageSeparatorSpacing = 7;
On 2012/07/18 03:26:30, Peter Kasting wrote:
> These should be using standardized values from layout_constants.h, e.g.
> kRelatedControlVerticalSpacing.

Done.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:35: const int
kPercentageFontSize = 20;
On 2012/07/18 03:26:30, Peter Kasting wrote:
> This should not be hard-coded, because otherwise it won't scale as the user
> scales up/down their system font size.  Instead you should use DeriveFont() to
> offset the default font size by some amount.

Done.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:110:
tab_contents_->web_contents()->GetBrowserContext())->
On 2012/07/18 03:26:30, Peter Kasting wrote:
> Nit: Just use tab_contents_->profile()

Done.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:127: if (auto_close_) {
On 2012/07/18 03:26:30, Peter Kasting wrote:
> Wait, so in the auto-close case, no button is available?  Why not?  Seems like
> not only would it be potentially useful as-is but would also inform users that
> they can reopen the bubble to access that functionality.
> 
> Also, if we do this, then so long as the user is hovering the text button, we
> shouldn't run the timer (and we should reset it when the mouse moves off the
> text button so the bubble doesn't instantly disappear).
> 
> Also, even if we don't do this, if the auto-close variant bubble is showing
and
> I click on the icon in the omnibox, do I then get the non-auto-close variant?

Done.

http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:135:
container->SetLayoutManager(new views::BoxLayout(
On 2012/07/18 03:26:31, Peter Kasting wrote:
> Why do you need a child container with its own layout manager?  Why can't the
> separator and text button be added as direct children?

This was so that there could be a different spacing between the percentage label
and the separator and the separator and the button. However, I agree with using
the standard spacing as you expressed earlier, so I'll remove this.

[Peter Kasting, 2012-07-19 03:02:38]

On 2012/07/18 23:49:38, Kyle Horimoto wrote:
> when you mouse over the percentage label, padding, or separator, everything
> works as expected. However, mousing over the button causes OnMouseExited() to
be
> fired, even though it is still within the bubble view. Any ideas as to why
this
> would be? Thanks!

Debug the callstack when OnMouseExited is fired.  I believe this is called
because we do something like recursively ask views (down the parent->child
chain) whether the mouse is inside them or not, and so there ought to be a way
for you to correct this case by providing the relevant handler on the containing
view that just returns true without checking whether the mouse is inside the
button.  Or something like that.

>
http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:276: // for the
> appropriate TabContents.
> On 2012/07/18 03:26:30, Peter Kasting wrote:
> > Nit: This seems a little sketchy since apparently this "assigning" action
> > happens completely outside our control or visibility -- later we just
happily
> > call GetWebContentsFromDelegate() on the delegate as well as
> > delegate_->GetTabContents().  Is it possible to make the tab contents be
> > non-NULL at the time the delegate is passed in to this object so there isn't
a
> > window like this?  If not, is there a specific time where we know the tab
> > contents will exist so we can e.g. DCHECK that in GetTabContents() and refer
> to
> > it in the comments here?
> 
> Hmm, not exactly. ToolbarView creates a LocationBarView when the window is
> created. At this point, we have only created the container (window) for a
> TabContents, not the actual TabContents (which is why we can't just call
> GetTabContents() here). After the window is set up,
> TabStripModel::AppendTabContents() is called with the current tab contents
> (e.g., the website being viewed or the NTP). At this point, |delegate_| (which
> is the ToolbarView that created the LocationBarView) can get a reference to
the
> TabContents. Unfortunately, there doesn't seem to be a way to check this
except
> to DCHECK() that the pointer returned by GetTabContents() is non-NULL :/.
Should
> I just do that?

Maybe take out the comment here (since ZoomView doesn't try to access the
delegate's TabContents immediately anyway) and add a comment in ZoomView
somewhere appropriate noting that the delegate's TabContents (a) varies as the
user changes tabs, so we need to fetch it explicitly instead of getting it in
the constructor, and (b) is guaranteed to be non-NULL by the time we allow
clicks on the ZoomView.

By the way, you should probably ensure that second condition is true and that
there isn't some weird race where if the TabContents is really slow to attach
the ZoomView could actually be clicked with no tab.

Also, this reminds me.  If the bubble is showing and the user changes tabs, do
we close it immediately?  If not, we should.

[Kyle Horimoto, 2012-07-23 21:54:07]

Comments inline. Additionally, I realized that passing the delegate rather than
the actual TabContents is actually a necessity for correctness, not just a hack.
If we were to pass the original TabContents at construction time, then these
TabContents would not change each time the tab is changed. Thus, the zoom
percentage shown would not be accurate, and the "Set to default" button would
not change zoom on the appropriate tab.

On 2012/07/19 03:02:38, Peter Kasting wrote:
> On 2012/07/18 23:49:38, Kyle Horimoto wrote:
> > when you mouse over the percentage label, padding, or separator, everything
> > works as expected. However, mousing over the button causes OnMouseExited()
to
> be
> > fired, even though it is still within the bubble view. Any ideas as to why
> this
> > would be? Thanks!
> 
> Debug the callstack when OnMouseExited is fired.  I believe this is called
> because we do something like recursively ask views (down the parent->child
> chain) whether the mouse is inside them or not, and so there ought to be a way
> for you to correct this case by providing the relevant handler on the
containing
> view that just returns true without checking whether the mouse is inside the
> button.  Or something like that.

Done.

>
http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
> > chrome/browser/ui/views/location_bar/location_bar_view.cc:276: // for the
> > appropriate TabContents.
> > On 2012/07/18 03:26:30, Peter Kasting wrote:
> > > Nit: This seems a little sketchy since apparently this "assigning" action
> > > happens completely outside our control or visibility -- later we just
> happily
> > > call GetWebContentsFromDelegate() on the delegate as well as
> > > delegate_->GetTabContents().  Is it possible to make the tab contents be
> > > non-NULL at the time the delegate is passed in to this object so there
isn't
> a
> > > window like this?  If not, is there a specific time where we know the tab
> > > contents will exist so we can e.g. DCHECK that in GetTabContents() and
refer
> > to
> > > it in the comments here?
> > 
> > Hmm, not exactly. ToolbarView creates a LocationBarView when the window is
> > created. At this point, we have only created the container (window) for a
> > TabContents, not the actual TabContents (which is why we can't just call
> > GetTabContents() here). After the window is set up,
> > TabStripModel::AppendTabContents() is called with the current tab contents
> > (e.g., the website being viewed or the NTP). At this point, |delegate_|
(which
> > is the ToolbarView that created the LocationBarView) can get a reference to
> the
> > TabContents. Unfortunately, there doesn't seem to be a way to check this
> except
> > to DCHECK() that the pointer returned by GetTabContents() is non-NULL :/.
> Should
> > I just do that?
> 
> Maybe take out the comment here (since ZoomView doesn't try to access the
> delegate's TabContents immediately anyway) and add a comment in ZoomView
> somewhere appropriate noting that the delegate's TabContents (a) varies as the
> user changes tabs, so we need to fetch it explicitly instead of getting it in
> the constructor, and (b) is guaranteed to be non-NULL by the time we allow
> clicks on the ZoomView.

Moved the comment to ZoomView.

> By the way, you should probably ensure that second condition is true and that
> there isn't some weird race where if the TabContents is really slow to attach
> the ZoomView could actually be clicked with no tab.

This condition is true. The TabContents must be set before the page is
displayed. The page must be displayed for its zoom to be changed. The zoom icon
will not be shown unless it listens to zoom change notifications. Thus,
TabContents must be set in order for this code to run.

> Also, this reminds me.  If the bubble is showing and the user changes tabs, do
> we close it immediately?  If not, we should.

Yep, we do!

[Kyle Horimoto, 2012-07-25 22:59:55]

On 2012/07/23 21:54:07, Kyle Horimoto wrote:
> Comments inline. Additionally, I realized that passing the delegate rather
than
> the actual TabContents is actually a necessity for correctness, not just a
hack.
> If we were to pass the original TabContents at construction time, then these
> TabContents would not change each time the tab is changed. Thus, the zoom
> percentage shown would not be accurate, and the "Set to default" button would
> not change zoom on the appropriate tab.
> 
> On 2012/07/19 03:02:38, Peter Kasting wrote:
> > On 2012/07/18 23:49:38, Kyle Horimoto wrote:
> > > when you mouse over the percentage label, padding, or separator,
everything
> > > works as expected. However, mousing over the button causes OnMouseExited()
> to
> > be
> > > fired, even though it is still within the bubble view. Any ideas as to why
> > this
> > > would be? Thanks!
> > 
> > Debug the callstack when OnMouseExited is fired.  I believe this is called
> > because we do something like recursively ask views (down the parent->child
> > chain) whether the mouse is inside them or not, and so there ought to be a
way
> > for you to correct this case by providing the relevant handler on the
> containing
> > view that just returns true without checking whether the mouse is inside the
> > button.  Or something like that.
> 
> Done.
> 
> >
>
http://codereview.chromium.org/10792020/diff/6001/chrome/browser/ui/views/loc...
> > > chrome/browser/ui/views/location_bar/location_bar_view.cc:276: // for the
> > > appropriate TabContents.
> > > On 2012/07/18 03:26:30, Peter Kasting wrote:
> > > > Nit: This seems a little sketchy since apparently this "assigning"
action
> > > > happens completely outside our control or visibility -- later we just
> > happily
> > > > call GetWebContentsFromDelegate() on the delegate as well as
> > > > delegate_->GetTabContents().  Is it possible to make the tab contents be
> > > > non-NULL at the time the delegate is passed in to this object so there
> isn't
> > a
> > > > window like this?  If not, is there a specific time where we know the
tab
> > > > contents will exist so we can e.g. DCHECK that in GetTabContents() and
> refer
> > > to
> > > > it in the comments here?
> > > 
> > > Hmm, not exactly. ToolbarView creates a LocationBarView when the window is
> > > created. At this point, we have only created the container (window) for a
> > > TabContents, not the actual TabContents (which is why we can't just call
> > > GetTabContents() here). After the window is set up,
> > > TabStripModel::AppendTabContents() is called with the current tab contents
> > > (e.g., the website being viewed or the NTP). At this point, |delegate_|
> (which
> > > is the ToolbarView that created the LocationBarView) can get a reference
to
> > the
> > > TabContents. Unfortunately, there doesn't seem to be a way to check this
> > except
> > > to DCHECK() that the pointer returned by GetTabContents() is non-NULL :/.
> > Should
> > > I just do that?
> > 
> > Maybe take out the comment here (since ZoomView doesn't try to access the
> > delegate's TabContents immediately anyway) and add a comment in ZoomView
> > somewhere appropriate noting that the delegate's TabContents (a) varies as
the
> > user changes tabs, so we need to fetch it explicitly instead of getting it
in
> > the constructor, and (b) is guaranteed to be non-NULL by the time we allow
> > clicks on the ZoomView.
> 
> Moved the comment to ZoomView.
> 
> > By the way, you should probably ensure that second condition is true and
that
> > there isn't some weird race where if the TabContents is really slow to
attach
> > the ZoomView could actually be clicked with no tab.
> 
> This condition is true. The TabContents must be set before the page is
> displayed. The page must be displayed for its zoom to be changed. The zoom
icon
> will not be shown unless it listens to zoom change notifications. Thus,
> TabContents must be set in order for this code to run.
> 
> > Also, this reminds me.  If the bubble is showing and the user changes tabs,
do
> > we close it immediately?  If not, we should.
> 
> Yep, we do!

pkasting, think I could get a review on this ASAP so that it can get out for
M22? Thanks!

[Peter Kasting, 2012-07-26 01:36:13]

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:9: #include
"chrome/common/pref_names.h"
Nit: Alphabetical order

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:24: // The number of
milliseconds the bubble should stay on the screen for if it
Nit: Removing "for" makes this sentence read better

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:31: // The font size of
the percentage label.
Nit: How about: "How many pixels larger the percentage label font should be,
compared with the default font."

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:128: double
default_zoom_level =
It seems like this whole function call can be replaced by:

  chrome_page_zoom::Zoom(tab_contents_->web_contents(),
content::PAGE_ZOOM_RESET);

Then we need to fix one other bug, which is that that function always resets the
zoom to "0" instead of to the appropriate default level.  This is wrong -- it
means ctrl-0 doesn't reset to default when the default is not 100%.

(There's one other weird bit about that function, which is the use of
GetMutableRendererPrefs()... I'm asking Jenn about that separately.)

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:169: ZoomBubbleView*
listener, const string16& text) :
Nit: One arg per line; ':' goes on next line (before first initializer)

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:181:
views::TextButton::OnMouseExited(event);
This doesn't seem right?  If we exit from the button upwards onto the rest of
the bubble won't this start the timer?

I think instead of this class you should maybe be able to do something like
ZoomBubbleView::set_notify_enter_exit_on_child()?  (Not sure whether to set it
true or false, the comments are confusing.)  If that doesn't work we could screw
with GetEventHandlerForPoint(), but that's more difficult to work with and I
think that other method was made for precisely the case you have here.

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:19: class ZoomBubbleView
: public views::ButtonListener,
Nit: Maybe makes sense to put BubbleDelegateView first (and its overriding
methods first, etc.) since that feels like the most applicable "implements-a"
class for this, as opposed to ButtonListener, which feels like a more peripheral
base class?  I'm not sure if I'm being super-vague here...

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:60: //
views::BubbleDelegateView method.
Nit: Group all overrides from the same base class together.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:95: // Give the button
access to ZoomBubbleView's internals.
It seems better to me to make Start/StopTimer() be public, which I think would
eliminate the need for this to be a friend.  You could also then declare and
define it completely within the .cc file rather than making it a member class.

(Note this is not necessary if we can eliminate this class entirely by mucking
with set_notify_enter_exit_on_child(), or in extreme GetEventHandlerForPoint(),
see comments in other file.)

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:23: // which requires the
current TabContents. Thus, a LocationBarView::Delegate
Nit: Maybe clearer: "...current TabContents, which changes as the user switches
tabs and thus can't be provided in the constructor.  Instead, a
LocationBarViewDelegate..."?

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:26: explicit
ZoomView(ToolbarModel* toolbar_model,
Nit: explicit not needed for 2-arg constructor

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:49: // The Delegate for this
view's corresponding LocationBarView.
Nit: Maybe "The delegate we can use to get the currently visible TabContents."?

[Kyle Horimoto, 2012-08-03 03:33:29]

Sorry it took me forever to get this CL back to you. Addressed all your
comments.

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:9: #include
"chrome/common/pref_names.h"
On 2012/07/26 01:36:13, Peter Kasting wrote:
> Nit: Alphabetical order

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:24: // The number of
milliseconds the bubble should stay on the screen for if it
On 2012/07/26 01:36:13, Peter Kasting wrote:
> Nit: Removing "for" makes this sentence read better

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:31: // The font size of
the percentage label.
On 2012/07/26 01:36:13, Peter Kasting wrote:
> Nit: How about: "How many pixels larger the percentage label font should be,
> compared with the default font."

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:128: double
default_zoom_level =
On 2012/07/26 01:36:14, Peter Kasting wrote:
> It seems like this whole function call can be replaced by:
> 
>   chrome_page_zoom::Zoom(tab_contents_->web_contents(),
> content::PAGE_ZOOM_RESET);
> 
> Then we need to fix one other bug, which is that that function always resets
the
> zoom to "0" instead of to the appropriate default level.  This is wrong -- it
> means ctrl-0 doesn't reset to default when the default is not 100%.
> 
> (There's one other weird bit about that function, which is the use of
> GetMutableRendererPrefs()... I'm asking Jenn about that separately.)

Done. Fix for that function in an upcoming CL.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:169: ZoomBubbleView*
listener, const string16& text) :
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: One arg per line; ':' goes on next line (before first initializer)

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:181:
views::TextButton::OnMouseExited(event);
On 2012/07/26 01:36:14, Peter Kasting wrote:
> This doesn't seem right?  If we exit from the button upwards onto the rest of
> the bubble won't this start the timer?
> 
> I think instead of this class you should maybe be able to do something like
> ZoomBubbleView::set_notify_enter_exit_on_child()?  (Not sure whether to set it
> true or false, the comments are confusing.)  If that doesn't work we could
screw
> with GetEventHandlerForPoint(), but that's more difficult to work with and I
> think that other method was made for precisely the case you have here.

Done.

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:19: class ZoomBubbleView
: public views::ButtonListener,
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: Maybe makes sense to put BubbleDelegateView first (and its overriding
> methods first, etc.) since that feels like the most applicable "implements-a"
> class for this, as opposed to ButtonListener, which feels like a more
peripheral
> base class?  I'm not sure if I'm being super-vague here...

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:60: //
views::BubbleDelegateView method.
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: Group all overrides from the same base class together.

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:95: // Give the button
access to ZoomBubbleView's internals.
On 2012/07/26 01:36:14, Peter Kasting wrote:
> It seems better to me to make Start/StopTimer() be public, which I think would
> eliminate the need for this to be a friend.  You could also then declare and
> define it completely within the .cc file rather than making it a member class.
> 
> (Note this is not necessary if we can eliminate this class entirely by mucking
> with set_notify_enter_exit_on_child(), or in extreme
GetEventHandlerForPoint(),
> see comments in other file.)

Ended up fixing the other part, so this is no longer necessary.

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

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:23: // which requires the
current TabContents. Thus, a LocationBarView::Delegate
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: Maybe clearer: "...current TabContents, which changes as the user
switches
> tabs and thus can't be provided in the constructor.  Instead, a
> LocationBarViewDelegate..."?

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:26: explicit
ZoomView(ToolbarModel* toolbar_model,
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: explicit not needed for 2-arg constructor

Done.

http://codereview.chromium.org/10792020/diff/20001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_view.h:49: // The Delegate for this
view's corresponding LocationBarView.
On 2012/07/26 01:36:14, Peter Kasting wrote:
> Nit: Maybe "The delegate we can use to get the currently visible
TabContents."?

Done.

[Peter Kasting, 2012-08-03 19:24:22]

LGTM

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

http://codereview.chromium.org/10792020/diff/28001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:10: #include
"chrome/browser/prefs/pref_service.h"
Nit: I think this is no longer necessary, maybe a few others of these can be
nuked too

[Kyle Horimoto, 2012-08-03 22:20:44]

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

http://codereview.chromium.org/10792020/diff/28001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:10: #include
"chrome/browser/prefs/pref_service.h"
On 2012/08/03 19:24:23, Peter Kasting wrote:
> Nit: I think this is no longer necessary, maybe a few others of these can be
> nuked too

Done.

[commit-bot: I haz the power, 2012-08-03 22:21:42]

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

[commit-bot: I haz the power, 2012-08-03 22:21:44]

Failed to apply patch for
chrome/browser/ui/views/location_bar/location_bar_view.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/location_bar/location_bar_view.cc
Hunk #3 FAILED at 1074.
Hunk #4 succeeded at 1304 with fuzz 2 (offset 1 line).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/location_bar/location_bar_view.cc.rej

[commit-bot: I haz the power, 2012-08-03 22:26:14]

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

[commit-bot: I haz the power, 2012-08-03 23:21:30]

Change committed as 149965