Bug 160068 – Media controls on apple.com don't disappear when movie finishes playing

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

[Wenson Hsieh]

Media controls on apple.com don't disappear when movie finishes playing

[Wenson Hsieh]

<rdar://problem/26668526>

[Wenson Hsieh]

Created attachment 284297 [details]
Patch

[Jeremy Jones]

Comment on attachment 284297 [details]
Patch

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

Once there is an ended event, from then on these controls are always hidden? That seems like a pretty strong rule. What if the src changes or what if we scrub back from the end and start playing again. Am I missing something or is that really the behavior we want?

Is it possible to have a streaming test case also since it has a different code path?

> Source/WebCore/html/HTMLMediaElement.cpp:4441
> +                m_hasEverSentEndEvent = true;

Seems like the streaming case also needs to add the behavior restriction.

[Wenson Hsieh]

The check for m_element.hasEverSentEndEvent() happens right before isElementLargeEnoughForMainContent(m_element). Before we call into this codepath, we've already established the following about the video:

- It is not currently ended
- The user has not gestured it (e.g. to play/pause)
- It is a <video> (with video)

Generally, this is enough to say that we should show media controls for it as long as it is large. This patch adds the additional constraint here that it must also not have ended once before, since if it has ended once already and the user has not gestured for it to begin again, we shouldn't be showing controls.

It the user has gestured again for the video to play, the check above for !hasBehaviorRestriction(RequireUserGestureToControlControlsManager) || ScriptController::processingUserGestureForMedia() will succeed, and we will show media controls.

(In reply to comment #3)
> Comment on attachment 284297 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284297&action=review
> 
> Once there is an ended event, from then on these controls are always hidden?
> That seems like a pretty strong rule. What if the src changes or what if we
> scrub back from the end and start playing again. Am I missing something or
> is that really the behavior we want?
> 
> Is it possible to have a streaming test case also since it has a different
> code path?

I'll add a test for the streaming case.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4441
> > +                m_hasEverSentEndEvent = true;
> 
> Seems like the streaming case also needs to add the behavior restriction.

Good catch. The restriction should apply here as well.

[Wenson Hsieh]

Created attachment 284386 [details]
Patch

[Jeremy Jones]

Need to rebase the patch, but it LGTM.

[Wenson Hsieh]

Created attachment 284387 [details]
Patch

[Wenson Hsieh]

Created attachment 284388 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 284388 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:926
> +    bool m_isDonePlaying : 1;

isDonePlaying is not a very good name, because it doesn't reflect the meaning of this boolean. With the current name, it isn't obvious what the difference between this value and m_playing is.

Second, I'm not sure that this variable really belongs here. From discussing the use of this with you over IRC, i'm not sure that HTMLMediaElement is really the class which is concerned with the use case that this variable is describing. Because of separation of concerns, it seems to me that HTMLMediaElement should be concerned with playing / pausing / seeking / etc. the video, and not concerned with keeping track of this state. It seems to me that this state belongs in something which can observe the HTMLMediaElement and make decisions based on what it sees.

Maybe I'm wrong, or maybe no such place exists.... either way, I'm not comfortable r+ing this patch. You should ask someone else who knows more about our media infrastructure.

[Wenson Hsieh]

Created attachment 284407 [details]
Patch

[Wenson Hsieh]

Created attachment 284418 [details]
Patch

[Wenson Hsieh]

Thinking more about this, I agree -- it's strange for the HTMLMediaElement to keep state around that's only relevant to the media controls heuristic. I believe that state would be better kept on the MediaElementSession in the form of a behavior restriction and have the HTMLMediaElement be able to toggle this switch on or off (which it already does today with RequireUserGestureToControlControlsManager).

(In reply to comment #9)
> Comment on attachment 284388 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284388&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.h:926
> > +    bool m_isDonePlaying : 1;
> 
> isDonePlaying is not a very good name, because it doesn't reflect the
> meaning of this boolean. With the current name, it isn't obvious what the
> difference between this value and m_playing is.
> 
> Second, I'm not sure that this variable really belongs here. From discussing
> the use of this with you over IRC, i'm not sure that HTMLMediaElement is
> really the class which is concerned with the use case that this variable is
> describing. Because of separation of concerns, it seems to me that
> HTMLMediaElement should be concerned with playing / pausing / seeking / etc.
> the video, and not concerned with keeping track of this state. It seems to
> me that this state belongs in something which can observe the
> HTMLMediaElement and make decisions based on what it sees.
> 
> Maybe I'm wrong, or maybe no such place exists.... either way, I'm not
> comfortable r+ing this patch. You should ask someone else who knows more
> about our media infrastructure.

[Wenson Hsieh]

Created attachment 284497 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 284497 [details]
Patch for landing

Clearing flags on attachment: 284497

Committed r203690: <http://trac.webkit.org/changeset/203690>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 284514 [details]
Patch

[Wenson Hsieh]

Accidentally reopened due to uploading a patch for the wrong bug. Please ignore!