Bug 193128 - Make DidPlayMediaPreventedFromPlaying autoplay event more generic
Summary: Make DidPlayMediaPreventedFromPlaying autoplay event more generic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Rajca
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-03 16:25 PST by Matt Rajca
Modified: 2019-01-04 11:46 PST (History)
5 users (show)

See Also:


Attachments
Patch (949.68 KB, patch)
2019-01-03 16:46 PST, Matt Rajca
jer.noble: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2019-01-03 16:25:32 PST
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
Comment 1 Matt Rajca 2019-01-03 16:46:53 PST
Created attachment 358288 [details]
Patch
Comment 2 EWS Watchlist 2019-01-03 16:48:57 PST
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.
Comment 3 Matt Rajca 2019-01-03 16:51:16 PST
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.
Comment 4 Jer Noble 2019-01-04 10:28:15 PST
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);
Comment 5 Matt Rajca 2019-01-04 11:07:27 PST
(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!
Comment 6 Matt Rajca 2019-01-04 11:46:42 PST
Landed: https://trac.webkit.org/changeset/239625/webkit
r239625