155956 - Zoom command is "maximizing to fill screen" instead of to fit contents

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Patrick Brice]

When you choose either Zoom Window from the Window Menu item, or click the
'zoom' ( - )widget on the title bar, the window "maximizes" to fill the screen,
rather than "zooming" to fit contents.

Comment 1

[Simon Fraser [no longer active]]

For most HTML pages, the contents stretch to fit the space available, so zooming
to the contents doesn't make much sense.

Comment 2

[Bill McGonigle (not currently reading bugmail; please contact directly)]

To play devil's advocate, there are plenty of web 'authors' who set all their
content in an absolute-sized tables.  Boo hiss, but still, these pages won't
stretch to fit a window.  

If there were an easy way to get a bounding box around the 'useful' content, the
window could zoom to that.  I don't mean to imply there is.

Comment 3

[Patrick Brice]

This is relatively easy to demonstrate. Open www.mozilla.org in a window sized very small in IE, OmniWeb, and Navigator.  Small enough that it will easily only display a small portion of the page (my display is set to 1280x1024).

If you choose Zoom Window or click the Zoom widget in IE or OmniWeb, the windows are sized to fit the content.  In both cases, the windows' widths are adjusted to fit the content left to right without scrolling.  However, because the page content is too 'long'', a scroll bar is added for vertical scrolling.  If it was possible to fit the content in the window top-to-bottom, then the zooming effect would have eliminated the vertical scroll bar, as well.

However, in Navigator, "zooming" always "maximizes" the window to full screen width.  You can see the same effect at www.apple.com.  This is not normal Mac OS behavior.  "Zoom" and "Maximize" (if available) are two completely different things.  Having the Zoom function as maximize is not what is expected by Mac users.  Hitting "zoom" a second time, should then return the window to it's original user defined size.

Comment 4

[Patrick Brice]

Additional comment:
Currently, the Mac OS already supports both the zoom and maximize (to fill screen).  Clicking Zoom should zoom window for "best fit".  Whereas, SHIFT + ZOOM should maximize to fill screen.

Comment 5

[Greg K.]

I don't think Chimera should bother trying to figure out how big a web page is
intended to be, unless maybe if the root element has hard dimensions styled then
it could happen.

Comment 6

[Carlos Salinas]

Um AFAICT only "zoom to best size" is supported. The "maximize to fill screen"
is the default case. Would be nice if Chimera zoomed to "best size" meaning the
minimum size needed to display the content...

BTW: According to the Cocoa docs:

-[NSWindow zoom:]

- (void)zoom:(id)sender
Toggles the size and location of the window between its standard state (provided
by the application as the "best" size to display the window's data) and its user
state (a new size and location the user may have set by moving or resizing the
window). For more information on the standard and user states, see
windowWillUseStandardFrame:defaultFrame:.

...

Comment 7

[Steve Smart]

*** Bug 178788 has been marked as a duplicate of this bug. ***

Comment 8

[Michael Brewer]

There has been much discussion about this bug on the Chimera mailing list.
Browsing the archives may prove interesting.

Comment 9

[Suraj K. Rai]

Any updates on this?  For those who want to see it done well..check out Omniweb.
 I really like their implementation of Zoom.

Comment 10

[Michael Brewer]

I agree. I'd really like to know whether or not this is under consideration by
the developers.

Comment 11

[Zachary]

Isn't the comman OPTION + Zoom?  That's what I've always used to maximizing.  
Maybe they're the same thing.

Comment 12

[Gabriel Radic]

Safari does this very well >:-]

Comment 13

[Mike Pinkerton (not reading bugmail)]

yes, we should do this if gecko supports it. if it doesn't however, we're SOL.

Comment 14

[Mike Pinkerton (not reading bugmail)]

i tried adding this to BWC:

- (NSRect)windowWillUseStandardFrame:(NSWindow *)sender
defaultFrame:(NSRect)defaultFrame
{
  NSRect stdFrame = defaultFrame;
  nsCOMPtr<nsIDOMWindow> contentWindow = getter_AddRefs([[mBrowserView
getBrowserView] getContentWindow]);
  if ( contentWindow ) {
    contentWindow->SizeToContent();
    stdFrame = [[self window] frame];
  }
  return stdFrame;
}

but it freaks out and doesn't always even resize things correctly (yahoo.com is
too small, for example). Not only that, once you do it the progress bar and the
security icon no longer display.

this needs more work, but is a starting point for someone who might want to get
it working.

Comment 15

[Jasper]

*** Bug 231258 has been marked as a duplicate of this bug. ***

Comment 16

[Torben]

Attachment: Implements window zooming

> i tried adding this to BWC:
>     contentWindow->SizeToContent();

AFAICT, SizeToContent() will resize the window twice, first gecko does it and
then we tell the OS to do it once more and lot of strange thing may happen.
This patch copies the calculation from DocumentViewerImpl::SizeToContent() and
then passes the need size (after adding space for other elements of the window
and correcting t y-origin) to the OS who does the actuall resize (as well as
making sure the window fits on the screen).

> and doesn't always even resize things correctly (yahoo.com is
> too small, for example).

We need to manually add space for the scrollbars if they will be shown.

> Not only that, once you do it the progress bar and the
> security icon no longer display.

Works with this patch.

Comment 17

[Simon Fraser [no longer active]]

How does this compile?
+              document->FlushPendingNotifications(Flush_Layout);

Comment 18

[Simon Fraser [no longer active]]

Never mind, I didn't see the underscore, and through that was an unquoted
literal string.

Comment 19

[Mike Pinkerton (not reading bugmail)]

instead of copying the code wholesale, we should add an API to get the info we
need from gecko.

Comment 20

[Torben]

Attachment: patch with gecko API

> instead of copying the code wholesale, we should add an API to
> get the info we need from gecko.

Added a method GetContentSize to nsIMarkupDocumentViewer which is picked up
through CHBrowserView.

There are two minor flaws with this patch (and the previous):

- If the height of the bookmarkstoolbar will be changed, this is not taken
  into acount and we get a threeway state: zoom-to-content-with +/- 20px,
  zoom-to-content, zoom-to-orginal-size (you can take this round trip as
  many times as you want). For me this can be seen on google's image search
  (but not the google home page). Probably need to add/rearrange some code to
  BookmarkToolbar.

