Bug 162843 – Media controls for Soundcloud easily fall out of sync with what's actually playing

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Wenson Hsieh]

Media controls for Soundcloud easily fall out of sync with what's actually playing

[Wenson Hsieh]

<rdar://problem/28176874>

[Wenson Hsieh]

Created attachment 290446 [details]
Patch

[Beth Dakin]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 290471 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 290471 [details]
Patch for landing

Clearing flags on attachment: 290471

Committed r206721: <http://trac.webkit.org/changeset/206721>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.