Bug 156089 – Add some logic to decide when a video can control the videoControlsManager

• Component: Media
• Status: RESOLVED FIXED

[Beth Dakin]

Add some logic to decide when a video can control the videoControlsManager

rdar://problem/23833752

[Beth Dakin]

Created attachment 275360 [details]
Patch

[Beth Dakin]

Created attachment 275421 [details]
Patch

[Eric Carlson]

Comment on attachment 275421 [details]
Patch

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

This looks good to me modulo the minor nits noted, but I am not a WK2 reviewer so someone else will have to give the official nod.

> Source/WebCore/html/HTMLMediaElement.cpp:4847
> +        if (endedPlayback() && document().page() && is<HTMLVideoElement>(*this))

As we discussed, this will cause issues when a video's src is changed in the 'ended' event to make a playlist. Probably worth filing a bug to investigate this.

> Source/WebCore/html/MediaElementSession.cpp:219
> +    // FIXME: Audio elements should be able to have a controls manager as well. Audio elements
> +    // should probably only have a controls manager if they started playing via a user gesture.

It would be good to have a bug number here.

> Source/WebCore/html/MediaElementSession.cpp:233
> +    if (renderer->clientWidth() >= elementMainContentMinimumWidth && renderer->clientHeight() >= elementMainContentMinimumHeight) {
> +        if (element.hasAudio() && element.hasVideo())
> +            return true;
> +    }

Do you care if the <video> is visible in the page or not?

[Beth Dakin]

Thanks Eric and Tim!

(In reply to comment #3)
> Comment on attachment 275421 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275421&action=review
> 
> This looks good to me modulo the minor nits noted, but I am not a WK2
> reviewer so someone else will have to give the official nod.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4847
> > +        if (endedPlayback() && document().page() && is<HTMLVideoElement>(*this))
> 
> As we discussed, this will cause issues when a video's src is changed in the
> 'ended' event to make a playlist. Probably worth filing a bug to investigate
> this.
> 

YouTube playlists do seem to work. Can you think of another playlist that might fail? I do want to file a bug, but I would like to have an example.

> > Source/WebCore/html/MediaElementSession.cpp:219
> > +    // FIXME: Audio elements should be able to have a controls manager as well. Audio elements
> > +    // should probably only have a controls manager if they started playing via a user gesture.
> 
> It would be good to have a bug number here.
> 

Added one: rdar://problem/25537071

> > Source/WebCore/html/MediaElementSession.cpp:233
> > +    if (renderer->clientWidth() >= elementMainContentMinimumWidth && renderer->clientHeight() >= elementMainContentMinimumHeight) {
> > +        if (element.hasAudio() && element.hasVideo())
> > +            return true;
> > +    }
> 
> Do you care if the <video> is visible in the page or not?

I think I might…still thinking this over actually. I can imagine kind of see it either way. If the video is still playing and the audio is still happening, it seems like it shouldn't matter that it has been scroll offscreen? But if the video stopped because it was scrolled away, then then we do care. We'll have to think this through.

Thank you!

http://trac.webkit.org/changeset/199022