Bug 67150 – Would like API to use a custom device scale factor for a particular WebView/WKView

• Component: WebKit API
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

Safari sometimes uses a windowless WebView to take snapshots of webpages. It would be handy for Safari to be able to tell WebKit what device scale factor to use for that WebView (presumably based on the scale factor at which the snapshots will be displayed).

[Radar WebKit Bug Importer]

<rdar://problem/10041016>

[Beth Dakin]

Created attachment 105693 [details]
Patch

[WebKit Review Bot]

Attachment 105693 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/mac/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit2/UIProcess/WebPageProxy.cpp:1124:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 105695 [details]
Patch with style fixes

[Darin Adler]

Comment on attachment 105693 [details]
Patch

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

AppKit uses the term “backing scale factor” for this. Is the name “device scale factor” the right one for our SPI? I know we use it internally.

review- because this doesn’t add API to WebKit2 (missing change to header)

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2067
> +- (void)setOverrideDeviceScaleFactor:(float)overrideScaleFactor

Maybe CGFloat instead of float? I know it’s a float internally.

It would be nice if there was a way to remove this. I guess you can do that by passing 0, but that seems a little indirect.

What if someone passes a NAN or infinity or something crazy like that?

Also, I think you forgot to add this to a header file.

How about putting this API on the WKPage instead of on the WKView? Seems it would be even more useful for WKPage objects being used for things other than backing an NSView.

> Source/WebKit/mac/WebView/WebViewData.mm:82
> +    overrideDeviceScaleFactor = 0;

This is not needed. Objective-C data members are automatically initialized to 0.

> Source/WebKit/mac/WebView/WebViewPrivate.h:552
> +- (void)_setOverrideDeviceScaleFactor:(float)overrideScaleFactor;

Maybe CGFloat instead of float? I know it’s a float internally.

[Jeff Miller]

Comment on attachment 105695 [details]
Patch with style fixes

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

If we believe we only need this feature on the Mac, most of this code could be Mac-only.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:148
> +    , m_overrideDeviceScaleFactor(0)

Can this ivar be Mac-only?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1124
> +    if (!m_overrideDeviceScaleFactor)

This if clause should be Mac only.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1129
> +void WebPageProxy::setOverrideDeviceScaleFactor(float overrideScaleFactor)

setOverrideDeviceScaleFactor() should be Mac-only, and its implementation moved to WebPageProxyMac.mm.

> Source/WebKit2/UIProcess/WebPageProxy.h:865
> +    float m_overrideDeviceScaleFactor;

As mentioned above, this ivar should be Mac-only.

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 105695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105695&action=review
> 
> If we believe we only need this feature on the Mac, most of this code could be Mac-only.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:148
> > +    , m_overrideDeviceScaleFactor(0)
> 
> Can this ivar be Mac-only?
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1124
> > +    if (!m_overrideDeviceScaleFactor)
> 
> This if clause should be Mac only.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:1129
> > +void WebPageProxy::setOverrideDeviceScaleFactor(float overrideScaleFactor)
> 
> setOverrideDeviceScaleFactor() should be Mac-only, and its implementation moved to WebPageProxyMac.mm.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:865
> > +    float m_overrideDeviceScaleFactor;
> 
> As mentioned above, this ivar should be Mac-only.

I'm not sure that we do want this to be Mac-only. Adam added m_deviceScaleFactor as a cross-platform member variable to WebPageProxy, so it seems to me that the override should be cross-platform as well.

[Darin Adler]

I don’t think this should be Mac-only.

[Darin Adler]

Comment on attachment 105695 [details]
Patch with style fixes

review- because this doesn’t add API to WebKit2 (missing change to header)

[Beth Dakin]

Created attachment 105711 [details]
Patch that addresses comments

Here's a patch that I believe addresses all comments so far, except for Darin's comment about strange input values. It's true that callers of this API that send in crazy float values are not protected. It's also true that to un-set the override, a caller would have to set the overrideBackingScaleFactor to 0, which is a little strange. I am open to suggestions here for restricting the values or adding additional API to turn the override off (instead of relying on 0).

[Darin Adler]

Comment on attachment 105711 [details]
Patch that addresses comments

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1140
> +    m_overrideBackingScaleFactor = overrideScaleFactor;
> +
> +    if (m_overrideBackingScaleFactor != m_deviceScaleFactor)
> +        m_drawingArea->deviceScaleFactorDidChange();

This if statement seems wrong for the case where the device scale factor was 2, the old override scale factor was 1, and the new override scale factor is 2. Clearly in that case we do want to call deviceScaleFactorDidChange.

The easiest way to write this correctly is probably this:

    float oldScaleFactor = deviceScaleFactor();

    m_overrideBackingScaleFactor = overrideScaleFactor;

    if (deviceScaleFactor() != oldScaleFactor)
        m_drawingArea->deviceScaleFactorDidChange();

> Source/WebKit2/UIProcess/API/C/WKPage.h:352
> +WK_EXPORT void WKPageSetOverrideBackingScaleFactor(WKPageRef page, float overrideScaleFactor);

Other WebKit2 API seem to use double rather the float. For example, see WKPageSetTextZoomFactor below.

Other WebKit2 APIs seem to include both setters and getters. For example, see WKPageGetTextZoomFactor below.

Since 0 is a magic number meaning “get the scale factor from the view”, it would be good to have a named constant for that magic number in the header.

All of these are things we can fix later, although the double vs. float thing seems worth doing up front to me.

> Source/WebKit/mac/WebView/WebView.mm:2803
> +    if (oldScaleFactor != overrideScaleFactor)
> +        _private->page->setDeviceScaleFactor(overrideScaleFactor);

This should be:

    if (oldScaleFactor != [self _deviceScaleFactor])

Otherwise, we will send an unnecessary setDeviceScaleFactor call in the case where we are setting the override scale factor back to 0.

[Beth Dakin]

I addressed all of Darin's comment except coming up with a named constant for 0 as the magic number before committing. I like the idea of having a constant for that, I just haven't come up with a good name yet.

Everything else was committed with revision 94122!

[Adam Roben (:aroben)]

Comment on attachment 105711 [details]
Patch that addresses comments

It would be great to have an API test for this. I'm happy to help you with that if needed.