Bug 162843 - Media controls for Soundcloud easily fall out of sync with what's actually playing
Summary: Media controls for Soundcloud easily fall out of sync with what's actually pl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-01 12:37 PDT by Wenson Hsieh
Modified: 2016-10-02 12:24 PDT (History)
4 users (show)

See Also:

Attachments
Patch (20.64 KB, patch)
2016-10-01 16:19 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Patch for landing (20.63 KB, patch)
2016-10-02 11:51 PDT, Wenson Hsieh 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2016-10-01 12:37:26 PDT 
Media controls for Soundcloud easily fall out of sync with what's actually playing
Comment 1 Wenson Hsieh 2016-10-01 12:37:54 PDT 
<rdar://problem/28176874>
Comment 2 Wenson Hsieh 2016-10-01 16:19:16 PDT 
Created attachment 290446 [details]
Patch
Comment 3 Beth Dakin 2016-10-01 18:38:45 PDT 
Comment on attachment 290446 [details]
Patch

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

I think this looks pretty good! I'd just like to hear answers to those questions.

> Source/WebCore/ChangeLog:9
> +        Currently, we audio elements are subject to the same main content restrictions as video elements. This is

"we audio"

> Source/WebCore/ChangeLog:13
> +        Furthermore, we currently forbid playing audio from showing controls if it has user gesture restrictions (i.e.

Should this say "we currently forbid AUTOplaying audio"?

> Source/WebCore/html/MediaElementSession.cpp:234
> +    if (m_element.muted()) {

Why did this case move up in the function?

> Source/WebCore/html/MediaElementSession.cpp:239
> +    if (m_element.document().isMediaDocument()) {

Same, why did this one move?
Comment 4 Wenson Hsieh 2016-10-02 09:08:17 PDT 
Comment on attachment 290446 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Currently, we audio elements are subject to the same main content restrictions as video elements. This is
> 
> "we audio"

Whoops! Removed the 'we'.

>> Source/WebCore/ChangeLog:13
>> +        Furthermore, we currently forbid playing audio from showing controls if it has user gesture restrictions (i.e.
> 
> Should this say "we currently forbid AUTOplaying audio"?

Yes. Fixed.

>> Source/WebCore/html/MediaElementSession.cpp:234
>> +    if (m_element.muted()) {
> 
> Why did this case move up in the function?

I wanted to move heuristics common to both <audio> and <video> elements to the top of the function, before the early exit happens for audio elements. I think the muted state and whether or not the session is in a media document is something we should take into consideration for both media element types.

>> Source/WebCore/html/MediaElementSession.cpp:239
>> +    if (m_element.document().isMediaDocument()) {
> 
> Same, why did this one move?

See the above response.
Comment 5 Wenson Hsieh 2016-10-02 11:51:54 PDT 
Created attachment 290471 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-10-02 12:24:44 PDT 
Comment on attachment 290471 [details]
Patch for landing

Clearing flags on attachment: 290471

Committed r206721: <http://trac.webkit.org/changeset/206721>
Comment 7 WebKit Commit Bot 2016-10-02 12:24:49 PDT 
All reviewed patches have been landed.  Closing bug.