[webkit-reviews] review granted: [Bug 67150] Would like API to use a custom device scale factor for a particular WebView/WKView : [Attachment 105711] Patch that addresses comments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 67150: Would like API to use a custom device scale factor for a particular
WebView/WKView
https://bugs.webkit.org/show_bug.cgi?id=67150

Attachment 105711: Patch that addresses comments
https://bugs.webkit.org/attachment.cgi?id=105711&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.