404290 - the size of popup windows needs to be specified in css pixels, not device pixels

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P4)

Description

[Daniel Brooks [:db48x]]

Currently when the dpi of the user's monitor is 144 or greater, all of the chrome and content visible in a window has it's size scaled up. However, the dimensions of popup windows or dialog windows is still assumed to be given in device pixels. This results in problems because the window will often be one quarter of the size necessary to display it's contents.

To be precise, all of the window features that you can specify when calling open or openDialog that are lengths should be in css pixels. This includes top, left, width, height, screenX/Y, outerWidth/Height and innerWidth/Height.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: 404290-1.diff

Comment 2

[Eli Friedman]

Comment on attachment 289267 [details] [diff] [review]
404290-1.diff

Looks correct to me; make sure to get an embedding peer for sr, though, because I have no authority to review patches in the embedding module.

Comment 3

[Benjamin Smedberg]

Comment on attachment 289267 [details] [diff] [review]
404290-1.diff

>Index: embedding/components/windowwatcher/src/Makefile.in

>-REQUIRES	= xpcom \
>-		  string \

>+REQUIRES	= xpcom								\
>+		  string								\

This seems like a gratuitous change to me, please don't.

>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp

>+  nsCOMPtr<nsIWidget> mainWidget;
>+  treeOwnerAsWin->GetMainWidget(getter_AddRefs(mainWidget));
>+  nsCOMPtr<nsIDeviceContext> ctx = mainWidget->GetDeviceContext();
>+
>+  float DevPixelsPerCSSPixel = float(ctx->AppUnitsPerCSSPixel()) / ctx->AppUnitsPerDevPixel();

Are all of these methods guaranteed to succeed? This needs review from roc or a widget owner, I'd think.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 289267 [details] [diff] [review]
404290-1.diff

Those can't really fail

Comment 5

[Daniel Brooks [:db48x]]

thanks, checked in.

Comment 6

[Stuart Morgan]

(In reply to comment #4)
> Those can't really fail

embedding/browser/webBrowser/nsDocShellTreeOwner.cpp doesn't seem to agree:

702 NS_IMETHODIMP
703 nsDocShellTreeOwner::GetMainWidget(nsIWidget** aMainWidget)
704 {
705     return NS_ERROR_NULL_POINTER;
706 }

This patch causes Camino to crash on every popup (see bug 406108)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you implement that method? We need to get a Gecko widget from somewhere so we can convert from CSS pixels to device pixels correctly.

Comment 8

[Stuart Morgan]

(In reply to comment #7)
> Can you implement that method?

If that refers to me, no; I have no familiarity with most of /embedding (nor is it at all clear to me from the idl what GetMainWidget is supposed to do, exactly). Someone who knows embedding will need to look at it--and I'd like to request that unless that's going to happen very soon the patch here be backed out in the meantime, as it cripples embeddors.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix regression

No need to back out. This patch should fix it; please try it.

Comment 10

[Stuart Morgan]

Sure, that fixes the crash, but it means that this bug isn't actually fixed for Camino (or presumably other embedders, since the code in question isn't Camino-specific)...

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, but that's better than leaving it unfixed for everyone.

Comment 12

[Stuart Morgan]

As long as this (or a follow-up bug blocking 348059) is left open for actually fixing this bug for embedders, that makes sense.

Comment 13

[Stuart Morgan]

Is there anything else you need from me before getting this reviewed and landed?

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Specifically, it would be...unpleasant...for us to have to live with this common crasher until whenever the tree opens again after fx3b2 (which date seems as-of-yet-undetermined, as far as I can tell).

Comment 15

[Stuart Morgan]

Comment on attachment 290899 [details] [diff] [review]
fix regression

r=me (josh suggested I get the ball rolling so this can land before freeze; I hope I'm not stepping on any toes)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 290899 [details] [diff] [review]
fix regression

So why not just get the widget from the docshell instead of getting it from the treeowner?  Is the problem that the docshell's widget is null at this point?

Comment 17

[Stuart Morgan]

Attachment: fix regression, v2

Fix v2, per discussion with bzbarsky. This gives Camino-like embedding clients a valid widget. (It lies about the DPI, so the behavior here still isn't right on the Mac, but at least it pushes the issue to the right place in the code).

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 291323 [details] [diff] [review]
fix regression, v2

sr=bzbarsky.  Please make sure to file a bug on the Thebes device context dpi thing if there isn't one already.  cc me and roc, as well as stuart and vlad on that bug?

Comment 19

[Stuart Morgan]

Comment on attachment 291323 [details] [diff] [review]
fix regression, v2

Landed on trunk.

Comment 20

[Stuart Morgan]

Filed bug 406668 for the DPI issue, since I didn't see an existing bug.

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Are there tests that would catch some of these bugs (e.g. the crasher bug 406108 that this bug exposed, bug 401328 [which I realize is a very different bug]) if the tests were being run by apps exercising embedding code (e.g., Camino, Epiphany, etc.)?  Just wondering aloud....

Comment 22

[Boris Zbarsky [:bzbarsky]]

Running mochitest in an embedding environment should catch window.open() bugs, I would think.