new windows are not always sized correctly on creation

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

3.50 KB, patch
Stuart Parmenter
: review+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
*** Bug 330582 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Blocks: 326469
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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

12 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.
(Assignee)

Comment 6

12 years ago
I figured this out, posting patch soon...
(Assignee)

Comment 7

12 years ago
Created attachment 217258 [details] [diff] [review]
fix v0.9

This solves most of the problem, there are a few quirks I want to check out before I request review.

Comment 8

12 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

12 years ago
It should have a capital H either way.
(Assignee)

Comment 10

12 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

12 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

12 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

12 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

12 years ago
Created attachment 217375 [details] [diff] [review]
fix v1.0
Attachment #217258 - Attachment is obsolete: true
Attachment #217375 - Flags: review?(pavlov)
Attachment #217258 - Flags: review?(mark)
(Assignee)

Comment 15

12 years ago
Created attachment 217377 [details] [diff] [review]
fix v1.1

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

12 years ago
To be clear, this patch also fixes 330585 now that I've discovered the strange Carbon behavior.

Updated

12 years ago
Attachment #217377 - Flags: review?(pavlov) → review+
(Assignee)

Comment 17

12 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.