Open Bug 523909 Opened 15 years ago Updated 2 years ago

Get rid of mIntrinsicallySized and SetIntrinsicallySized as they are unneeded.

Categories

(Core :: XUL, defect)

defect

Tracking

()

People

(Reporter: Natch, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: helpwanted, platform-parity)

Attachments

(1 file, 9 obsolete files)

See bug 509828, setting the size of the window should make it intrinsically sized.
Flags: wanted1.9.2?
Flags: in-testsuite?
You mean *not* intrinsically sized, right? :-)
Yes, I've been misusing the word all along... :/

One question though, what happens with nsIAppshellService::SIZE_TO_CONTENT now? There are callers that specify a specific aInitialHeight and aInitialWidth...

With your code sample in the dependent bug we'll SizeToContent whenever a) the dimensions weren't specified in openDialog, b) there's no size specified on the <window> element. Previously, callers had the option of specifying SIZE_TO_CONTENT.
(In reply to comment #2)
> One question though, what happens with nsIAppshellService::SIZE_TO_CONTENT now?
> There are callers that specify a specific aInitialHeight and aInitialWidth...
I could only find one in-tree caller and it seemed to pass in completely bogus values. Everyone else seems to pass in SIZE_TO_CONTENT. I don't know how frozen this stuff is, otherwise I would suggest eliminating those parameters.

> With your code sample in the dependent bug we'll SizeToContent whenever a) the
> dimensions weren't specified in openDialog, b) there's no size specified on the
> <window> element. Previously, callers had the option of specifying
> SIZE_TO_CONTENT.
Yeah, I guess I eliminated the wrong variable; we would be better off just having mIntrinsicallySized and mIntrinsicallyPositioned variables.
After discussion with Neil, this actually works already, so what needs to get done here is remove all of this old cruft.
Summary: Specifying height and width features in openWindow should not size the window to its contents → Get rid of mIntrinsicallySized and SetIntrinsicallySized as they are unneeded.
Attached patch patch (obsolete) — Splinter Review
Here's the patch, with a test to ensure the SizeToContent stuff still works. Behavior should be exactly the same with regard to everything (windows with/without attributes, windows opened with a size, windows lacking both).

The test for the SizeToContent is a bit lame (the check itself), but I can't figure out a reliable way to calculate what the size should be.
Attachment #407964 - Flags: superreview?(neil)
Attachment #407964 - Flags: review?(neil)
Comment on attachment 407964 [details] [diff] [review]
patch

[Can't r+sr in Gecko these days]

>+    ok(win.innerHeight > 80 && win.innerHeight < 100, "The window should be sized to its contents, height=80px");
>+    ok(win.innerWidth > 80 && win.innerWidth < 150, "The window should be sized to its contents, width=80px");
Why such a big range? The height should be about right, but you might find the width is off by 1. Maybe if you made the spacer bigger to start with you wouldn't have to worry about the window manager enforcing a minimum size. (And of course make the extrinsic size call use even bigger numbers.)

>+<richlistbox id="richlist" height="80" minheight="80" maxheight="80" width="80" minwidth="80" maxwidth="80"/>
Just make this a spacer. And you don't need all those attributes; height and width should be quite enough.

>-  rv = JustCreateTopWindow(nsnull, url,
>-                           chromeMask, initialWidth, initialHeight,
>-                           PR_TRUE, aAppShell, getter_AddRefs(newWindow));
As you're not using initialWidth and initialHeight any more you should remove their definitions too.

>   nsresult rv = window->Initialize(parent, center ? aParent : nsnull,
>                                    aAppShell, aUrl,
>-                                   aInitialWidth, aInitialHeight,
>+                                   1, 1,
Follow up to remove the initial width/height from Initialize?
Attachment #407964 - Flags: superreview?(neil)
Attachment #407964 - Flags: review?(neil)
Attachment #407964 - Flags: review+
I thought I'd found a bug in the patch.

But then I realised it was sessionstore overriding the browser's size!
I'll make all those changes over the weekend, just a few notes:

1) The window height was always bigger by 4px and the width by like 20-30, I couldn't find a way to reliably figure out the expected size, so I went with what I thought would pass on all platforms. I'll try out the changes you mentioned and see what I can do.

2) Do I need sr from someone else then?

3) At the widget level, there needs to be some size specified it seems, which is why the original code substituted SIZE_TO_CONTENT with 1. I just went with that, but I'll file the followup.
(In reply to comment #8)
> 1) The window height was always bigger by 4px and the width by like 20-30
Using the following XUL:
<?xml version="1.0"?>
<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<spacer width="300" height="300"/>
</window>
I got an innerHeight of 300 and an innerWidth of 301. (The innerWidth is 1 off because of someone's silly rounding error allowance which I previously tried and failed to get removed.)

> 2) Do I need sr from someone else then?
I think you do, IIRC because you're changing a public API.

