[webkit-reviews] review denied: [Bug 129146] WebKit2 View Gestures (Smart Magnification): Support for iOS : [Attachment 225295] fix mac build

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Tim Horton
<thorton at apple.com>'s request for review:
Bug 129146: WebKit2 View Gestures (Smart Magnification): Support for iOS
https://bugs.webkit.org/show_bug.cgi?id=129146

Attachment 225295: fix mac build
https://bugs.webkit.org/attachment.cgi?id=225295&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=225295&action=review


> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:325
> +- (RetainPtr<CGImageRef>)_takeViewSnapshot
> +{
> +    // FIXME: We should be able to use acquire an IOSurface directly,
instead of going to CGImage here and back in ViewSnapshotStore.
> +    UIGraphicsBeginImageContextWithOptions(self.bounds.size, YES,
self.window.screen.scale);
> +    [self drawViewHierarchyInRect:[self bounds] afterScreenUpdates:NO];
> +    UIImage *image = UIGraphicsGetImageFromCurrentImageContext();
> +    UIGraphicsEndImageContext();
> +    return image.CGImage;
> +}

Let's fix everything at once, otherwise it is just ugly.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:331
> +    const double maximumZoomDuration = 0.4;
> +    const double minimumZoomDuration = 0.1;
> +    const double zoomDurationFactor = 0.3;

I think Darin would prefer no const here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:341
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);

You could move this bellow, just before it is needed.
Shouldn't the type be CGSize or something?

I don't think the scrollview's bounds is ever correct when you want to show
something. You should use the unobscuredRect.

Fun fact, the unobscuredRect can change in response to the zoom. This means the
centering is not strictly correct. This is also wrong on WebKit1.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:343
> +    double currentScaleFactor = [_scrollView zoomScale];
> +    double relativeScale = scale / currentScaleFactor;

Shouldn't those be CGFloat?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:346
> +    WebCore::FloatRect targetRectInScrollView = [_scrollView
convertRect:targetRect fromView:_contentView.get()];
> +    WebCore::FloatRect scaledTargetRectInScrollView =
targetRectInScrollView;

CGRect?

I prefer the name to say in which coordinate system the rect is. For example,
targetRectInScrollViewCoordinate.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:347
> +    scaledTargetRectInScrollView.scale(relativeScale, relativeScale);

targetRectInScrollViewAfterZoom or something like that?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:356
> +    if (scaledTargetRectInScrollView.width() > scrollViewSize.width())
> +	   zoomCenter.setX(origin.x());
> +    if (scaledTargetRectInScrollView.height() > scrollViewSize.height())
> +	   zoomCenter.setY(origin.y());

I think you should use the unobscured rect here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:365
> +    contentOffset = contentOffset.expandedTo(WebCore::FloatPoint());

???

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:373
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);
> +    WebCore::FloatSize contentSize([_scrollView contentSize]);
> +    WebCore::FloatPoint currentContentOffset([_scrollView contentOffset]);

I think you should use unobscuredRect for the scrollViewSize and
currentContentOffset, and use WKContentView's size instead of [_scrollView
contentSize] instead.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:376
> +    WebCore::FloatPoint originInScrollView([_scrollView convertPoint:origin
fromView:_contentView.get()]);
> +    WebCore::FloatRect targetRectInScrollView([_scrollView
convertRect:targetRect fromView:_contentView.get()]);

inScrollViewCoordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> +    float scrollDistance = (currentContentOffset -
newContentOffset).diagonalLength();

float? :)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:420
> +    CGSize scrollViewSize = [_scrollView bounds].size;

[_scrollView bounds] is probably incorrect.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:452
> +    if (!layerTreeTransaction.scaleWasSetByUIProcess() && ![_scrollView
isZooming] && ![_scrollView isZoomBouncing] && ![_scrollView _isAnimatingZoom])


Sorry!

> Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:50
> +static const CGFloat smartMagnificationPanScrollThresholdZoomedOut = 60;
> +static const CGFloat smartMagnificationPanScrollThresholdIPhone = 100;
> +static const CGFloat smartMagnificationPanScrollThresholdIPad = 150;
> +static const CGFloat smartMagnificationElementPadding = 0.05;
> +
> +static const double smartMagnificationMaximumScale = 1.6;
> +static const double smartMagnificationMinimumScale = 0;

CGFloat and double?