- some pages will try to resize to get all text one one line and then we get
  a very low but wide window, not nice at all :-( See 
  http://www.dokpro.uio.no/ordboksoek.html for an example. I think the right
  thing to do would be to not change the width, just the height (this is what
  safari does). I'm just not sure how we can do this.

Comment 21

[Torben]

Comment on attachment 183910 [details] [diff] [review]
patch with gecko API

Hold the review for now. I have fixed the issue when the bookmarks toolbar
changes its height, will try to fix the other flaw too.

Comment 22

[Torben]

Attachment: Updated patch, now handles resizing of the personal toolbar

Still haven't found a way to fix the other issue, beging to doubt it is posible
without changing the reflow logic of gecko :-( Will keep investigating though.

Anyway, I think this is much better than what we have today, it even beats
Safari on some pages (e.g. google.com).

Comment 23

[Torben]

Comment on attachment 184181 [details] [diff] [review]
Updated patch, now handles resizing of the personal toolbar

I found one more flaw: if the window already has the correct size (or is very
close to it), Camino will assume that it is already zoomed and change it to the
default size (which seems to 800x540 px). The window then get "stuck" at that
size.
Making sure that the window's height grows with at least 11 px seems to fix
this, patch coming as soon as I have tested some more.

Comment 24

[Torben]

Attachment: Updated patch, still not perfect

Moved the calculating of the PTheight up to reduce the number of calculations,
switched the witdh/heightchange logic, a positive number now means that the
window grows. The width/height of the scrollers is now got from the OS (I'm
assuming Camino uses the standard OS scroller). Also added a minimum width,
which is the same as the smallest I can manually resize a window to.

Found yet a flaw: if the window only contains a plugin, the content size will
always be approx. 200 x 200 px no matter how big the plugin is. I guess we
should not change the window size when we have a plugin (this is what Safari
do). Then I need to fix the problem when the window don't change its size...

> Making sure that the window's height grows with at least 11 px seems to
> fix this...

Except when the window is the need size +11 px :-( I think it will be better to
stop Camino resizing to this default size than work around it.

Comment 25

[Torben]

Attachment: Working path

I think this is as good as it gets. I still can't figure out why the OS stores
the wrong size for new Camino-windows so I had to cache that inside the code,
also added a check for whether to zoom or not based on this.

The problem mentioned in comment 20 is solved by avoiding a reflow if possible.
Unfortunately there doesn't seem to be any way of getting a width that is
smaller than the window without reflowing (the height is easy, as well as if
the width is larger than the window), this would have simplified to code a lot
:-(

If the content is reflowed but the window is not resized, the layout may be
seriously distorted (e.g. google.com gets left aligned and not centered,
clicking an link will center this instead of following the link). To fix this I
have added an automatic up and downsizing if the window is not zoomed.

If the window is empty or it only contains a plugin no zooming is done. This is
the same as Safari does but maybe we should zoom to window in the later case?

Comment 26

[Samuel Sidler (old account; do not CC)]

Since there's a working patch here, can someone give it a review?

Mike, Simon, if this is simple enough, can it go in 09?

Comment 27

[Simon Fraser [no longer active]]

I wish there was a way to do this without having to do a reflow.

Comment 28

[Boris Zbarsky [:bzbarsky]]

There won't be one until the reflow branch lands.  Once that lands, you could
maybe get the intrinsic width without having to reflow, but to find the
resulting height you still have to reflow at that width -- no other way to get
that information, really.

Comment 29

[Torben]

(In reply to comment #28)
> There won't be one until the reflow branch lands.  Once that lands, you could
> maybe get the intrinsic width without having to reflow, but to find the
> resulting height you still have to reflow at that width

If there are no reflow the resulting height would be the same as the original,
so I can't see how this would be a problem. This is what I have used in the
patch, unfortuantely I have to do a reflow to get the width if it is smaller
than the window.

BTW do you have bug number(s) for the reflow branch work?

Comment 30

[Torben]

Comment on attachment 184856 [details] [diff] [review]
Working path


Since the reflow branch won't land before 1.9 and Camino 1.0 will be based on
the 1.8-branch, I think we should use this for 1.0 (it's far better than what
we've got and IMHO better than safari too).

> Added a method GetContentSize to nsIMarkupDocumentViewer

I'm not sure this makes sense for a interim solution, maybe we should do every
thing in the Camino-code ATM? OTOH I guess the current implementation of
nsIMarkupDocumentViewer will go too...

bz: any ideas?

Comment 31

[Boris Zbarsky [:bzbarsky]]

What's the question, exactly?

Comment 32

[Torben]

(In reply to comment #31)
> What's the question, exactly?

Short version: will the reflow branch use the same interfaces (esp.
nsIMarkupDocumentViewer.idl/nsIDOMWindow.idl) as today or will there be major
changes?

(Alternate short version: could you accept the suggested changes to
nsIMarkupDocumentViewer.idl and nsDocumentViewer.cpp in attachment 184856 [details] [diff] [review]?)

Longer version: my suggested patch adds a method in nsIMarkupDocumentViewer.idl,
if there will be a way to get the intrinsic width without having to reflow after
the reflow branch lands, this method may not be needed anymore. However, since
Camino 1.0 will be based on 1.8, we can't wait for the reflow branch to land :-(
We therefor have two options: 1) add a (possible shortlived[1]) method to
nsIMarkupDocumentViewer; 2) temporarily do all this within Camino-code (this
won't be pretty, but it will work). Could you accept option 1? (I'm sure Mike
and Simon will like option 2.)

[1] if the answer to the short question is "no", I don't think the lifetime
should matter, if it is "yes", however...

I hope I have made myself somewhat clearer :-)

Comment 33

[Boris Zbarsky [:bzbarsky]]

> will the reflow branch use the same interfaces

Yes, for now.  At least for the interfaces you listed.

> could you accept the suggested changes

It's still not clear to me why calling SizeToContent() is not acceptable here.

> this method may not be needed anymore

You'll need something, unless you plan to start using nsIFrame in your code.

Comment 34

[Torben]

(In reply to comment #33)

> It's still not clear to me why calling SizeToContent() is not acceptable
> here.

This is how window zooming is supposed to work on OS X

  1. The OS asks us for the size to obtain a “best fit”
     frame for the window. (windowWillUseStandardFrame:defaultFrame:)

  2. The OS adjusts the resulting frame, if necessary, to fit on the
     current screen.

  3. The OS compares the resulting frame to the current frame to determine
     whether the window’s standard frame is currently displayed. If the 
     current frame is within a few pixels of the standard frame in size and
     location, it is considered a match.

  4. The OS determines a new frame. If the window is currently in the
     standard state, the new frame represents the user state, saved during
     a previous zoom or resize. If the window is currently in the user state,
     the new frame represents the standard state, computed in step 1 above.

  5. The OS sets the new frame.

So we need to find the correct size in 1, but not do any resizing before 5.
Since SizeToContent() does the actual resizing the window will be resized twice
and the logic in step 2-4 destroyed which means it won't be possible to unzoom
the window.

SizeToContent() also have some other flaws, it doesn't add space for scrollbars
so the new window may be to small as well as the errors noted in comment 20.
Fixing this would require yet an other resize.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Ah, I see.  Yeah, I think it's fine to add a getIntrinsicSize function or
something to document viewer.  Please make it share as much code as possible
(probably almost all of it) with sizeToContent.  And please clearly document
what exactly it returns.

Comment 36

[Torben]

Attachment: Cleaned-up patch

bz: could you look at the changes to nsIMarkupDocumentViewer.idl and
nsDocumentViewer.cpp (there's no additional review field)?

I've moved the size calculation out from SizeToContent to GetIntrisicSize. I've
added an additional test for when the page only hosts a full-page plugin since
the returned value will be wrong (always approx. 100 x 100 px), if you don't
want it here I'll try to move it into Camino-code.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 191111 [details] [diff] [review]
Cleaned-up patch

Probably won't be able to review till I get back on the 14th.  Quick notes:  Is
"pixels" CSS pixels or device pixels?  I assume the former, and this should be
documented.

Comment 38

[Torben]

> Is "pixels" CSS pixels or device pixels?  I assume the former, and this should
> be documented.

Actually it is device pixels.


Index: camino/src/embedding/CHBrowserView.mm
===================================================================

+  nsElement->GetOffsetWidth(&contentWidth);
+  nsElement->GetOffsetHeight(&contentHeight);

The behaviour of GetOffSetHeight have just changed (maybe bug 296639?), this now
need to be

+  nsElement->GetClientWidth(&contentWidth);
+  nsElement->GetScrollHeight(&contentHeight);

I'll wait for review comments before I attach a new patch.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Actually it is device pixels.

Not per your code...

> The behaviour of GetOffSetHeight have just changed

Please file a bug with a testcase showing the problem?  cc me?  That changing is
not good.

Comment 40

[Torben]

(In reply to comment #39)
> > Actually it is device pixels.
> 
> Not per your code...

You probably know this a lot better than me but AFAICT
presContext->TwipsToPixels() calls DeviceContext->AppUnitsToDevUnits() which
"convert from application defined units to device units"?[1]

> > The behaviour of GetOffSetHeight have just changed
> 
> Please file a bug with a testcase showing the problem?

Probably bug 260498, I guess the code of one of the sites I tested on has
changed recently. Anyway, I think ClientWidth and ScrollHeight is what I really
want (the documentation on these are scarce).

[1] http://lxr.mozilla.org/mozilla/source/gfx/public/nsIDeviceContext.h#316

Comment 41

[Torben]

BTW, 

> when the page only hosts a full-page
> plugin [...] the returned value will be wrong

I've filed bug 302930 about this since it is a problem with the current
sizeToContent() too.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> presContext->TwipsToPixels() calls DeviceContext->AppUnitsToDevUnits()

That's going to change in 1.9, as I understand.  At the moment we don't have a
concept of "CSS pixel".  When we do, that's what this method should be
returning, and this should be documented.

Comment 43

[Torben]

> > presContext->TwipsToPixels() calls DeviceContext->AppUnitsToDevUnits()
> 
> That's going to change in 1.9, as I understand.  At the moment we don't have
> a concept of "CSS pixel".  When we do, that's what this method should be
> returning, and this should be documented.

AFAICT the callers of GetIntrisicSize() would need device pixels to do the
actual resizing, returning CSS pixels would mean we have to do two conversions,
twips->CSS pxs in the method and CSS->Dev pxs in the caller.

But I'll let you decide if you want CSS or Dev pixels and then I'll make sure to
document this.

BTW, this is what bug 177805 attachment 185921 [details] [diff] [review] proposes for SizeToContent():

-   pixelScale = presContext->TwipsToPixels();
-   width = PRInt32((float)shellArea.width*pixelScale);
-   height = PRInt32((float)shellArea.height*pixelScale);
+   width = presContext->AppUnitsToDevPixels(shellArea.width);
+   height = presContext->AppUnitsToDevPixels(shellArea.height);

Comment 44

[Boris Zbarsky [:bzbarsky]]

> AFAICT the callers of GetIntrisicSize() would need device pixels to do the
> actual resizing

Yes, but GetIntrinsicSize can't tell which device they need this for (in
particular, it can't tell they plan to use the result to resize a widget as
opposed to doing something else entirely), so...

Comment 45

[Mike Pinkerton (not reading bugmail)]

where are we on this? is the last patch
(https://bugzilla.mozilla.org/attachment.cgi?id=191111) still something that i
should review, or are you guys still discussing?

Comment 46

[Torben]

(In reply to comment #45)
> where are we on this? is the last patch
> (https://bugzilla.mozilla.org/attachment.cgi?id=191111) still something that i
> should review, or are you guys still discussing?

I think we agree, so it would be nice if you could review the Camino-parts of
the patch (but see the last part of comment 38).

Comment 47

[Mike Pinkerton (not reading bugmail)]

+  // Change the window size, but don't let it be to narrow
+  stdFrame.size.width = PR_MAX(210, stdFrame.size.width + widthChange);

would be nice if this |210| was a constant defined somewhere. it seems quite
arbitrary.

+  // Camino gets confused if we try to zoom a window that has not been zoomed
+  // before and we're not changing its size

how does it get confused? what goes wrong? should we try to fix that instead of
hacking around it?

+  // The layout may have been distorted when we tried to figure out the new size,
+  // to correct it we do a hidden resizing

can you explain *why* this fixes it? It seems awfully abnormal.

+// If the window is resized update the cached windowframe unless we are zoming
the window

fix typo -> "zooming"

Comment 48

[Torben]

(In reply to comment #47)
> +  // Change the window size, but don't let it be to narrow
> +  stdFrame.size.width = PR_MAX(210, stdFrame.size.width + widthChange);
> 
> would be nice if this |210| was a constant defined somewhere. it seems quite
> arbitrary.

This is the smallest width that I manually could resize a Camino window to,
couldn't find it defined anywhere but I can add something like

  const int kMinWindowWidth = 210; 

> +  // Camino gets confused if we try to zoom a window that has not been 
> +  // zoomed before and we're not changing its size
> 
> how does it get confused? what goes wrong? should we try to fix that instead 
> of hacking around it?

See comment 23, the OS somehow caches the wrong size and position for new
windows (800 x 540 px, centered on screen) so we would "zoom" to an unwanted
size and get stuck. Fixing this would be great (and simplify my zoom code),
however, AFAICT we're doing the right thing when we create windows and this
shouldn't have happend at all :-( Any help here will be appreciated.
 
> +  // The layout may have been distorted when we tried to figure out the new 
> +  // size, to correct it we do a hidden resizing
> 
> can you explain *why* this fixes it? It seems awfully abnormal.

A resize forces gecko to update the layout so it fits the new size. "hidden"
should be removed from the comment though, it is visible if you know what to
look for (but I doubt most users will notice it). I couldn't find any way to
make this happen without resizing.

> +// If the window is resized update the cached windowframe unless we are 
> zoming the window
> 
> fix typo -> "zooming"

Will do.

I'll wait for bz's review before attaching a new patch (unless you want it earlier).

Comment 49

[Wevah (Nate Weaver)]

(In reply to comment #48)
> (In reply to comment #47)
> > +  // Change the window size, but don't let it be to narrow
> > +  stdFrame.size.width = PR_MAX(210, stdFrame.size.width + widthChange);
> > 
> > would be nice if this |210| was a constant defined somewhere. it seems quite
> > arbitrary.
> 
> This is the smallest width that I manually could resize a Camino window to,
> couldn't find it defined anywhere but I can add something like
> 
>   const int kMinWindowWidth = 210; 
> 

Can you use |[self minSize].width| ?

Comment 50

[Mike Pinkerton (not reading bugmail)]

yeah, i was just typing what wevah said until i saw his comment. agreed, ask the
window what its min size is and use that.

Comment 51

[Torben]

Attachment: Updated patch w/o "confusion"-workaround

> +  // Camino gets confused if we try to zoom a window that has not been 
> +  // zoomed before and we're not changing its size
> 
> how does it get confused? what goes wrong? should we try to fix that instead 

> of hacking around it?

I think I finally figured this out, windows are opened with
initWithWindowNibName: which gives a size of 800x542, when we later resize the
window with setFrameUsingName: this is not cached by the OS :-( Calling
setWindowFrameAutosaveName: fixes this, however we must use a unique name for
every window,

+    // add the window to the defaults system with a unique name, ...
+    [self setWindowFrameAutosaveName: [NavigatorWindowFrameSaveName
		   stringByAppendingString: [NSString stringWithUUID]]];

Min window width is now |[[self window] minSize].width| and updated the comment
in nsIMarkupDocumentViewer.idl to tell that this should return CSS pixels.

Comment 52

[Torben]

Re attachment 192527 [details] [diff] [review]

+   // void getIntrinsicSize(out long width, out long height);
+   void getContentSize(out long width, out long height);

Aargh, forgot to do the actuall renaming, for review purpose please pretend that
all instances of (G/g)etContentSize really is (G/g)etIntrinsicSize.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 192527 [details] [diff] [review]
Updated patch w/o "confusion"-workaround

For future reference, using the -p option to diff (so "cvs diff -up8" or
something) makes things a lot easier to review.

Just looking at the non-camino changes here:

>Index: docshell/base/nsIMarkupDocumentViewer.idl
>+   * Calculates the preferred size (ie the minum size where scrollbars are not

s/minum/minimum/, please.

>+   * @param width   the preferred width in pixels

@param width [out]

>+   * @param height  the preferred height in pixels

Same.

>Index: layout/base/nsDocumentViewer.cpp

So you're allowing calling GetIntrinsicSize on a subframe... and that will
reflow the subframe, but how does one go about resizing it after that?	Should
you have the checks for subframe down in GetIntrinsicSize?

>@@ -2923,12 +2923,43 @@
>+   NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width+1, height),

Would it make more sense to have the +1 happening down in GetIntrinsicSize?   I
think that would make more sense, esp. because that lets us later make the +1
conditional on whether it's actually needed...

r=bzbarsky with those changes.

Comment 54

[Boris Zbarsky [:bzbarsky]]

And my apologies that this took so long.  :(

I assume camino would like this in 1.8?

Comment 55

[Simon Fraser [no longer active]]

+    // add the window to the defaults system with a unique name, needed for
correct zooming
+    [self setWindowFrameAutosaveName: [NavigatorWindowFrameSaveName
stringByAppendingString: [NSString stringWithUUID]]];

Doesn't this mean that org.mozilla.camino.plist will fill up with thousands of
saved window frames?

Comment 56

[Torben]

bz
> >Index: layout/base/nsDocumentViewer.cpp
> 
> So you're allowing calling GetIntrinsicSize on a subframe... and that will
> reflow the subframe, but how does one go about resizing it after that?	
> Should you have the checks for subframe down in GetIntrinsicSize?

I was hoping that once in the future we would be able to do this without
reflowing (so calling GetIntrinsicSize on a subframe wouldn't be a problem), but
it is probably better to not allow this ATM.
 
> >@@ -2923,12 +2923,43 @@
> >+   NS_ENSURE_SUCCESS(treeOwner->SizeShellTo(docShellAsItem, width+1, 
> height),
> Would it make more sense to have the +1 happening down in GetIntrinsicSize?   
> I think that would make more sense, esp. because that lets us later make the 
> +1 conditional on whether it's actually needed...

I was trying to minimise the changes to core code, but I'll do as you say (and
then I don't have to add 1 in the Camino code either :-)

smfr
> +    [self setWindowFrameAutosaveName: [NavigatorWindowFrameSaveName
> stringByAppendingString: [NSString stringWithUUID]]];
>
> Doesn't this mean that org.mozilla.camino.plist will fill up with thousands
> of saved window frames?

I stupidly assumed the OS would clean this up, but it doesn't :-(

Adding something like
 [NSWindow removeFrameUsingName:[[self window] frameAutosaveName]];
to windowWillClose may work (but probably not if Camino crashes :-( ) I'll try
this and attach a new patch as soon as I can, hopefully tonight.

Comment 57

[Torben]

>  [NSWindow removeFrameUsingName:[[self window] frameAutosaveName]];

For some reason I can't understand this doesn't remove windows that have been
zoomed (the never-zoomed windows works as expected) :-(
Sigh, I'll guess I have to revert to the workaround (in my earlier testing this
almost does as expected). New patch hopefully tonight (I appologies that this
goes a bit slow, my build computer only have intermittent internet connection ATM).

Comment 58

[Torben]

Attachment: Return of the hack

Fixed bz comments, added back the hack. Moved the adding of the extra 1 px into
(g/G)etIntrisicSize.

Comment 59

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 199569 [details] [diff] [review]
Return of the hack

r=pink

Comment 60

[Samuel Sidler (old account; do not CC)]

Comment on attachment 199569 [details] [diff] [review]
Return of the hack

Because this has checkins into non-Camino code, I'm requesting approval of the patch.

This touches the following non-Camino files:
mozilla/docshell/base/nsIMarkupDocumentViewer.idl
mozilla/layout/base/nsDocumentViewer.cpp
Boris, is there any reason why this couldn't land before 1.8rc1 (i.e., is it a "safe" patch)?

Comment 61

[Boris Zbarsky [:bzbarsky]]

Hmm... The only Gecko change is that sizeToContent() will no longer try to size plugin documents (which never worked anyway).  So this should be reasonably safe...

Comment 62

[Paul Borokhov (lensovet)]

Will there be any way to turn this off once it lands? or only using a custom build? because i might be in the minority but i'm quite happy with the way it is now, and i can't find a keyboard modifier that will force the zoom action to occuppy the whole screen.

Comment 63

[Asa Dotzler [:asa]]

Comment on attachment 199569 [details] [diff] [review]
Return of the hack

too late for changes that could impact core. Camino can take this on a mini-branch. We should figure out how to do this in an API compatible way for Firefox 2 as well

Comment 64

[Christian :Biesinger (don't email me, ping me on IRC)]

docshell/base/nsIMarkupDocumentViewer.idl MUST get a new IID

Comment 65

[Samuel Sidler (old account; do not CC)]

Is there a way we can get this in either the 1.8.0.1 or 1.8.1?

Comment 66

[Torben]

(In reply to comment #65)
> Is there a way we can get this in either the 1.8.0.1 or 1.8.1?

I assume the best thing would be to get this in to the trunk for some testing. I'll upload a patch with a new IID for docshell/base/nsIMarkupDocumentViewer.idl and the two misspellings of "zooming" fixed tonight or tomorrow. 

Comment 67

[Torben]

Attachment: With updated UIID and without misspellings

Comment 68

[Samuel Sidler (old account; do not CC)]

Comment on attachment 192527 [details] [diff] [review]
Updated patch w/o "confusion"-workaround

Clearing review request (sorry for bugspam).

Comment 69

[Samuel Sidler (old account; do not CC)]

Since we didn't get approval for the patch, moving to Camino 1.1. Hopefully we can get this on the 1.8.1 branch.

Comment 70

[Daniel Veditz [:dveditz]]

I think we're not accepting interface (IDL) changes on the 1.8 branch, certainly not for 1.8.0.1

It's generally safer to add new entry points to the end of an interface than in the middle.

Comment 71

[Benjamin Smedberg]

Comment on attachment 204219 [details] [diff] [review]
With updated UIID and without misspellings

On the 1.8 branch no XPCOM interface changes are allowed. If you need something like this you'll need to add a new interface (nsIMarkupDocumentViewer2).

Comment 72

[Torben]

Attachment: Patch w/o core changes

> On the 1.8 branch no XPCOM interface changes are allowed.

I suggest we just drop the core changes for now. This means that the width won't shrink upon zooming ("de-zooming" works fine) but I would assume zooming is mostly used when the window is too small anyway. As a bonus the force-resize-after-reflow hack (in windowShouldZoom) can be removed.

Comment 73

[Mike Pinkerton (not reading bugmail)]

can you get someone else to r= this before i sr? thanks.

Comment 74

[Torben]

Comment on attachment 213803 [details] [diff] [review]
Patch w/o core changes

Smokey, Stuart (or any one else) care to review? After using this for a while I actualy think this works better now than with the core changes, even though we don't shrink the original width, we avoid the sometimes-reflow-sometimes-not dilemma.

Comment 75

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Maybe Håkan can take a look ;)

Comment 76

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 213803 [details] [diff] [review]
Patch w/o core changes

Cancelling the sr request until this has an r+ (per comment 73, and to make the queue stop lying)

Comment 77

[Håkan Waara]

Comment on attachment 213803 [details] [diff] [review]
Patch w/o core changes

Sorry for not getting to this quicker. It's a big patch, with changes that will require me to invest quite some time to really understand it.

I'd like to see some QA testing on this patch to make sure it's doing the right thing before I take all that time to do a proper review.

So if anyone is interested in moving this forward, please apply the patch, and test it.

(Also, this bug is very long; just summarizing what the actual behavior of this feature will be will help.)

Thanks!

Comment 78

[Torben]

> It's a big patch, [...]

The real changes are not that big, all the changes in bookmarks/BookmarkToolbar.mm are just moving some of the code to a separate function. And the rest is mainly an awkward way of getting a size (unfortunately there is no easy way :-( ).

> I'd like to see some QA testing [...]

Well, I've been running with this for ages and it works really well... oh wait, you probably meant by someone else than the patch author ;-) Anyone feel brave?

> Also, this bug is very long; just summarizing what the actual behavior of
> this feature will be will help.

Extremely short version: make window zoom do what it's supposed to do on OS X.

Longer version: figure out how to do the logic in comment #34 (which is a shortened version of Apple's documentation of NSWindow's zoom function), -- basically find the minimum size which the current window's content can be shown without scrollbars, OS X will do the rest for us.

And there is some workaround for the problem that the OS think the window's original size is what is given in the nib-file (which is not true, see comment #23, #51, #55, and #56 (I wish there was an easy way to describe this but there isn’t :-( Unless someone wants to try all of the old patches you'll have to trust me).

Comment 79

[froodian (Ian Leue)]

I'm happy to do some QA testing on this, but it's bitrotted.  Torben, do you think you could upload a new patch? :)

Comment 80

[Torben]

Attachment: Patch updated to latest trunk

Comment 81

[froodian (Ian Leue)]

A couple things:

1. about:bookmarks and about:history zoom to <10px tall (but the right width)
2. when you zoom from about:blank, we should probably the retain the current behavior of maximizing (with the current patch, zooming from about:blank does nothing)
3. there's a weird "three views" cycle you can get into, which seems odd and unwanted.  STR:
     1. Go to google.com
     2. Make the window as narrow as it will go
     3. Click "zoom" repeatedly
The first zoom behaves like you'd expect, the second zoom shrinks the window vertically about a status bar's worth, and the third zoom brings you back to the narrow window.

Other than that, this looks pretty good from a QA standpoint.  It looks sexier than you'd think to be able to resize to a website's optimal width.

Comment 82

[Torben]

> 1. about:bookmarks and about:history zoom to <10px tall (but the right width)

Several of the about:'s act strange :-( But some also works right, I'll try to find a fix.

> 2. when you zoom from about:blank, we should probably the retain the current
> behavior of maximizing (with the current patch, zooming from about:blank does
> nothing)

Personaly I prefer doing nothing. Anyone else have some opinions?

> 3. there's a weird "three views" cycle you can get into, which seems odd and
> unwanted.  STR:
>      1. Go to google.com
>      2. Make the window as narrow as it will go
>      3. Click "zoom" repeatedly
> The first zoom behaves like you'd expect, the second zoom shrinks the window
> vertically about a status bar's worth, and the third zoom brings you back to
> the narrow window.

The problem is the bottom links which reflows when the width changes, I've seen this on some other pages too. Not sure there is anything I can do to fix this...

Comment 83

[Paul Borokhov (lensovet)]

(In reply to comment #82)
> > 2. when you zoom from about:blank, we should probably the retain the current
> > behavior of maximizing (with the current patch, zooming from about:blank does
> > nothing)
> 
> Personaly I prefer doing nothing. Anyone else have some opinions?
Well, I personally prefer the behavior froodian describes - i.e. fill to screen. I love this feature.

Comment 84

[froodian (Ian Leue)]

A few questions to keep in mind:

- I must have tested this, but I can't remember doing it.  This plays nicely in all multiple tab situations, right?
- Does this play nicely with zoom all? (bug 181978)
- Does this affect the zoom of the downloads window in any way?

Comment 85

[Torben]

> A few questions to keep in mind:
> 
> - I must have tested this, but I can't remember doing it.  This plays nicely in
> all multiple tab situations, right?

Yes, zooming an unzoomed tab will zoom the window, zooming a zoomed tab restores the original window size no matter if you have switched, opened and/or closed tabs.

> - Does this play nicely with zoom all? (bug 181978)

Haven't tested yet but since (AFAIK) zoom all just sends a zoom command to every window I see no reason why this shouldn't work as expected.

> - Does this affect the zoom of the downloads window in any way?

IIRC, no.

I'll test some more and try to find a solution for the about:s during the week-end.

Comment 86

[Torben]

Attachment: Updated patch

Changes from the last patch:
about:blank now maximizes to the screen, bookmarks and history does too. Same thing if there is an error calculating the new size (this also fixes about:config). The rest of the about:s works well.

No problems with Zoom All, and no effect on the download window.

Comment 87

[Håkan Waara]

Comment on attachment 228627 [details] [diff] [review]
Updated patch

This patch fails to apply (in BWC)

Comment 88

[Håkan Waara]

Comment on attachment 228627 [details] [diff] [review]
Updated patch

Review of the CHBrowserView chunks:

>Index: src/embedding/CHBrowserView.h
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/embedding/CHBrowserView.h,v
>retrieving revision 1.38
>diff -u -8 -r1.38 CHBrowserView.h
>--- src/embedding/CHBrowserView.h       6 Jul 2006 08:10:57 -0000       1.38
>+++ src/embedding/CHBrowserView.h       9 Jul 2006 16:09:07 -0000
>@@ -246,16 +246,19 @@
> - (BOOL)canRedo;
> 
> - (void)biggerTextSize;
> - (void)smallerTextSize;
> 
> - (BOOL)canMakeTextBigger;
> - (BOOL)canMakeTextSmaller;
> 
>+// get the intrisic size of the content (for window zooming)
>+- (NSSize)getIntrinsicSize;
>+

Please don't use getFoo for getters, it's not objective-c style. Simply |intrinsicSize| would be better.

> // ideally these would not have to be called from outside the CHBrowerView, but currently
> // the cocoa impl of nsIPromptService is at the app level, so it needs to call down
> // here. We'll just turn around and call the CHBrowserContainer methods
> - (void)doBeforePromptDisplay;
> - (void)doAfterPromptDismissal;
> 
> - (void)setActive: (BOOL)aIsActive;
> 
>Index: src/embedding/CHBrowserView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/embedding/CHBrowserView.mm,v
>retrieving revision 1.71
>diff -u -8 -r1.71 CHBrowserView.mm
>--- src/embedding/CHBrowserView.mm      6 Jul 2006 08:10:57 -0000       1.71
>+++ src/embedding/CHBrowserView.mm      9 Jul 2006 16:09:09 -0000
>@@ -112,18 +112,20 @@
> // Cut/copy/paste
> #include "nsIClipboardCommands.h"
> #include "nsIInterfaceRequestorUtils.h"
> 
> // Undo/redo
> #include "nsICommandManager.h"
> #include "nsICommandParams.h"
> 
>-#include "GeckoUtils.h"
>+#include "nsIDOMElement.h"
>+#include "nsIDOMNSHTMLElement.h"
> 
>+#include "GeckoUtils.h"
> 
> const char kPersistContractID[] = "@mozilla.org/embedding/browser/nsWebBrowserPersist;1";
> const char kDirServiceContractID[] = "@mozilla.org/file/directory_service;1";
> 
> #define MIN_TEXT_ZOOM 0.01f
> #define MAX_TEXT_ZOOM 20.0f
> 
> // fix warnings
>@@ -1278,16 +1280,53 @@
>     zoom = max;
> 
>   markupViewer->SetTextZoom(zoom);
> }
> 
> 
> #pragma mark -
> 
>+- (NSSize)getIntrinsicSize
>+{
>+  NSSize contentSize = {0, 0};
>+  
>+  nsCOMPtr<nsIDOMWindow> contentWindow = getter_AddRefs([self getContentWindow]);
>+  nsCOMPtr<nsIDOMDocument> domDocument;
>+  contentWindow->GetDocument(getter_AddRefs(domDocument));
>+  nsCOMPtr<nsIDOMElement> docElement;
>+  domDocument->GetDocumentElement(getter_AddRefs(docElement));
>+  nsCOMPtr<nsIDOMNSHTMLElement> nsElement = do_QueryInterface(docElement);
>+  if (!nsElement)
>+    return contentSize;

You could be a little more paranoid about null results from QI here, just in case.

>+
>+  PRInt32 contentWidth = 0, contentHeight = 0;
>+  nsElement->GetClientWidth(&contentWidth); // nsElement->GetScrollWidth(&contentWidth);
>+  nsElement->GetScrollHeight(&contentHeight);

Why don't we get the scrollWidth as well? Looks like you tried it and decided against it, but would be interesting to know why. :-)

>+
>+  // Add 1 px extra to the height and width to guard against these values being rounded down
>+  contentSize.width  = contentWidth + 1;
>+  contentSize.height = contentHeight + 1;
>+
>+  // If the window has horizontal scrollers, add the max x offset and return
>+  nsCOMPtr<nsIDOMWindowInternal> domWindow = do_QueryInterface(contentWindow);
>+  if (!domWindow)
>+    return contentSize;
>+
>+  PRInt32 scrollMaxX = 0;
>+  domWindow->GetScrollMaxX(&scrollMaxX);
>+  if (scrollMaxX > 0) 
>+    contentSize.width  += scrollMaxX;

Would this chunk be needed if we got the scrollWidth?

Finally, can you factorize this method into something more generic (maybe something returning the intrinsic size of any document) and put it in GeckoUtils? That's a good place to hide away XPCOM/Gecko stuff.

I'll continue with the review of the other stuff + test in the next round. Keep up the good work!

Comment 89

[Håkan Waara]

A correction, I don't mean null results from QI, I mean null results in general (from getter_AddRefs() in this case).

Comment 90

[Torben]

> >+  PRInt32 contentWidth = 0, contentHeight = 0;
> >+  nsElement->GetClientWidth(&contentWidth);
> > // nsElement->GetScrollWidth(&contentWidth);
> >+  nsElement->GetScrollHeight(&contentHeight);
> 
> Why don't we get the scrollWidth as well? Looks like you tried it and decided
> against it, but would be interesting to know why. :-)

Because it doesn't always give the right result, http://vg.no being one example :-( However, after some more tesing, javascript:document.body.scrollWidth does AFAICT always give the correct result so the problem may be that I somehow get the wrong nsIDOMNSHTMLElement. Anyone know an easy way to do document.body.scrollWidth in objective-c?

[snip]

> Would this chunk be needed if we got the scrollWidth?

If I could get the correct value, no.

> Finally, can you factorize this method into something more generic (maybe
> something returning the intrinsic size of any document) and put it in
> GeckoUtils? That's a good place to hide away XPCOM/Gecko stuff.

If it makes the above easier, I'm all for it :-)

Comment 91

[Håkan Waara]

(In reply to comment #90)
> > >+  PRInt32 contentWidth = 0, contentHeight = 0;
> > >+  nsElement->GetClientWidth(&contentWidth);
> > > // nsElement->GetScrollWidth(&contentWidth);
> > >+  nsElement->GetScrollHeight(&contentHeight);
> > 
> > Why don't we get the scrollWidth as well? Looks like you tried it and decided
> > against it, but would be interesting to know why. :-)
> 
> Because it doesn't always give the right result, http://vg.no being one example
> :-( However, after some more tesing, javascript:document.body.scrollWidth does
> AFAICT always give the correct result so the problem may be that I somehow get
> the wrong nsIDOMNSHTMLElement. 

I /think/ document.body would be bad, because it'd be html-only (right?). 

I don't claim to understand the diff between clientWidth/scrollWidth, so I think that's the first step: to know what they are and use the Right one (whichever that is). :-)

> Anyone know an easy way to do document.body.scrollWidth in objective-c?

Not sure what you mean. We need to get this info from Gecko, and there is no way to do with pure objective-c, afaik.

Comment 92

[Torben]

> I don't claim to understand the diff between clientWidth/scrollWidth, so I
> think that's the first step: to know what they are and use the Right one
> (whichever that is). :-)

Neither do I, and there's very little documentation :-( However, through trial I think scrollWidth is what we want but there seems to be a bug involving <pre>-elements (bug 346591) (Why this doesn't affect document.body.scrollWidth I don't know.) I suggest we stick to clientWidth + scrollMax for now and update when/if this get fixed.

> can you factorize this method into something more generic (maybe
> something returning the intrinsic size of any document) and put it in
> GeckoUtils?

Will do and update the patch, hopefully during the week.

Comment 93

[Torben]

Attachment: Updated patch, XPCOM-stuff moved to GeckoUtils

>> I don't claim to understand the diff between clientWidth/scrollWidth, so I
>> think that's the first step: to know what they are and use the Right one
>> (whichever that is). :-)

> I think scrollWidth is what we want but there seems to be a bug involving
> <pre>-elements 

Actually there's a problem with all non-scrollable elements. Unfortunately this is the expected behavior which makes scrollWidth a no-op :-( clientWidth + scrollMaxX will have to stay.

> can you factorize this method [CHBrowserView's getIntrinsicSize ] into
> something more generic (maybe
> something returning the intrinsic size of any document) and put it in
> GeckoUtils?

moved to GeckoUtils::GetIntrisicSize w/null checks everywhere. Still uses nsIDOMWindow since we need to get the nsIDOMWindowInternal.

> This patch fails to apply (in BWC)

BrowserWindowController.mm contains tabs :-( use the -l option with patch

Comment 94

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 234654 [details] [diff] [review]
Updated patch, XPCOM-stuff moved to GeckoUtils

This is one of our most requested features and has high community interest, but it really needs to land by the next milestone in order to take it for 1.1.

Unfortunately, the patch has bitrotted in the past 2 months :( 2 hunks in BWC and 2 in GeckoUtils.h

Håkan, Torben: can we come up with a solid plan to get an updated patch and an expedient review with an aim to get this in the next milestone?

Comment 95

[Torben]

Attachment: Unbitrotted patch

Comment 96

[Håkan Waara]

Comment on attachment 243891 [details] [diff] [review]
Unbitrotted patch

Sorry for letting this wait so long, Torben. It's a big patch, and takes some time to really understand. Here is a first code-review, I've yet to test it. I must say it looks great though!

>Index: src/bookmarks/BookmarkToolbar.h
>Index: src/bookmarks/BookmarkToolbar.mm

Are there any real changes to the bookmark toolbar reflow, or are you just moving around code? If there are changes, please give details.

>Index: src/browser/BrowserWindowController.h
>Index: src/browser/BrowserWindowController.mm

I think mOrigFrame can use a less cryptic var name, something like mLastFrameSize?

If I understand the mShouldZoom checks in windowWillUseStandardFrame:defaultFrame, you basically check if there'll be any substantial change if we'd zoom (using the magic constant of 10, please define it as a constant instead!), and set the boolean accordingly.

What would you say about removing mShouldZoom, and doing the checks live in windowShouldZoom? Also, the repeated code should be factored out somehow. Prefer MAX()/MIN() to nspr helper functions.

It will be easier to follow the logic in windowWillUseStandardFrame:defaultFrame: when it's shorter (things factored out, and perhaps some logic moved to windowShouldZoom, or another helper method?).


>Index: src/extensions/GeckoUtils.cpp
>Index: src/extensions/GeckoUtils.h

GetIntrinsicSize() looks good. There *is* actually documentation on scrollWidth, clientWidth, and scrollMaxX on developer.mozilla.org!

* http://developer.mozilla.org/en/docs/DOM:element.scrollWidth
* http://developer.mozilla.org/en/docs/DOM:element.clientWidth
* http://developer.mozilla.org/en/docs/DOM:window.scrollMaxX

It's a tough call to choose what to use here. clientWidth is suboptimal because it doesn't include margin or border. scrollWidth is probably generally better since it seems to not be HTML-specific and hopefully includes margin+border. What was the problem you encountered in using it? 

scrollMaxX is probably only useful together with clientWidth, as clientWidth only looks at what's visible, if I understand it correctly.

I don't think we should block the patch if we don't find the perfect solution here; we can always fix and adjust things later, but it's better to make a first informed decision based on these docs together with real-world testing, to see what works.

Comment 97

[Torben]

(In reply to comment #96)

> Are there any real changes to the bookmark toolbar reflow, or are you just
> moving around code? If there are changes, please give details.

Only movement of code to separate out the computeHeight function, nothing should have changed regarding the reflow.

> If I understand the mShouldZoom checks in
> windowWillUseStandardFrame:defaultFrame, you basically check if there'll be 
> any substantial change if we'd zoom [...], and set the boolean accordingly.

Correct.

> What would you say about removing mShouldZoom, and doing the checks live in
> windowShouldZoom? Also, the repeated code should be factored out somehow.

The problem is that the OS might already have changed our calculated frame size :-( There are two solutions to this, either we save the calculated frame or we compare the suggested size to the one in the nib-file. The first one is probably the cleanest and most future proof, I just have to figure out a good name for the new variable (and the corresponding the magic constant). 

> GetIntrinsicSize() looks good. There *is* actually documentation on
> scrollWidth, clientWidth, and scrollMaxX on developer.mozilla.org!
> 
> It's a tough call to choose what to use here. clientWidth is suboptimal 
> because it doesn't include margin or border. scrollWidth is probably 
> generally better since it seems to not be HTML-specific and hopefully 
> includes margin+border. What was the problem you encountered in using it?

Since we're getting the clientWidth of the window, border and margins shouldn't be a problem, the only thing we're missing is the width of the scrollbar (and that's a good thing since we add it later if needed).

The problem with scrollWidth is with non-scrollable elements, there scrollWidth = offsetWidth, which is not what we want :-(

> I don't think we should block the patch if we don't find the perfect solution
> here; we can always fix and adjust things later

Yes, I plan to reexamine this when the reflow-branch lands.

Will fix the other bits and make a new patch tonight.

Comment 98

[Torben]

Attachment: Updated patch

> > What would you say about removing mShouldZoom, and doing the checks live in
> > windowShouldZoom? Also, the repeated code should be factored out somehow.
> 
> There are two solutions to this, [...]

Neither of which works :-( And we need mShouldZoom in windowDidResize too. Factored out the mShouldZoom-logic in a new function setZoomState.

Otherwise everything in comment #96 should be fixed.

Comment 99

[Håkan Waara]

Comment on attachment 244120 [details] [diff] [review]
Updated patch

>+  PRInt32 contentWidth = 0, contentHeight = 0;
>+  GeckoUtils::GetIntrisicSize(contentWindow, &contentWidth, &contentHeight);
>+  if (contentWidth <= 0 || contentHeight <= 0) {
>+    // Something went wrong, maximize to screen
>+    [self setZoomState:defaultFrame defaultFrame:defaultFrame];
>+    return defaultFrame;
>+  }

Is this case really realistic? Can clientWidht and maxX ever, even in theory, return something negative? I think it's better to assert if it's something that "may" happen once every 4000 years. :) If there are other cases you think really will not ever-ever happen, but you still want the sanity check, please assert there too.

>+
>+  // Get the current content size and calculate the changes.
>+  NSSize curFrameSize = [[mBrowserView getBrowserView] frame].size;
>+  float widthChange   = contentWidth  - curFrameSize.width;
>+  float heightChange  = contentHeight - curFrameSize.height;
>+
>+  // Change the window size, but don't let it be to narrow
>+  NSRect stdFrame     = [[self window] frame];
>+  stdFrame.size.width = MAX([[self window] minSize].width, stdFrame.size.width + widthChange);
>+
>+  if ([mPersonalToolbar isShown])
>+    // if the personal toolbar is shown we need to adjust for its height change
>+    heightChange += [mPersonalToolbar computeHeight: stdFrame.size.width startingAtIndex: 0]

Please put the arguments immediately after the colon, this applies everywhere.

>+  // ScrollHeight always gets the wanted height but ScrollWidth does not, therefor we find
>+  // the ClientWidth and adds the max x offset if the window has a horizontal scroller

Update all the comments in GetIntrinsicWidth() to explain the rationale of why you are using the DOM properties you do (since you know have the info exactly how they work from MDC), and you might even put a link to those pages there.

Since it's an area we might need to polish later, for extreme cases (who knows) it's better to be really clear about how it works, why, etc.

r=me with changes addressed!   I don't know who is a better super-reviewer for this. Maybe Simon, if he feels like it?

Comment 100

[Torben]

(In reply to comment #99)
> (From update of attachment 244120 [details] [diff] [review] [edit])
> >+  PRInt32 contentWidth = 0, contentHeight = 0;
> >+  GeckoUtils::GetIntrisicSize(contentWindow, &contentWidth, 
> >                                              &contentHeight);
> >+  if (contentWidth <= 0 || contentHeight <= 0) {
> >+    // Something went wrong, maximize to screen
> >+    [self setZoomState:defaultFrame defaultFrame:defaultFrame];
> >+    return defaultFrame;
> >+  }
> 
> Is this case really realistic? Can clientWidht and maxX ever, even in theory,
> return something negative? I think it's better to assert if it's something 
> that "may" happen once every 4000 years. :) If there are other cases you 
> think really will not ever-ever happen, but you still want the sanity check, 
> please assert there too.

Negative no, but zero yes - IIRC this will happen for full-page plugins. Updated patch coming late tonight.

Comment 101

[Torben]

Attachment: hwaara r+ patch

> > >+  PRInt32 contentWidth = 0, contentHeight = 0;
> > >+  GeckoUtils::GetIntrisicSize(contentWindow, &contentWidth, 
> > >                                              &contentHeight);
> > >+  if (contentWidth <= 0 || contentHeight <= 0) {
> > >+    // Something went wrong, maximize to screen
> > >+    [self setZoomState:defaultFrame defaultFrame:defaultFrame];
> > >+    return defaultFrame;
> > >+  }
> > 
> > Is this case really realistic? Can clientWidht and maxX ever, even in
> > theory, return something negative?

> Negative no, but zero yes - IIRC this will happen for full-page plugins.
 
I remebered incorrectly, plugins doesn't give us any problems. However, after testing zooming with all of my bookmarks I found one site* where contentHeight incorrectly was 1 (because of the padding I do in GetIntrisicSize). I have updated the test to <= 1.

* http://til.gamingsource.net/obbooks/

Comment 102

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Because of testing implications, we should try to get this in a2.

Comment 103

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 244305 [details] [diff] [review]
hwaara r+ patch

+  // If the window is empty or the bookmark manager is loaded maximize to screen  
+  if ([[self getBrowserWrapper] isEmpty] || [self bookmarkManagerIsVisible]) {
+    [self setZoomState:defaultFrame defaultFrame:defaultFrame];
+    return defaultFrame;
+  }

will this be the whole screen, or will it leave the hard drives/finder visible on the right? i think we want to leave a buffer on the right.

sr=pink

Comment 104

[Chris Lawson (gone)]

(In reply to comment #103)

> i think we want to leave a buffer on the right.

Safari doesn't, FWIW. Firefox, on the other hand, does.

I prefer Safari's behaviour. It's what Mac apps have traditionally done.

Comment 105

[froodian (Ian Leue)]

A(In reply to comment #103)
> will this be the whole screen, or will it leave the hard drives/finder visible
> on the right? i think we want to leave a buffer on the right.

Users don't necessarily have drives/finder visible at all, not to mention on the right-that's just the default.  If they didn't exist, would we still leave space for them?  I think we want to zoom to the whole screen personally.

Comment 106

[Mike Pinkerton (not reading bugmail)]

ok i just tried mail and textEdit and they both zoom to the full screen. *shrug*. we can always change that if we get too many complaints.

The "leave space on the right" goes way way back in macOS lore, which is why we do it in firefox. Maybe I'm just old.

Comment 107

[Torben]

Attachment: Post bug 360345 patch

Comment 108

[froodian (Ian Leue)]

Checked in on trunk and 1.8branch (congrats torben!)

Comment 109

[Chris Lawson (gone)]

v. on trunk version 2006111605.

Comment 110

[Jonathan Thurlow]

This doesn't work 100% consistently yet.  Please see https://bugzilla.mozilla.org/show_bug.cgi?id=361049

Comment 111

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Clearing since this landed some time ago.