Bug 193128 – Make DidPlayMediaPreventedFromPlaying autoplay event more generic

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

Today, the "DidPlayMediaPreventedFromPlaying" autoplay event is only sent for media prevented from playing. It could be generalized to "DidPlayMediaWithUserGesture" with a flag that indicates whether or not autoplay was actually prevented or not. Moreover, we can include a flag that indicates whether the media element in question is main content. Clients will then know in more cases when media was played with a user gesture, whether or not it has sound, as well as its main content status.

Work towards fixing: rdar://34554231

[Matt Rajca]

Created attachment 358288 [details]
Patch

[EWS Watchlist]

Attachment 358288 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:264:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Matt Rajca]

This pertains to:

> static_assert(static_cast<size_t>(HTMLMediaElement::AutoplayEventPlaybackState::None) == 0

which is not new to this patch (a name was just updated).

(In reply to Build Bot from comment #2)
> Attachment 358288 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/html/HTMLMediaElement.cpp:264:  Tests for true/false,
> null/non-null, and zero/non-zero should all be done without equality
> comparisons.  [readability/comparison_to_zero] [5]
> Total errors found: 1 in 17 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

[Jer Noble]

Comment on attachment 358288 [details]
Patch

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

r=me, with nits.

> Source/WebCore/html/HTMLMediaElement.cpp:-3576
> -            handleAutoplayEvent(AutoplayEvent::DidPlayMediaPreventedFromPlaying);

This line...

> Source/WebCore/html/HTMLMediaElement.cpp:3577
> +            handleAutoplayEvent(AutoplayEvent::DidPlayMediaWithUserGesture);

And this line seem to not be parallel changes. Is this new enum correct?

> Source/WebCore/html/HTMLMediaElement.cpp:3938
> +        if (m_autoplayEventPlaybackState == AutoplayEventPlaybackState::StartedWithoutUserGesture)
> +            handleAutoplayEvent(AutoplayEvent::DidAutoplayMediaPastThresholdWithoutUserInterference);
> +        else {
> +            ASSERT(m_autoplayEventPlaybackState == AutoplayEventPlaybackState::StartedWithUserGesture);
> +            handleAutoplayEvent(AutoplayEvent::DidPlayMediaWithUserGesture);
> +        }

I think this could be dramatically simplified to:

handleAutoplayEvent(m_autoplayEventPlaybackState == AutoplayEventPlaybackState::StartedWithoutUserGesture ? AutoplayEvent::DidAutoplayMediaPastThresholdWithoutUserInterference : AutoplayEvent::DidPlayMediaWithUserGesture);

[Matt Rajca]

(In reply to Jer Noble from comment #4)
> Comment on attachment 358288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358288&action=review
> 
> r=me, with nits.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:-3576
> > -            handleAutoplayEvent(AutoplayEvent::DidPlayMediaPreventedFromPlaying);
> 
> This line...
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3577
> > +            handleAutoplayEvent(AutoplayEvent::DidPlayMediaWithUserGesture);
> 
> And this line seem to not be parallel changes. Is this new enum correct?

Yes, it's correct. DidPlayMediaWithUserGesture is a more generic version of DidPlayMediaPreventedFromPlaying. handleAutoplayEvent itself will set a flag indicating whether or not playback was actually prevented and it's up to clients to check the flag if they want the previous behavior.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3938
> > +        if (m_autoplayEventPlaybackState == AutoplayEventPlaybackState::StartedWithoutUserGesture)
> > +            handleAutoplayEvent(AutoplayEvent::DidAutoplayMediaPastThresholdWithoutUserInterference);
> > +        else {
> > +            ASSERT(m_autoplayEventPlaybackState == AutoplayEventPlaybackState::StartedWithUserGesture);
> > +            handleAutoplayEvent(AutoplayEvent::DidPlayMediaWithUserGesture);
> > +        }
> 
> I think this could be dramatically simplified to:
> 
> handleAutoplayEvent(m_autoplayEventPlaybackState ==
> AutoplayEventPlaybackState::StartedWithoutUserGesture ?
> AutoplayEvent::DidAutoplayMediaPastThresholdWithoutUserInterference :
> AutoplayEvent::DidPlayMediaWithUserGesture);

Will do!

[Matt Rajca]

Landed: https://trac.webkit.org/changeset/239625/webkit
r239625