[chromium] Remove WebScreenInfo.{horizontal,vertical}DPI
Created attachment 173326 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 173326 [details] Patch Attachment 173326 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14771765
Comment on attachment 173326 [details] Patch Attachment 173326 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/14777615
Comment on attachment 173326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173326&action=review Nice cleanup, looks good. Slightly unsure what the status of Win 8 port is w.r.t. deviceScaleFactor. > Source/WebKit/chromium/src/win/WebScreenInfoFactory.cpp:-68 > - results.horizontalDPI = GetDeviceCaps(hdc, LOGPIXELSX); I notice that this code doesn't set deviceScaleFactor. Surely some Windows 8 devices have non-zero deviceScaleFactor? Ditto for X11 below. Alternatively if you're deliberately hardcoding the default value of 1, a comment to clarify this might be good.
(In reply to comment #5) > Nice cleanup, looks good. Slightly unsure what the status of Win 8 port is w.r.t. deviceScaleFactor. Your guess is as good as mine. Right now I think neither X11 nor Windows enables the viewport tag, but obviously that might change going forward. > > > Source/WebKit/chromium/src/win/WebScreenInfoFactory.cpp:-68 > > - results.horizontalDPI = GetDeviceCaps(hdc, LOGPIXELSX); > > I notice that this code doesn't set deviceScaleFactor. Surely some Windows 8 devices have non-zero deviceScaleFactor? Ditto for X11 below. Alternatively if you're deliberately hardcoding the default value of 1, a comment to clarify this might be good. That's right, both Windows and X11 default to a device scale factor of 1 and I didn't want to change that. I have no idea how Windows 8 fits in here. I'll add a comment to both files.
Created attachment 174289 [details] Patch Added FIXME for win/x11.
Comment on attachment 174289 [details] Patch Attachment 174289 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14943115
Created attachment 175483 [details] Patch Rebased because a new test using horizontalDPI (WebFrameTest.ScaleFactorShouldNotOscillate) got added. Adam, are you still happy with this?
Comment on attachment 175483 [details] Patch Attachment 175483 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14960062 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Created attachment 175641 [details] Patch Retry EWS.
Comment on attachment 175641 [details] Patch Clearing flags on attachment: 175641 Committed r135580: <http://trac.webkit.org/changeset/135580>
All reviewed patches have been landed. Closing bug.
The change broke compilation on Chromium Win: http://build.webkit.org/builders/Chromium%20Win%20Release/builds/51008/steps/compile-webkit/logs/stdio I landed a quick fix http://trac.webkit.org/changeset/135593, please check if we can do a better job there.
Sorry about the breakage and thank you for the quick fix. Your change looks fine by me.