WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
Summary
Would like API to use a custom device scale factor for a particular WebView/W...
Adam Roben (:aroben)
Reported
2011-08-29 13:30:06 PDT
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).
Attachments
Patch
(7.31 KB, patch)
2011-08-30 14:20 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Patch with style fixes
(7.30 KB, patch)
2011-08-30 14:26 PDT
,
Beth Dakin
no flags
Details
Formatted Diff
Diff
Patch that addresses comments
(7.84 KB, patch)
2011-08-30 15:44 PDT
,
Beth Dakin
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2011-08-29 13:30:47 PDT
<
rdar://problem/10041016
>
Beth Dakin
Comment 2
2011-08-30 14:20:18 PDT
Created
attachment 105693
[details]
Patch
WebKit Review Bot
Comment 3
2011-08-30 14:24:20 PDT
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
Comment 4
2011-08-30 14:26:53 PDT
Created
attachment 105695
[details]
Patch with style fixes
Darin Adler
Comment 5
2011-08-30 14:28:02 PDT
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 6
2011-08-30 14:37:27 PDT
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
Comment 7
2011-08-30 14:43:28 PDT
(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
Comment 8
2011-08-30 14:53:58 PDT
I don’t think this should be Mac-only.
Darin Adler
Comment 9
2011-08-30 14:54:19 PDT
Comment on
attachment 105695
[details]
Patch with style fixes review- because this doesn’t add API to WebKit2 (missing change to header)
Beth Dakin
Comment 10
2011-08-30 15:44:04 PDT
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 11
2011-08-30 16:01:55 PDT
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
Comment 12
2011-08-30 16:32:26 PDT
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 13
2011-08-31 05:42:26 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug