[webkit-reviews] review granted: [Bug 183503] Improvements to fullscreen; new UI and security features : [Attachment 335484] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 9 17:33:27 PST 2018
Dean Jackson <dino at apple.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 183503: Improvements to fullscreen; new UI and security features
https://bugs.webkit.org/show_bug.cgi?id=183503
Attachment 335484: Patch
https://bugs.webkit.org/attachment.cgi?id=335484&action=review
--- Comment #7 from Dean Jackson <dino at apple.com> ---
Comment on attachment 335484
--> https://bugs.webkit.org/attachment.cgi?id=335484
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
> + at interface WKFullScreenViewController () <UIGestureRecognizerDelegate,
UIToolbarDelegate>
> + at property (assign, nonatomic) WKWebView *_webView; // Cannot be retained,
see <rdar://problem/14884666>.
> + at property (readonly, nonatomic) WebFullScreenManagerProxy* _manager;
> + at property (readonly, nonatomic) CGFloat _effectiveFullscreenInsetTop;
> + at 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
> + at 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>)tr
ansitionContext
> {
> - [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
> + at property (nonatomic, retain, nullable) UIView
*targetViewForSecondaryMaterialOverlay;
> + at 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
> + at synthesize _stackView=_stackView;
> + at 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.
More information about the webkit-reviews
mailing list