Bug 70556 – [Chromium] Plumb DPI info into PlatformScreen

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Fady Samuel]

In order to properly implement the Viewport Meta tag, we need to make a full size web page's CSS pixel be equal to a reference pixel in size. To do the proper scaling calculations, we need access to the screen dpi in WebKit.

So plumb the DPI info through the following:

PlatformScreen.h
PlatformScreenChromium.cpp
PlatformSupport
WebWidgetClient
WebScreenInfo
WebScreenInfoFactory (x11) where we have a X Display to inquire things from.

[Fady Samuel]

Created attachment 112880 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Early Warning System Bot]

Comment on attachment 112880 [details]
Patch

Attachment 112880 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10240100

[Daniel Bates]

Comment on attachment 112880 [details]
Patch

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 112880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112880&action=review

What about the other factories?  Mac and Windows?

> Source/WebCore/platform/PlatformScreen.h:50
> +    int screenDpi(Widget*);

I think you should write "DPI" (see webkit style guide).

> Source/WebKit/chromium/public/WebScreenInfo.h:39
> +    // The screen (width) dpi.

when you say "width" here, do you mean horizontal?

[Fady Samuel]

> 
> I think you should write "DPI" (see webkit style guide).

Use CamelCase. Capitalize the first letter, including all letters in an acronym, in a class, struct, protocol, or namespace name. Lower-case the first letter, including all letters in an acronym, in a variable or function name.

It seems like the style I used is correct?

> 
> > Source/WebKit/chromium/public/WebScreenInfo.h:39
> > +    // The screen (width) dpi.
> 
> when you say "width" here, do you mean horizontal?

Yes, I will include both horizontal and vertical DPI in a subsequent patch, and add Windows/Mac support

[Fady Samuel]

Created attachment 117004 [details]
Patch

[Fady Samuel]

Plumbing both horizontal and vertical dpi now. Adding support for windows/mac shortly.

[Early Warning System Bot]

Comment on attachment 117004 [details]
Patch

Attachment 117004 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10682469

[Fady Samuel]

Created attachment 117010 [details]
Patch

[Fady Samuel]

Made it compile on qt (just a stub, no implementation).

[Fady Samuel]

Created attachment 117057 [details]
Patch

[Fady Samuel]

Made it compile on WebKit Mac (I think), and works on Chromium Mac (with a caveat).

[Fady Samuel]

