Jesse Ruderman Reporter
	Description • 12 years ago

1. Load the testcase.
2. Click button #1: "Open an empty window"
3. Click button #5: "w.sizeToContent();"

Result: window is extremely tiny. (Possible security implications: full-screen button covers the close button on Mac; window could "hide"; maybe spoofing or clickjacking.)

Expected: window becomes 100x100 or so, like button #3 does.

Strangely, it is clamped correctly at the upper end (you can see this with buttons #2, #5, and #4.)

	Jesse Ruderman Reporter
	Comment 1 • 12 years ago

This is moot if you haven't checked "Allow scripts to: Move or resize popup windows".

	Mounir Lamouri (:mounir) Assignee
	Comment 2 • 12 years ago

Is that MocOS only?

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

Comment on attachment 636241 [details] [diff] [review]
Patch v1

r=me if you fix the comment in the IDL to reflect reality.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 12 years ago

Mounir, anything preventing landing the patch (with the comment fixed)?

	Mounir Lamouri (:mounir) Assignee
	Comment 6 • 12 years ago

IIRC, I think the patch was failing some tests. I will send it again to try and see.

	Mounir Lamouri (:mounir) Assignee
	Comment 7 • 12 years ago

This is breaking layout/base/tests/test_bug458898.html

Locally, if I open a page with a 100/200 div, sizeToContent() produce a size that has scrollbars while, before, it wouldn't.

See: https://tbpl.mozilla.org/?tree=Try&rev=d878ab5527ea

I will try to have a quick look at it later but I unfortunately doesn't have much cycles to dedicate :(

	Mounir Lamouri (:mounir) Assignee
	Comment 8 • 12 years ago

Olli, feel free to take this review from Boris ;)

Locally, it is fixing the test breakage.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 12 years ago

Hmm.  So why is the new code better than the old?

	Mounir Lamouri (:mounir) Assignee
	Comment 10 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #9)
> Hmm.  So why is the new code better than the old?

My original patch was changing the behaviour of the method.
nsIMarkupDocumentViewer.sizeToContent() was using nsIDocShellTreeOwner.sizeShellTo() to set the size and my patch was using nsIBaseWindow.setSize() because it was used in most sizing methods in nsGlobalWindow. I was assuming those two methods would likely do the same thing but obviously they are not. Actually, reading more the code, I realise SetInner{Width,Height} are using nsGlobalWindow::SetDocShellWidthAndHeight(), which I could use. This is actually the behiaiour sizeToContent() should have: the inner{Width,Height} should be changed, not the outer. I would assume nsIBaseWindow.setSize() does change the outher size.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 12 years ago

Comment on attachment 682458 [details] [diff] [review]
Fix test breakage

Ah, yes.  The basewindow thing off the treeowner would set the outer size, indeed.

r=me if you use SetDocShellWidthAndHeight.

	Mounir Lamouri (:mounir) Assignee
	Comment 12 • 12 years ago

(this is a test + a reminder to fix the Windows errors)

	Mounir Lamouri (:mounir) Assignee
	Comment 13 • 12 years ago

(this is a test + a reminder to fix the Windows errors)

	Mounir Lamouri (:mounir) Assignee
	Comment 14 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/204665ad4a24

(and now, my bugzilla dashboard can show me the pending "needinfo" ;))

	Phil Ringnalda (:philor)
	Comment 15 • 12 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7456dc02bd because the test timed out on Android.

	Mounir Lamouri (:mounir) Assignee
	Comment 16 • 12 years ago

Relanded with the test disabled on Android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caab3921e4c2

there is no reason to have the test enabled on Android given that Firefox Android opens a tab everytime a popup is opened.

	Phil Ringnalda (:philor)
	Comment 17 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/204665ad4a24

	Ed Morley [:emorley]
	Comment 18 • 12 years ago

(In reply to Phil Ringnalda (:philor) from comment #15)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7456dc02bd because
> the test timed out on Android.

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/ce7456dc02bd

	Ed Morley [:emorley]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/caab3921e4c2

	Jean-Yves Perrier [:teoli]
	Comment 20 • 10 years ago

Documentation updated:
https://developer.mozilla.org/en-US/docs/Web/API/Window.sizeToContent
https://developer.mozilla.org/en-US/Firefox/Releases/20