Summary: | Update the media element's presentation mode properly after going in and out of full screen via the Full Screen API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ada Chan <adachan> | ||||||||
Component: | Media | Assignee: | Ada Chan <adachan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric.carlson, sam, simon.fraser, thorton | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Ada Chan
2016-02-29 13:48:14 PST
Related to <rdar://problem/24172607> Created attachment 272642 [details]
Patch
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. (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. Created attachment 272676 [details]
Patch
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 (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! Created attachment 272700 [details]
Patch
Committed: http://trac.webkit.org/changeset/197478 |