Bug 88375 – Settings::defaultDeviceScaleFactor is redundant with Page::deviceScaleFactor

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Settings::defaultDeviceScaleFactor is redundant with Page::deviceScaleFactor

[Adam Barth]

Created attachment 145894 [details]
Work in progress

[Adam Barth]

I'd like to offer this patch for discussion.  The part that I'm least sure about is the interaction with applyDefaultDeviceScaleFactorInCompositor.  There also appears to be one call site on the Chromium side for WebSettings::setDefaultDeviceScaleFactor, so I need to dig into that as well.

[James Robinson]

Comment on attachment 145894 [details]
Work in progress

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:3500
> +        if (m_webSettings->applyDefaultDeviceScaleFactorInCompositor()) {
> +            m_deviceScaleInCompositor = page()->deviceScaleFactor();

This looks correct to me.

[Dana Jansens]

Comment on attachment 145894 [details]
Work in progress

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

>> Source/WebKit/chromium/src/WebViewImpl.cpp:3500
>> +            m_deviceScaleInCompositor = page()->deviceScaleFactor();
> 
> This looks correct to me.

To me also.

Strictly speaking, I think the (... && page()->deviceScaleFactor() == 1) check should be done still to guard the setPageScaleFactorLimits. But we are (re)moving that call in a separate CL anyhow.

Should this patch rename WebSettings::applyDefaultDeviceScaleFactorInCompositor to applyDeviceScaleFactorInCompositor then?

[Adam Barth]

> Should this patch rename WebSettings::applyDefaultDeviceScaleFactorInCompositor to applyDeviceScaleFactorInCompositor then?

That sounds like a good idea, but I'd be inclined to do that in a separate patch.

[Adam Barth]

Comment on attachment 145894 [details]
Work in progress

Ok.  It sounds like this patch isn't too far off base, so maybe it makes sense to nominate it for a real review.

[James Robinson]

Comment on attachment 145894 [details]
Work in progress

R=me (although you have some merge conflicts somewhere)

[Adam Barth]

Created attachment 146150 [details]
Patch for landing

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

[Build Bot]

Comment on attachment 146150 [details]
Patch for landing

Attachment 146150 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12905979

[Adam Barth]

9>WebKit.exp : error LNK2001: unresolved external symbol "public: void __thiscall WebCore::Settings::setDefaultDeviceScaleFactor(int)" (?setDefaultDeviceScaleFactor@Settings@WebCore@@QAEXH@Z)

[Adam Barth]

Created attachment 146171 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146171 [details]
Patch for landing

Rejecting attachment 146171 [details] from commit-queue.

New failing tests:
WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag
Full output: http://queues.webkit.org/results/12910287

[WebKit Review Bot]

Created attachment 146274 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 146171 [details]
Patch for landing

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

New failing tests:
WebFrameTest.DeviceScaleFactorUsesDefaultWithoutViewportTag

[WebKit Review Bot]

Created attachment 146276 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Committed r119752: <http://trac.webkit.org/changeset/119752>