Bug 45585 – Add zoom support to WebKit2 API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Sam Weinig]

Add zoom support to WebKit2 API.

[Sam Weinig]

<rdar://problem/7660657>

[Sam Weinig]

Created attachment 67290 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/MiniBrowser/mac/BrowserWindowController.h:37:  _zoomTextOnly is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 12 files


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

Comment on attachment 67290 [details]
Patch

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

> WebKit2/Shared/CoreIPCSupport/WebPageMessageKinds.h:57
> +    SetPageZoomFactor,
> +    SetTextZoomFactor,
> +    SetPageAndTextZoomFactors,
Please sort. Better yet, why not eliminate SetPageZoomFactor and SetTextZoomFactor and always send SetPageAndTextZoomFactors?

> WebKit2/UIProcess/WebPageProxy.cpp:403
> +    m_textZoomFactor = zoomFactor;
> +    process()->send(WebPageMessage::SetTextZoomFactor, m_pageID, CoreIPC::In(m_textZoomFactor)); 
Should this return early and save the IPC if m_textZoomFactor is already equal to zoomFactor?

> WebKit2/UIProcess/API/C/WKPage.cpp:156
> +float WKPageGetTextZoomFactor(WKPageRef pageRef)
Why float and not double?

> WebKitTools/MiniBrowser/mac/BrowserWindowController.m:168
> +#define kWKDefaultMinimumZoomFactor (.5f)
> +#define kWKDefaultMaximumZoomFactor (3.0f)
> +#define kWKDefaultZoomFactorRatio (1.2f)
Why do these have a WK prefix if they’re not defined in WebKit?

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 67290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67290&action=prettypatch
> 
> > WebKit2/Shared/CoreIPCSupport/WebPageMessageKinds.h:57
> > +    SetPageZoomFactor,
> > +    SetTextZoomFactor,
> > +    SetPageAndTextZoomFactors,
> Please sort. Better yet, why not eliminate SetPageZoomFactor and SetTextZoomFactor and always send SetPageAndTextZoomFactors?

Ok.

> 
> > WebKit2/UIProcess/WebPageProxy.cpp:403
> > +    m_textZoomFactor = zoomFactor;
> > +    process()->send(WebPageMessage::SetTextZoomFactor, m_pageID, CoreIPC::In(m_textZoomFactor)); 
> Should this return early and save the IPC if m_textZoomFactor is already equal to zoomFactor?

Yes. Will change.

> 
> > WebKit2/UIProcess/API/C/WKPage.cpp:156
> > +float WKPageGetTextZoomFactor(WKPageRef pageRef)
> Why float and not double?

At what level do you think I should do the double to float conversion?

> > WebKitTools/MiniBrowser/mac/BrowserWindowController.m:168
> > +#define kWKDefaultMinimumZoomFactor (.5f)
> > +#define kWKDefaultMaximumZoomFactor (3.0f)
> > +#define kWKDefaultZoomFactorRatio (1.2f)
> Why do these have a WK prefix if they’re not defined in WebKit?

Oh, no good reason. Will change.

(In reply to comment #5)
> At what level do you think I should do the double to float conversion?

I would use doubles in WebKit and convert to float when calling into WebCore, but I don’t think it’s worth changing now.

Comment on attachment 67290 [details]
Patch

r=me if you add the equality tests

[Sam Weinig]

Created attachment 67294 [details]
Updated patch

[Sam Weinig]

Landed in http://trac.webkit.org/changeset/67287.