Bug 88114 - Don't hardcode target dpi of 160 (it should be 96 on desktop)
: Don't hardcode target dpi of 160 (it should be 96 on desktop)
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To:
:
:
: 87407 88047
: 89984
  Show dependency treegraph
 
Reported: 2012-06-01 11:35 PST by
Modified: 2012-06-29 07:58 PST (History)


Attachments
Patch (2.77 KB, patch)
2012-06-21 09:33 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.61 KB, patch)
2012-06-21 10:37 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.40 KB, patch)
2012-06-22 05:36 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (24.64 KB, patch)
2012-06-26 16:36 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.33 KB, patch)
2012-06-27 17:50 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2012-06-27 18:50 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2012-06-28 08:14 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff
Patch (27.53 KB, patch)
2012-06-28 08:54 PST, Konrad Piascik
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-06-01 11:35:57 PST
Since bug 87407 (and bug 88047) the deviceScaleFactor is computed as deviceDPI/160, where deviceDPI is supposedly the true dpi of the device.

This works on phones, but is incorrect on laptops/desktops. The definition of a CSS pixel (http://www.w3.org/TR/css3-values/#reference-pixel) requires a pixel density of 96dpi on devices used at arms length (the reason that 160 works well on phones is because they're held at about 60% of an arm length).

It's also common to discretize deviceScaleFactor to the nearest 0.25 or so.

So laptop/desktop ports that are considering turning on ENABLE(VIEWPORT) should consider factoring out 160 into a value passed by the embedding port (e.g. in ChromeClient.h) based on it's knowledge of the device's typical distance from the viewer.

Alternatively, instead of passing a value for the target dpi (e.g. 160), have the embedder provide the deviceScaleFactor directly (for example Android would probably provide DisplayMetrics.density[1], while iOS would just return 2 if retina else 1). This might be more flexible than having WebCore decide how to discretize fractional values.

ChromeOS folks, this might be of interest since afaik you plan to ship ENABLE(VIEWPORT).
Apple MacBook folks might also be interested, given rumors of high resolution displays which will require appropriate deviceScaleFactor.

[1]: http://developer.android.com/reference/android/util/DisplayMetrics.html#density
------- Comment #1 From 2012-06-02 05:41:41 PST -------
I'm fine with any of these changes, though I fear some ports might get this wrong if we don't get it documented well somewhere.
------- Comment #2 From 2012-06-21 09:33:19 PST -------
Created an attachment (id=148817) [details]
Patch
------- Comment #3 From 2012-06-21 10:00:55 PST -------
(From update of attachment 148817 [details])
Attachment 148817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13027169
------- Comment #4 From 2012-06-21 10:37:48 PST -------
Created an attachment (id=148833) [details]
Patch
------- Comment #5 From 2012-06-21 13:09:40 PST -------
(From update of attachment 148833 [details])
This looks like it's on the right track, but the patch doesn't do anything.  We should make this change when it actually does something.
------- Comment #6 From 2012-06-22 05:36:26 PST -------
Created an attachment (id=149003) [details]
Patch
------- Comment #7 From 2012-06-22 10:14:04 PST -------
(From update of attachment 149003 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=149003&action=review

This patch looks reasonable, but I think there are other places where we use the 160 constant in WebCore.  Have you grepped WebCore to make sure you've plumbed this value to all the right places?

> Source/WebKit/blackberry/Api/WebPage.cpp:3439
>      int deviceDPI = int(roundf((currentPPI.width() + currentPPI.height()) / 2));

It seems like another option is to get rid of the deviceDPI parameter to computeViewportAttributes entirely and just pass in the device pixel ratio directly.
------- Comment #8 From 2012-06-25 09:08:19 PST -------
(In reply to comment #7)
> (From update of attachment 149003 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149003&action=review
> 
> This patch looks reasonable, but I think there are other places where we use the 160 constant in WebCore.  Have you grepped WebCore to make sure you've plumbed this value to all the right places?
I did a grep for 160 and found a few places that it's being used.  I'm not sure what sort of solution we want to this.  Do we want a static const defined in ViewportArguments.h or...?
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:3439
> >      int deviceDPI = int(roundf((currentPPI.width() + currentPPI.height()) / 2));
> 
> It seems like another option is to get rid of the deviceDPI parameter to computeViewportAttributes entirely and just pass in the device pixel ratio directly.

Getting rid of deviceDPI would not be a good idea imho.  The calculation of device pixel ratio made by computeViewportAttributes will still need to be made by every embedder calling this method baded on their device's DPI.  Doing this would cause more problems that it would solve.
------- Comment #9 From 2012-06-25 09:51:28 PST -------
> Getting rid of deviceDPI would not be a good idea imho.

As far as I can tell, the only thing it's used for is divide by the targetDPI (and that's the only thing the targetDPI is used for).  Would it be easier for the caller to divide the two and then just pass in the device pixel ratio?

For context, if you look at the APIs on Mac OS X for getting this information, it returns the device pixel ratio, which we then multiply by 160 to get the (fake) deviceDPI.  At least on OS X, it would seem more direct to just deal with the device pixel ratio.

Anyway, I don't feel that strongly about it.
------- Comment #10 From 2012-06-25 10:21:49 PST -------
(In reply to comment #9)
> > Getting rid of deviceDPI would not be a good idea imho.
> 
> As far as I can tell, the only thing it's used for is divide by the targetDPI (and that's the only thing the targetDPI is used for).  Would it be easier for the caller to divide the two and then just pass in the device pixel ratio?
> 
> For context, if you look at the APIs on Mac OS X for getting this information, it returns the device pixel ratio, which we then multiply by 160 to get the (fake) deviceDPI.  At least on OS X, it would seem more direct to just deal with the device pixel ratio.
> 
> Anyway, I don't feel that strongly about it.

The division of the deviceDPI by the targetDPI will happen on most ports that don't use WebKit2.  And for now having the division happen in one place is better than in multiple places imho.
------- Comment #11 From 2012-06-25 10:24:56 PST -------
(From update of attachment 149003 [details])
Ok.
------- Comment #12 From 2012-06-25 10:37:37 PST -------
(From update of attachment 149003 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=149003&action=review

>>> Source/WebKit/blackberry/Api/WebPage.cpp:3439
>>>      int deviceDPI = int(roundf((currentPPI.width() + currentPPI.height()) / 2));
>> 
>> It seems like another option is to get rid of the deviceDPI parameter to computeViewportAttributes entirely and just pass in the device pixel ratio directly.
> 
> Getting rid of deviceDPI would not be a good idea imho.  The calculation of device pixel ratio made by computeViewportAttributes will still need to be made by every embedder calling this method baded on their device's DPI.  Doing this would cause more problems that it would solve.

In theory every embedder would perform such a calculation. But in practice iOS always chooses to use 1 or 2 for devicePixelRatio (depending on retina); Android ports will usually directly use the scaling factor provided by the OS[1]; and desktop ports will often hardcode it to 1 unless reliable dpi information is available (they currently just pass 160 * devicePixelRatio as the deviceDPI).

Essentially, most ports are in practice applying implementation-specific rounding to the device-width to snap it to "nice" values (e.g. 320, 360, 600, 1024), and as a result the devicePixelRatio gets snapped to a corresponding nice (though sometimes fractional) value (e.g. 1, 1.33333, 1.5, 2, 2.25). These values will approximately match what a true dpi-based calculation would have given, but not quite. Whether this snapping is actually useful is debatable, but it does give nice device-width values, and it helps to avoid rounding errors in a couple of cases.

Given this, I'm inclined to agree with abarth that computeViewportAttributes should directly accept a devicePixelRatio/deviceScaleFactor instead of deviceDPI and targetDPI, though we should document this very clearly to emphasize that the devicePixelRatio should be computed using a calculation approximately based on true dpi and average distance the screen is held from the user's eyes (which can be estimated from the physical device size, which can be calculated if you know the true dpi and native screen resolution).

[1]: http://developer.android.com/reference/android/util/DisplayMetrics.html#density
------- Comment #13 From 2012-06-25 10:47:58 PST -------
(In reply to comment #12)
> (From update of attachment 149003 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149003&action=review
> 
> >>> Source/WebKit/blackberry/Api/WebPage.cpp:3439
> >>>      int deviceDPI = int(roundf((currentPPI.width() + currentPPI.height()) / 2));
> >> 
> >> It seems like another option is to get rid of the deviceDPI parameter to computeViewportAttributes entirely and just pass in the device pixel ratio directly.
> > 
> > Getting rid of deviceDPI would not be a good idea imho.  The calculation of device pixel ratio made by computeViewportAttributes will still need to be made by every embedder calling this method baded on their device's DPI.  Doing this would cause more problems that it would solve.
> 
> In theory every embedder would perform such a calculation. But in practice iOS always chooses to use 1 or 2 for devicePixelRatio (depending on retina); Android ports will usually directly use the scaling factor provided by the OS[1]; and desktop ports will often hardcode it to 1 unless reliable dpi information is available (they currently just pass 160 * devicePixelRatio as the deviceDPI).
> 
> Essentially, most ports are in practice applying implementation-specific rounding to the device-width to snap it to "nice" values (e.g. 320, 360, 600, 1024), and as a result the devicePixelRatio gets snapped to a corresponding nice (though sometimes fractional) value (e.g. 1, 1.33333, 1.5, 2, 2.25). These values will approximately match what a true dpi-based calculation would have given, but not quite. Whether this snapping is actually useful is debatable, but it does give nice device-width values, and it helps to avoid rounding errors in a couple of cases.
> 
> Given this, I'm inclined to agree with abarth that computeViewportAttributes should directly accept a devicePixelRatio/deviceScaleFactor instead of deviceDPI and targetDPI, though we should document this very clearly to emphasize that the devicePixelRatio should be computed using a calculation approximately based on true dpi and average distance the screen is held from the user's eyes (which can be estimated from the physical device size, which can be calculated if you know the true dpi and native screen resolution).
> 
> [1]: http://developer.android.com/reference/android/util/DisplayMetrics.html#density

Does this block the already reviewed change?  It seems more like a bug in the computeViewportAttributes method itself rather than its inputs.
------- Comment #14 From 2012-06-25 10:51:19 PST -------
> Does this block the already reviewed change?  It seems more like a bug in the computeViewportAttributes method itself rather than its inputs.

It sounds like there's some more to discuss there.
------- Comment #15 From 2012-06-25 10:55:27 PST -------
I do think it would be better if WebCore always worked in terms of device pixel ratios rather than DPIs.  Some platforms work in terms of DPI, which is fine, but other platforms work in terms of pixel ratios.  It seems odd for those platforms to need to make up a fake DPI just to appease WebCore.
------- Comment #16 From 2012-06-25 11:05:32 PST -------
(In reply to comment #15)
> I do think it would be better if WebCore always worked in terms of device pixel ratios rather than DPIs.  Some platforms work in terms of DPI, which is fine, but other platforms work in terms of pixel ratios.  It seems odd for those platforms to need to make up a fake DPI just to appease WebCore.

I agree that it's easier for platforms that work in terms of a DPI to convert to a pixel ratio.  Do we want to overload the computeViewportAttributes method?  So that it can be called with either a pixelRatio or a DPI?  I could have the one that accepts the DPI call the pixel ratio version after performing the division calculation.  This would me a more minimalistic change since none of the DRT support classes wouldn't need to be re-written.
------- Comment #17 From 2012-06-25 11:17:45 PST -------
Rather than overload the function, it would be better (in my view) to remove the DPI parameter.  I'm not sure how big a change that would be, but it would give us a sense for how feasible it is to remove the notion of DPI from WebCore.  If that turns out to be infeasible, we might want to return to the approach in your current patch.
------- Comment #18 From 2012-06-25 11:49:07 PST -------
(In reply to comment #17)
> Rather than overload the function, it would be better (in my view) to remove the DPI parameter.  I'm not sure how big a change that would be, but it would give us a sense for how feasible it is to remove the notion of DPI from WebCore.  If that turns out to be infeasible, we might want to return to the approach in your current patch.

This is the list of files that will need to be modified if we replace the deviceDPI parameter by the devicePixelRatio instead.  This does not include all the LayoutTests that will need to be updated to pass in a devicePixelRatio instead of a DPI.

\Source\WebCore\dom\ViewportArguments.cpp
\Source\WebCore\dom\ViewportArguments.h
\Source\WebKit\blackberry\Api\WebPage.cpp
\Source\WebKit\blackberry\WebKitSupport\DumpRenderTreeSupport.cpp
\Source\WebKit\blackberry\WebKitSupport\DumpRenderTreeSupport.h
\Source\WebKit\chromium\public\WebView.h
\Source\WebKit\chromium\src\ChromeClientImpl.cpp
\Source\WebKit\efl\ewk\ewk_view.cpp
\Source\WebKit\efl\WebCoreSupport\DumpRenderTreeSupportEfl.cpp
\Source\WebKit\efl\WebCoreSupport\DumpRenderTreeSupportEfl.h
\Source\WebKit\gtk\WebCoreSupport\DumpRenderTreeSupportGtk.cpp
\Source\WebKit\gtk\WebCoreSupport\DumpRenderTreeSupportGtk.h
\Source\WebKit\gtk\webkit\webkitviewportattributes.cpp
\Source\WebKit\qt\Api\qwebpage.cpp
\Source\WebKit\qt\WebCoreSupport\DumpRenderTreeSupportQt.cpp
\Source\WebKit2\WebProcess\WebPage\WebPage.cpp
\Tools\DumpRenderTree\LayoutTestController.cpp
\Tools\DumpRenderTree\LayoutTestController.h
\Tools\DumpRenderTree\blackberry\LayoutTestControllerBlackBerry.cpp
\Tools\DumpRenderTree\efl\LayoutTestControllerEfl.cpp
\Tools\DumpRenderTree\gtk\LayoutTestControllerGtk.cpp
\Tools\DumpRenderTree\qt\LayoutTestControllerQt.cpp
\Tools\DumpRenderTree\qt\LayoutTestControllerQt.h
\Tools\DumpRenderTree\win\LayoutTestControllerWin.cpp
\Tools\DumpRenderTree\wx\LayoutTestControllerWx.cpp
\Tools\WebKitTestRunner\InjectedBundle\LayoutTestController.cpp
\Tools\WebKitTestRunner\InjectedBundle\LayoutTestController.h
\Tools\WebKitTestRunner\InjectedBundle\Bindings\LayoutTestController.idl
------- Comment #19 From 2012-06-25 12:55:58 PST -------
We might want to do the LayoutTestController and LayoutTest changes in a separate pass.
------- Comment #20 From 2012-06-26 09:04:33 PST -------
(In reply to comment #19)
> We might want to do the LayoutTestController and LayoutTest changes in a separate pass.

Half of the files mentioned in comment 18 still need to be touched even if it's just a change to the method signature of computeViewportAttributes.

I've filed https://bugs.webkit.org/show_bug.cgi?id=89984 to track the LayoutTestController and LayoutTest/fast/viewport changes.  I'll have a minimalist patch shortly.
------- Comment #21 From 2012-06-26 16:36:57 PST -------
Created an attachment (id=149627) [details]
Patch
------- Comment #22 From 2012-06-26 17:00:51 PST -------
(From update of attachment 149627 [details])
Attachment 149627 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13088631
------- Comment #23 From 2012-06-26 18:01:36 PST -------
This patch looks like a step in the right direction.  The changes to the callers are a bit shallow in the sense that you're just dividing by 160 instead of pushing back the DPI calculation, but I can understand why you don't want this patch to spider.

Maybe add FIXMEs to these locations?  Also, I wonder if we should still have a name for this constant (perhaps in ViewportArguments.h) so we can grep the codebase for it more easily.  Maybe we should name the constant something with deprecated in its name and put the FIXME comment next to its declaration to explain that we'd like to remove all uses of this constant in favor of just using the device scale factor everywhere.
------- Comment #24 From 2012-06-26 18:56:50 PST -------
(In reply to comment #23)
> This patch looks like a step in the right direction.  The changes to the callers are a bit shallow in the sense that you're just dividing by 160 instead of pushing back the DPI calculation, but I can understand why you don't want this patch to spider.
> 
> Maybe add FIXMEs to these locations?  Also, I wonder if we should still have a name for this constant (perhaps in ViewportArguments.h) so we can grep the codebase for it more easily.  Maybe we should name the constant something with deprecated in its name and put the FIXME comment next to its declaration to explain that we'd like to remove all uses of this constant in favor of just using the device scale factor everywhere.
I'll update the patch tomorrow with FIXMEs for each port and create a deprecated constant in ViewportArguments.h as well as the correct symbol in WebCore.exp.in

Should I open a bug for each call site or on bug to track all call sites?
------- Comment #25 From 2012-06-26 20:51:08 PST -------
> I'll update the patch tomorrow with FIXMEs for each port and create a deprecated constant in ViewportArguments.h as well as the correct symbol in WebCore.exp.in

It's probably fine to have one FIXME comment near the deprecated constant.  Folks can follow the trail from the use of the constant to its definition to the FIXME comment.

> Should I open a bug for each call site or on bug to track all call sites?

You can file a bug if you like, but it's not necessary as long as the issue is documented in the code.
------- Comment #26 From 2012-06-27 06:21:27 PST -------
(From update of attachment 149627 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=149627&action=review

> Source/WebKit/blackberry/Api/WebSettings.h:238
> +    double devicePixelRaio() const;

Typo in the method name.
------- Comment #27 From 2012-06-27 17:50:17 PST -------
Created an attachment (id=149833) [details]
Patch
------- Comment #28 From 2012-06-27 17:56:43 PST -------
(From update of attachment 149833 [details])
This looks great.  Thank you.
------- Comment #29 From 2012-06-27 17:58:30 PST -------
(In reply to comment #28)
> (From update of attachment 149833 [details] [details])
> This looks great.  Thank you.

I'll wait for EWS to run, then commit.  Thanks Adam.
------- Comment #30 From 2012-06-27 18:50:21 PST -------
Created an attachment (id=149849) [details]
Patch
------- Comment #31 From 2012-06-27 18:51:02 PST -------
Sorry missed a refactor to GTK.  All should be good now.
------- Comment #32 From 2012-06-27 19:13:08 PST -------
(From update of attachment 149849 [details])
Thanks
------- Comment #33 From 2012-06-27 19:54:14 PST -------
(From update of attachment 149849 [details])
Attachment 149849 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13093960
------- Comment #34 From 2012-06-28 08:14:47 PST -------
Created an attachment (id=149957) [details]
Patch
------- Comment #35 From 2012-06-28 08:49:06 PST -------
(From update of attachment 149957 [details])
Compile errors on blackberry
------- Comment #36 From 2012-06-28 08:54:38 PST -------
Created an attachment (id=149959) [details]
Patch
------- Comment #37 From 2012-06-29 05:52:15 PST -------
(From update of attachment 149959 [details])
Clearing flags on attachment: 149959

Committed r121555: <http://trac.webkit.org/changeset/121555>
------- Comment #38 From 2012-06-29 05:52:23 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #39 From 2012-06-29 07:46:48 PST -------
fast/viewport/viewport-91.html layout test fails after r121555 on Qt, EFL, GTK: 

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-91-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-91-actual.txt 
@@ -1,2 +1,2 @@
-viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
+viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
------- Comment #40 From 2012-06-29 07:58:53 PST -------
(In reply to comment #39)
> fast/viewport/viewport-91.html layout test fails after r121555 on Qt, EFL, GTK: 
> 
> --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-91-expected.txt 
> +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-91-actual.txt 
> @@ -1,2 +1,2 @@
> -viewport size 320x356 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000
> +viewport size 480x534 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000

I'm looking into this now....