[Mac] Implement the basic zoom bubble.

[Mac] Implement the basic zoom bubble.

The bubble can be opened in response to a click on the zoom decoration in the Omnibox,
or when the page zoom changes and it is opened automatically. In the second case,
it is dismissed automatically after a fixed period of time.

BUG=128817
TEST=Zoom a web page in or out. A zoom icon appears in the Omnibox. Clicking it opens the bubble.128817
TBR=sky@chromium.org

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

[Robert Sesek, 2013-02-15 19:33:22]

Screen shot: http://cl.ly/image/39041r3v0f1y

[sail, 2013-02-15 21:05:12]

Looks really good. Thanks for taking this on.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:18: -
(void)performLayout;
how about initializeSubviews?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:71: if (contents_)
this test is already done in updateZoomPercent

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:83: [NSObject
cancelPreviousPerformRequestsWithTarget:self];
Nit, cancelPreviousPerformRequestsWithTarget:selector:object: would be better.
Specially since this overrides a system class.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:104: NSRect frame =
[resetButton frame];
buttonFrame? specially since you use it bellow

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:109: // Add a
horizontal separator.
Why add a separator, the Windows UI doesn't have one.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:118: NSMakeRect(0,
NSMaxY(frame) + 2*kPadding, 0, 0)]);
space before and after *

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:119: [zoomPercent_
sizeToFit];
should this go above addSubview?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:122: [zoomPercent_
setStringValue:@"%"];
I don't think you need this line

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:137: [zoomPercent_
setStringValue:
Do you have to worry about the text field being wider than the window width?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:140: [zoomPercent_
sizeToFit];
Alternatively, you could make the text field as wide as the window and just set
the text alignment using setAlignment:NSCenterTextAlignment

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:14: class
ZoomBubbleControllerTest : public CocoaTest {
use typedef instead?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:29:
[static_cast<InfoBubbleWindow*>([controller window]) setDelayOnClose:NO];
base::mac::ObjcStrictCast ?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:56:
[controller showForWebContents:NULL anchoredAt:NSZeroPoint autoClose:YES];
How about doing [controller_ setAutoCloseDelayForTesting:0] above this line?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.h (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.h:39: // The bubble that
this decoration shows. Weak, owns self.
In a different review thakis recommended using a scoped_nsobject for this and
having the bubble controller retain itself.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:43: [bubble_
onZoomChanged];
Do you need to allocate ZoomBubbleController here?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:48: frame.origin.x +=
frame.size.width - image_size.width;
This code would be easier to understand if it didn't reuse frame. Something
like:

NSRect image_frame;
image_frame.size = [GetImage() size];
image_frame.origin.x = NSMaxX(frame) - NSWidth(image_frame);
image_frame.origin.y = NSMinY(frame);

[Robert Sesek, 2013-02-15 21:39:20]

New screen shot: http://cl.ly/image/1w2k261a3x3M

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:18: -
(void)performLayout;
On 2013/02/15 21:05:12, sail wrote:
> how about initializeSubviews?

We call it performLayout in all the other bubbles, so I did that.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:71: if (contents_)
On 2013/02/15 21:05:12, sail wrote:
> this test is already done in updateZoomPercent

Yes, it didn't used to. Folded together.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:83: [NSObject
cancelPreviousPerformRequestsWithTarget:self];
On 2013/02/15 21:05:12, sail wrote:
> Nit, cancelPreviousPerformRequestsWithTarget:selector:object: would be better.
> Specially since this overrides a system class.

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:104: NSRect frame =
[resetButton frame];
On 2013/02/15 21:05:12, sail wrote:
> buttonFrame? specially since you use it bellow

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:109: // Add a
horizontal separator.
On 2013/02/15 21:05:12, sail wrote:
> Why add a separator, the Windows UI doesn't have one.

Didn't realize that. Removed.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:118: NSMakeRect(0,
NSMaxY(frame) + 2*kPadding, 0, 0)]);
On 2013/02/15 21:05:12, sail wrote:
> space before and after *

Removed.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:119: [zoomPercent_
sizeToFit];
On 2013/02/15 21:05:12, sail wrote:
> should this go above addSubview?

