Bug 143560 – Add a persistent, fixed scale factor to make it easy to scale down WK(Web)Views

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

[Tim Horton]

Add a persistent, fixed scale factor to make it easy to scale down WK(Web)Views

[Tim Horton]

Created attachment 250429 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 250429 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKViewPrivate.h:97
> +@property (nonatomic, setter=_setAutomaticallyComputesFixedLayoutSizeFromViewScale:) BOOL _automaticallyComputesFixedLayoutSizeFromViewScale;

Please add a comment stating that this overrides _fixedLayoutSize. It looks like it also implicitly enables does _setFixedLayoutEnabled:YES? You should say that too.

Maybe we really need:

_setFixedLayoutMode:{NoFixedLayout, FixedLayoutBasedOnViewScale, FixedSizeLayout}

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:99
> +- (void)_killWebContentProcess;

Why doesn't Mac get _didRelaunchProcess ?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:547
> +    [self _setFixedLayoutSize:NSMakeSize(self.frame.size.width * inverseScale, self.frame.size.height * inverseScale)];

Do we want explicit rounding/flooring of that size?

[Tim Horton]

Created attachment 250568 [details]
Patch

[Darin Adler]

Comment on attachment 250568 [details]
Patch

Do we need any range checking to avoid zeroes and infinities and other crazy values?

[Tim Horton]

(In reply to comment #4)
> Comment on attachment 250568 [details]
> Patch
> 
> Do we need any range checking to avoid zeroes and infinities and other crazy
> values?

Seems like a good idea. Will add and land.

[Tim Horton]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 250568 [details]
> > Patch
> > 
> > Do we need any range checking to avoid zeroes and infinities and other crazy
> > values?
> 
> Seems like a good idea. Will add and land.

Going to add an ASSERT in WebPageProxy and throw exceptions in WKView and WKWebView, after talking to Dan/Anders.

[Alexey Proskuryakov]

Several 32-bit build failures from this patch:

/Volumes/Data/slave/gala-production-archive/build/build-Production/WebKit2.roots/Sources/WebKit2/UIProcess/API/mac/WKView.mm:570:31: error: no viable conversion from 'NSSize' (aka '_NSSize') to 'CGSize'

/Volumes/Data/slave/gala-production-archive/build/build-Production/WebKit2.roots/Sources/WebKit2/UIProcess/API/mac/WKView.mm:367:17: error: property '_automaticallyComputesFixedLayoutSizeFromViewScale' requires method '_automaticallyComputesFixedLayoutSizeFromViewScale' to be defined - use @synthesize, @dynamic or provide a method implementation in this class implementation [-Werror,-Wobjc-property-implementation]

/Volumes/Data/slave/gala-production-archive/build/build-Production/WebKit2.roots/Sources/WebKit2/UIProcess/API/mac/WKView.mm:367:17: error: property '_automaticallyComputesFixedLayoutSizeFromViewScale' requires method '_setAutomaticallyComputesFixedLayoutSizeFromViewScale:' to be defined - use @synthesize, @dynamic or provide a method implementation in this class implementation [-Werror,-Wobjc-property-implementation]

[Alexey Proskuryakov]

Attempted fixes in r182781. Anyone willing to donate hardware for 32-bit EWS?

Tim's patch landed in r182772, so marking the bug as resolved.

[Darin Adler]

(In reply to comment #5)
> > Do we need any range checking to avoid zeroes and infinities and other crazy
> > values?
> 
> Seems like a good idea. Will add and land.

Slight refactoring suggestion on these. Instead of this:

    if (viewScale <= 0 || isnan(viewScale) || isinf(viewScale))
        [NSException raise:NSInvalidArgumentException format:@"View scale should be a positive number"];

I suggest writing this:

    if (!(viewScale > 0) || isinf(viewScale))
        [NSException raise:NSInvalidArgumentException format:@"View scale should be a positive number"];

I think it’s nice to take advantage of the natural property of NAN that it returns false when every you compare it to anything else. And the code ends up matching the exception string slightly more closely since it’s a check for a positive number rather than a check that it’s not zero or negative.