Allow new zoom bubble to override current one.

Allow new zoom bubble to override current one.

At present, if a user changes zoom on OSX while the zoom bubble is
already being shown, the new zoom bubble (with the most up to date
information) is not shown. This CL changes this behaviour by allowing
a new zoom bubble to be created, and replace the existing one.

Example: load a PDF and repeatedly click the zoom +/- controls in the
lower-right corner of the viewer.

BUG=444995
Committed: https://crrev.com/a95ea7031e472fd5d0968955989c963381dfaf5f
Cr-Commit-Position: refs/heads/master@{#313564}

[wjmaclean, 2015-01-22 18:44:11]

wjmaclean@chromium.org changed reviewers:
+ sky@chromium.org

[wjmaclean, 2015-01-22 18:45:41]

sky@ - I thought I'd start the review process with you given your knowledge of
the Zoom bubble UI, but feel free to add additional eyes if you're not
comfortable with the Cocoa-ness of this.

[sky, 2015-01-22 20:40:56]

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

https://codereview.chromium.org/849693006/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm:65: bubble_ =
[[ZoomBubbleController alloc] initWithParentWindow:[field window]
This seems like you're going to end up with two bubbles? Is that right? Wouldn't
it be better to change the existing bubble?

[wjmaclean, 2015-01-22 22:03:31]

I've uploaded a new patch that re-uses the current bubble if it exists.

[sky, 2015-01-23 00:38:38]

sky@chromium.org changed reviewers:
+ rohitrao@chromium.org
- sky@chromium.org

[sky, 2015-01-23 00:38:39]

I don't know enough about the cocoa side to know if the code works ok with being
called multiple times.
sky->rohitrao

[rohitrao (ping after 24h), 2015-01-23 14:11:01]

What happens if you call [bubble_ onZoomChanged] when bubble_ already exists? 
Does that provide the behavior we're looking for?

Why is the calling code invoking ShowBubble() in this situation, instead of
something like UpdateIfNeccesary()?

On a currentish Mac canary build, I am unable to reproduce this bug.  When I
click outside the omnibox, the zoom bubble automatically disappears and never
comes back.

[wjmaclean, 2015-01-23 14:19:38]

On 2015/01/23 14:11:01, rohitrao wrote:
> What happens if you call [bubble_ onZoomChanged] when bubble_ already exists? 
> Does that provide the behavior we're looking for?
> 
> Why is the calling code invoking ShowBubble() in this situation, instead of
> something like UpdateIfNeccesary()?
> 
> On a currentish Mac canary build, I am unable to reproduce this bug.  When I
> click outside the omnibox, the zoom bubble automatically disappears and never
> comes back.

[wjmaclean, 2015-01-23 14:20:45]

Please ignore #c8 ... I hit send before I had a chance to type a reply (which I
am still pondering).

[wjmaclean, 2015-01-23 14:42:59]

Ok, here goes ...

> What happens if you call [bubble_ onZoomChanged] when bubble_ already exists? 
> Does that provide the behavior we're looking for?

No, it seems to close the existing zoom bubble and does not display a new one.
The end result is the same as the original symptoms. To be specific, I tried

void ZoomDecoration::ShowBubble(BOOL auto_close) {
  if (bubble_) {
    [bubble_ onZoomChanged];
    return;
  }

// remainder of showBubble() unchanged

> Why is the calling code invoking ShowBubble() in this situation, instead of
> something like UpdateIfNeccesary()?

I don't know why the code was written this way. I don't know much about Cocoa
I'm afraid.

> On a currentish Mac canary build, I am unable to reproduce this bug.  When I
> click outside the omnibox, the zoom bubble automatically disappears and never
> comes back.

There was a separate issue that was preventing the zoom bubble from showing at
all for the PDF viewer, but a fix for it went in yesterday. With that fix, the
bug still reproduces on ToT.

The repro steps are:
1) Load any PDF page, e.g. http://www.education.gov.yk.ca/pdf/pdf-test.pdf
2) Using the PDF viewer controls (inside the tab are, bottom right corner),
click the zoom-in icon.
3) While the zoom bubble is still showing, click the zoom-in icon again.

[wjmaclean, 2015-01-23 14:50:12]

On 2015/01/23 14:42:59, wjmaclean wrote:
> Ok, here goes ...
> 
> > What happens if you call [bubble_ onZoomChanged] when bubble_ already
exists? 
> > Does that provide the behavior we're looking for?

I should point out that either of the patch sets posted thus far do produce the
desired behaviour, and appear to work fine on my Mac workstation, though I can't
say much about other Mac plattforms). Whether the second patch represents a
significant performance improvement over the first one I cannot say.

[rohitrao (ping after 24h), 2015-01-23 20:52:12]

rohitrao@chromium.org changed reviewers:
+ erikchen@chromium.org

[rohitrao (ping after 24h), 2015-01-23 20:52:13]

Some notes from our IM discussion:

