Bug 88375 - Settings::defaultDeviceScaleFactor is redundant with Page::deviceScaleFactor
Summary: Settings::defaultDeviceScaleFactor is redundant with Page::deviceScaleFactor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 66687
  Show dependency treegraph
 
Reported: 2012-06-05 16:32 PDT by Adam Barth
Modified: 2012-06-07 14:21 PDT (History)
15 users (show)

See Also:


Attachments
Work in progress (12.29 KB, patch)
2012-06-05 16:36 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (13.12 KB, patch)
2012-06-06 17:03 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (14.69 KB, patch)
2012-06-06 18:23 PDT, Adam Barth
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-03 (540.30 KB, application/zip)
2012-06-07 06:30 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (485.96 KB, application/zip)
2012-06-07 06:36 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-06-05 16:32:28 PDT
Settings::defaultDeviceScaleFactor is redundant with Page::deviceScaleFactor
Comment 1 Adam Barth 2012-06-05 16:36:59 PDT
Created attachment 145894 [details]
Work in progress
Comment 2 Adam Barth 2012-06-05 16:40:14 PDT
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.
Comment 3 James Robinson 2012-06-05 16:44:26 PDT
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.
Comment 4 Dana Jansens 2012-06-06 10:46:14 PDT
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?
Comment 5 Adam Barth 2012-06-06 14:26:24 PDT
> 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.
Comment 6 Adam Barth 2012-06-06 14:26:59 PDT
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.
Comment 7 James Robinson 2012-06-06 16:13:00 PDT
Comment on attachment 145894 [details]
Work in progress

R=me (although you have some merge conflicts somewhere)
Comment 8 Adam Barth 2012-06-06 17:03:52 PDT
Created attachment 146150 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-06-06 17:06:40 PDT
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 10 Build Bot 2012-06-06 18:16:18 PDT
Comment on attachment 146150 [details]
Patch for landing

Attachment 146150 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12905979
Comment 11 Adam Barth 2012-06-06 18:19:54 PDT
9>WebKit.exp : error LNK2001: unresolved external symbol "public: void __thiscall WebCore::Settings::setDefaultDeviceScaleFactor(int)" (?setDefaultDeviceScaleFactor@Settings@WebCore@@QAEXH@Z)
Comment 12 Adam Barth 2012-06-06 18:23:46 PDT
Created attachment 146171 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-06-07 06:30:35 PDT
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
Comment 14 WebKit Review Bot 2012-06-07 06:30:40 PDT
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
Comment 15 WebKit Review Bot 2012-06-07 06:36:19 PDT
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
Comment 16 WebKit Review Bot 2012-06-07 06:36:30 PDT
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
Comment 17 Adam Barth 2012-06-07 14:21:37 PDT
Committed r119752: <http://trac.webkit.org/changeset/119752>