[webkit-reviews] review denied: [Bug 178924] Implement WKFullscreenWindowController for iOS. : [Attachment 325191] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Oct 27 15:05:06 PDT 2017
Simon Fraser (smfr) <simon.fraser at apple.com> has denied review:
Bug 178924: Implement WKFullscreenWindowController for iOS.
https://bugs.webkit.org/show_bug.cgi?id=178924
Attachment 325191: Patch
https://bugs.webkit.org/attachment.cgi?id=325191&action=review
--- Comment #15 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 325191
--> https://bugs.webkit.org/attachment.cgi?id=325191
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=325191&action=review
> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6065
> +#if ENABLE(FULLSCREEN_API) && PLATFORM(IOS)
I would flip these around (check for platform first).
> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6077
> + _fullScreenWindowController = adoptNS([[WKFullScreenWindowController
alloc] initWithWebView:self page:*_page.get()]);
Why does it need the view and the page? Can't it get the page from the view?
> Source/WebKit/UIProcess/API/Cocoa/_WKFullscreenDelegate.h:45
> +#if TARGET_OS_IPHONE
> +- (void)_webViewWillEnterElementFullscreen:(WKWebView *)webView;
> +- (void)_webViewDidEnterElementFullscreen:(WKWebView *)webView;
> +- (void)_webViewWillExitElementFullscreen:(WKWebView *)webView;
> +- (void)_webViewDidExitElementFullscreen:(WKWebView *)webView;
> +#else
> - (void)_webViewWillEnterFullscreen:(NSView *)webView;
> - (void)_webViewDidEnterFullscreen:(NSView *)webView;
> - (void)_webViewWillExitFullscreen:(NSView *)webView;
> - (void)_webViewDidExitFullscreen:(NSView *)webView;
> +#endif
Is this a deliberate mismatch because we think the iOS names are better? Or
because there's some other difference?
> Source/WebKit/UIProcess/Cocoa/FullscreenClient.h:76
> +#if PLATFORM(MAC)
> bool webViewWillEnterFullscreen : 1;
> bool webViewDidEnterFullscreen : 1;
> bool webViewWillExitFullscreen : 1;
> bool webViewDidExitFullscreen : 1;
> +#else
> + bool webViewWillEnterElementFullscreen : 1;
> + bool webViewDidEnterElementFullscreen : 1;
> + bool webViewWillExitElementFullscreen : 1;
> + bool webViewDidExitElementFullscreen : 1;
> +#endif
Seems a bit stupid to propagate the naming difference into the implementation.
> Source/WebKit/UIProcess/Cocoa/FullscreenClient.mm:59
> +#if PLATFORM(MAC)
> m_delegateMethods.webViewWillEnterFullscreen = [delegate
respondsToSelector:@selector(_webViewWillEnterFullscreen:)];
> m_delegateMethods.webViewDidEnterFullscreen = [delegate
respondsToSelector:@selector(_webViewDidEnterFullscreen:)];
> m_delegateMethods.webViewWillExitFullscreen = [delegate
respondsToSelector:@selector(_webViewWillExitFullscreen:)];
> m_delegateMethods.webViewDidExitFullscreen = [delegate
respondsToSelector:@selector(_webViewDidExitFullscreen:)];
> +#else
> + m_delegateMethods.webViewWillEnterElementFullscreen = [delegate
respondsToSelector:@selector(_webViewWillEnterElementFullscreen:)];
> + m_delegateMethods.webViewDidEnterElementFullscreen = [delegate
respondsToSelector:@selector(_webViewDidEnterElementFullscreen:)];
> + m_delegateMethods.webViewWillExitElementFullscreen = [delegate
respondsToSelector:@selector(_webViewWillExitElementFullscreen:)];
> + m_delegateMethods.webViewDidExitElementFullscreen = [delegate
respondsToSelector:@selector(_webViewDidExitElementFullscreen:)];
> +#endif
Ditto.
> Source/WebKit/UIProcess/Cocoa/FullscreenClient.mm:70
> +#if PLATFORM(MAC)
> if (m_delegateMethods.webViewWillEnterFullscreen)
> [m_delegate.get() _webViewWillEnterFullscreen:m_webView];
> +#else
> + if (m_delegateMethods.webViewWillEnterElementFullscreen)
> + [m_delegate.get() _webViewWillEnterElementFullscreen:m_webView];
> +#endif
Ugh.
> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:627
> void PageClientImpl::closeFullScreenManager()
Does one really close a manager?
> Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:629
> + [m_webView closeFullScreenWindowController];
You don't close a controller either. Why is this not telling the controller to
close?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.h:54
> +- (void)beganEnterFullScreenWithInitialFrame:(const
WebCore::IntRect&)initialFrame finalFrame:(const WebCore::IntRect&)finalFrame;
> +- (void)beganExitFullScreenWithInitialFrame:(const
WebCore::IntRect&)initialFrame finalFrame:(const WebCore::IntRect&)finalFrame;
We generally pass UI rects around as FloatRects to match CGRect. Since this is
Obj-C, maybe you should use CGRects here.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:44
> +#define ANIMATION_DELAY 0.0
> +#define ANIMATION_DURATION 0.2
These should be consts using Seconds or NSTimeInterval.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:45
> +#define BOXES 0
BOXES!
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:58
> + [otherView removeFromSuperview];
What if you just lost the only reference to otherView here?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:60
> + [view removeFromSuperview];
view may be destroyed here too.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:74
> + double _savedScale = 1;
Maybe savedPageScale to differentiate from _savedZoomScale. Also group the
float/double things together.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:104
> + _savedScale = [webView _pageScale];
> + _savedObscuredInsets = [webView _obscuredInsets];
> + _savedEdgeInset = [[webView scrollView] contentInset];
> + _savedContentOffset = [[webView scrollView] contentOffset];
> + _savedScrollIndicatorInsets = [[webView scrollView]
scrollIndicatorInsets];
> + _savedTopContentInset = page->topContentInset();
> + _savedViewScale = [webView _viewScale];
> + _savedZoomScale = [[webView scrollView] zoomScale];
I think _pageScale and zoomScale are generally the same.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:111
> + at interface _WKTapDelgatingView: UIView
"WKTapDelgatingView" - delegating
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:123
> +- (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event
hitTest is not a tap. hitTest is just a UIView API call, so I don't think this
is the right function to perform delegation on.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:134
> + at property (retain) NSArray *savedConstraints;
> + at property (retain) UIView *contentView;
> + at property (retain) id target;
> + at property (assign) SEL action;
nonatomic?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:170
> + [[visualEffectView contentView] setBackgroundColor:[UIColor
colorWithRed:(43.0 / 255.0) green:(46.0 / 255.0) blue:(48.0 / 255.0)
alpha:1.0]];
Where did these magic numbers come from?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:190
> + UIView *backLayerTintView = [[UIView alloc]
initWithFrame:visualEffectView.bounds];
RetainPtr<>
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:197
> + UIView *topLayerTintView = [[UIView alloc]
initWithFrame:visualEffectView.bounds];
RetainPtr<>
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:211
> + self.view = [[UIView alloc] initWithFrame:[[UIScreen mainScreen]
bounds]];
Leak here.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:214
> + CGRect doneButtonRect = CGRectMake(10, 20, 60, 47);
Is this appropriate when the system language is RTL?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:216
> + _visualEffectView = [self
createVisualEffectViewWithFrame:doneButtonRect];
Need an adoptNS() to avoid a leak. Actually no,
"createVisualEffectViewWithFrame" is just incorrectly named (it returns an
autorelease object).
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:233
> + _cancelButton.get().tintColor = [UIColor colorWithWhite:1.0 alpha:0.55];
> + _cancelButton.get().layer.compositingFilter = [CAFilter
filterWithType:kCAFilterPlusL];
Weird mixture of dot and [] syntax in all this code. Generally we use [] to
avoid the .get()
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:261
> + [NSObject cancelPreviousPerformRequestsWithTarget:self
selector:@selector(hideCancelButton) object:nil];
Super weird.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:275
> + [NSObject cancelPreviousPerformRequestsWithTarget:self
selector:@selector(hideCancelButton) object:nil];
Also weird. Normally you'd just maintain some "is animating" state.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:289
> +- (void)setTarget:(id)target action:(SEL)action
> +{
> + self.target = target;
> + self.action = action;
> +}
What are the target and action of a view controller? Not really sure these make
sense. Makes this sound more like a UIControl thing.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:303
> + at property (retain) UIViewController* viewController;
> + at property NSRect initialFrame;
> + at property NSRect finalFrame;
> + at property BOOL presenting;
nonatomic.
@property (getter=isPresenting) BOOL presenting;
I think we use CGRects in UIKit code.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:325
> + UIView *maskView = [[[UIView alloc] init] autorelease];
Why do you autorelease here, but you did explicit retain/release elsewhere?
Choose one (preferably RetainPtr<>).
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:333
> + CGAffineTransform scaleTransform =
CGAffineTransformMakeScale(scaleRect.width() /
FloatRect(fullscreenFrame).width(), scaleRect.height() /
FloatRect(fullscreenFrame).height());
FloatRect(fullscreenFrame).width() -> fullscreenFrame.size.width ?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:334
> + CGAffineTransform translateTransform =
CGAffineTransformMakeTranslation(CGRectGetMidX(inlineFrame)-CGRectGetMidX(fulls
creenFrame), CGRectGetMidY(inlineFrame)-CGRectGetMidY(fullscreenFrame));
Spaces around -
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:336
> + resetOriginTransform = CGAffineTransformMakeTranslation(0, 0);
This is just CGAffineTransformIdentity.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:344
> +#if BOXES
Better name please.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:348
> + inlineRepView = [[UIView alloc] initWithFrame:inlineFrame];
leaked.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:353
> + fullscreenRepView = [[UIView alloc] initWithFrame:fullscreenFrame];
leaked.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:367
> + if ((self.presenting && !success) || (!self.presenting && success))
self.isPresenting
does "presenting" mean "in the process of animating in", or "is showing"? Needs
a better name.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:387
> + WebKit::WebPageProxy* _page;
Does it need to store this separate from the view?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:398
> + NSRect _initialFrame;
> + NSRect _finalFrame;
CGRects.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:442
> + _fullscreenViewController = [[_WKFullScreenViewController alloc] init];
leak
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:451
> + if ([self isFullScreen])
> + return;
How would you already be in fullscreen if you've just created the view
controllers and stuff?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:460
> + _webViewPlaceholder = adoptNS([[UIView alloc] init]);
Please set a name on this view so it shows up in layer tree dumps.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:476
> + [[_webViewPlaceholder window] insertSubview:_webView atIndex:0];
Isn't [_webViewPlaceholder window] the same as the _webView's old window at
this point, so all you're doing is moving the _webView to be the first view in
the window? Seems odd to do this via [_webViewPlaceholder window]
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:485
> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0 *
NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
(int64_t)(0 * NSEC_PER_SEC) is just 0, right?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:486
> + _repaintCallback =
VoidCallback::create([self](WebKit::CallbackBase::Error) {
Do we know that |self| will stay alive until the callback fires?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:487
> + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0
* NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
Here too.
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:575
> + _repaintCallback =
VoidCallback::create([self](WebKit::CallbackBase::Error) {
Do we know that |self| will stay alive until the callback fires?
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:606
> + WKFullscreenAnimationController* ac = [[WKFullscreenAnimationController
alloc] init];
Don't use short names like "ac"
> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:616
> + WKFullscreenAnimationController* ac = [[WKFullscreenAnimationController
alloc] init];
Ditto.
More information about the webkit-reviews
mailing list