Bug 178924 – Implement WKFullscreenWindowController for iOS.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jeremy Jones]

Implement WKFullscreenWindowController for iOS.

[Jeremy Jones]

rdar://problem/34697120

[Jeremy Jones]

Created attachment 325130 [details]
Patch

[Build Bot]

Attachment 325130 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:268:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:268:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:369:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:369:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:468:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:468:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 325131 [details]
Patch

[Build Bot]

Attachment 325131 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:267:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:267:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:368:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:368:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:467:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:467:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 325137 [details]
Patch

[Jeremy Jones]

Created attachment 325140 [details]
Patch

[Build Bot]

Attachment 325140 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:366:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:366:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:465:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:465:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 325188 [details]
Patch

[Build Bot]

Attachment 325188 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:364:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:364:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:463:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:463:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 325191 [details]
Patch

[Build Bot]

Attachment 325191 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:265:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:364:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:364:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:463:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:463:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 325191 [details]
Patch

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

r=me, with nit. One question: does this change mean that <video>'s webkitEnterFullScreen() will now use element fullscreen?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:358
> +#if BOXES
> +    UIView *inlineRepView = nil;
> +    UIView *fullscreenRepView = nil;
> +    {
> +        inlineRepView = [[UIView alloc] initWithFrame:inlineFrame];
> +        inlineRepView.layer.borderColor = UIColor.greenColor.CGColor;
> +        inlineRepView.layer.borderWidth = 6.0;
> +        [containerView addSubview:inlineRepView];
> +
> +        fullscreenRepView = [[UIView alloc] initWithFrame:fullscreenFrame];
> +        fullscreenRepView.layer.borderColor = UIColor.redColor.CGColor;
> +        fullscreenRepView.layer.borderWidth = 6.0;
> +        [containerView addSubview:fullscreenRepView];
> +    }
> +#endif

Can these be safely removed? I see that BOXES is defined as 0.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:365
> +    } completion:^(BOOL finished){
> +        BOOL success = ![transitionContext transitionWasCancelled];

Interesting that finished != success.

[Keith Miller]

Comment on attachment 325191 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10860
> +				3FFDDB971F5780E10050E593 /* WKFullScreenWindowControllerIOS.mm in Sources */,

It looks like you're compiling this standalone. Can you add it to SourcesIOS.txt?

[Simon Fraser (smfr)]

Comment on attachment 325191 [details]
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
> +@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
> +@property (retain) NSArray *savedConstraints;
> +@property (retain) UIView *contentView;
> +@property (retain) id target;
> +@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
> +@property (retain) UIViewController* viewController;
> +@property NSRect initialFrame;
> +@property NSRect finalFrame;
> +@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(fullscreenFrame), 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.

[Jeremy Jones]

(In reply to Jer Noble from comment #13)
> Comment on attachment 325191 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325191&action=review
> 
> r=me, with nit. One question: does this change mean that <video>'s
> webkitEnterFullScreen() will now use element fullscreen?

No, because this feature is still off by default.
I have separated out the patch to fix this in HTMLMedia element and it will be in before we enable this feature.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:358
> > +#if BOXES
> > +    UIView *inlineRepView = nil;
> > +    UIView *fullscreenRepView = nil;
> > +    {
> > +        inlineRepView = [[UIView alloc] initWithFrame:inlineFrame];
> > +        inlineRepView.layer.borderColor = UIColor.greenColor.CGColor;
> > +        inlineRepView.layer.borderWidth = 6.0;
> > +        [containerView addSubview:inlineRepView];
> > +
> > +        fullscreenRepView = [[UIView alloc] initWithFrame:fullscreenFrame];
> > +        fullscreenRepView.layer.borderColor = UIColor.redColor.CGColor;
> > +        fullscreenRepView.layer.borderWidth = 6.0;
> > +        [containerView addSubview:fullscreenRepView];
> > +    }
> > +#endif
> 
> Can these be safely removed? I see that BOXES is defined as 0.

Removed.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:365
> > +    } completion:^(BOOL finished){
> > +        BOOL success = ![transitionContext transitionWasCancelled];
> 
> Interesting that finished != success.

I'll investigate. I thought that transitionWasCancelled was more semantically what I wanted, thought they may actually be the same in this case.

[Jeremy Jones]

(In reply to Simon Fraser (smfr) from comment #15)
> Comment on attachment 325191 [details]
> 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).

Done. And in the comment in the #endif

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

I guess I can do that from WKWebViewInternal.h. I'll file a bug to fix this in the Mac element fullscreen implementations too.

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

Yes, they are better. The problem with the existing names is that WKUIDelegatePrivate has methods that collide with these. So it is impossible to implement both of these delegate protocols in one class.

I'm adding the iOS versions with this patch, but with some follow up work, I plan to add the new ones to MacOS also. Adding and deprecating on MacOS will require keeping track of both the old and new selectors, until we are ready to completely stop supporting the old selector names.

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

See comment above. This is a *smart* way to be able to keep track of both sets of selectors if we switch to the new names on MacOS :-)

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

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.

I know right! :-)

