Bug 204128 – pageZoom/setPageZoom: should not be in a Mac-only part of WKWebView.mm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Brady Eidson]

pageZoom/setPageZoom: should not be in a Mac-only part of WKWebView.mm

[Brady Eidson]

Created attachment 383389 [details]
Patch

[Alex Christensen]

Comment on attachment 383389 [details]
Patch

Could you add a unit test that uses this?
Could we move all the Mac-specific code to WKWebViewMac.mm to avoid similar mistakes in the future?

[Tim Horton]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 383389 [details]
> Patch
> 
> Could you add a unit test that uses this?
> Could we move all the Mac-specific code to WKWebViewMac.mm to avoid similar
> mistakes in the future?

Have definitely considered doing this (for iOS too, only keep shared code in WKWebView.mm), but it's kind of irritating (see WKContentViewInteraction irritation).

[Alex Christensen]

As long as you're touching WKWebView.h, is there any reason we have multiple sections with #if !TARGET_OS_IPHONE ?  Could they be merged into one section?

[Brady Eidson]

(In reply to Alex Christensen from comment #4)
> As long as you're touching WKWebView.h, is there any reason we have multiple
> sections with #if !TARGET_OS_IPHONE ?  Could they be merged into one section?

Separate patch for the no-behavior-change-cleanup-only, plz.

[Alex Christensen]

Ok, but a unit test still wouldn't hurt.  What happens if you call setPageZoom on iOS before this?

[Brady Eidson]

Created attachment 383469 [details]
Patch

[Brady Eidson]

(In reply to Alex Christensen from comment #6)
> Ok, but a unit test still wouldn't hurt.  

Of course I was doing a unit test! And it's in the new patch.

> What happens if you call
> setPageZoom on iOS before this?

It would set the value of a synthesized property.

E.g. - nothing.

Yay testing!

[Alex Christensen]

Comment on attachment 383469 [details]
Patch

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

> Tools/TestWebKitAPI/cocoa/TestWKWebView.mm:194
> +        id result = [self objectByEvaluatingJavaScript:@"getClientWidth()"];

It is strange that this calls a JavaScript function that is not defined in TestWKWebView.  Please move.

[Brady Eidson]

Created attachment 383474 [details]
Patch

[Brady Eidson]

Created attachment 383499 [details]
Patch to explore EWS failure on iOS

[Brady Eidson]

Created attachment 383515 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 383515 [details]
Patch

Clearing flags on attachment: 383515

Committed r252458: <https://trac.webkit.org/changeset/252458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57194511>