Closed Bug 613076 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget: Android, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: mbrubeck, Assigned: azakai)

References

(Blocks 1 open bug, )

Details

(Keywords: css3)

Attachments

(2 files, 2 obsolete files)

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.
tracking-fennec: --- → ?
Depends on: 600880
tracking-fennec: ? → 2.0b4+
Assignee: nobody → azakai
Attached patch patch (obsolete) — Splinter Review
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)
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.
Attached patch patch v2Splinter Review
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+
Attached patch ignore this (obsolete) — Splinter Review
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
Blocks: 616348
http://hg.mozilla.org/mozilla-central/rev/15cdaa1d538b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 616668
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.