Closed Bug 330581 Opened 18 years ago Closed 18 years ago

new windows are not always sized correctly on creation

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 2 obsolete files)

If you use a Cocoa build of Firefox, new browser windows are often smaller than they should be. In some cases the window size seems to be coming from the profile and getting set correctly, but then susequent windows will be only the size of the XUL menu bar and bookmark bar.
*** Bug 330582 has been marked as a duplicate of this bug. ***
Blocks: cocoa
I spent some time debugging this today, and while I didn't figure out why this happens I did collect some information.

First of all, I noticed that Carbon Firefox builds tend to save good information in "localstore.rdf". Here is an excerpt from the Carbon Firefox "localstore.rdf" after browsing for a bit and quitting:

  <RDF:Description RDF:about="chrome://browser/content/browser.xul#main-window"
                   sizemode="normal"
                   width="807"
                   height="850"
                   screenX="103"
                   screenY="22" />

Here is an excerpt from the Cocoa Firefox "localstore.rdf" after browsing for a bit and quitting:

  <RDF:Description RDF:about="chrome://browser/content/browser.xul#main-window"
                   screenX="0"
                   screenY="0"
                   width="0"
                   height="0"
                   sizemode="normal" />

I am not sure of the specifics of how this rdf data is used.

When we open a window, it is created with a size of either 0,0 or 1,1, which seems to be standard behavior (not a bug, happens in Carbon and Cocoa Firefox). Soon after we get a resize call, which in Carbon has a reasonable size and in Firefox has the bogus small window size. I stopped gdb where we get the bogus window size call, here is part of the stack:

--------------------------------------------
Breakpoint 2, nsCocoaWindow::Resize (this=0x28caa220, aWidth=513, aHeight=77, aRepaint=1) at /Users/josh/src/ff_cocoa_debug/mozilla/widget/src/cocoa/nsCocoaWindow.mm:504
504       printf("XXXJOSH nsCocoaWindow resized to: %d,%d\n", aWidth, aHeight);
(gdb) bt
#0  nsCocoaWindow::Resize (this=0x28caa220, aWidth=513, aHeight=77, aRepaint=1) at /Users/josh/src/ff_cocoa_debug/mozilla/widget/src/cocoa/nsCocoaWindow.mm:504
#1  0x1094c654 in nsXULWindow::SetSize (this=0x2b26cdd0, aCX=513, aCY=77, aRepaint=1) at /Users/josh/src/ff_cocoa_debug/mozilla/xpfe/appshell/src/nsXULWindow.cpp:587
#2  0x1094d743 in nsXULWindow::SizeShellTo (this=0x2b26cdd0, aShellItem=0x2b207424, aCX=513, aCY=77) at /Users/josh/src/ff_cocoa_debug/mozilla/xpfe/appshell/src/nsXULWindow.cpp:1718
#3  0x109460d5 in nsChromeTreeOwner::SizeShellTo (this=0x2b2b0b00, aShellItem=0x2b207424, aCX=513, aCY=77) at /Users/josh/src/ff_cocoa_debug/mozilla/xpfe/appshell/src/nsChromeTreeOwner.cpp:274
#4  0x119951ef in DocumentViewerImpl::SizeToContent (this=0x2b2f4960) at /Users/josh/src/ff_cocoa_debug/mozilla/layout/base/nsDocumentViewer.cpp:3135
#5  0x10953a39 in nsXULWindow::OnChromeLoaded (this=0x2b26cdd0) at /Users/josh/src/ff_cocoa_debug/mozilla/xpfe/appshell/src/nsXULWindow.cpp:972
#6  0x10959f7f in nsWebShellWindow::OnStateChange (this=0x2b26cdd0, aProgress=0x2b2073a4, aRequest=0x28cfbb70, aStateFlags=786448, aStatus=0) at /Users/josh/src/ff_cocoa_debug/mozilla/xpfe/appshell/src/nsWebShellWindow.cpp:627
...
(gdb) c
Continuing.
XXXJOSH nsCocoaWindow resized to: 513,77
--------------------------------------------

