Closed Bug 1180535 Opened 4 years ago Closed 4 years ago

Dispatch the media-playback notification when navigating away from a page that has a media element playing

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

When navigating away from a document, we mute the playing media elements
through the NotifyOwnerDocumentActivityChanged() notification.
Sometimes, that function may notify the audio channel agent through its
call to AddRemoveSelfReference() which may call
UpdateAudioChannelPlayingState() and notify the agent, but when we're
navigating away from the page, playingThroughTheAudioChannel will always
be equal to mPlayingThroughTheAudioChannel, which causes us to not
notify the audio channel agent.

This patch fixes this by separating NotifyOwnerDocumentActivityChanged()
from its internal consumers, and forcefully notifying the audio channel
agent when we navigate away.
Blocks: 486262
Assignee: nobody → ehsan
Comment on attachment 8629719 [details] [diff] [review]
Dispatch the media-playback notification when navigating away from a page that has a media element playing

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

::: dom/base/test/mochitest.ini
@@ +247,5 @@
>  [test_audioWindowUtils.html]
>  [test_audioNotification.html]
>  skip-if = buildapp == 'mulet'
> +[test_audioNotificationStopOnNavigation.html]
> +skip-if = buildapp == 'mulet'

why no the mulet?
Attachment #8629719 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #2)
> Review of attachment 8629719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/test/mochitest.ini
> @@ +247,5 @@
> >  [test_audioWindowUtils.html]
> >  [test_audioNotification.html]
> >  skip-if = buildapp == 'mulet'
> > +[test_audioNotificationStopOnNavigation.html]
> > +skip-if = buildapp == 'mulet'
> 
> why no the mulet?

test_audioNotification.html was disabled on mulet in bug 1027242 because it failed.  I could not care less about mulet so wanted to spend 0 time investigating why that is, and whether my other tests would fail similarly or not, so I just disabled it.  :-)
The cause of the failure is that NotifyOwnerDocumentActivityChanged is a virtual function, and when the internal HTMLMediaElement code called it, the HTMLVideoElement version would be called.  But NotifyOwnerDocumentActivityChangedInternal is not virtual, so in that case, we would be skipping HTMLVideoElement::NotifyOwnerDocumentActivityChanged.

The solution is to make NotifyOwnerDocumentActivityChangedInternal instead.
Flags: needinfo?(ehsan)
sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=727b321502a6 since i think the changes broke the cpp tests on windows very frequently with timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=11595725&repo=mozilla-inbound
Flags: needinfo?(ehsan)
I think bug 1113086 comment 197 explains what happened here!
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/97161d5cfb8c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1191491
You need to log in before you can comment on or make changes to this bug.