> 
> > Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:627
> >  void PageClientImpl::closeFullScreenManager()
> 
> Does one really close a manager?

This is used in both iOS and MacOS. The idea of "closing" fullscreen, is a better semantic match for Mac where we are used to closing windows.

If you feel strongly about this name, I'll file a bug about changing everything over to calling this exitFullscreen().

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

This is matching the existing naming convention for the feature. I'll file a bug to revisit this name for Mac and iOS. 

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

Agreed. I'll file a bug for this and this change would affect MacOS too.

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

Deleted these here, and I use a const NSTimeInterval in the relevant code.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:45
> > +#define BOXES 0
> 
> BOXES!

Gone!

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:58
> > +    [otherView removeFromSuperview];
> 
> What if you just lost the only reference to otherView here?

Gone! Not necessary to remove before inserting.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:60
> > +    [view removeFromSuperview];
> 
> view may be destroyed here too.

Caller should either be holding a ref to that view, or expect it to be released.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:74
> > +    double _savedScale = 1;
> 
> Maybe savedPageScale to differentiate from _savedZoomScale. Also group the
> float/double things together.

Renamed. The double is actually a CGFloat. I also changed other types to CGFloat as appropriate.

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

zoomScale is gone.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:111
> > +@interface _WKTapDelgatingView: UIView
> 
> "WKTapDelgatingView" - delegating

+e Done

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

Agreed, but I don't know a better way to do this at the moment. This view covers the entire page content and must not interfere with any taps or touches on the WKWebView. So, it must fail hitTest.

But we also need to use it to detect user interaction to show the exit button when touch event happens.

I'll write a bug to find a way to make this work with a UIGestureRecognizer on the WKWebView.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:134
> > +@property (retain) NSArray *savedConstraints;
> > +@property (retain) UIView *contentView;
> > +@property (retain) id target;
> > +@property (assign) SEL action;
> 
> nonatomic?

Added.

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

HI. They match other magic numbers on the platform.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:190
> > +    UIView *backLayerTintView = [[UIView alloc] initWithFrame:visualEffectView.bounds];
> 
> RetainPtr<>

Done. I audited the code and fixed this pattern anywhere there is object creation.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:197
> > +    UIView *topLayerTintView = [[UIView alloc] initWithFrame:visualEffectView.bounds];
> 
> RetainPtr<>

Ditto.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:211
> > +    self.view = [[UIView alloc] initWithFrame:[[UIScreen mainScreen] bounds]];
> 
> Leak here.

added -autorelease.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:214
> > +    CGRect doneButtonRect = CGRectMake(10, 20, 60, 47);
> 
> Is this appropriate when the system language is RTL?

No. I'll file a bug.

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

renamed createVisualEffectViewWithFrame to visualEffectViewWithFrame.

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

I switched to [] everywhere in the file.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:261
> > +    [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(hideCancelButton) object:nil];
> 
> Super weird.

You prefer NSTimer over -performSelector:withObject:afterDelay: for delayed actions?

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

I'm not sure what you mean. This is about scheduling and canceling a delayed action to hide the cancel button.

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

This is on a UIView not a UIViewController. This target/action pattern of a UIControl matches the behavior of this view nicely, but because it is a transparent view with no selection, highlight, etc.. state, it doesn't quite fit as a UIControl.

From a previous comment, I may be able to replace this with a custom UIGestureRecognizer.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:303
> > +@property (retain) UIViewController* viewController;
> > +@property NSRect initialFrame;
> > +@property NSRect finalFrame;
> > +@property BOOL presenting;
> 
> nonatomic.

Added.

> 
> @property (getter=isPresenting) BOOL presenting;
> 
> I think we use CGRects in UIKit code.

Done. All NSRects replaced with CGRects.

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

Switched to RetainPtr<> everywhere. And switched autorelease to adoptNS() here.

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

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:334
> > +    CGAffineTransform translateTransform = CGAffineTransformMakeTranslation(CGRectGetMidX(inlineFrame)-CGRectGetMidX(fullscreenFrame), CGRectGetMidY(inlineFrame)-CGRectGetMidY(fullscreenFrame));
> 
> Spaces around -

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:336
> > +    resetOriginTransform = CGAffineTransformMakeTranslation(0, 0);
> 
> This is just CGAffineTransformIdentity.

Done. Actually removed, since this was test code.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:344
> > +#if BOXES
> 
> Better name please.

Gone.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:348
> > +        inlineRepView = [[UIView alloc] initWithFrame:inlineFrame];
> 
> leaked.

Gone.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:353
> > +        fullscreenRepView = [[UIView alloc] initWithFrame:fullscreenFrame];
> 
> leaked.

Gone.

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

It means animating "in" as opposed to animating "out".

Changed to:
@property (nonatomic, getter=isAnimatingIn) BOOL animatingIn;

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:387
> > +    WebKit::WebPageProxy* _page;
> 
> Does it need to store this separate from the view?

WKWebViewInternal has -[WKWebView _page], so I can get rid of this.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:398
> > +    NSRect _initialFrame;
> > +    NSRect _finalFrame;
> 
> CGRects.

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:442
> > +    _fullscreenViewController = [[_WKFullScreenViewController alloc] init];
> 
> leak

