HTMLMediaElement should exitFullscreen when view is removed from the window.

[Jeremy Jones]

HTMLMediaElement should exitFullscreen when view is removed from the window.

[Jeremy Jones]

Created attachment 230728 [details]
Patch

Comment on attachment 230728 [details]
Patch

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

> Source/WebCore/page/Page.cpp:887
> +#if PLATFORM(IOS)
> +    else {
> +        for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext())
> +            frame->dismissModalFullscreen();
> +    }
> +#endif

There shouldn’t be (and there currently isn’t) any platform-specific code in Page.cpp.

[Jeremy Jones]

Created attachment 230735 [details]
Patch

[Jeremy Jones]

(In reply to comment #2)
> (From update of attachment 230728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230728&action=review
> 
> > Source/WebCore/page/Page.cpp:887
> > +#if PLATFORM(IOS)
> > +    else {
> > +        for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext())
> > +            frame->dismissModalFullscreen();
> > +    }
> > +#endif
> 
> There shouldn’t be (and there currently isn’t) any platform-specific code in Page.cpp.

Removed #if PLATFORM(IOS) where possible.

[Simon Fraser (smfr)]

Comment on attachment 230735 [details]
Patch

Why doesn't this all happen via an exitFullscreen() code path? Bad to pollute Document with yet more fullscreen gunk.

[Jeremy Jones]

Created attachment 230876 [details]
Patch

[Jeremy Jones]

(In reply to comment #5)
> (From update of attachment 230735 [details])
> Why doesn't this all happen via an exitFullscreen() code path? Bad to pollute Document with yet more fullscreen gunk.

Per our conversation. This is now much simpler. WebView and WebPageProxy request exit-fullscreen from  WebVideoFullscreenController and WebVideoFullscreenManagerProxy respectively.

[Jeremy Jones]

Created attachment 230880 [details]
Patch

[Eric Carlson]

Comment on attachment 230880 [details]
Patch

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

This looks good to me but I am not a WK2 reviewer so someone else will need to r+.

> Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:203
>      WebThreadRun(^{
>          if (m_mediaElement->isFullscreen())

Nit: this can be done in a follow-up patch, but you should NULL-check m_mediaElement here as well.

[Tim Horton]

Comment on attachment 230880 [details]
Patch

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

WK2 part is fine with me, the rest has ericc's review.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1103
>  #endif
>  
> +#if PLATFORM(IOS)
> +    if ((mayHaveChanged & ViewState::IsInWindow) && !(m_viewState & ViewState::IsInWindow)) {
> +        // When leaving the current page, close the video fullscreen.
> +        if (m_videoFullscreenManager)
> +            m_videoFullscreenManager->requestExitFullscreen();
> +    }
> +#endif
> +
>      updateBackingStoreDiscardableState();

Merge these and put the #ifs inside the conditional, maybe?

[Eric Carlson]

Created attachment 231017 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 231017 [details]
Patch for landing.

Clearing flags on attachment: 231017

Committed r168439: <http://trac.webkit.org/changeset/168439>

[Jon Lee]

<rdar://problem/16777354>