Bug 183503 – Improvements to fullscreen; new UI and security features

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Improvements to fullscreen; new UI and security features

[Jer Noble]

Created attachment 335404 [details]
Patch

[Jer Noble]

Created attachment 335472 [details]
Patch

[Jer Noble]

Created attachment 335479 [details]
Patch

[Jer Noble]

Created attachment 335483 [details]
Patch

[Jer Noble]

Created attachment 335484 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:410:  The parameter name "height" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:65:  Declaration has space between type name and * in m_xWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:66:  Declaration has space between type name and * in m_yWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:68:  Declaration has space between type name and * in m_xWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:69:  Declaration has space between type name and * in m_yWeight * pow  [whitespace/declaration] [3]
Total errors found: 5 in 12 files


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

[Dean Jackson]

Comment on attachment 335484 [details]
Patch

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

> Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:27
> +#include "FullscreenTouchSecheuristic.h"

As mentioned on IRC, i thought this name was a typo.

> Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:29
> +#if ENABLE(FULLSCREEN_API) && PLATFORM(IOS)

Is FULLSCREEN_API disabled for IOS_SIMULATOR and MINIMAL_SIMULATOR? And watchOS and tvOS?

> Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:72
> +        return scaledDistance * (m_rampUpSpeed / deltaTime);

Does m_rampUpSpeed ever change?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.h:36
> +namespace WebKit {
> +class WebPageProxy;
> +}

Never used.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:61
> +    void setParent(WKFullScreenViewController *parent) { m_parent = parent; }
> +
> +    void rateChanged(bool isPlaying, float) override
> +    {
> +        m_parent.playing = isPlaying;
> +    }
> +
> +    void pictureInPictureActiveChanged(bool active) override
> +    {
> +        m_parent.pictureInPictureActive = active;
> +    }

