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

RESOLVED FIXED

Status

()

Core
Widget: Android
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mbrubeck, Assigned: azakai)

Tracking

(Blocks: 1 bug, {css3})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b3+)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

7 years ago
tracking-fennec: --- → ?
(Reporter)

Updated

7 years ago
Depends on: 600880
tracking-fennec: ? → 2.0b4+
(Assignee)

Updated

7 years ago
Assignee: nobody → azakai
(Assignee)

Comment 1

7 years ago
List of DPIs: http://en.wikipedia.org/wiki/List_of_displays_by_pixel_density
(Assignee)

Comment 2

7 years ago
Created attachment 493869 [details] [diff] [review]
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.
Attachment #493869 - Flags: review?(jones.chris.g)
(Assignee)

Updated

7 years ago
OS: Android → All
Summary: GetDPI is wrong in the content process on Android, breaks CSS media queries → GetDPI is wrong in the content process, breaks CSS media queries
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.
Attachment #493869 - Flags: review?(jones.chris.g) → review-
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?
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.
(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.
(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.
So in other words, go through TabChild/TabParent.  Luckily, PuppetWidget already has an mTabChild, right?
Yes, { TabChild, TabParent }==PBrowser.  We already have PuppetWidget code to deal with PBrowser, for IME.
(Assignee)

Comment 10

7 years ago
Created attachment 494151 [details] [diff] [review]
patch v2

Rewrote to the approach described in the last few comments.
Attachment #493869 - Attachment is obsolete: true
Attachment #494151 - Flags: review?(jones.chris.g)
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.
Attachment #494151 - Flags: review?(jones.chris.g) → review+
Created attachment 494821 [details] [diff] [review]
ignore this
Assignee: azakai → blassey.bugs
Attachment #494151 - Attachment is obsolete: true
Attachment #494821 - Flags: review+
Attachment #494151 - Attachment is obsolete: false
Assignee: blassey.bugs → azakai
tracking-fennec: 2.0b4+ → ?
tracking-fennec: ? → 2.0b3+
Attachment #494821 - Attachment description: patch for checkin → ignore this
Attachment #494821 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Blocks: 616348
(Assignee)

Comment 13

7 years ago
Created attachment 494890 [details] [diff] [review]
PATCH_FOR_CHECKIN

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/15cdaa1d538b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 616668
(Assignee)

Comment 15

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

http://crash-stats.mozilla.com/report/index/489ec3bb-09e2-4d7a-b950-639ff2101230
You need to log in before you can comment on or make changes to this bug.