Bug 45585 - Add zoom support to WebKit2 API
Summary: Add zoom support to WebKit2 API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-10 22:44 PDT by Sam Weinig
Modified: 2010-09-10 23:51 PDT (History)
2 users (show)

See Also:


Attachments
Patch (98.80 KB, patch)
2010-09-10 22:47 PDT, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff
Updated patch (106.05 KB, patch)
2010-09-10 23:39 PDT, Sam Weinig
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-09-10 22:44:50 PDT
Add zoom support to WebKit2 API.
Comment 1 Sam Weinig 2010-09-10 22:45:01 PDT
<rdar://problem/7660657>
Comment 2 Sam Weinig 2010-09-10 22:47:26 PDT
Created attachment 67290 [details]
Patch
Comment 3 WebKit Review Bot 2010-09-10 22:55:53 PDT
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 4 mitz 2010-09-10 23:20:26 PDT
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?
Comment 5 Sam Weinig 2010-09-10 23:24:30 PDT
(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.
Comment 6 mitz 2010-09-10 23:27:20 PDT
(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 7 mitz 2010-09-10 23:27:58 PDT
Comment on attachment 67290 [details]
Patch

r=me if you add the equality tests
Comment 8 Sam Weinig 2010-09-10 23:39:51 PDT
Created attachment 67294 [details]
Updated patch
Comment 9 Sam Weinig 2010-09-10 23:51:39 PDT
Landed in http://trac.webkit.org/changeset/67287.