Bug 101772 – [chromium] Remove WebScreenInfo.{horizontal,vertical}DPI

• Component: New Bugs
• Status: RESOLVED FIXED

[Sami Kyöstilä]

[chromium] Remove WebScreenInfo.{horizontal,vertical}DPI

[Sami Kyöstilä]

Created attachment 173326 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 173326 [details]
Patch

Attachment 173326 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14771765

[Peter Beverloo (cr-android ews)]

Comment on attachment 173326 [details]
Patch

Attachment 173326 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14777615

[John Mellor]

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.

[Sami Kyöstilä]

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

[Sami Kyöstilä]

Created attachment 174289 [details]
Patch

Added FIXME for win/x11.

[WebKit Review Bot]

Comment on attachment 174289 [details]
Patch

Attachment 174289 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14943115

[Sami Kyöstilä]

Created attachment 175483 [details]
Patch

Rebased because a new test using horizontalDPI (WebFrameTest.ScaleFactorShouldNotOscillate) got added. Adam, are you still happy with this?

[WebKit Review Bot]

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

[Sami Kyöstilä]

Created attachment 175641 [details]
Patch

Retry EWS.

[WebKit Review Bot]

Comment on attachment 175641 [details]
Patch

Clearing flags on attachment: 175641

Committed r135580: <http://trac.webkit.org/changeset/135580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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.

[Sami Kyöstilä]

Sorry about the breakage and thank you for the quick fix. Your change looks fine by me.