[Fullscreen] Add a pinch-to-exit gesture
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.