Bug 150906 – [iOS] Fullscreen -> PiP should resume to Fullscreen, not inline

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[iOS] Fullscreen -> PiP should resume to Fullscreen, not inline

[Jer Noble]

Created attachment 264883 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1435:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[Jer Noble]

Created attachment 264901 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1435:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[Jon Lee]

rdar://problem/20596271

[Eric Carlson]

Comment on attachment 264901 [details]
Patch

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

This looks fine to me, but a WK2 reviewer will need to give it an official r+.

> Source/WebCore/ChangeLog:22
> +        Add methods to for clients to request a specific fullscreen mode, and to query whether
> +        the page is currently visible.

Nit: "to for clients"?

[Simon Fraser (smfr)]

Comment on attachment 264901 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        When "restoring" the user interface when exiting PiP, we should return to Fullscreen if that

"restoring" reads like butchers quotes.

> Source/WebCore/html/HTMLVideoElement.cpp:405
> +    LOG(Media, "HTMLVideoElement::webkitSetPresentationMode(%p) - setting to \"%s\"", this, mode.utf8().data());

You could LOG_STREAM(Media, stream << "HTMLVideoElement::webkitSetPresentationMode(" << this << ") - setting to " << mode);

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:657
>      if ([_videoSublayer superlayer] != self)
>          return;
>  
> -    [_videoSublayer setPosition:CGPointMake(CGRectGetMidX(bounds), CGRectGetMidY(bounds))];
> -    
>      if (![_avPlayerController delegate])
>          return;

Would be nice to wrap these checks in a function that describes what state you expect these to be true in.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:838
> +    WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];

Do we know that [pipView layer] is always a WebAVPlayerLayer? Could some other framework break this under us?

[Jer Noble]

(In reply to comment #7)
> Comment on attachment 264901 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264901&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        When "restoring" the user interface when exiting PiP, we should return to Fullscreen if that
> 
> "restoring" reads like butchers quotes.

Okay, I'll "remove" them.

> > Source/WebCore/html/HTMLVideoElement.cpp:405
> > +    LOG(Media, "HTMLVideoElement::webkitSetPresentationMode(%p) - setting to \"%s\"", this, mode.utf8().data());
> 
> You could LOG_STREAM(Media, stream <<
> "HTMLVideoElement::webkitSetPresentationMode(" << this << ") - setting to "
> << mode);

Nice!

> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:657
> >      if ([_videoSublayer superlayer] != self)
> >          return;
> >  
> > -    [_videoSublayer setPosition:CGPointMake(CGRectGetMidX(bounds), CGRectGetMidY(bounds))];
> > -    
> >      if (![_avPlayerController delegate])
> >          return;
> 
> Would be nice to wrap these checks in a function that describes what state
> you expect these to be true in.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:838
> > +    WebAVPlayerLayer *pipPlayerLayer = (WebAVPlayerLayer *)[pipView layer];
> 
> Do we know that [pipView layer] is always a WebAVPlayerLayer? Could some
> other framework break this under us?

Yes. The PiP view will always have a WebAVPlayerLayer because WebAVPictureInPicturePlayerLayerView has a -layerClass member which returns [WebAVPlayerLayer class].

[Jer Noble]

Created attachment 266482 [details]
Patch for landing

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1434:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


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

[WebKit Commit Bot]

Comment on attachment 266482 [details]
Patch for landing

Clearing flags on attachment: 266482

Committed r193340: <http://trac.webkit.org/changeset/193340>