When you click on the pdf zoom button for the second time, the bubble loses
focus and therefore starts to animate out.  However, the bubble takes some time
to animate, and windowWillClose: is not called until the animation is finished. 
This means that when ShowBubble() is called as a result of the mouse click,
bubble_ is still non-nil, even though the bubble is about to disappear.

Patchset#2 forces the bubble to become visible again, but this isn't great
because it's now in a half-torn-down state.

Patchset#1 creates a new bubble, but when the old bubble finishes animating out
later, it will call ZoomDecoration::OnClose(), which unconditionally sets
bubble_ to nil, even though the new bubble is still visible.  This will cause
ZoomDecoration to have incorrect knowledge about the state of the world, which
also seems bad.

It may be necessary to reset bubble_ to nil as soon as a bubble starts animating
out, rather than only after the animation is finished.  That should help get our
bookkeeping straight.

+Erik to see if he has any better ideas.

[wjmaclean, 2015-01-23 21:46:07]

On 2015/01/23 20:52:13, rohitrao wrote:
> Some notes from our IM discussion:
> 
> When you click on the pdf zoom button for the second time, the bubble loses
> focus and therefore starts to animate out.  However, the bubble takes some
time
> to animate, and windowWillClose: is not called until the animation is
finished. 
> This means that when ShowBubble() is called as a result of the mouse click,
> bubble_ is still non-nil, even though the bubble is about to disappear.
> 
> Patchset#2 forces the bubble to become visible again, but this isn't great
> because it's now in a half-torn-down state.
> 
> Patchset#1 creates a new bubble, but when the old bubble finishes animating
out
> later, it will call ZoomDecoration::OnClose(), which unconditionally sets
> bubble_ to nil, even though the new bubble is still visible.  This will cause
> ZoomDecoration to have incorrect knowledge about the state of the world, which
> also seems bad.
> 
> It may be necessary to reset bubble_ to nil as soon as a bubble starts
animating
> out, rather than only after the animation is finished.  That should help get
our
> bookkeeping straight.
> 
> +Erik to see if he has any better ideas.

Patchset #3 implements an intermediate option, in which the (about to animate
away) bubble is forced to close (and reset bubble_) immediately, since we want
to animate in a new bubble anyways.

[erikchen, 2015-01-23 22:30:42]

tl dr: The implementation of ShowBubble() should nuke the old bubble, and then
show a new one.

The reason that this bug exists, and the reason you haven't been able to find a
simple solution is because the existing code is poorly structured. The
visibility of the UI widget is partially controlled by zoom_decoration, and
partially controlled by zoom_bubble_controller.

I'm going to operate under the assumption that you don't want to refactor this
Cocoa code, and provide the simplest, non-fragile solution I could come up with.
The new code will not be optimal in terms of memory/CPU consumption, but will be
visually better than the existing code, as well as fix the problem you're aiming
for.

1. Add the comment "If a bubble is already showing, the |auto_close| timer is
reset. " to the declaration of ShowBubble(), since location_bar_view_mac.mm
expects this behavior. 
2. Make the ivar |delegate| of zoom_bubble_controller mutable by adding a method
or property.
3. At the beginning of the implementation of ShowBubble(), add the following
code:

"""
ZoomBubbleController* old_bubble = bubble_;
old_bubble.delegate = nil;
"""

4. And at the end, add:

"""
[old_bubble setAnimationBehavior:NSWindowAnimationBehaviorNone]
[old_bubble orderOut:nil];
[old_bubble close]; 
"""

I don't think these lines of code conflict with zoom_bubble_controller's
implementation of auto-closing, but you may want to double check this.

5. (Optional) This isn't related to your change, but I'd appreciate it if you
nil-ed the delegate of ZoomBubbleController in ~ZoomDecoration() and
ZoomDecoration::OnClose().

[wjmaclean, 2015-01-27 01:16:05]

PTAL

I think this is what you described, but I've never done a property in Objective
C before, so let me know if I got it right.

[erikchen, 2015-01-27 01:23:10]

You did the property right. lgtm.

Over to rohit@ for an OWNER review.

[wjmaclean, 2015-01-27 18:15:22]

On 2015/01/27 01:23:10, erikchen wrote:
> You did the property right. lgtm.
> 
> Over to rohit@ for an OWNER review.

Thanks.

I'm a little confused by the build errors on the try-bots ... I can build the
same targets on my Mac WS (OSX 10.9.5) and all is happy. Any thoughts on why
it's different on the bots? Would the bots be building LKGR and there's a
mismatch there?

[erikchen, 2015-01-27 19:00:27]

On 2015/01/27 18:15:22, wjmaclean wrote:
> On 2015/01/27 01:23:10, erikchen wrote:
> > You did the property right. lgtm.
> > 
> > Over to rohit@ for an OWNER review.
> 
> Thanks.
> 
> I'm a little confused by the build errors on the try-bots ... I can build the
> same targets on my Mac WS (OSX 10.9.5) and all is happy. Any thoughts on why
> it's different on the bots? Would the bots be building LKGR and there's a
> mismatch there?

Chrome gets built with OSX 10.6 SDK. The method I suggested you use is only
available in 10.7+.

It looks like the file info_bubble_window.mm has its own implementation of this
behavior as the property |allowedAnimations|. You'll want to set that property
to kAnimateNone instead.

[wjmaclean, 2015-01-27 19:25:35]

On 2015/01/27 19:00:27, erikchen wrote:
> On 2015/01/27 18:15:22, wjmaclean wrote:
> > On 2015/01/27 01:23:10, erikchen wrote:
> > > You did the property right. lgtm.
> > > 
> > > Over to rohit@ for an OWNER review.
> > 
> > Thanks.
> > 
> > I'm a little confused by the build errors on the try-bots ... I can build
the
> > same targets on my Mac WS (OSX 10.9.5) and all is happy. Any thoughts on why
> > it's different on the bots? Would the bots be building LKGR and there's a
> > mismatch there?
> 
> Chrome gets built with OSX 10.6 SDK. The method I suggested you use is only
> available in 10.7+.
> 
> It looks like the file info_bubble_window.mm has its own implementation of
this
> behavior as the property |allowedAnimations|. You'll want to set that property
> to kAnimateNone instead.

Does the order of setting the allowAnimation property and the orderOut value
matter? If not, then ZoomBubbleController.closeWithoutAnimation should work, no?

[wjmaclean, 2015-01-27 19:38:39]

On 2015/01/27 19:25:35, wjmaclean wrote:
> Does the order of setting the allowAnimation property and the orderOut value
> matter? If not, then ZoomBubbleController.closeWithoutAnimation should work,
no?

Follow-up question ... if we're about to close the window anyways, do we really
need orderOut?

[erikchen, 2015-01-27 19:43:24]

On 2015/01/27 19:38:39, wjmaclean wrote:
> On 2015/01/27 19:25:35, wjmaclean wrote:
> > Does the order of setting the allowAnimation property and the orderOut value
> > matter? If not, then ZoomBubbleController.closeWithoutAnimation should work,
> no?
> 
> Follow-up question ... if we're about to close the window anyways, do we
really
> need orderOut?

-closeWithoutAnimation should do the right thing. I added the -orderOut because
I'm paranoid - if the animation looks fine without it, then I'd be fine with
dropping it.

[wjmaclean, 2015-01-28 17:11:14]

On 2015/01/27 19:43:24, erikchen wrote:
> On 2015/01/27 19:38:39, wjmaclean wrote:
> > On 2015/01/27 19:25:35, wjmaclean wrote:
> > > Does the order of setting the allowAnimation property and the orderOut
value
> > > matter? If not, then ZoomBubbleController.closeWithoutAnimation should
work,
> > no?
> > 
> > Follow-up question ... if we're about to close the window anyways, do we
> really
> > need orderOut?
> 
> -closeWithoutAnimation should do the right thing. I added the -orderOut
because
> I'm paranoid - if the animation looks fine without it, then I'd be fine with
> dropping it.

rohitrao@ - PTAL?

It would be nice to get this fix into M41 if possible ...

[rohitrao (ping after 24h), 2015-01-28 17:57:37]

LGTM

What happens if you open a zoom bubble and then press either the + or - button? 
It seems like we'll now destroy the old bubble and create a new one, but the new
bubble will fade in.  Does this look weird?

https://codereview.chromium.org/849693006/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h (right):

https://codereview.chromium.org/849693006/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h:51: @property
(nonatomic) ZoomBubbleControllerDelegate* delegate;
No space after @property.

[wjmaclean, 2015-01-28 18:51:48]

Yes, we do create a new bubble that animates in. I think it looks ok, but of
course that's my personal opinion, and people with more UI savvy than me might
find it weird. I like the fact that it gives a visual cue with greater impact
than just changing the numbers inside an existing bubble (after all, the zoom
bubble is meant to be a warning to a user that the zoom has changed through some
means other than the wrench menu).

There's nothing to stop us from revisiting the UX aspects of this if it turns
out that users find it visually disruptive.

https://codereview.chromium.org/849693006/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h (right):

https://codereview.chromium.org/849693006/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h:51: @property
(nonatomic) ZoomBubbleControllerDelegate* delegate;
On 2015/01/28 17:57:37, rohitrao wrote:
> No space after @property.

Done.

[wjmaclean, 2015-01-28 18:51:56]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2015-01-28 18:52:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/849693006/100001

[commit-bot: I haz the power, 2015-01-28 19:44:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-28 19:45:53]

Patchset 6 (id:??) landed as
https://crrev.com/a95ea7031e472fd5d0968955989c963381dfaf5f
Cr-Commit-Position: refs/heads/master@{#313564}

[wjmaclean, 2015-01-29 20:55:05]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/867183006/ by wjmaclean@chromium.org.

The reason for reverting is: The changes in this CL are causing a crash:
crbug.com/453564.