Mac: Fix zoom (green maximize) button.

Mac: Fix zoom (green maximize) button.

We always maximally zoom vertically, since doing otherwise is too hard.
(We'd have to figure out the width and get the renderer to calculate the
height -- and somehow do this asynchronously.)

BUG=17472
TEST=Zoom/unzoom from a variety of window configurations (size and position), with the Dock in the different possible positions, with web content of differing intrinsic size.

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

[viettrungluu-gmail, 2009-08-22 20:12:52]

Could someone please take a look at this? Thanks.

[Mark Mentovai, 2009-08-23 04:50:06]

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/173254/diff/1/2#oldcode360
Line 360: // that leaves room for icons on the right side. Use the full (usable)
height
I would expect something that actually does consider the intrinsic vertical
height.  You seem to be getting rid of any height adjustment on (unshifted) zoom
altogether.

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/1/2#newcode372
Line 372: // To prevent strange results on portrait displays, the basic minimum
zoomed
If you've got a 1.6:1 display in portrait mode (1:1.6?), 60% of the height is
really close to the width.  This won't leave much room for icons at all.

If you've got a 16:9 display in portrait mode, 60% of the height is actually
larger than the width.  Now zoom is always going to result in a window wider
than the screen.  Oops.

I think that this would be much better off simply using the width and
subtracting some value appropriate for a column or two of icons.  The subtrahend
might be a pain to actually figure given resolution independence and
customizable icon sizes and grid spacing, so you might just want to use some
constant in the 200-300 range for now.

http://codereview.chromium.org/173254/diff/1/2#newcode390
Line 390: // Never shrink from the current size on zoom.
Uh-oh.  Really?  Why?

[viettrungluu-gmail, 2009-08-23 05:39:58]

Thanks for looking, Mark.

I (personally) think my patch produces quite good behaviour, and seems to be the
behaviour of Safari and Camino (though Safari sometimes gets confused and does
something random). Most other apps behave fairly similarly, or treat the zoom
button as a maximize button, or act semi-randomly.

Apple's HIG is fairly vague on what the zoom button is supposed to do. (Link:
<http://developer.apple.com/documentation/UserExperience/Conceptual/AppleHIGui...)
Strictly speaking, it allows shrinking when zooming (not speaking about
"unzooming", i.e., returning to the user state). This may be desirable for
single-document windows (with a very well-defined document size -- e.g., an
image editor or similar), but I don't know about in the case of a tabbed
browser.

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (left):

http://codereview.chromium.org/173254/diff/1/2#oldcode360
Line 360: // that leaves room for icons on the right side. Use the full (usable)
height
On 2009/08/23 04:50:07, Mark Mentovai wrote:
> I would expect something that actually does consider the intrinsic vertical
> height.  You seem to be getting rid of any height adjustment on (unshifted)
zoom
> altogether.

No, I use the full available height -- provided by the default frame.

I would want the intrinsic vertical height considered, but that's hard. Why?
Because you'd have to figure out the width you're zooming to, which likely
changes the height ... which you'd have to then go ask the renderer for.

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/1/2#newcode372
Line 372: // To prevent strange results on portrait displays, the basic minimum
zoomed
On 2009/08/23 04:50:07, Mark Mentovai wrote:
> If you've got a 1.6:1 display in portrait mode (1:1.6?), 60% of the height is
> really close to the width.  This won't leave much room for icons at all.

