Bug 154834 – Update the media element's presentation mode properly after going in and out of full screen via the Full Screen API

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ada Chan]

If both FULLSCREEN_API and VIDEO_PRESENTATION_MODE are defined, make sure both functionalities coexist nicely with each other.  That means:
- If a media element goes into Full Screen via the webkitRequestFullscreen(), retrieving webkitPresentationMode afterwards should return "fullscreen".
- If a media element exits Full Screen via webkitExitFullscreen(), retrieving webkitPresentationMode afterwards should return "inline".

[Ada Chan]

Related to <rdar://problem/24172607>

[Ada Chan]

Created attachment 272642 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 272642 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5272
> +    if (document().settings() && document().settings()->fullScreenEnabled()) {

I don't think you need to null-check document().settings(). If we get here with null settings, something is seriously wrong.

> Source/WebCore/html/HTMLMediaElement.cpp:5280
> +        if (Element* fullscreenElement = document().webkitCurrentFullScreenElement()) {
> +            if (fullscreenElement->contains(this))
> +                document().webkitCancelFullScreen();

I think this deserves a comment, or perhaps if (mode == VideoFullscreenModePictureInPicture). I think it's doing "if entering a different fullscreen mode, get out of the current fullscreen", right?

> Source/WebCore/html/HTMLMediaElement.cpp:5339
> +    if (oldVideoFullscreenMode != VideoFullscreenModeNone && oldVideoFullscreenMode != VideoFullscreenModeStandard && document().page() && is<HTMLVideoElement>(*this)) {

Doesn't this mean if (oldVideoFullscreenMode == VideoFullscreenModePictureInPicture ?
Should not need to null-check document().page().

> Source/WebCore/html/HTMLMediaElement.cpp:5340
> +        if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode))

Why this supports check?

> Source/WebCore/html/HTMLMediaElement.cpp:5350
> +#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
> +void HTMLMediaElement::ancestorWillEnterFullscreen()
> +{

I would prefer to #ifdef the contents, not the entire member function. For example, you call this unguarded in Element::willBecomeFullscreenElement().

> Source/WebCore/html/HTMLMediaElement.cpp:5351
> +    if (m_videoFullscreenMode != VideoFullscreenModeNone && document().page() && is<HTMLVideoElement>(*this)) {

Don't null-check document().page().

The is<HTMLVideoElement>(*this) check suggests that you should move this implementation up to HTMLVideoElement.

> Source/WebCore/page/ChromeClient.h:346
> +    virtual void exitVideoFullscreenWithoutAnimationForVideoElement(WebCore::HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode /*targetMode*/) { }

Weird that exit... takes a targetMode. I think some naming needs to improve to clarify.

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.h:121
> +#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
> +    void exitVideoFullscreenWithoutAnimationForVideoElement(WebCore::HTMLVideoElement&, WebCore::HTMLMediaElementEnums::VideoFullscreenMode);
> +#endif

Would prefer to have the definition outside the #ifdef.

[Ada Chan]

(In reply to comment #3)
> Comment on attachment 272642 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272642&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5272
> > +    if (document().settings() && document().settings()->fullScreenEnabled()) {
> 
> I don't think you need to null-check document().settings(). If we get here
> with null settings, something is seriously wrong.

The null check was there before my change. But I'll remove it if you don't think it's necessary.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5280
> > +        if (Element* fullscreenElement = document().webkitCurrentFullScreenElement()) {
> > +            if (fullscreenElement->contains(this))
> > +                document().webkitCancelFullScreen();
> 
> I think this deserves a comment, or perhaps if (mode ==
> VideoFullscreenModePictureInPicture). I think it's doing "if entering a
> different fullscreen mode, get out of the current fullscreen", right?

Yes, I'll add the following comment:

If we are not going to standard fullscreen mode, exit full screen if this media element is contained in the current full screen element.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5339
> > +    if (oldVideoFullscreenMode != VideoFullscreenModeNone && oldVideoFullscreenMode != VideoFullscreenModeStandard && document().page() && is<HTMLVideoElement>(*this)) {
> 
> Doesn't this mean if (oldVideoFullscreenMode ==
> VideoFullscreenModePictureInPicture ?
> Should not need to null-check document().page().

As discussed offline, I'll change this to a switch statement. The null check for document().page() was done in similar code in HTMLMediaElement::exitFullscreen(). But I'll remove it if you don't think it's necessary.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5340
> > +        if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode))
> 
> Why this supports check?

Because we do a similar supports check before exiting fullscreen mode in HTMLMediaElement::exitFullscreen().

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5350
> > +#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
> > +void HTMLMediaElement::ancestorWillEnterFullscreen()
> > +{
> 
> I would prefer to #ifdef the contents, not the entire member function. For
> example, you call this unguarded in Element::willBecomeFullscreenElement().

OK, fixed.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5351
> > +    if (m_videoFullscreenMode != VideoFullscreenModeNone && document().page() && is<HTMLVideoElement>(*this)) {
> 
> Don't null-check document().page().
> 
> The is<HTMLVideoElement>(*this) check suggests that you should move this
> implementation up to HTMLVideoElement.

Good point. Moved.

> 
> > Source/WebCore/page/ChromeClient.h:346
> > +    virtual void exitVideoFullscreenWithoutAnimationForVideoElement(WebCore::HTMLVideoElement&, HTMLMediaElementEnums::VideoFullscreenMode /*targetMode*/) { }
> 
> Weird that exit... takes a targetMode. I think some naming needs to improve
> to clarify.

I renamed it to exitVideoFullscreenWithoutAnimationToMode.

> 
> > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.h:121
> > +#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
> > +    void exitVideoFullscreenWithoutAnimationForVideoElement(WebCore::HTMLVideoElement&, WebCore::HTMLMediaElementEnums::VideoFullscreenMode);
> > +#endif
> 
> Would prefer to have the definition outside the #ifdef.

Fixed.

Thanks for the feedback.  I'll post an updated patch soon.

[Ada Chan]

Created attachment 272676 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 272676 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5352
> +        if (is<HTMLVideoElement>(*this)) {
> +            if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode))
> +                document().page()->chrome().client().exitVideoFullscreenWithoutAnimationToMode(downcast<HTMLVideoElement>(*this), VideoFullscreenModeStandard);
> +        }

This code should also be up in HTMLVideoElement. It looks very similar to ancestorWillEnterFullscreen()

> Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.h:119
> +    void exitVideoFullscreenWithoutAnimationToMode(WebCore::HTMLVideoElement&, WebCore::HTMLMediaElementEnums::VideoFullscreenMode);

Name is a bit hard to parse. It could be:

exitVideoFullscreen (WithoutAnimation) ToMode
or
exitVideoFullscreen, WithoutAnimationToMode

[Ada Chan]

(In reply to comment #6)
> Comment on attachment 272676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272676&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5352
> > +        if (is<HTMLVideoElement>(*this)) {
> > +            if (document().page()->chrome().client().supportsVideoFullscreen(oldVideoFullscreenMode))
> > +                document().page()->chrome().client().exitVideoFullscreenWithoutAnimationToMode(downcast<HTMLVideoElement>(*this), VideoFullscreenModeStandard);
> > +        }
> 
> This code should also be up in HTMLVideoElement. It looks very similar to
> ancestorWillEnterFullscreen()

I'll add a new method in HTMLVideoElement that does this called:

void exitToFullscreenModeWithoutAnimationIfPossible(HTMLMediaElementEnums::VideoFullscreenMode fromMode, HTMLMediaElementEnums::VideoFullscreenMode toMode);

This will be called by both HTMLVideoElement::ancestorWillEnterFullscreen() and HTMLMediaElement::willBecomeFullscreenElement().

> 
> > Source/WebKit2/WebProcess/cocoa/WebVideoFullscreenManager.h:119
> > +    void exitVideoFullscreenWithoutAnimationToMode(WebCore::HTMLVideoElement&, WebCore::HTMLMediaElementEnums::VideoFullscreenMode);
> 
> Name is a bit hard to parse. It could be:
> 
> exitVideoFullscreen (WithoutAnimation) ToMode
> or
> exitVideoFullscreen, WithoutAnimationToMode

Renamed to exitVideoFullscreenToModeWithoutAnimation.

Thanks for the review!

[Ada Chan]

Created attachment 272700 [details]
Patch

[Ada Chan]

Committed:
http://trac.webkit.org/changeset/197478