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)
Core
XUL
Tracking
()
NEW
People
(Reporter: Natch, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: helpwanted, platform-parity)
Attachments
(1 file, 9 obsolete files)
26.46 KB,
patch
|
Details | Diff | Splinter Review |
See bug 509828, setting the size of the window should make it intrinsically sized.
Flags: wanted1.9.2?
Flags: in-testsuite?
Comment 1•15 years ago
|
||
You mean *not* intrinsically sized, right? :-)
Reporter | ||
Comment 2•15 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•15 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•15 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•15 years ago
|
||
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•15 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•15 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•15 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•15 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•15 years ago
|
||
Updated to comments.
Attachment #407964 -
Attachment is obsolete: true
Attachment #408236 -
Flags: superreview?(jag)
Reporter | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → --
Reporter | ||
Updated•15 years ago
|
Attachment #408236 -
Flags: superreview?(jag) → superreview?(jst)
Flags: wanted1.9.2? → wanted1.9.2-
Comment 11•15 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•15 years ago
|
||
Can you mark the patch? Thanks.
Updated•15 years ago
|
Attachment #408236 -
Flags: superreview?(jst) → superreview+
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 14•15 years ago
|
||
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
Comment 15•15 years ago
|
||
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•15 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•15 years ago
|
||
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•15 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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
Missed the actual widget call :/
Attachment #409715 -
Attachment is obsolete: true
Reporter | ||
Comment 22•15 years ago
|
||
I suck :( Forgot to qrefresh
Attachment #409718 -
Attachment is obsolete: true
Reporter | ||
Comment 23•15 years ago
|
||
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•15 years ago
|
Status: ASSIGNED → NEW
Keywords: helpwanted,
pp
Reporter | ||
Comment 24•15 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•15 years ago
|
||
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•14 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
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•