[webkit-reviews] review granted: [Bug 131469] [iOS][WK2] Add the initial WebProcess handling of animated resize for rotation : [Attachment 229011] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 131469: [iOS][WK2] Add the initial WebProcess handling of animated resize
for rotation
https://bugs.webkit.org/show_bug.cgi?id=131469

Attachment 229011: Patch
https://bugs.webkit.org/attachment.cgi?id=229011&action=review

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229011&action=review


> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:31
> +#import <CoreGraphics/CGFloat.h>

this is sorted wrong

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:480
> +	   double currentTargetScale = [[_contentView layer]
affineTransform].a;

pulling things out of transforms at random is slightly scary unless you know
you're the only person touching them.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:482
> +	   _resizeAnimationTransformAdjustements =
CATransform3DMakeScale(scale, scale, 0);

the first "e" in adjustments is not real.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:489
> +	   _resizeAnimationTransformAdjustements.m41 = (currentContentOffset.x
- newContentOffset.x) / currentTargetScale;

can't you use one of the CATransform functions to do this

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1300
> +	   double verticalScrollAdjusment =
_resizeAnimationTransformAdjustements.m42 * currentScale;

a *different* misspelling of adjustment :)

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1738
> +	   double scaleDifference = targetScale / scale;

this would look a lot prettier with FloatRect/friends' helpers (supporting
scale, adding and subtracting sizes and points, etc.)

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1766
> +	   double verticalAdjustement = 0;

nopppppe

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:1770
> +	       horizontalAdjustment += - newExposedContentRect.x();

why "+= -"?