1366167 - Add test for "Enable DRM" prompt.

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

The "Enable DRM" prompt that shows up when EME is not enabled and EME content is requested by script to be played broke recently (Bug 1360863), and no one noticed because we don't have a test. We should have a test.

If this breaks, users who don't have EME enabled (which is all Linux users by default) will not be prompted to enable DRM when they try to watch DRM protected video; there will be no feedback, it will just not work. So we need a test to ensure it doesn't regress.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1366167 - Test for Enable DRM prompt.

Review commit: https://reviewboard.mozilla.org/r/140874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140874/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8869324 [details]
Bug 1366167 - Test for Enable DRM prompt.

Mike: I'm having trouble writing this test, would you be able to offer me some pointers?

The test itself seems to run to completion and is testing what I want to test, but it times out.

The test causes the "Enable DRM" notification box to drop down, and then clicks the notification box's button. This runs ensureEMEEnabled() from browser-media.js, which sets some prefs to enable EME and reload the content page. The test completes. But it then doesn't shut down and times out.

I was able to bisect the problem and determine that if I remove the pref sets in ensureEMEEnabled(), the test doesn't time out. Is there some problem with using Services.prefs.setBoolPref() from inside UI code while running chrome mochitests?

Any ideas how I can make this test work?

Comment 3

[Johann Hofmann [:johannh]]

Maybe Mike can say more about this, but it sounds like a bug in Mochitest to me. I wouldn't worry about it too much and just use setBoolPref and clearUserPref and make a bug with a reduced test case in Testing -> Mochitest.

I find it a bit confusing that the DRM "notice" is a doorhanger when EME is active on a page while the EME prompt is a notification box. We should make the notification box a doorhanger instead, because IIRC we want to reduce notification boxes as a pattern. Chris, would you mind if I open a new bug for that?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Johann Hofmann [:johannh] from comment #3)
> Maybe Mike can say more about this, but it sounds like a bug in Mochitest to
> me. I wouldn't worry about it too much and just use setBoolPref and
> clearUserPref and make a bug with a reduced test case in Testing ->
> Mochitest.

I can't land my test without this fixed, so I do need to worry about it, or I need to land my test without actually testing that the "Enable DRM" button enables DRM. I could make the test just test that the notification bar shows for example.

I'll file a bug with a reduced test case.

 
> I find it a bit confusing that the DRM "notice" is a doorhanger when EME is
> active on a page while the EME prompt is a notification box. We should make
> the notification box a doorhanger instead, because IIRC we want to reduce
> notification boxes as a pattern. Chris, would you mind if I open a new bug
> for that?


The original work for add the notification box was done by Gijs in bug 1111153.

I don't much care whether the UI is a notification box or a door hanger, provided it's modal and it's obvious, as if the user has EME disabled, some sites just won't work, and unless there's a obvious way to enable EME the user is at a dead end.

I suspect that the decision to make it a notification bar may have been so that accidentally clicking somewhere on the page wouldn't make the doorhanger disappear, as I think that's the behaviour with doorhangers? If we made the "Enable DRM" prompt a doorhanger, we'd need to ensure it only disappeared if the user click on a dismiss button on the doorhanger itself. Otherwise it's easy for the user to get stuck in a state where it's not obvious how to make the video they paid money for to work.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8869324 [details]
Bug 1366167 - Test for Enable DRM prompt.

The problem here seems to have been caused by SpecialPowers.pushPrefEnv() having its mind blown by the UI code changing the same prefs it had pushed a change for.

Switching to setting/unsetting the prefs our UI toggles before the test makes things happy.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8869324 [details]
Bug 1366167 - Test for Enable DRM prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140874/diff/1-2/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8869324 [details]
Bug 1366167 - Test for Enable DRM prompt.

https://reviewboard.mozilla.org/r/140874/#review145040

::: browser/base/content/test/plugins/browser_enable_DRM_prompt.js:5
(Diff revision 2)
> +/*
> + * Bug 1366167 - Tests that the "Enable DRM" prompt shows if EME is requested while EME is disabled.
> + */
> +
> +const TEST_URL = "https://example.com/browser/browser/base/content/test/plugins/empty_file.html";

Please use something like:

```js
const TEST_URL = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "https://example.com") + "empty_file.html";
```

::: browser/base/content/test/plugins/browser_enable_DRM_prompt.js:8
(Diff revision 2)
> +  await BrowserTestUtils.withNewTab({
> +    gBrowser,
> +    url: TEST_URL,
> +  }, async function(browser) {

Nit: you can just pass the string URL instead of the object. :-)

::: browser/base/content/test/plugins/browser_enable_DRM_prompt.js:51
(Diff revision 2)
> +    // Verify the "Enable DRM" button is there.
> +    let buttons = notification.querySelectorAll(".notification-button");
> +    is(buttons.length, 1, "Should have one button.");
> +
> +    // Prepare a Promise that should resolve when the "Enable DRM" button's
> +    // page reload goes completes.

Nit: grammar. s/goes//, I guess?

::: browser/base/content/test/plugins/browser_enable_DRM_prompt.js:62
(Diff revision 2)
> +    // Restore the preferences to their pre-test state.
> +    Services.prefs.setBoolPref("media.eme.enabled", emeWasEnabled);
> +    Services.prefs.setBoolPref("media.gmp-widevinecdm.enabled", cdmWasEnabled);

Can you put this in a cleanup function instead of at the bottom? So directly after setting these to their potentially non-default values (at the top of the test), have:

```js
registerCleanupFunction(function() {
    Services.prefs.setBoolPref("media.eme.enabled", emeWasEnabled);
    Services.prefs.setBoolPref("media.gmp-widevinecdm.enabled", cdmWasEnabled);
});
```

That avoids these being left at non-default values if the test times out / fails somehow, thus potentially interfering with other tests.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8869324 [details]
Bug 1366167 - Test for Enable DRM prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140874/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5dc81141d3e4
Test for Enable DRM prompt. r=Gijs

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dc81141d3e4