Bug 186821

Created attachment 343108 [details]
Patch

<rdar://problem/41212279>

Comment on attachment 343108 [details]
Patch

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

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:289
> +    progressTransform.a *= scale;
> +    progressTransform.b *= scale;
> +    progressTransform.c *= scale;
> +    progressTransform.d *= scale;

What? Scale is just in a and d.  And why not just use CGAffineTransformScale?

Comment on attachment 343108 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:289
>> +    progressTransform.d *= scale;
> 
> What? Scale is just in a and d.  And why not just use CGAffineTransformScale?

Doesn’t matter much since b and c will always be zero. But yes I’ll use CGAffineTransformscale here.

Created attachment 343248 [details]
Patch

Comment on attachment 343248 [details]
Patch

Attachment 343248 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8279500

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/formdata.htm

Created attachment 343257 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

Comment on attachment 343248 [details]
Patch

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

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.h:43
> +@property (assign, nonatomic) BOOL animating;

should the getter be ‘isAnimating’? :P

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:787
> +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];
> +        else if (_interactivePinchDismissGestureRecognizer.get().state == UIGestureRecognizerStateBegan)
> +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];

You seem to be repeating yourself. Maybe these cases should be ||’d?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:984
> +        if (progress > 0.25 || (progress > 0 && velocity > 5))

Is this what other people usually use? I feel like I’ve seen fancier things.

Comment on attachment 343248 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:787
>> +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];
> 
> You seem to be repeating yourself. Maybe these cases should be ||’d?

Or the second one should be pinch? I’m not sure.

(In reply to Tim Horton from comment #9)
> Comment on attachment 343248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343248&action=review
> 
> >> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:787
> >> +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];
> > 
> > You seem to be repeating yourself. Maybe these cases should be ||’d?
> 
> Or the second one should be pinch? I’m not sure.

The second one should be pinch.

(In reply to Tim Horton from comment #8)
> Comment on attachment 343248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343248&action=review
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.h:43
> > +@property (assign, nonatomic) BOOL animating;
> 
> should the getter be ‘isAnimating’? :P

Probably should.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:787
> > +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];
> > +        else if (_interactivePinchDismissGestureRecognizer.get().state == UIGestureRecognizerStateBegan)
> > +            anchor = [_interactivePanDismissGestureRecognizer locationInView:_fullscreenViewController.get().view];
> 
> You seem to be repeating yourself. Maybe these cases should be ||’d?

This should be using the pinch recognizer in the else statement.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mm:984
> > +        if (progress > 0.25 || (progress > 0 && velocity > 5))
> 
> Is this what other people usually use? I feel like I’ve seen fancier things.

KISS. (I'll see what other dismiss gestures use.)

Created attachment 343371 [details]
Patch for landing

Comment on attachment 343371 [details]
Patch for landing

Clearing flags on attachment: 343371

Committed r233104: <https://trac.webkit.org/changeset/233104>

All reviewed patches have been landed.  Closing bug.