Probably should guard for m_parent being nullptr, since it isn't required in the constructor.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:65
> +    void setInterface(PlaybackSessionInterfaceAVKit* interface)
> +    {
> +        if (m_interface && m_interface->playbackSessionModel())

Check m_interface != interface?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:93
> +- (CGSize)intrinsicContentSize
> +{
> +    return _extrinsicContentSize;
> +}

I thought you didn't need this, even when you provide the setter?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:102
> +@interface WKFullScreenViewController () <UIGestureRecognizerDelegate, UIToolbarDelegate>
> +@property (assign, nonatomic) WKWebView *_webView; // Cannot be retained, see <rdar://problem/14884666>.
> +@property (readonly, nonatomic) WebFullScreenManagerProxy* _manager;
> +@property (readonly, nonatomic) CGFloat _effectiveFullscreenInsetTop;
> +@end

Is this public, or an extension? If it is an internal-only extension, I'm not sure if we use () or (Private) any more. Probably ().

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:109
> +    RetainPtr<_WKExtrinsicButton> _cancelButton;
> +    RetainPtr<_WKExtrinsicButton> _pipButton;
> +    RetainPtr<UIButton> _locationButton;

Do these need to be retained? (I never know)

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:129
> +    _secheuristic.setRampUpSpeed(Seconds(0.25));
> +    _secheuristic.setRampDownSpeed(Seconds(1.));

If these are the only places that set the values, it might be better to have them not-settable and just use constants?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:146
> +    _playbackClient.setParent(nullptr);

It's just a pointer object in WKFullScreenViewControllerPlaybackSessionModelClient, so no real need to do this here.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:159
> +    [UIView animateWithDuration:showHideAnimationDuration animations: ^{

Nit: No space before ^

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:189
> +    PlaybackSessionManagerProxy* playbackSessionManager = page ? page->playbackSessionManager() : NULL;

nullptr

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:190
> +    PlatformPlaybackSessionInterface* playbackSessionInterface = playbackSessionManager ? playbackSessionManager->controlsManagerInterface() : NULL;

nullptr

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:193
> +    PlaybackSessionModel* playbackSessionModel = playbackSessionInterface ? playbackSessionInterface->playbackSessionModel() : NULL;

nullptr

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:200
> +@synthesize prefersStatusBarHidden=_prefersStatusBarHidden;
> +- (void)setPrefersStatusBarHidden:(BOOL)value

Nit: blank line.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:219
> +    if (!_playing)
> +        [self showUI];
> +    else {
> +        [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(hideUI) object:nil];
> +        NSTimeInterval hideDelay = autoHideDelay;
> +        [self performSelector:@selector(hideUI) withObject:nil afterDelay:hideDelay];
> +    }

don't we want to cancel any hideUI requests that were started when we move to the !_playing stage?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:235
> +    [self setView:adoptNS([[UIView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)]).get()];

Was the idea here that the created RetainPtr stays alive until the end of loadView?

I'm not sure why you call adoptNS if your only reference is inside self.view.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:246
> +    [_cancelButton setTintColor:[UIColor whiteColor]];

Do we always want to hard-code the tint? What is the default?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:269
> +    UILayoutGuide* safeArea = self.view.safeAreaLayoutGuide;
> +    UILayoutGuide* margins = self.view.layoutMarginsGuide;

* on the other side

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:311
> +    [coordinator animateAlongsideTransition: ^(id<UIViewControllerTransitionCoordinatorContext> context) {

Nit: no space before ^

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:316
> +        void (^webViewUpdateBlock)() = ^{
> +            [self._webView _overrideLayoutParametersWithMinimumLayoutSize:size maximumUnobscuredSizeOverride:size];
> +        };
> +
> +        [self._webView _beginAnimatedResizeWithUpdates:webViewUpdateBlock];

Why make the local variable?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:334
> +#pragma mark -
> +#pragma mark UIGestureRecognizerDelegate

You did this with one pragma above.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:349
> +#pragma mark -
> +
> +#pragma mark -

Why two?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:426
> +    NSString *alertTitle = @"Deceptive Website Warning";
> +    NSString *alertMessage = [NSString stringWithFormat:@"The website \"%@\" may be a deceptive website. Would you like to exit fullscreen?", (NSString *)self.location];

Needs to be localizable.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:431
> +    UIAlertAction* defaultAction = [UIAlertAction actionWithTitle:@"Exit Fullscreen" style:UIAlertActionStyleDefault handler:^(UIAlertAction * action) {
> +        [self _cancelAction:action];
> +    }];

It's confusing that the exit button calls cancelAction, but the next UIAlertAction is called cancelAction. 
Shouldn't this just be calling exit full screen directly?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:146
> +- (NSTimeInterval)transitionDuration:(id<UIViewControllerContextTransitioning>)transitionContext
>  {
> -    [NSObject cancelPreviousPerformRequestsWithTarget:self];
> -    [[NSNotificationCenter defaultCenter] removeObserver:self];
> -
> -    [super dealloc];
> +    const NSTimeInterval animationDuration = 0.2;
> +    return animationDuration;
>  }

Is there any reason to have this as a function?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:185
> +    _maskView = adoptNS([[UIView alloc] init]);
> +    [_maskView setBackgroundColor:[UIColor blackColor]];
> +    [_maskView setBounds:_initialMaskViewBounds];
> +    [_maskView setCenter:_initialMaskViewCenter];

You only need to create this once (I assume this instance can stay around)

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:195
> +    [UIView animateWithDuration:[self transitionDuration:transitionContext] delay:0 options:UIViewAnimationOptionCurveEaseInOut animations:^{

Yeah, maybe just use a static const for duration. And do we want ease in out over a regular ease?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:206
> +    if (([self isAnimatingIn] && !transitionCompleted) || (![self isAnimatingIn] && transitionCompleted))
> +        [_animatingView removeFromSuperview];

Do you need to actually test? You're about to set the RetainPtr to nil, so it will get deleted anyway.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:210
> +    _maskView = nil;
> +    _animatingView = nil;

Right, so here is where you nuke the views, but you could also just keep them around. You know that if you see one animation, you're almost certainly going to see another (the exit fullscreen).

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:220
> +    _maskView = adoptNS([[UIView alloc] init]);
> +    [_maskView setBackgroundColor:[UIColor blackColor]];
> +    [_maskView setBounds:_initialMaskViewBounds];
> +    [_maskView setCenter:_initialMaskViewCenter];

Maybe there should be a helper to create the mask view, since you do it more than once. Especially if you reuse it.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:228
> +- (void)updateWithPercent:(CGFloat)percent

I thought other UIView animations always talked about progress as [0,1]. It's a bit strange that we use percent, but still mean [0,1] not [0,100].

Maybe this should just be setProgress:

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:270
> +-(void)end:(BOOL)cancelled {

Nit: space after -

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:306
> +    self = [super init];
> +    if (!self)
> +        return nil;

We tend to do this:

    if (!(self = [super init]))
        return nil;

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:327
> +- (void)updateInteractiveTransition:(CGFloat)percentComplete withTranslation:(CGSize)translation
>  {
> -    return !_showsStatusBar;
> +    [_animator updateWithPercent:percentComplete translation:translation anchor:_anchor];
> +    [_context updateInteractiveTransition:percentComplete];

Again, I think other APIs just use "progress"

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:427
> +    _rootViewController.get().view.backgroundColor = [UIColor clearColor];
> +    _rootViewController.get().view.autoresizingMask = (UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight);

Other places in this patch you use [] syntax to avoid the .get()

[[_rootViewController view] setBackgroundColor:]

I agree that looks ugly.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:439
> +    [[_fullscreenViewController view] setFrame:[[_rootViewController view] bounds]];

... but here's an example.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:447
> +    [[_fullscreenViewController view] addGestureRecognizer:_startDismissGestureRecognizer.get()];

... and here

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:452
> +    [[_fullscreenViewController view] addGestureRecognizer:_interactiveDismissGestureRecognizer.get()];

... and here.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:461
> +    [[_webViewPlaceholder layer] setName:@"Fullscreen Placeholder View"];

Again, need localization.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:636
> +    if (auto* page = [_webView _page])
> +        [_webView _page]->forceRepaint(_repaintCallback.copyRef());

Just page->forceRepaint(...

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:804
> +    if (LinkPresentationLibrary())
> +        domain = [url _lp_simplifiedDisplayString];

Interesting!

> Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:810
> +        text = @"data:";

Do we expect anyone to understand what this means?

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.h:40
> +@property (nonatomic, retain, nullable) UIView *targetViewForSecondaryMaterialOverlay;
> +@property (nonatomic, readonly) UIView *contentView;

Add a blank line above these.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:42
> +        effects.get() = @[[UIVisualEffect effectCompositingColor:[UIColor colorWithRed:(43.0 / 255.0) green:(46.0 / 255.0) blue:(48.0 / 255.0) alpha:1.0] withMode:UICompositingModeNormal alpha:1.0]];

Where does this magic come from? We should get the color from somewhere rather than hardcode it.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:56
> +            [UIVisualEffect effectCompositingColor:[UIColor blackColor] withMode:UICompositingModeNormal alpha:0.55],
> +            [UIBlurEffect effectWithBlurRadius:UIRoundToScreenScale(17.5, [UIScreen mainScreen])],
> +            [UIColorEffect colorEffectSaturate:1.8],
> +            [UIVisualEffect effectCompositingColor:[UIColor whiteColor] withMode:UICompositingModeNormal alpha:0.14]

Same here.

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:163
> +@synthesize _stackView=_stackView;
> +@synthesize _visualEffectView=_visualEffectView;

Why do you need these? Can't they just be @synthesize _stackView;

> Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:185
> +        [self _setContinuousCornerRadius:((CGRectGetHeight(bounds) > 40.0) ? 16.0 : 8.0)];

More magic numbers.

[Jer Noble]

(In reply to Dean Jackson from comment #7)
> Comment on attachment 335484 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335484&action=review
> 
> > Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:72
> > +        return scaledDistance * (m_rampUpSpeed / deltaTime);
> 
> Does m_rampUpSpeed ever change?

It could, conceivably, change. It currently doesn't.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.h:36
> > +namespace WebKit {
> > +class WebPageProxy;
> > +}
> 
> Never used.

Removed.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:61
> > +    void setParent(WKFullScreenViewController *parent) { m_parent = parent; }
> > +
> > +    void rateChanged(bool isPlaying, float) override
> > +    {
> > +        m_parent.playing = isPlaying;
> > +    }
> > +
> > +    void pictureInPictureActiveChanged(bool active) override
> > +    {
> > +        m_parent.pictureInPictureActive = active;
> > +    }
> 
> Probably should guard for m_parent being nullptr, since it isn't required in
> the constructor.

Since it's an Objective-C object, it being nullptr isn't dangerous.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:65
> > +    void setInterface(PlaybackSessionInterfaceAVKit* interface)
> > +    {
> > +        if (m_interface && m_interface->playbackSessionModel())
> 
> Check m_interface != interface?

Done.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:93
> > +- (CGSize)intrinsicContentSize
> > +{
> > +    return _extrinsicContentSize;
> > +}
> 
> I thought you didn't need this, even when you provide the setter?

This is an overridden UIView method, -intrinsicContentSize (vs. -extrinsicContentSize).

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:102
> > +@interface WKFullScreenViewController () <UIGestureRecognizerDelegate, UIToolbarDelegate>
> > +@property (assign, nonatomic) WKWebView *_webView; // Cannot be retained, see <rdar://problem/14884666>.
> > +@property (readonly, nonatomic) WebFullScreenManagerProxy* _manager;
> > +@property (readonly, nonatomic) CGFloat _effectiveFullscreenInsetTop;
> > +@end
> 
> Is this public, or an extension? If it is an internal-only extension, I'm
> not sure if we use () or (Private) any more. Probably ().

It's an internal-only extension.  It has to be () or the automatic @property generation stuff doesn't work.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:109
> > +    RetainPtr<_WKExtrinsicButton> _cancelButton;
> > +    RetainPtr<_WKExtrinsicButton> _pipButton;
> > +    RetainPtr<UIButton> _locationButton;
> 
> Do these need to be retained? (I never know)

Probably not, since they're retained by the views that they're added to, but I really would hate to have that not be true in the future.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:129
> > +    _secheuristic.setRampUpSpeed(Seconds(0.25));
> > +    _secheuristic.setRampDownSpeed(Seconds(1.));
> 
> If these are the only places that set the values, it might be better to have
> them not-settable and just use constants?

I am imagining some diagnostic UI that lets you change the values. I'm also imagining running the heuristic over a large set of simulated data and tweaking all the settings to find the "best" settings for that data set.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:146
> > +    _playbackClient.setParent(nullptr);
> 
> It's just a pointer object in
> WKFullScreenViewControllerPlaybackSessionModelClient, so no real need to do
> this here.

Good point, but now that you mention it, we definitely need to remove the _playbackClient as a client of whatever it's current interface is. Will change.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:159
> > +    [UIView animateWithDuration:showHideAnimationDuration animations: ^{
> 
> Nit: No space before ^

Removed.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:189
> > +    PlaybackSessionManagerProxy* playbackSessionManager = page ? page->playbackSessionManager() : NULL;
> 
> nullptr

Changed.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:190
> > +    PlatformPlaybackSessionInterface* playbackSessionInterface = playbackSessionManager ? playbackSessionManager->controlsManagerInterface() : NULL;
> 
> nullptr

Ditto.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:193
> > +    PlaybackSessionModel* playbackSessionModel = playbackSessionInterface ? playbackSessionInterface->playbackSessionModel() : NULL;
> 
> nullptr

Ditto.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:200
> > +@synthesize prefersStatusBarHidden=_prefersStatusBarHidden;
> > +- (void)setPrefersStatusBarHidden:(BOOL)value
> 
> Nit: blank line.

Like, you want a blank line between the two? Added.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:219
> > +    if (!_playing)
> > +        [self showUI];
> > +    else {
> > +        [NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(hideUI) object:nil];
> > +        NSTimeInterval hideDelay = autoHideDelay;
> > +        [self performSelector:@selector(hideUI) withObject:nil afterDelay:hideDelay];
> > +    }
> 
> don't we want to cancel any hideUI requests that were started when we move
> to the !_playing stage?

-showUI will do that.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:235
> > +    [self setView:adoptNS([[UIView alloc] initWithFrame:CGRectMake(0, 0, 100, 100)]).get()];
> 
> Was the idea here that the created RetainPtr stays alive until the end of
> loadView?
>
> I'm not sure why you call adoptNS if your only reference is inside self.view.

No, just till the end of the call. It's a inline way of avoiding doing -autorelease when you're doing +alloc, -init.
 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:246
> > +    [_cancelButton setTintColor:[UIColor whiteColor]];
> 
> Do we always want to hard-code the tint? What is the default?

"If the system cannot find a nondefault color in the hierarchy, this property’s value is a system-defined color instead."  So it depends on the system.  So yes, I think we want to hard-code this.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:269
> > +    UILayoutGuide* safeArea = self.view.safeAreaLayoutGuide;
> > +    UILayoutGuide* margins = self.view.layoutMarginsGuide;
> 
> * on the other side

Swapped.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:311
> > +    [coordinator animateAlongsideTransition: ^(id<UIViewControllerTransitionCoordinatorContext> context) {
> 
> Nit: no space before ^

Removed.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:316
> > +        void (^webViewUpdateBlock)() = ^{
> > +            [self._webView _overrideLayoutParametersWithMinimumLayoutSize:size maximumUnobscuredSizeOverride:size];
> > +        };
> > +
> > +        [self._webView _beginAnimatedResizeWithUpdates:webViewUpdateBlock];
> 
> Why make the local variable?

No good reason.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:334
> > +#pragma mark -
> > +#pragma mark UIGestureRecognizerDelegate
> 
> You did this with one pragma above.

Will collapse.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:349
> > +#pragma mark -
> > +
> > +#pragma mark -
> 
> Why two?

No good reason.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:426
> > +    NSString *alertTitle = @"Deceptive Website Warning";
> > +    NSString *alertMessage = [NSString stringWithFormat:@"The website \"%@\" may be a deceptive website. Would you like to exit fullscreen?", (NSString *)self.location];
> 
> Needs to be localizable.

Good point! Will WEB_UI_STRING() all these strings.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:431
> > +    UIAlertAction* defaultAction = [UIAlertAction actionWithTitle:@"Exit Fullscreen" style:UIAlertActionStyleDefault handler:^(UIAlertAction * action) {
> > +        [self _cancelAction:action];
> > +    }];
> 
> It's confusing that the exit button calls cancelAction, but the next
> UIAlertAction is called cancelAction. 

Renamed these to "exitAction" and "stayAction"

> Shouldn't this just be calling exit full screen directly?

That's a function of the window controller, which is why the "-action" property exists now.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:146
> > +- (NSTimeInterval)transitionDuration:(id<UIViewControllerContextTransitioning>)transitionContext
> >  {
> > -    [NSObject cancelPreviousPerformRequestsWithTarget:self];
> > -    [[NSNotificationCenter defaultCenter] removeObserver:self];
> > -
> > -    [super dealloc];
> > +    const NSTimeInterval animationDuration = 0.2;
> > +    return animationDuration;
> >  }
> 
> Is there any reason to have this as a function?

It's an override of the UIViewControllerAnimatedTransitioning protocol.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:185
> > +    _maskView = adoptNS([[UIView alloc] init]);
> > +    [_maskView setBackgroundColor:[UIColor blackColor]];
> > +    [_maskView setBounds:_initialMaskViewBounds];
> > +    [_maskView setCenter:_initialMaskViewCenter];
> 
> You only need to create this once (I assume this instance can stay around)

Ok.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:195
> > +    [UIView animateWithDuration:[self transitionDuration:transitionContext] delay:0 options:UIViewAnimationOptionCurveEaseInOut animations:^{
> 
> Yeah, maybe just use a static const for duration. And do we want ease in out
> over a regular ease?

Like I said above, we need to implement the protocol method, but they can both use the same static const. We use EaseInEaseOut on Mac, so I planned on doing the same here until HI told me otherwise.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:206
> > +    if (([self isAnimatingIn] && !transitionCompleted) || (![self isAnimatingIn] && transitionCompleted))
> > +        [_animatingView removeFromSuperview];
> 
> Do you need to actually test? You're about to set the RetainPtr to nil, so
> it will get deleted anyway.

No, it won't, we'll just un-ref it. The animating view is just the presented view controller's view. The idea is that if the animation needs to reverse, it's less likely to glitch if you just leave it attached.

Also, keep in mind that this code was just moved around, and isn't new.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:210
> > +    _maskView = nil;
> > +    _animatingView = nil;
> 
> Right, so here is where you nuke the views, but you could also just keep
> them around. You know that if you see one animation, you're almost certainly
> going to see another (the exit fullscreen).

Good point, but this class just gets used for a single animation. A new WKFullscreenAnimationController gets created for each enter or dismiss animation.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:220
> > +    _maskView = adoptNS([[UIView alloc] init]);
> > +    [_maskView setBackgroundColor:[UIColor blackColor]];
> > +    [_maskView setBounds:_initialMaskViewBounds];
> > +    [_maskView setCenter:_initialMaskViewCenter];
> 
> Maybe there should be a helper to create the mask view, since you do it more
> than once. Especially if you reuse it.

Sure!

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:228
> > +- (void)updateWithPercent:(CGFloat)percent
> 
> I thought other UIView animations always talked about progress as [0,1].
> It's a bit strange that we use percent, but still mean [0,1] not [0,100].

Sure, we can change it to progress.

> Maybe this should just be setProgress:
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:270
> > +-(void)end:(BOOL)cancelled {
> 
> Nit: space after -
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:306
> > +    self = [super init];
> > +    if (!self)
> > +        return nil;
> 
> We tend to do this:
> 
>     if (!(self = [super init]))
>         return nil;
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:327
> > +- (void)updateInteractiveTransition:(CGFloat)percentComplete withTranslation:(CGSize)translation
> >  {
> > -    return !_showsStatusBar;
> > +    [_animator updateWithPercent:percentComplete translation:translation anchor:_anchor];
> > +    [_context updateInteractiveTransition:percentComplete];
> 
> Again, I think other APIs just use "progress"
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:427
> > +    _rootViewController.get().view.backgroundColor = [UIColor clearColor];
> > +    _rootViewController.get().view.autoresizingMask = (UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight);
> 
> Other places in this patch you use [] syntax to avoid the .get()
> 
> [[_rootViewController view] setBackgroundColor:]
> 
> I agree that looks ugly.
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:439
> > +    [[_fullscreenViewController view] setFrame:[[_rootViewController view] bounds]];
> 
> ... but here's an example.

Yeah, I should use .get() syntax here.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:447
> > +    [[_fullscreenViewController view] addGestureRecognizer:_startDismissGestureRecognizer.get()];
> 
> ... and here
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:452
> > +    [[_fullscreenViewController view] addGestureRecognizer:_interactiveDismissGestureRecognizer.get()];
> 
> ... and here.

Dittos.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:461
> > +    [[_webViewPlaceholder layer] setName:@"Fullscreen Placeholder View"];
> 
> Again, need localization.

Really? This is debug info only.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:636
> > +    if (auto* page = [_webView _page])
> > +        [_webView _page]->forceRepaint(_repaintCallback.copyRef());
> 
> Just page->forceRepaint(...

Changed.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:804
> > +    if (LinkPresentationLibrary())
> > +        domain = [url _lp_simplifiedDisplayString];
> 
> Interesting!
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenWindowControllerIOS.mmSource/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:810
> > +        text = @"data:";
> 
> Do we expect anyone to understand what this means?

I think this code was taken wholesale from other places we display the EV name.

> > Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.h:40
> > +@property (nonatomic, retain, nullable) UIView *targetViewForSecondaryMaterialOverlay;
> > +@property (nonatomic, readonly) UIView *contentView;
> 
> Add a blank line above these.
> 
> > Source/WebKit/UIProcess/ios/fullscreen/WKFullscreenStackView.mm:163
> > +@synthesize _stackView=_stackView;
> > +@synthesize _visualEffectView=_visualEffectView;
> 
> Why do you need these? Can't they just be @synthesize _stackView;

If we don't @synthesize them, they'll end up with ivar names like __stackView, rather than _stackView.

[Jer Noble]

Created attachment 335504 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/WebKit/UIProcess/ios/fullscreen/WKFullScreenViewController.mm:409:  The parameter name "height" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:65:  Declaration has space between type name and * in m_xWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:66:  Declaration has space between type name and * in m_yWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:68:  Declaration has space between type name and * in m_xWeight * pow  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/ios/fullscreen/FullscreenTouchSecheuristic.cpp:69:  Declaration has space between type name and * in m_yWeight * pow  [whitespace/declaration] [3]
Total errors found: 5 in 10 files


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

[WebKit Commit Bot]

Comment on attachment 335504 [details]
Patch for landing

Clearing flags on attachment: 335504

Committed r229512: <https://trac.webkit.org/changeset/229512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38397853>