Don't hardcode target dpi of 160 (it should be 96 on desktop)

• Component: New Bugs
• Status: RESOLVED FIXED

[John Mellor]

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

[Kenneth Rohde Christiansen]

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.

[Konrad Piascik]

Created attachment 148817 [details]
Patch

[Build Bot]

Comment on attachment 148817 [details]
Patch

Attachment 148817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13027169

[Konrad Piascik]

Created attachment 148833 [details]
Patch

[Adam Barth]

Comment on attachment 148833 [details]
Patch

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.

[Konrad Piascik]

Created attachment 149003 [details]
Patch

[Adam Barth]

Comment on attachment 149003 [details]
Patch

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.

[Konrad Piascik]

(In reply to comment #7)
> (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?
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.

[Adam Barth]

> 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.

[Konrad Piascik]

(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.

[Adam Barth]

Comment on attachment 149003 [details]
Patch

Ok.

[John Mellor]

Comment on attachment 149003 [details]
Patch

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

[Konrad Piascik]

(In reply to comment #12)
> (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

Does this block the already reviewed change?  It seems more like a bug in the computeViewportAttributes method itself rather than its inputs.

[Adam Barth]

> 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.

[Adam Barth]

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.

[Konrad Piascik]

(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.

[Adam Barth]

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.

[Konrad Piascik]

(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

[Adam Barth]

We might want to do the LayoutTestController and LayoutTest changes in a separate pass.

[Konrad Piascik]

(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.

[Konrad Piascik]

Created attachment 149627 [details]
Patch

[Build Bot]

Comment on attachment 149627 [details]
Patch

Attachment 149627 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13088631

[Adam Barth]

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.

[Konrad Piascik]

(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?

[Adam Barth]

> 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.

[Rob Buis]

Comment on attachment 149627 [details]
Patch

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.

[Konrad Piascik]

Created attachment 149833 [details]
Patch

[Adam Barth]

Comment on attachment 149833 [details]
Patch

This looks great.  Thank you.

[Konrad Piascik]

(In reply to comment #28)
> (From update of attachment 149833 [details])
> This looks great.  Thank you.

I'll wait for EWS to run, then commit.  Thanks Adam.

[Konrad Piascik]

Created attachment 149849 [details]
Patch

[Konrad Piascik]

Sorry missed a refactor to GTK.  All should be good now.

[Adam Barth]

Comment on attachment 149849 [details]
Patch

Thanks

[Gyuyoung Kim]

Comment on attachment 149849 [details]
Patch

Attachment 149849 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13093960

[Konrad Piascik]

Created attachment 149957 [details]
Patch

[Konrad Piascik]

Comment on attachment 149957 [details]
Patch

Compile errors on blackberry

[Konrad Piascik]

Created attachment 149959 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149959 [details]
Patch

Clearing flags on attachment: 149959

Committed r121555: <http://trac.webkit.org/changeset/121555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

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

[Konrad Piascik]

(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....