1200208 - Tab audio indicator doesn't show up on the huffingtonpost Parkersburg article

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

http://highline.huffingtonpost.com/articles/en/welcome-to-beautiful-parkersburg/

Comment 1

[(no longer active)]

This happens because the page calls .play() on the video element before it has its metadata loaded, so HasAudio() returns false when we register the audio channel agent and we end up not emitting the audio-playback notification.  We need to deal with HasAudio() changing correctly in MetadataLoaded.

Comment 2

[(no longer active)]

Attachment: Send the audio-playback notification when the page calls HTMLMediaElement::Play() before the metadata has been fully loaded

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8654943 [details] [diff] [review]
Send the audio-playback notification when the page calls HTMLMediaElement::Play() before the metadata has been fully loaded

Review of attachment 8654943 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +4519,5 @@
> +{
> +  if (!mAudioChannelAgent) {
> +    nsresult rv;
> +    mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1", &rv);
> +    if (!mAudioChannelAgent) {

we should actually use 'rv' here. something like:

if (NS_WARN_IF(NS_FAILED(rv))) {
  return false;
}

MOZ_ASSERT(mAudioChannelAgent);

::: dom/html/HTMLMediaElement.h
@@ +58,5 @@
>  
>  class nsITimer;
>  class nsRange;
>  class nsIRunnable;
> +class AutoNotifyAudioChannelAgent;

alphabetic order.

@@ +1049,5 @@
>    void UpdateReadyStateInternal();
>  
>    // Notifies the audio channel agent when the element starts or stops playing.
>    void NotifyAudioChannelAgent(bool aPlaying);
> +  friend class AutoNotifyAudioChannelAgent;

move this at the beginning of the class definition.

Comment 4

[(no longer active)]

Comment on attachment 8654943 [details] [diff] [review]
Send the audio-playback notification when the page calls HTMLMediaElement::Play() before the metadata has been fully loaded

Approval Request Comment
[Feature/regressing bug #]: Tab audio indicator feature polish
[User impact if declined]: See comment 0.  The audio indicator may not appear on some websites that play audio.
[Describe test coverage new/current, TreeHerder]: Tested locally and has automated tests.
[Risks and why]: Pretty self-contained change, should not be very risky. 
[String/UUID change made/needed]: None.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b264cf230a0

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=13504562&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/faa1e553d63a

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e27f15635a

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c26af850666

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e90965135940

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e90965135940

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654943 [details] [diff] [review]
Send the audio-playback notification when the page calls HTMLMediaElement::Play() before the metadata has been fully loaded

Polish a new feature, taking it.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/530b62fff9e7

Comment 13

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed using latest Nightly 43.0a1 (buildID: 20150913030204) and latest Aurora 42.0a2 (buildID: 20150914004020).