[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