Bug 70556

Created attachment 112880 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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

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

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?

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

Created attachment 117004 [details] Patch

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

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

Created attachment 117010 [details] Patch

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

Created attachment 117057 [details] Patch

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

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

Created attachment 118106 [details] Patch

Created attachment 118107 [details] Patch

Comment on attachment 118107 [details] Patch This is required before enabling the viewport meta tag.

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

Created attachment 118148 [details] Patch

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.

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?

Created attachment 118240 [details] Patch for landing

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.

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

Comment on attachment 118240 [details] Patch for landing Clearing flags on attachment: 118240 Committed r102301: <http://trac.webkit.org/changeset/102301>

All reviewed patches have been landed. Closing bug.

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.

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

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.

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

> 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

> 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();