Switch to RetainPtr<> and adoptNS().

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

This is now moved to the top of the function.

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

Added:
     [[_webViewPlaceholder layer] setName:@"Fullscreen Placeholder View"];

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

Ok, I added:
        RetainPtr<UIWindow> webWindow = [_webView window];
And we use webWindow instead.

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

Changed that to:
[protectedSelf = RetainPtr<WKFullScreenWindowController>(self)]

Because c++ lambdas don't automatically retain referenced objc objects.


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

This is an objc-c block, it should automatically retain referenced obj-c objects.

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

Same fix as above:
Changed that to:
[protectedSelf = RetainPtr<WKFullScreenWindowController>(self)]

Because c++ lambdas don't automatically retain referenced objc objects.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:606
> > +    WKFullscreenAnimationController* ac = [[WKFullscreenAnimationController alloc] init];
> 
> Don't use short names like "ac"

s/ac/animationController/

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:616
> > +    WKFullscreenAnimationController* ac = [[WKFullscreenAnimationController alloc] init];
> 
> Ditto.

Ditto.

[Jeremy Jones]

Simon, these two are for the changes specific to this code:

https://bugs.webkit.org/show_bug.cgi?id=179028 iOS Element fullscreen done button should handle RTL languages.
https://bugs.webkit.org/show_bug.cgi?id=179029 iOS element fullscreen should use a UIGestureRecognizer to detect user interaction.

[Jeremy Jones]

Add three more that affect the other Platform's implementations too:

https://bugs.webkit.org/show_bug.cgi?id=179030 Element fullscreen PageClientImpl::closeFullScreenManager sounds weird
https://bugs.webkit.org/show_bug.cgi?id=179032 Mac WKFullScreenWindowController doesn't need a ref to _page, it can get it from the WebView.
https://bugs.webkit.org/show_bug.cgi?id=179033 Element fullscreen should use FloatRects instead of IntRects in beganEnterFullScreenWithInitialFrame

[Jeremy Jones]

Created attachment 325420 [details]
Patch

[Build Bot]

Attachment 325420 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:258:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:258:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:339:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:339:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:431:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:431:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 325420 [details]
Patch

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

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

CGRects?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:120
> +- (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event
> +{
> +    [[self target] performSelector:[self action]];
> +    return nil;
> +}

I think it's too fragile to use this. UIKIt could change at any time and add a new call to this which would break fullscreen. You really need to figure out if it's an actual user gesture.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:188
> +    [backLayerTintView release];

Nope.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:195
> +    [topLayerTintView release];

Nope.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:204
> +    [self setView:[[[UIView alloc] initWithFrame:[[UIScreen mainScreen] bounds]] autorelease]];

We prefer to avoid autorelease, and use RetainPtr<>

[Simon Fraser (smfr)]

Also, let's figure out how to test this.

[Jeremy Jones]

(In reply to Simon Fraser (smfr) from comment #22)
> Comment on attachment 325420 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325420&action=review
> 
> > 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;
> 
> CGRects?

Sure, and I'll file a bug to update the Mac version to use NSRects.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:120
> > +- (UIView *)hitTest:(CGPoint)point withEvent:(UIEvent *)event
> > +{
> > +    [[self target] performSelector:[self action]];
> > +    return nil;
> > +}
> 
> I think it's too fragile to use this. UIKIt could change at any time and add
> a new call to this which would break fullscreen. You really need to figure
> out if it's an actual user gesture.

I'll see what I can do...

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:188
> > +    [backLayerTintView release];
> 
> Nope.

Gone.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:195
> > +    [topLayerTintView release];
> 
> Nope.

Gone.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:204
> > +    [self setView:[[[UIView alloc] initWithFrame:[[UIScreen mainScreen] bounds]] autorelease]];
> 
> We prefer to avoid autorelease, and use RetainPtr<>

adoptNS(...).get() seems to be the pattern used elsewhere.

[Jeremy Jones]

Created attachment 325663 [details]
Patch for landing.

[Build Bot]

Attachment 325663 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:256:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:256:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:337:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:337:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:429:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:429:  Missing space before {  [whitespace/braces] [5]
Total errors found: 6 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

(In reply to Keith Miller from comment #14)
> Comment on attachment 325191 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325191&action=review
> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:10860
> > +				3FFDDB971F5780E10050E593 /* WKFullScreenWindowControllerIOS.mm in Sources */,
> 
> It looks like you're compiling this standalone. Can you add it to
> SourcesIOS.txt?

This file is for WebKit: 
WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm

The only SourcesIOS.txt I find is Source/WebCore/SourcesIOS.txt and it looks like it is only for WebCore files.

Am I really supposed to add it here or is there another a separate file somewhere for WebKit?

[WebKit Commit Bot]

Comment on attachment 325663 [details]
Patch for landing.

Clearing flags on attachment: 325663

Committed r224313: <https://trac.webkit.org/changeset/224313>

[Jeremy Jones]

*** Bug 178921 has been marked as a duplicate of this bug. ***