Summary: | Make DidPlayMediaPreventedFromPlaying autoplay event more generic | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Rajca <mrajca> | ||||
Component: | Media | Assignee: | Matt Rajca <mrajca> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, jer.noble, mrajca, wenson_hsieh | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Matt Rajca
2019-01-03 16:25:32 PST
Created attachment 358288 [details]
Patch
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.
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 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); (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! |