Bug 167214 - Record whether a media element was prevented from playing without user interaction
Summary: Record whether a media element was prevented from playing without user intera...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-19 14:14 PST by Matt Rajca
Modified: 2017-01-20 18:32 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.25 KB, patch)
2017-01-19 14:18 PST, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (15.03 KB, patch)
2017-01-20 15:55 PST, Matt Rajca
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2017-01-19 14:14:20 PST
This state will be used to notify clients when a user explicitly starts playback of a media element that was prevented from autoplaying.
Comment 1 Matt Rajca 2017-01-19 14:18:18 PST
Created attachment 299269 [details]
Patch
Comment 2 WebKit Commit Bot 2017-01-19 16:29:37 PST
Attachment 299269 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLMediaElement.cpp:3169:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jer Noble 2017-01-20 10:05:45 PST
Comment on attachment 299269 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:3106
> +    bool preventedPlaybackWithoutUserGesture = false;
> +    if (!m_mediaSession->playbackPermitted(*this, &preventedPlaybackWithoutUserGesture)) {
> +        if (preventedPlaybackWithoutUserGesture)
> +            m_preventedFromPlayingWithoutUserGesture = true;
>          return;
> +    }

When is m_preventedFromPlayingWithoutUserGesture ever reset to false?

> Source/WebCore/html/HTMLMediaElement.h:719
> -    bool canTransitionFromAutoplayToPlay() const;
> +    bool canTransitionFromAutoplayToPlay(bool* preventedPlaybackWithoutUserGesture) const;

This and ...

> Source/WebCore/html/MediaElementSession.h:55
> -    bool playbackPermitted(const HTMLMediaElement&) const;
> +    bool playbackPermitted(const HTMLMediaElement&, bool* preventedPlaybackWithoutUserGesture) const;

This.

I don't like this "out boolean pointer parameter" change.  We should some other way of representing failure, on the order of "ExceptionOr<>".  For example, something like:

enum class PlaybackDenialReason {
    UserGestureRequired,
    PageConsentRequired,
    FullscreenRequired,
};
template <typename T>
class SuccessOr : public optional<T> {
public:
    explicit constexpr operator bool() const { return !optional::operator bool(); }
};

And have these methods return SuccessOr<PlaybackDenialReason>.  I.e.:

    SuccessOr<PlaybackDenialReason> playbackPermitted(const HTMLMediaElement&) const;

So the call sites can remain exactly the same:

    if (!playbackPermitted(m_element)) { ... }

And in the places we care about why playback was denied, we can look at the return value:

    auto success = canTransitionFromAutoplayToPlay(&preventedPlaybackWithoutUserGesture));
    if (success) { ... } else {
        m_preventedFromPlayingWithoutUserGesture = success.value() == PlaybackDenialReason:: UserGestureRequired;
    }
Comment 4 Matt Rajca 2017-01-20 13:59:13 PST
(In reply to comment #3)
> Comment on attachment 299269 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299269&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3106
> > +    bool preventedPlaybackWithoutUserGesture = false;
> > +    if (!m_mediaSession->playbackPermitted(*this, &preventedPlaybackWithoutUserGesture)) {
> > +        if (preventedPlaybackWithoutUserGesture)
> > +            m_preventedFromPlayingWithoutUserGesture = true;
> >          return;
> > +    }
> 
> When is m_preventedFromPlayingWithoutUserGesture ever reset to false?

It will get reset when the user starts playing media again with a user gesture. There's currently a FIXME in that code path but I'll make it part of this patch.

> 
> > Source/WebCore/html/HTMLMediaElement.h:719
> > -    bool canTransitionFromAutoplayToPlay() const;
> > +    bool canTransitionFromAutoplayToPlay(bool* preventedPlaybackWithoutUserGesture) const;
> 
> This and ...
> 
> > Source/WebCore/html/MediaElementSession.h:55
> > -    bool playbackPermitted(const HTMLMediaElement&) const;
> > +    bool playbackPermitted(const HTMLMediaElement&, bool* preventedPlaybackWithoutUserGesture) const;
> 
> This.
> 
> I don't like this "out boolean pointer parameter" change.  We should some
> other way of representing failure, on the order of "ExceptionOr<>".  For
> example, something like:
> 
> enum class PlaybackDenialReason {
>     UserGestureRequired,
>     PageConsentRequired,
>     FullscreenRequired,
> };
> template <typename T>
> class SuccessOr : public optional<T> {
> public:
>     explicit constexpr operator bool() const { return !optional::operator
> bool(); }
> };
> 
> And have these methods return SuccessOr<PlaybackDenialReason>.  I.e.:
> 
>     SuccessOr<PlaybackDenialReason> playbackPermitted(const
> HTMLMediaElement&) const;
> 
> So the call sites can remain exactly the same:
> 
>     if (!playbackPermitted(m_element)) { ... }
> 
> And in the places we care about why playback was denied, we can look at the
> return value:
> 
>     auto success =
> canTransitionFromAutoplayToPlay(&preventedPlaybackWithoutUserGesture));
>     if (success) { ... } else {
>         m_preventedFromPlayingWithoutUserGesture = success.value() ==
> PlaybackDenialReason:: UserGestureRequired;
>     }

Thanks for the suggestion - that would make the call sites cleaner. Working on a v2 of the patch now.
Comment 5 Matt Rajca 2017-01-20 15:55:11 PST
Created attachment 299398 [details]
Patch
Comment 6 WebKit Commit Bot 2017-01-20 15:56:34 PST
Attachment 299398 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/SuccessOr.h:35:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/SuccessOr.h:36:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:2259:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:2260:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:2261:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:2262:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:2263:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 WebKit Commit Bot 2017-01-20 18:32:46 PST
Comment on attachment 299398 [details]
Patch

Clearing flags on attachment: 299398

Committed r211008: <http://trac.webkit.org/changeset/211008>
Comment 8 WebKit Commit Bot 2017-01-20 18:32:50 PST
All reviewed patches have been landed.  Closing bug.