Closed
Bug 613076
Opened 14 years ago
Closed 14 years ago
GetDPI is wrong in the content process, breaks CSS media queries
Categories
(Core Graveyard :: Widget: Android, defect)
Core Graveyard
Widget: Android
Tracking
(fennec2.0b3+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b3+ | --- |
People
(Reporter: mbrubeck, Assigned: azakai)
References
()
Details
(Keywords: css3)
Attachments
(2 files, 2 obsolete files)
6.40 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
3.68 KB,
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
tracking-fennec: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0b4+
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → azakai
Assignee | ||
Comment 1•14 years ago
|
||
List of DPIs: http://en.wikipedia.org/wiki/List_of_displays_by_pixel_density
Assignee | ||
Comment 2•14 years ago
|
||
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•14 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.
Comment 8•14 years ago
|
||
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•14 years ago
|
||
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+
Comment 12•14 years ago
|
||
Assignee: azakai → blassey.bugs
Attachment #494151 -
Attachment is obsolete: true
Attachment #494821 -
Flags: review+
Updated•14 years ago
|
Attachment #494151 -
Attachment is obsolete: false
Updated•14 years ago
|
Assignee: blassey.bugs → azakai
tracking-fennec: 2.0b4+ → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0b3+
Updated•14 years ago
|
Attachment #494821 -
Attachment description: patch for checkin → ignore this
Attachment #494821 -
Attachment is obsolete: true
Assignee | ||
Comment 13•14 years ago
|
||
Comment 14•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/15cdaa1d538b
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•14 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
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•