Get rid of mIntrinsicallySized and SetIntrinsicallySized as they are unneeded.

NEW
Unassigned

Status

()

Core
XUL
8 years ago
8 years ago

People

(Reporter: Natch, Unassigned)

Tracking

(Blocks: 1 bug, {helpwanted, pp})

Trunk
helpwanted, pp
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

8 years ago
See bug 509828, setting the size of the window should make it intrinsically sized.
Flags: wanted1.9.2?
Flags: in-testsuite?

Comment 1

8 years ago
You mean *not* intrinsically sized, right? :-)
(Reporter)

Comment 2

8 years ago
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.

Comment 3

8 years ago
(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.
(Reporter)

Comment 4

8 years ago
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.
(Reporter)

Comment 5

8 years ago
Created attachment 407964 [details] [diff] [review]
patch

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 6

8 years ago
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+

Comment 7

8 years ago
I thought I'd found a bug in the patch.

But then I realised it was sessionstore overriding the browser's size!
(Reporter)

Comment 8

8 years ago
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.

Comment 9

8 years ago
(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.
(Reporter)

Comment 10

8 years ago
Created attachment 408236 [details] [diff] [review]
patch

Updated to comments.
Attachment #407964 - Attachment is obsolete: true
Attachment #408236 - Flags: superreview?(jag)
(Reporter)

Updated

8 years ago
Status: NEW → ASSIGNED
Priority: P2 → --
(Reporter)

Updated

8 years ago
Blocks: 524326
(Reporter)

Updated

8 years ago
Attachment #408236 - Flags: superreview?(jag) → superreview?(jst)
Flags: wanted1.9.2? → wanted1.9.2-

Comment 11

8 years ago
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.
(Reporter)

Comment 12

8 years ago
Can you mark the patch?

Thanks.

Updated

8 years ago
Attachment #408236 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 13

8 years ago
Created attachment 409244 [details] [diff] [review]
patch - nit addressed

Thanks all.
Attachment #408236 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
I pushed this to the tryserver twice, and both times got the same 23 failures in TestWinTSF:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257040582.1257048394.11145.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257029422.1257037427.21998.gz
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
(Reporter)

Comment 16

8 years ago
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.
(Reporter)

Comment 17

8 years ago
Created attachment 409635 [details] [diff] [review]
test fix.

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)
(Reporter)

Comment 18

8 years ago
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)
(Reporter)

Comment 19

8 years ago
Created attachment 409651 [details] [diff] [review]
More fixes.

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)
(Reporter)

Comment 20

8 years ago
Created attachment 409715 [details] [diff] [review]
even more fixes

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)
(Reporter)

Comment 21

8 years ago
Created attachment 409718 [details] [diff] [review]
even more fixes

Missed the actual widget call :/
Attachment #409715 - Attachment is obsolete: true
(Reporter)

Comment 22

8 years ago
Created attachment 409719 [details] [diff] [review]
even more fixes

I suck :(

Forgot to qrefresh
Attachment #409718 - Attachment is obsolete: true
(Reporter)

Comment 23

8 years ago
Created attachment 409952 [details] [diff] [review]
what I tried

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
(Reporter)

Updated

8 years ago
Status: ASSIGNED → NEW
Keywords: helpwanted, pp
(Reporter)

Comment 24

8 years ago
I have another idea of what possibly went wrong here, pushed to try. I'll post the results here when they're in.
(Reporter)

Comment 25

8 years ago
Created attachment 410293 [details] [diff] [review]
another attempt

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
(Reporter)

Comment 26

8 years ago
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
You need to log in before you can comment on or make changes to this bug.