If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

default DPI on windows mobile?

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dougt, Assigned: blassey)

Tracking

({fixed1.9.1})

unspecified
mozilla1.9.1b2
x86
Windows XP
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 342887 [details] [diff] [review]
patch v.1

in the nsThebesDeviceContext,  on windows, we call GetDeviceCaps to figure out the DPI for the display.  On the htc, this value is 360 (iirc, it was ~360).  This caused the browser window to only fill about 1/4 of the screen.  We hard coded this value to be 96 and that seems to have worked for the most part, however that caused the fennec "canvas" to not display:

http://dougt.files.wordpress.com/2008/10/200810101458.jpg?w=480&h=640

When we set this value to 48, we did get the correct result:

http://people.mozilla.org/~blassey/fennec_splash.PNG
Comment on attachment 342887 [details] [diff] [review]
patch v.1

drive by comment, setting it to 96 has the same effect.
Attachment #342887 - Flags: review?(pavlov)
(Reporter)

Comment 2

9 years ago
was your image at 96 or 48?
Why not just set layout.css.dpi instead of hardcoding it?
There's actually something horked here, unrelated to the dpi -- for some reason everything is 2x the size in fennec by default, even when the system DPI is 96.  Not sure why this doesn't show on maemo, but it shows up on the desktop at least... I think it's related to the default CSS hardcoding some (very large) font sizes.  Setting the dpi to 48 would cause gecko to scale everything down by 1/2 (since CSS pixels are 96dpi).  Not sure what's going on.
(In reply to comment #2)
> was your image at 96 or 48?

48, but I tried 96 as well and it looks the same.

(In reply to comment #3)
> Why not just set layout.css.dpi instead of hardcoding it?

if we just set that, web content looks right because we're masking mAppUnitsPerDevPixel not being set correctly, but canvas is broken.  This is (I think) because nsCanvasRenderingContext2D does a lot of calculations based on mAppUnitsPerDevPixel, which is not set correctly.

Comment 6

9 years ago
Comment on attachment 342887 [details] [diff] [review]
patch v.1

this patch is not what we want -- we need to understand why things are breaking.  360 should be fine here.
Attachment #342887 - Flags: review?(pavlov) → review-
the problem is GetDeviceCaps isn't returning 360, but garbage instead.  Checking MSDN, I found this:

> Note   This API is part of the complete Windows CE OS package as provided by
> Microsoft. The functionality of a particular platform is determined by the
> original equipment manufacturer (OEM) and some devices may not support this API.
Created attachment 343866 [details] [diff] [review]
converts css pixels to dev pixels

In XulWindow::SetSize, we're using CSS pixels to set the size of the native window which (I presume) expects device pixels.  This patch converts the css pixels to app units, and app units to device pixels.  Is there a way to do that in one step?
Assignee: doug.turner → blassey
Attachment #342887 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #343866 - Flags: review?(pavlov)
Attachment #343866 - Attachment is patch: true
Attachment #343866 - Attachment mime type: application/octet-stream → text/plain

Updated

9 years ago
Attachment #343866 - Flags: review?(pavlov) → review?(roc)
(Reporter)

Comment 9

9 years ago
Comment on attachment 343866 [details] [diff] [review]
converts css pixels to dev pixels

roc is travelling, maybe bz can look at this for your blassey.
Attachment #343866 - Flags: review?(roc) → review?(bzbarsky)
(In reply to comment #7)
> the problem is GetDeviceCaps isn't returning 360, but garbage instead. 
> Checking MSDN, I found this:
> 
> > Note   This API is part of the complete Windows CE OS package as provided by
> > Microsoft. The functionality of a particular platform is determined by the
> > original equipment manufacturer (OEM) and some devices may not support this API.

We should be able to hardcode it to some appropriate value then (360 etc), maybe a set of different values depending on what we know about the device that it's being used on..
(In reply to comment #10)
> (In reply to comment #7)
> > the problem is GetDeviceCaps isn't returning 360, but garbage instead. 
> > Checking MSDN, I found this:
> > 
> > > Note   This API is part of the complete Windows CE OS package as provided by
> > > Microsoft. The functionality of a particular platform is determined by the
> > > original equipment manufacturer (OEM) and some devices may not support this API.
> 
> We should be able to hardcode it to some appropriate value then (360 etc),
> maybe a set of different values depending on what we know about the device that
> it's being used on..

turns out GetDeviceCaps is returning a sensible value of 192.  The debugger says OSVal and dpi are 0, but printf'ing them shows them to be 192.
The coordinate conversions look ok, but I'm not sure we want to depend on having a prescontext here.  Seems safer to just create our own device context for mWindow and then do the conversions using that.
That is, basically reimplement nsPresContext::AppUnitsToDevPixels using our own device context.
Created attachment 344431 [details] [diff] [review]
removes need for nsPresContext
Attachment #343866 - Attachment is obsolete: true
Attachment #344431 - Flags: review?(bzbarsky)
Attachment #343866 - Flags: review?(bzbarsky)
Comment on attachment 344431 [details] [diff] [review]
removes need for nsPresContext

Excellent.  Good catch on just getting the device context off the widget.
Attachment #344431 - Flags: superreview+
Attachment #344431 - Flags: review?(bzbarsky)
Attachment #344431 - Flags: review+
(Reporter)

Updated

9 years ago
Flags: blocking1.9.1?
Comment on attachment 344431 [details] [diff] [review]
removes need for nsPresContext

Risk: Seems minimal.  Scariest operation is getting appUnitsPerDevPixel from the device context, which should always exist.


Reward: Correctly handles window sizes on high dpi screens.
Attachment #344431 - Flags: approval1.9.1b2?
Comment on attachment 344431 [details] [diff] [review]
removes need for nsPresContext

a1.9.1b2=beltzner
Attachment #344431 - Flags: approval1.9.1b2? → approval1.9.1b2+
Pushed: http://hg.mozilla.org/mozilla-central/rev/0486e61caa41
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2

Updated

9 years ago
Depends on: 469933
I think this fix caused a regression; windows such as the Add-ons manager or the Prefs dialog are twice as large as they should be when layout.css.dpi is set. The initial size specified when the window is opened is scaled as expected, but then the next time the window is opened, the size read from the localstore is also scaled. Since the size in the localstore is already the result of scaling the initial size, this results in windows that are much larger than they ought to be.
sounds like we need to scale to app units before storing the value.  Can you file a bug on that?
Flags: blocking1.9.1? → blocking1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0486e61caa41
Keywords: fixed1.9.1

Comment 22

9 years ago
(In reply to comment #20)
> sounds like we need to scale to app units before storing the value.

Please consider also Bug 469933.
Did a bug ever get filed on comment 19?
(In reply to comment #23)
> Did a bug ever get filed on comment 19?

yep: bug 471729
Depends on: 471729
You need to log in before you can comment on or make changes to this bug.