1180421 - Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[(no longer active)]

These events are dispatched to a XUL browser element when a document loaded
inside them starts or stops media playback.

Comment 1

[(no longer active)]

Attachment: Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

Comment 2

[(no longer active)]

Hmm, it's probably a good idea to also test the case where an iframe plays back media...

Comment 3

[(no longer active)]

Attachment: Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

These events are dispatched to a XUL browser element when a document loaded
inside them starts or stops media playback.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629599 [details] [diff] [review]
Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

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

::: toolkit/content/browser-content.js
@@ +648,5 @@
>  };
>  FindBar.init();
> +
> +let MediaPlaybackListener = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),

Newlines between properties.

Also, let's stick with the short-form for function definition instead of mixing with long-form on observe.

@@ +650,5 @@
> +
> +let MediaPlaybackListener = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> +  init() {
> +    Services.obs.addObserver(this, "media-playback", false);

We need to remove this observer on content unload.

::: toolkit/content/tests/browser/browser_mediaPlayback.js
@@ +27,5 @@
> +}
> +
> +add_task(function*() {
> +  yield new Promise((resolve) => {
> +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},

Why do you need to use SpecialPowers here? This browser test is running with full chrome-priv's in the parent process. I think you can just synchronously use Services.prefs.setBoolPref, no? Or does this do the work of resetting the pref once the test is done? As an alternative to that, you could always do the sync pref write, and do registerCleanupFunction to set it back. I can't say I care too much though.

@@ +31,5 @@
> +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> +                              resolve);
> +  });
> +
> +  add_task(getTest(PAGE));

We generally don't nest add_tasks. I'm actually surprised this works.

These should probably just be put outside of this outer task.

I actually suggest re-writing this using some of our handy BrowserTestUtils gunk to do some of the heavy lifting for you, as follows:

function waitForPlaybackStopped(browser) {
  return BrowserTestUtils.waitForEvent(browser,
                                       "DOMMediaPlaybackStopped",
                                       false, (event) => {
    is(event.originalTarget, browser, "Event must be dispatched to correct browser.");
    return true;
  });
}


function waitForPlaybackStarted(browser) {
  return BrowserTestUtils.waitForEvent(browser,
                                       "DOMMediaPlaybackStarted",
                                       false, (event) => {
    is(event.originalTarget, browser, "Event must be dispatched to correct browser.");
    return true;
  });
}

add_task(function*() {
  // Your pref flip
});

add_task(function* test_page() {
  yield BrowserTestUtils.withNewTab({
    gBrowser,
    url: PAGE,
  }, function*(browser) {
    yield waitForPlaybackStarted(browser);
    yield waitForPlaybackStopped(browser);
  });
});

add_task(function* test_frame() {
  yield BrowserTestUtils.withNewTab({
    gBrowser,
    url: FRAME,
  }, function*(browser) {
    yield waitForPlaybackStarted(browser);
    yield waitForPlaybackStopped(browser);
  });
});

There's probably some de-duplication you can do there, but don't go crazy. I'm just advocating the use of the BrowserTestUtils stuff.

::: toolkit/content/widgets/browser.xml
@@ +662,5 @@
>        <property name="pageReport"
>           onget="return this.blockedPopups;"
>           readonly="true"/>
>  
> +      <method name="mediaPlaybackStarted">

Instead of exposing these as public members, I think it might be better to just fire these from the place where we're handling the message.