I'll look into this some more later, but if anyone has any ideas that would be great.
Apparently localstore.rdf is used for saving persistent attributes on XUL elements. So we should investigate how/when |NS_IMETHODIMP nsXULWindow::SavePersistentAttributes()| is getting data for saving to localstore.rdf.
Is that call specifically bogus?

Disclaimer: It was quite some time ago I used XUL daily, so I don't know if my information helps, anyway here are my thoughts.

It looks like the XUL window in action here is simply calling sizeToContent() (in its "onload" handler?) which makes it just slim enough to display the controls it contains.

I think localstore.rdf is used for persistence of where windows are positioned, their sizes and so on.  No idea who's supposed to enforce/read that data, though I'd guess XUL base does that.
I figured this out, posting patch soon...
Attached patch fix v0.9 (obsolete) — Splinter Review
This solves most of the problem, there are a few quirks I want to check out before I request review.
Comment on attachment 217258 [details] [diff] [review]
fix v0.9

Just trying to wrap my head around all the cocoa-to-gecko converting logic. What happens to this code when a user has two screens, one above the other?  

* Would the gecko-rect be per-screen or be smart about its position and size relative to the other screen?

* Perhaps highestScreenPoint() should be highestPointOfAllScreens() or something indicating its globalness.
It should have a capital H either way.
Comment on attachment 217258 [details] [diff] [review]
fix v0.9

I've satisfied my curiosity about some other issues during window creation, and I don't think they are related to this. I was looking at how we stagger windows. This patch does seem to be what we want.

I will make the function name more clear for highestScreenPoint(), and capitalize it if you want on checkin.
Attachment #217258 - Flags: review?(mark)
Comment on attachment 217258 [details] [diff] [review]
fix v0.9

I thought that this was supposed to return a window's bounds on a particular screen.  Isn't this patch equivalent to returning mBounds?
(In reply to comment #11)
> (From update of attachment 217258 [details] [diff] [review] [edit])
> I thought that this was supposed to return a window's bounds on a particular
> screen.  Isn't this patch equivalent to returning mBounds?

As discussed offline, mBounds doesn't have a valid origin for top-level windows (it is 0,0). However, we should be returning current-screen-relative coordinates which my patch does not do and thus it probably won't play nice on mulitple monitors.
(In reply to comment #11)
> (From update of attachment 217258 [details] [diff] [review] [edit])
> I thought that this was supposed to return a window's bounds on a particular
> screen.  Isn't this patch equivalent to returning mBounds?

Correction. Carbon widgets return global screen coordinates, not coords specific to the current screen.

In Carbon widgets, GetScreenBounds returns info for the content area of the window, while Windows and Linux seem to return info for the entire window (decoration and all).

If I match Windows behavior, then every new window that gets created is larger than the last. If I match Carbon behavior, then window staggering (y origin) is off by about 22 pixels. I'm going to submit a patch to match Carbon behavior and track down the staggering separately.
Attached patch fix v1.0 (obsolete) — Splinter Review
Attachment #217258 - Attachment is obsolete: true
Attachment #217375 - Flags: review?(pavlov)
Attachment #217258 - Flags: review?(mark)
Attached patch fix v1.1Splinter Review
Turns out the Carbon imlementation does something I totally strange and unexpected. It returns the origin for the *entire* window, but the size of only the content area. This patch matches that behavior, so we fix this bug and the staggering bug at the same time.

That said, this is really weird behavior that I'd like to do away with someday. It is very confusing.
Attachment #217375 - Attachment is obsolete: true
Attachment #217377 - Flags: review?(pavlov)
Attachment #217375 - Flags: review?(pavlov)
To be clear, this patch also fixes 330585 now that I've discovered the strange Carbon behavior.
Attachment #217377 - Flags: review?(pavlov) → review+
landed on trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: