Bug 131469

Summary: [iOS][WK2] Add the initial WebProcess handling of animated resize for rotation
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: enrica, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Benjamin Poulain 2014-04-09 18:39:44 PDT
[iOS][WK2] Add the initial WebProcess handling of animated resize for rotation
Comment 1 Benjamin Poulain 2014-04-09 18:46:17 PDT
Created attachment 229011 [details]
Patch
Comment 2 Tim Horton 2014-04-09 18:58:57 PDT
Comment on attachment 229011 [details]
Patch

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 "+= -"?
Comment 3 Benjamin Poulain 2014-04-09 19:34:08 PDT
Created attachment 229018 [details]
Patch
Comment 4 Benjamin Poulain 2014-04-09 19:40:52 PDT
Thanks for the review!
English is too hard :)

> > 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

The reason I modify the matrix in place is I do not want the composition of the translation with the scale, we need the translation to be in the target scale until the end of the animation.
Comment 5 Benjamin Poulain 2014-04-09 20:39:54 PDT
Comment on attachment 229018 [details]
Patch

Clearing flags on attachment: 229018

Committed r167060: <http://trac.webkit.org/changeset/167060>
Comment 6 Benjamin Poulain 2014-04-09 20:39:58 PDT
All reviewed patches have been landed.  Closing bug.