Summary: | Record whether a media element was prevented from playing without user interaction | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, jer.noble | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Matt Rajca
2017-01-19 14:14:20 PST
Created attachment 299269 [details]
Patch
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 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; } (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. Created attachment 299398 [details]
Patch
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 on attachment 299398 [details] Patch Clearing flags on attachment: 299398 Committed r211008: <http://trac.webkit.org/changeset/211008> All reviewed patches have been landed. Closing bug. |