(In reply to comment #5)
> (From update of attachment 112880 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112880&action=review
> 
> What about the other factories?  Mac and Windows?
> 
> > Source/WebCore/platform/PlatformScreen.h:50
> > +    int screenDpi(Widget*);
> 
> I think you should write "DPI" (see webkit style guide).
> 
> > Source/WebKit/chromium/public/WebScreenInfo.h:39
> > +    // The screen (width) dpi.
> 
> when you say "width" here, do you mean horizontal?

Darin, I have untested Windows code for DPI plumbing but this works for Mac and Linux now. Is it possible to move towards landing this patch as it is (maybe after updating the changelog to say something useful), and then enabling DPI plumbing on Windows in a separate patch? I don't have unrestricted access to a Windows box at the moment (I share it with others, and occasionally use it when no one else is using it so getting the Windows changes done will take me a little longer). Mind you, there's only about 4 lines necessary to enable this on Windows.

[Fady Samuel]

Created attachment 118106 [details]
Patch

[Fady Samuel]

Created attachment 118107 [details]
Patch

[Fady Samuel]

Comment on attachment 118107 [details]
Patch

This is required before enabling the viewport meta tag.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118107&action=review

> Source/WebCore/page/Screen.h:47
> +        unsigned horizontalDpi() const;

webkit style would be horizontalDPI and verticalDPI

> Source/WebKit/chromium/src/x11/WebScreenInfoFactory.cpp:47
> +    const float InchesPerMillimeter = 25.4;

nit: inchesPerMillimeter

[Fady Samuel]

Created attachment 118148 [details]
Patch

[Fady Samuel]

Comment on attachment 118107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118107&action=review

>> Source/WebCore/page/Screen.h:47
>> +        unsigned horizontalDpi() const;
> 
> webkit style would be horizontalDPI and verticalDPI

Done.

>> Source/WebKit/chromium/src/x11/WebScreenInfoFactory.cpp:47
>> +    const float InchesPerMillimeter = 25.4;
> 
> nit: inchesPerMillimeter

Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118148&action=review

> Source/WebCore/platform/PlatformScreen.h:50
> +    int screenHorizontalDPI(Widget*);

maybe PlatformScreen functions should also return 'unsigned'?  why be different?  i'm guessing you were going for consistency.  hmm...

> Source/WebKit/chromium/src/mac/WebScreenInfoFactory.mm:85
> +    results.horizontalDPI = (int)deviceDPI.width;

shouldn't this use C++ style casts?

[Fady Samuel]

Created attachment 118240 [details]
Patch for landing

[Fady Samuel]

Comment on attachment 118148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118148&action=review

>> Source/WebCore/platform/PlatformScreen.h:50
>> +    int screenHorizontalDPI(Widget*);
> 
> maybe PlatformScreen functions should also return 'unsigned'?  why be different?  i'm guessing you were going for consistency.  hmm...

Yea, I'll leave it as is for consistency for now. There's another bug for cleanup of all this stuff (plus some refactoring out of WebKit for an implementation of WebScreenInfoFactory)

>> Source/WebKit/chromium/src/mac/WebScreenInfoFactory.mm:85
>> +    results.horizontalDPI = (int)deviceDPI.width;
> 
> shouldn't this use C++ style casts?

Done.

[WebKit Review Bot]

Comment on attachment 118240 [details]
Patch for landing

Rejecting attachment 118240 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
112974 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium
Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
No such file or directory at /mnt/git/webkit-commit-queue/Tools/Scripts/webkitdirs.pm line 2078.

Full output: http://queues.webkit.org/results/10790007

[WebKit Review Bot]

Comment on attachment 118240 [details]
Patch for landing

Clearing flags on attachment: 118240

Committed r102301: <http://trac.webkit.org/changeset/102301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Sorry to revert on such short notice, but this has too big of a negative impact to risk leaving in past the next milestone cut.

See http://code.google.com/p/chromium/issues/detail?id=111404 for description of the issues with this mechanism.

[James Robinson]

(In reply to comment #27)
> Sorry to revert on such short notice, but this has too big of a negative impact to risk leaving in past the next milestone cut.
> 
> See http://code.google.com/p/chromium/issues/detail?id=111404 for description of the issues with this mechanism.

Gah I'm a dumbass, ignore me please! This comment and the rollout have nothing to do with this patch, which I haven't touched. Was meant for another bug.

[Kenneth Rohde Christiansen]

Comment on attachment 118240 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=118240&action=review

> Source/WebKit/chromium/src/PlatformSupport.cpp:1061
> +int PlatformSupport::screenHorizontalDPI(Widget* widget)
> +{
> +    WebWidgetClient* client = toWebWidgetClient(widget);
> +    if (!client)
> +        return 0;
> +    return client->screenInfo().horizontalDPI;
> +}

DPI in Web is defines as density per CSS inch and not device inch. This is what is used for the 'resolution' media query.

I wonder how you are actually using this. How are you calculating the device-pixel-ratio from this? as that depends on the kind of device. A device at arms length would have it calculated at 96 device DPI where as a mobile device usually uses 160 (which corresponds to 96 at around 60% of arms length)

> Source/WebKit/chromium/src/x11/WebScreenInfoFactory.cpp:47
> +    const float inchesPerMillimeter = 25.4;

the style is to not use const for local variables.

[Kenneth Rohde Christiansen]

Comment on attachment 118240 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=118240&action=review

> Source/WebCore/ChangeLog:13
> +        Make DPI information accessible from WebKit through
> +        PlatformScreen. This is useful when making scaling 
> +        computations on various devices (e.g. Viewport meta tag).
> +
> +        This patch adds DPI plumbing on Chromium Win/Mac/Linux
> +        platforms.

I don't get this patch :-)

Well, exposing the DPI through PlatformScreen via the WebScreenInfo makes a lot of sense for what you describe, but I don't see why you need to expose this to window.screen (even though you do not declare this in the IDL).

So why am I complaining here? :) Well because I would like to use screen.horizontalDPI etc for the resolution media query, instead of introducing something similar, but that is specced to not use device DPI but dots per CSS inch (basically devicePixelRatio * 96).

[Kenneth Rohde Christiansen]

> Well, exposing the DPI through PlatformScreen via the WebScreenInfo makes a lot of sense for what you describe, but I don't see why you need to expose this to window.screen (even though you do not declare this in the IDL).
> 
> So why am I complaining here? :) Well because I would like to use screen.horizontalDPI etc for the resolution media query, instead of introducing something similar, but that is specced to not use device DPI but dots per CSS inch (basically devicePixelRatio * 96).

I had a quick look at the chromium source and you don't seem to be using anything else than the WebScreenInfo

content/renderer/render_widget.cc:

     const WebKit::WebScreenInfo& screen_info

 ...

 120 #if defined(OS_CHROMEOS) || defined(OS_MACOSX)
 121   device_scale_factor_ = screen_info.verticalDPI / kStandardDPI;
 122   // Unless an explicit scale factor was provided for testing, ensure the scale
 123   // is integral.
 124   if (!CommandLine::ForCurrentProcess()->HasSwitch(
 125       switches::kForceDeviceScaleFactor))
 126     device_scale_factor_ = static_cast<int>(device_scale_factor_);
 127   device_scale_factor_ = std::max(1.0f, device_scale_factor_);
 128 #endif

[Kenneth Rohde Christiansen]

>  120 #if defined(OS_CHROMEOS) || defined(OS_MACOSX)
>  121   device_scale_factor_ = screen_info.verticalDPI / kStandardDPI;
>  122   // Unless an explicit scale factor was provided for testing, ensure the scale
>  123   // is integral.
>  124   if (!CommandLine::ForCurrentProcess()->HasSwitch(
>  125       switches::kForceDeviceScaleFactor))
>  126     device_scale_factor_ = static_cast<int>(device_scale_factor_);
>  127   device_scale_factor_ = std::max(1.0f, device_scale_factor_);
>  128 #endif

And for Android it is not used:

content/browser/renderer_host/render_widget_host_view_android.cc:

493 // static
494 void RenderWidgetHostViewPort::GetDefaultScreenInfo(
495     WebKit::WebScreenInfo* results) {
496   DeviceInfo info;
497   const int width = info.GetWidth();
498   const int height = info.GetHeight();
499   results->horizontalDPI = 160 * info.GetDPIScale();
500   results->verticalDPI = 160 * info.GetDPIScale();