> 3) At the widget level, there needs to be some size specified it seems, which
> is why the original code substituted SIZE_TO_CONTENT with 1. I just went with
> that, but I'll file the followup.
Yes, but in this case we're initialising an nsWebShellWindow; I'm assuming here we can simply move the 1, 1 to nsWebShellWindow's call into widget.
Attached patch patch (obsolete) — Splinter Review
Updated to comments.
Attachment #407964 - Attachment is obsolete: true
Attachment #408236 - Flags: superreview?(jag)
Status: NEW → ASSIGNED
Priority: P2 → --
Blocks: 524326
Attachment #408236 - Flags: superreview?(jag) → superreview?(jst)
Flags: wanted1.9.2? → wanted1.9.2-
Comment on attachment 408236 [details] [diff] [review]
patch

sr=jag if you still want it. One nit:

@@ -285,24 +280,22 @@ nsAppShellService::CalculateWindowZLevel

...

 {
   *aResult = nsnull;
-
+  

Undo that white space change.
Can you mark the patch?

Thanks.
Attachment #408236 - Flags: superreview?(jst) → superreview+
Attached patch patch - nit addressed (obsolete) — Splinter Review
Thanks all.
Attachment #408236 - Attachment is obsolete: true
Keywords: checkin-needed
And the Mac unit test failures, which looked sort of random the first time, turned out to be the same both times, too:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257029423.1257042896.16308.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257040583.1257054024.6021.gz
Repushed to try with a fix for the WinTSF errors, the Mac errors started with a known sporadic error, bug 524128, so I'll see what happens now.
Attached patch test fix. (obsolete) — Splinter Review
This fixes the WinTSF errors, Mac try is still running, but the rest are pretty clean.

Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257112775.1257120481.14934.gz&fulltext=1 (success, modulo bug 525739)

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257112774.1257121197.22500.gz&fulltext=1 (success, modulo bug 525739 bug 518534 bug 458688 bug 521802 bug 521802 bug 521889) No WinTSF errors.

I'll post the Mac results as soon as they come in, I don't see how they should be a problem though.
Attachment #409244 - Attachment is obsolete: true
Attachment #409635 - Flags: review?(neil)
Comment on attachment 409635 [details] [diff] [review]
test fix.

Mac still doesn't like this change. Looking into it...

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257112775.1257126283.13312.gz&fulltext=1
Attachment #409635 - Attachment is obsolete: true
Attachment #409635 - Flags: review?(neil)
Attached patch More fixes. (obsolete) — Splinter Review
Even hidden windows need a size <sigh>. I've pushed this change to try to see how it reacts, this is the only real difference pre->post patch.

Results will be posted once they're in!
Attachment #409651 - Flags: review?(neil)
Attached patch even more fixes (obsolete) — Splinter Review
Mac didn't like that change either, so I've restored the aInitial[Width|Height] arguments to JustCreateTopWindow so that the hidden window can specify the size at the widget call. Sending this to try now.
Attachment #409651 - Attachment is obsolete: true
Attachment #409651 - Flags: review?(neil)
Attached patch even more fixes (obsolete) — Splinter Review
Missed the actual widget call :/
Attachment #409715 - Attachment is obsolete: true
Attached patch even more fixes (obsolete) — Splinter Review
I suck :(

Forgot to qrefresh
Attachment #409718 - Attachment is obsolete: true
Attached patch what I tried (obsolete) — Splinter Review
So try didn't like the previous patch (at least OSX didn't like it), so I tried replacing createTopLevelWindow's aIntialHeight and aInitialWidth arguments, and also replacing CreateNewContentWindow's "random" 615x480 arguments, all to no avail.

I'm kind of lost now, as I don't have a Mac, but it seems the issue is with the hidden window. The problem is that it gets focus (I think that's a problem right there already) and won't relinquish it. If anyone with a Mac can help me out, I'd appreciate it. Specifically, why does Mac treat the hidden window differently (i.e. on Mac we do SetVisibility(PR_TRUE) on the hidden window)?

See: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsAppShellService.cpp?mark=183-190#176
Attachment #409719 - Attachment is obsolete: true
Status: ASSIGNED → NEW
Keywords: helpwanted, pp
I have another idea of what possibly went wrong here, pushed to try. I'll post the results here when they're in.
Attached patch another attemptSplinter Review
So, what I figured was that unless SetSize is called in the new world (without mIntrinsicallySized) the window will be sized to its contents. So I put a SetSize call in all the places where CreateTopLevelWindow was being called with anything other than SIZE_TO_CONTENT specifying the same size as before. This turned out to be the hidden window and in CreateNewContentWindow, but Mac still disliked the change. Linux was green, the rest:

Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257351572.1257361627.23119.gz&fulltext=1 (similar, although not the same, errors as before)

WINNT: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257351572.1257360074.5066.gz&fulltext=1 (one error, seemed pretty random to me, not sure though).

The problem is that for some reason on Mac the hidden window gets focused and when myWindow.focus() is called it doesn't really do anything and eventually leads to a timeout. The numerous errors after the third error are a result of the third error and can be ignored. I have yet to find a reason as to why this would happen, maybe I have to look at what the test before does (like calling focus() on the hidden window?) or maybe this just can't be removed?

In any event, still helpwanted.
Attachment #409952 - Attachment is obsolete: true
Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...
Assignee: highmind63 → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.