It does?

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:122: [zoomPercent_
setStringValue:@"%"];
On 2013/02/15 21:05:12, sail wrote:
> I don't think you need this line

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:137: [zoomPercent_
setStringValue:
On 2013/02/15 21:05:12, sail wrote:
> Do you have to worry about the text field being wider than the window width?

I played with a bunch of locales and couldn't get it to overflow.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm:140: [zoomPercent_
sizeToFit];
On 2013/02/15 21:05:12, sail wrote:
> Alternatively, you could make the text field as wide as the window and just
set
> the text alignment using setAlignment:NSCenterTextAlignment

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:14: class
ZoomBubbleControllerTest : public CocoaTest {
On 2013/02/15 21:05:12, sail wrote:
> use typedef instead?

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:29:
[static_cast<InfoBubbleWindow*>([controller window]) setDelayOnClose:NO];
On 2013/02/15 21:05:12, sail wrote:
> base::mac::ObjcStrictCast ?

Done.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/brows...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm:56:
[controller showForWebContents:NULL anchoredAt:NSZeroPoint autoClose:YES];
On 2013/02/15 21:05:12, sail wrote:
> How about doing [controller_ setAutoCloseDelayForTesting:0] above this line?

This test only takes 1.6s to run, and it doesn't then need a bunch of test-only
code in the impl. Would prefer not.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.h (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.h:39: // The bubble that
this decoration shows. Weak, owns self.
On 2013/02/15 21:05:12, sail wrote:
> In a different review thakis recommended using a scoped_nsobject for this and
> having the bubble controller retain itself.

I wrote it like that earlier but the closeObserver actually makes it nice to
keep the same pattern as the other bubbles as well as making it testable.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:43: [bubble_
onZoomChanged];
On 2013/02/15 21:05:12, sail wrote:
> Do you need to allocate ZoomBubbleController here?

Nope. Not yet, c.f. CL description.

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:48: frame.origin.x +=
frame.size.width - image_size.width;
On 2013/02/15 21:05:12, sail wrote:
> This code would be easier to understand if it didn't reuse frame. Something
> like:
> 
> NSRect image_frame;
> image_frame.size = [GetImage() size];
> image_frame.origin.x = NSMaxX(frame) - NSWidth(image_frame);
> image_frame.origin.y = NSMinY(frame);

This code is literally copied and pasted from another decoration.

[sail, 2013-02-15 21:58:20]

LGTM!

One small nit. I think having a fixed width window doesn't look good. I like the
Windows UI much better: http://imgur.com/fWYKtm4

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration.h (right):

https://codereview.chromium.org/12286006/diff/1/chrome/browser/ui/cocoa/locat...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.h:39: // The bubble that
this decoration shows. Weak, owns self.
On 2013/02/15 21:39:20, rsesek wrote:
> On 2013/02/15 21:05:12, sail wrote:
> > In a different review thakis recommended using a scoped_nsobject for this
and
> > having the bubble controller retain itself.
> 
> I wrote it like that earlier but the closeObserver actually makes it nice to
> keep the same pattern as the other bubbles as well as making it testable.

It should still be easy to test. In closeObserver you just do bubble_reset()
instead of bubble = nil;

[Robert Sesek, 2013-02-19 17:29:05]

On 2013/02/15 21:58:20, sail wrote:
> LGTM!
> 
> One small nit. I think having a fixed width window doesn't look good. I like
the
> Windows UI much better: http://imgur.com/fWYKtm4

I think having a bubble that small looks awkward :/

[Robert Sesek, 2013-02-19 17:56:56]

Actually, PTAL. Implementing the auto-open logic is trivial, so I've rolled it
into this.

[sail, 2013-02-19 18:11:06]

second LGTM

[commit-bot: I haz the power, 2013-02-19 18:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/12286006/11001

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

Presubmit check for 12286006-11001 failed and returned exit status 1.

INFO:root:Found 9 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

[Robert Sesek, 2013-02-19 18:49:58]

+sky for gyp OWNERS

[sail, 2013-02-19 18:50:51]

On 2013/02/19 18:49:58, rsesek wrote:
> +sky for gyp OWNERS

FYI Trivial gyp changes can be TBRed.

[commit-bot: I haz the power, 2013-02-19 18:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/12286006/11001

[commit-bot: I haz the power, 2013-02-19 20:55:41]

Change committed as 183284