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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jaas, Assigned: jaas)
References
Details
Attachments
(1 file, 2 obsolete files)
3.50 KB,
patch
|
pavlov
:
review+
|
Details | Diff | Splinter Review |
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. ***
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.
Comment 4•18 years ago
|
||
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.
Comment 5•18 years ago
|
||
It gets them from here :( http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/cocoa/nsCocoaWindow.mm&rev=1.44&mark=526-529#526 (via nsXULWindow::GetPositionAndSize)
This solves most of the problem, there are a few quirks I want to check out before I request review.
Comment 8•18 years ago
|
||
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.
Comment 9•18 years ago
|
||
It should have a capital H either way.
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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?
Assignee | ||
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
(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.
Assignee | ||
Comment 14•18 years ago
|
||
Attachment #217258 -
Attachment is obsolete: true
Attachment #217375 -
Flags: review?(pavlov)
Attachment #217258 -
Flags: review?(mark)
Assignee | ||
Comment 15•18 years ago
|
||
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)
Assignee | ||
Comment 16•18 years ago
|
||
To be clear, this patch also fixes 330585 now that I've discovered the strange Carbon behavior.
Updated•18 years ago
|
Attachment #217377 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 17•18 years ago
|
||
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.
Description
•