AFAICT, *no* Mac app considers leaving room for icons. Apple's HIG says nothing
about this either. (Presumably, "we have the Dock, we don't need no stinkin'
icons".)

> If you've got a 16:9 display in portrait mode, 60% of the height is actually
> larger than the width.  Now zoom is always going to result in a window wider
> than the screen.  Oops.

Note the "bounded by available width" part of the comment. So it ends up filling
the available horizontal space. Presumably, one has the dock on the bottom in
this situation, so it ends up filling the screen (minus Dock and menu bar). Not
terrific, but a reasonable format for reading web pages. [There are problems
with being smart vertically; I'll comment on this elsewhere.]

> I think that this would be much better off simply using the width and
> subtracting some value appropriate for a column or two of icons.  The
subtrahend
> might be a pain to actually figure given resolution independence and
> customizable icon sizes and grid spacing, so you might just want to use some
> constant in the 200-300 range for now.

The minimum used to be 700, because some web sites have small intrinsic widths.
Obviously, I pulled "60%" out of my *ss and don't pretend this is optimal. Maybe
50% or 67% or 75% would be better. Perhaps better still would be to figure out
the DPI of the screen and go for a certain physical size -- but even this has
problems (is everything set up correctly to find a correct DPI? how far is the
user from the screen?).

http://codereview.chromium.org/173254/diff/1/2#newcode390
Line 390: // Never shrink from the current size on zoom.
On 2009/08/23 04:50:07, Mark Mentovai wrote:
> Uh-oh.  Really?  Why?

Note that "zoom" is different from "unzoom" (go hidden window states). I'm
following what most apps do, when they do something reasonable, rather than
something random.

The zoom vs. unzoom is a secret toggle, made even more confusing by Cocoa's
consideration of a fudge factor (so if you zoom a window, and resize/move it
just a bit, you're still zoomed).

[pink (ping after 24hrs), 2009-08-28 01:56:38]

I'll try to apply this and play with it tomorrow.

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/1/2#newcode362
Line 362: // |frame| already fills the current screen. Never touch y and height
since we
really? i found in my initial impl that |frame| was mostly useless and just a
translation of the current window frame (and current window size) to 0,0. Hence
its lack of use in the method.

http://codereview.chromium.org/173254/diff/1/2#newcode390
Line 390: // Never shrink from the current size on zoom.
On 2009/08/23 05:39:58, viettrungluu wrote:
> On 2009/08/23 04:50:07, Mark Mentovai wrote:
> > Uh-oh.  Really?  Why?
> 
> Note that "zoom" is different from "unzoom" (go hidden window states). I'm
> following what most apps do, when they do something reasonable, rather than
> something random.
> 
> The zoom vs. unzoom is a secret toggle, made even more confusing by Cocoa's
> consideration of a fudge factor (so if you zoom a window, and resize/move it
> just a bit, you're still zoomed).

Can you at least add a comment at to why, as opposed to just stating that we
should never shrink?

[viettrungluu-gmail, 2009-08-28 04:03:39]

I studied how Cocoa uses this method more carefully, and wrote a big comment
reflecting my understanding (or possibly misunderstanding). (Quite possibly, the
confusing way Cocoa goes about zooming/unzooming windows accounts for erratic
behaviour on the part of many programs in this aspect.)

I'm concerned that |preferred_width()| may not always give very good answers. It
gives perfect answers for simple things (e.g., an image), but seems to give
pretty bad answers for more complicated things (e.g., the wikipedia home page).

http://codereview.chromium.org/173254/diff/1/2
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/1/2#newcode362
Line 362: // |frame| already fills the current screen. Never touch y and height
since we
On 2009/08/28 01:56:38, pink wrote:
> really? i found in my initial impl that |frame| was mostly useless and just a
> translation of the current window frame (and current window size) to 0,0.
Hence
> its lack of use in the method.

Weird. At least on my system, on my builds, |frame| is as described in Apple's
documentation. Maybe something else was messing it up?

http://codereview.chromium.org/173254/diff/1/2#newcode390
Line 390: // Never shrink from the current size on zoom.
On 2009/08/28 01:56:38, pink wrote:
> On 2009/08/23 05:39:58, viettrungluu wrote:
> > On 2009/08/23 04:50:07, Mark Mentovai wrote:
> > > Uh-oh.  Really?  Why?
> > 
> > Note that "zoom" is different from "unzoom" (go hidden window states). I'm
> > following what most apps do, when they do something reasonable, rather than
> > something random.
> > 
> > The zoom vs. unzoom is a secret toggle, made even more confusing by Cocoa's
> > consideration of a fudge factor (so if you zoom a window, and resize/move it
> > just a bit, you're still zoomed).
> 
> Can you at least add a comment at to why, as opposed to just stating that we
> should never shrink?

Done.

[pink (ping after 24hrs), 2009-09-01 15:04:04]

This has a bug where if you move a window onto a 2nd monitor, then back to the
first and click the zoom button, it will jump back to the 2nd monitor.

Otherwise it looks pretty good.

[viettrungluu, 2009-09-02 03:46:41]

You're not going to like this, but I'm going to blame Apple.

First, concrete repro steps (for that "bug"):
1. Zoom the window on monitor #1.
2. Move it to an odd position on monitor #2.
3. Zoom and unzoom it (i.e., press the green button twice).
4. Move it back to monitor #1, but make sure the window is "at the top", so that
it is in a state similar to the one it had after step 1.
5. "Unzoom" it. It will pop back to monitor #2.

(If you're seeing something significantly different, please let me know!)

Why this is working as "designed" (it's stupid, but the way the zoom/unzoom
works is kind of stupid):
- By doing step 3, Cocoa records the "user state" of the window as being the
oddly-positioned one.
- In step 5, Cocoa asks for the "standard state" (i.e., zoomed size/position) of
the window. Since the window is big enough and in a reasonable position, it
decides that it's actually already in the "standard state" (i.e., already
zoomed). So it decides that it should "unzoom" back to the "user state", which
was on the other monitor.

Other notes:
- It works this way with other browsers (Safari and Camino, for two) as well.
- It's possible to get this sort of behaviour with MacChrome before this patch:
in step 4, you have to carefully position the window so that it's close enough
to the left edge (and the top edge).

Can we do anything about this stupid behaviour?
- It's not obvious how. We're never told whether we're zooming or unzooming;
we're just asked for the "standard/zoomed state" whenever the green button is
pressed, and if the window is already in that state, Cocoa restores it to its
saved "user state".
- We could subvert this directly, but not in any reasonable way.
- We could implement |-windowShouldZoom:toFrame:|, and deny any zooms which
cause the window to change screens. This may even be reasonable.

[viettrungluu, 2009-09-02 05:09:11]

So Patch Set 6 implements |-windowShouldZoom:toFrame:| and should prevent
unzooms from making the window jump around. *However*, this is not standard
Cocoa behaviour (which is to be confusing)....

[pink (ping after 24hrs), 2009-09-02 15:00:33]

lgtm, seems like it would be nice to break apart some of the checks into
testable methods, but not quite sure how.

http://codereview.chromium.org/173254/diff/10001/10002
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/10001/10002#newcode398
Line 398: contents->view()->preferred_width() + kScrollbarWidth);
only need to indent 4 from previous line here.

http://codereview.chromium.org/173254/diff/10001/10002#newcode445
Line 445: if (NSEqualRects(newScreenFrame, curScreenFrame))
can't you just compare curScreen and newScreen? Do you need to compare the
rects?

[viettrungluu-gmail, 2009-09-02 16:20:55]

Thanks, could you take a look at the shiny new test? (Unfortunately, I don't see
how to test |-windowShouldZoom| in a reasonable way.)

http://codereview.chromium.org/173254/diff/10001/10002
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/173254/diff/10001/10002#newcode398
Line 398: contents->view()->preferred_width() + kScrollbarWidth);
On 2009/09/02 15:00:33, pink wrote:
> only need to indent 4 from previous line here.

Done.

http://codereview.chromium.org/173254/diff/10001/10002#newcode445
Line 445: if (NSEqualRects(newScreenFrame, curScreenFrame))
On 2009/09/02 15:00:33, pink wrote:
> can't you just compare curScreen and newScreen? Do you need to compare the
> rects?

You're right, we can just compare pointers. Done.

[pink (ping after 24hrs), 2009-09-02 16:47:10]

lgtm with extra constant comments discussed on irc.