613076 - GetDPI is wrong in the content process, breaks CSS media queries

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

nsWindow::GetDPI always returns the fallback value of 96 dpi when called from the content process on Android, because it can't access the Java bridge to get the correct value.  This breaks CSS "resolution" media queries and "mozmm" physical units in web content.

This might require a remoting solution like bug 600880.

Comment 1

[Alon Zakai (:azakai)]

List of DPIs: http://en.wikipedia.org/wiki/List_of_displays_by_pixel_density

Comment 2

[Alon Zakai (:azakai)]

Attachment: patch

So, there are 2 separate issues: Android cannot access the DPI in the child, and all platforms have puppet widgets in the child which don't call the proper GetDPI.

The patch implements cjones' suggestion from the IRC discussion: Remote the DPI on all platforms, and have the puppet widget code read the remoted value. So this fixes the problem for all platforms.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 493869 [details] [diff] [review]
patch

>diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp
> ContentChild::ContentChild()
> #ifdef ANDROID
>  : mScreenSize(0, 0)
>+ , mDPI(160)

Leave this uninitialized, or init to 0 or -1.  It must be set before
being used.  (I suspect I know where this magic value came from, based
on the rest of the patch ...)

IPC bits are OK.

>diff --git a/gfx/src/thebes/Makefile.in b/gfx/src/thebes/Makefile.in
>diff --git a/gfx/src/thebes/nsThebesDeviceContext.cpp b/gfx/src/thebes/nsThebesDeviceContext.cpp

This part of the patch is incorrect.  nsWindow::GetDPI() is the
widget's real DPI on screen, but ThebesDeviceContext is just a drawing
context and can have all kinds of weird DPIs set; note that
ThebesDeviceContext uses nsWindow to get a DPI if other sources of
information don't yield an override, and that that same logic runs in
the content process.  This patch would end up sending DPI changes for
temporary DeviceContexts created for printing, e.g.  Also,
ThebeDevices doesn't seem like it should should know about PContent.
Here you'd need to use the non-force-create version of
|ContentParent::GetSingleton()|, but this code will need to change
anyway.

We'll eventually need code to broadcast DPI changes, but that's not a
concern on mobile devices so I'm OK with doing that in a followup.

We should do what we discussed over IRC: when a new ContentParent is
created, find a canonical nsIWidget to ask GetDPI() and forward that
value along to the ContentChild.  I don't know how to do that; CC'ing
some folks who might.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Howdy --- the problem here is,

When a new ContentParent is created, how can one find a canonical nsIWidget to ask GetDPI()?  ContentParent creation happens around here http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#222 .  Is there some way to pull a top-level nsIWidget out of a hat from here?

Comment 5

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

There is usually a hidden window (see nsIAppShell::GetHiddenWindow), but maybe not in embedding.

But GetDPI is intended to be window-specific. It should return the DPI of the screen the window is on. Accordingly, there should be some association between a ContentParent and a real window.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> But GetDPI is intended to be window-specific. It should return the DPI of the
> screen the window is on. Accordingly, there should be some association between
> a ContentParent and a real window.

There may be one ContentParent for any number of tabs, so any number of windows, so theoretically any number of screens (but of course not on mobile platforms).

Alon, it looks like we'll need to request the DPI synchronously through PBrowser once and then cache the value.  There's code in PuppetWidget doing something like this for IME, for an example.  We can still leave notifications of updated DPI as a TODO.

Comment 7

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

(In reply to comment #6)
> (In reply to comment #5)
> > But GetDPI is intended to be window-specific. It should return the DPI of the
> > screen the window is on. Accordingly, there should be some association between
> > a ContentParent and a real window.
> 
> There may be one ContentParent for any number of tabs, so any number of
> windows, so theoretically any number of screens (but of course not on mobile
> platforms).

Then you need to route GetDPI to the window associated with the tab it was called for. Or store the DPI per-tab.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So in other words, go through TabChild/TabParent.  Luckily, PuppetWidget already has an mTabChild, right?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, { TabChild, TabParent }==PBrowser.  We already have PuppetWidget code to deal with PBrowser, for IME.

Comment 10

[Alon Zakai (:azakai)]

Attachment: patch v2

Rewrote to the approach described in the last few comments.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 494151 [details] [diff] [review]
patch v2

> bool
>+TabParent::RecvGetDPI(float* aValue)
>+{
>+  nsCOMPtr<nsIWidget> widget = GetWidget();
>+  NS_ABORT_IF_FALSE(widget, "Must have a widget to find the DPI!");

There might be some edge cases when we can't find a widget here, but I can't think of any off-hand and would like to know about them if they arise, so this looks fine.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: ignore this

Comment 13

[Alon Zakai (:azakai)]

Attachment: PATCH_FOR_CHECKIN

Comment 14

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/15cdaa1d538b

Comment 15

[Alon Zakai (:azakai)]

Seems we can crash with no widget to find the DPI,

http://crash-stats.mozilla.com/report/index/489ec3bb-09e2-4d7a-b950-639ff2101230