@@ +665,5 @@
>  
> +      <method name="mediaPlaybackStarted">
> +        <body>
> +          <![CDATA[
> +            var event = document.createEvent("Events");

let, not var

@@ +675,5 @@
> +
> +      <method name="mediaPlaybackStopped">
> +        <body>
> +          <![CDATA[
> +            var event = document.createEvent("Events");

let, not var

Comment 5

[(no longer active)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> @@ +650,5 @@
> > +
> > +let MediaPlaybackListener = {
> > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> > +  init() {
> > +    Services.obs.addObserver(this, "media-playback", false);
> 
> We need to remove this observer on content unload.

You don't mean when the document loaded inside |content| is unloaded, right?  Since this should stay alive as long as the tab is open...

I guess I don't really understand the lifetime of the objects created in this script.  I can't find anything in this file that handles any kind of unregistration.  Can you please show me what I should do here?

> ::: toolkit/content/tests/browser/browser_mediaPlayback.js
> @@ +27,5 @@
> > +}
> > +
> > +add_task(function*() {
> > +  yield new Promise((resolve) => {
> > +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> 
> Why do you need to use SpecialPowers here? This browser test is running with
> full chrome-priv's in the parent process. 

The reason I use SpecialPowers here is not because of permissions but because I want to use the pushPrefEnv function (and that is implemented on the SpecialPowers global object).  See below.

> I think you can just synchronously
> use Services.prefs.setBoolPref, no? Or does this do the work of resetting
> the pref once the test is done?

Yes, that is why we use this.  Also, there are other existing browser-chrome tests that use it too.  <http://mxr.mozilla.org/mozilla-central/search?string=pushPrefEnv&find=browser&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central>  :-)

> As an alternative to that, you could always
> do the sync pref write, and do registerCleanupFunction to set it back. I
> can't say I care too much though.

Well, the nice thing about this method is that you list the prefs only in one place, while your suggestion means that you need to list them twice (and run the risk of forgetting to update on in the future), or reinvent pushPrefEnv.  So I prefer to keep using it if that's OK.

Comment 6

[(no longer active)]

So I tried to use BrowserTestUtils, and I ran into problems, which are caused because I can't get enough control over the timings of the events.

Here is what happens.  By the time that BrowserTestUtils.withNewTab() executes its callback, my test page has loaded and caused the media playback event to happen.  So my test just sits there and waits for waitForEvent to finish.  I could work around this by waiting in the test page but that seems like a recipe for getting intermittent failures.  :(

What gives?

Comment 7

[(no longer active)]

Attachment: Test changes for bug 1180421 (WIP)

This is what I have right now...

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #6)
> So I tried to use BrowserTestUtils, and I ran into problems, which are
> caused because I can't get enough control over the timings of the events.
> 
> Here is what happens.  By the time that BrowserTestUtils.withNewTab()
> executes its callback, my test page has loaded and caused the media playback
> event to happen.  So my test just sits there and waits for waitForEvent to
> finish.  I could work around this by waiting in the test page but that seems
> like a recipe for getting intermittent failures.  :(
> 
> What gives?

Ah. :/ That's my bad - I failed to recognize that your test pages execute their script immediately.

Perhaps try:

yield BrowserTestUtils.withNewTab({
  gBrowser,
  url: "about:blank",
}, test_on_browser(PAGE));

and

yield BrowserTestUtils.withNewTab({
  gBrowser,
  url: "about:blank",
}, test_on_browser(FRAME));

function test_on_browser(url) {
  return function*(browser) {
    browser.loadURI(url);
    yield wait_for_event(browser, "DOMMediaPlaybackStarted");
    yield wait_for_event(browser, "DOMMediaPlaybackStopped");
  };
}

Comment 9

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #5)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> > @@ +650,5 @@
> > > +
> > > +let MediaPlaybackListener = {
> > > +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
> > > +  init() {
> > > +    Services.obs.addObserver(this, "media-playback", false);
> > 
> > We need to remove this observer on content unload.
> 
> You don't mean when the document loaded inside |content| is unloaded, right?
> Since this should stay alive as long as the tab is open...

Correct - but you've added this observer inside of browser-content.js, which should be loaded once per tab. We should listen for unload like this:

addEventListener("unload", () => {
  MediaPlaybackListener.uninit();
});

Where uninit removes the observer.

> 
> > ::: toolkit/content/tests/browser/browser_mediaPlayback.js
> > @@ +27,5 @@
> > > +}
> > > +
> > > +add_task(function*() {
> > > +  yield new Promise((resolve) => {
> > > +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> > 
> > Why do you need to use SpecialPowers here? This browser test is running with
> > full chrome-priv's in the parent process. 
> 
> The reason I use SpecialPowers here is not because of permissions but
> because I want to use the pushPrefEnv function (and that is implemented on
> the SpecialPowers global object).  See below.
> 
> > I think you can just synchronously
> > use Services.prefs.setBoolPref, no? Or does this do the work of resetting
> > the pref once the test is done?
> 
> Yes, that is why we use this.  Also, there are other existing browser-chrome
> tests that use it too. 
> <http://mxr.mozilla.org/mozilla-central/
> search?string=pushPrefEnv&find=browser&findi=&filter=^[^\0]*%24&hitlimit=&tre
> e=mozilla-central>  :-)
> 
> > As an alternative to that, you could always
> > do the sync pref write, and do registerCleanupFunction to set it back. I
> > can't say I care too much though.
> 
> Well, the nice thing about this method is that you list the prefs only in
> one place, while your suggestion means that you need to list them twice (and
> run the risk of forgetting to update on in the future), or reinvent
> pushPrefEnv.  So I prefer to keep using it if that's OK.

Yep, sounds good.

Comment 10

[(no longer active)]

Attachment: Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

This should address all of the review comments.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8631353 [details] [diff] [review]
Add support for DOMMediaPlaybackStarted and DOMMediaPlaybackStopped events

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

Looks good - thanks ehsan!

::: toolkit/content/widgets/browser.xml
@@ +665,5 @@
>  
> +      <method name="mediaPlaybackStarted">
> +        <body>
> +          <![CDATA[
> +            var event = document.createEvent("Events");

let, not var

@@ +675,5 @@
> +
> +      <method name="mediaPlaybackStopped">
> +        <body>
> +          <![CDATA[
> +            var event = document.createEvent("Events");

let, not var

Comment 12

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

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