1111146 - [EME] Implement master pref for playing DRM content.

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Justin Dolske [:Dolske]]

See user story in bug 1089876.

We want a master pref in Preferences -> Content to allow disabling all EME/CDM/DRM/BBQ support. It should be enabled by default, but allow a user to opt-out of DRM support.

Basically, a checkbox and link:

  [X] Play DRM Content   _Learn More_

Bug 1089876 comment 1 indicates this should basically be checkbox UI for the media.eme.enabled pref.

In support of the user stories in bug 1089873 and bug 1089882, the "Learn More" link provides an opportunity for the user to learn about what DRM is. (SUMO page TBD, initial wordsmithing in bug 1095734.)

Depending on the order in which pieces land, we'll also want the Addon Manager side to honor this pref (to inhibit CDM installs/updates and indicate any CDMs as disabled).

I believe we also want to use this pref to be used by users who are fundamentally opposed to DRM, so that any already-installed CDMs will be removed from their system. The exact way that functions is TBD -- we could immediately uninstall when clearing the checkbox, but it might be less of a footgun to uninstall at the next browser restart. [Again, depending on the order of this bug and the Addon Manager work, this part could be spun out into a followup.]

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[:Gijs (he/him)]

Asked on IRC, but maybe here is better: was there UI design work for the pref here beyond comment #0?

All the other in-content panes, when using headered subsections, have all their prefs in sections. It seems odd to just remove the Popups header here and leave the prefs without a header; seems like we should sooner create a new section below the fonts & colors one. I don't think there is any particular reason to lob it together with popups beside aesthetics, so I'd personally prefer not to do that. Dão, can you clarify?

Comment 3

[Dão Gottwald [:dao]]

I modeled this after the legacy preferences window where "contentRows-1" (hint: the same generic, non-popup-blocking specific id is used in the in-content markup) doesn't have a header. I don't see the point in adding headers for single checkboxes either. I guess we could add a "General" header like under Security, but again, I don't really see the value.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8555835 [details] [diff] [review]
patch

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

Generally looks good, but see needinfo and the comment below.

::: browser/components/preferences/in-content/content.xul
@@ +52,5 @@
> +                    label="&playDRMContent.label;" accesskey="&playDRMContent.accesskey;"/>
> +        </vbox>
> +        <hbox pack="end">
> +          <label id="playDRMContentLink" class="text-link" value="&playDRMContent.learnMore.label;"
> +                 href="https://wiki.mozilla.org/Media/EME"/>

Shouldn't this link be localized?

Comment 5

[:Gijs (he/him)]

Philipp, thoughts regarding comment #2 / comment #3 ?

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #4)
> > +          <label id="playDRMContentLink" class="text-link" value="&playDRMContent.learnMore.label;"
> > +                 href="https://wiki.mozilla.org/Media/EME"/>
> 
> Shouldn't this link be localized?

I don't think so. First of all it's a placeholder. The final link should go to a SUMO page that doesn't exist yet, and AFAIK SUMO does content negotiation such that the link doesn't need to be locale specific.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

It should probably be pref-ified, though (either via app.support.baseURL or a separate one-off pref).

Comment 8

[Dão Gottwald [:dao]]

Reusing app.support.baseURL sounds about right, but it's not gonna work well as long as the page doesn't exist. Ignoring for a moment that I'm not even sure what page id to use: Are we ok with linking to a 404 error page for now?

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v2

now using app.support.baseURL + "drm-content" (leads to Page Not Found)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8556379 [details] [diff] [review]
patch v2

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

r=me, I think a 404 is fine for now. Would still be good to have UX feedback on the headers thing in the in-content prefs. If there isn't any by the end of this week, please do ping me so this doesn't fall through the cracks.

Comment 11

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/3484b7d55720

As discussed on IRC, landed with the "Plug-ins" header string kept in the dtd for now, pending a UX evaluation.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

IIRC Javaun picked up EME work from Kev. We need to coordinate with someone on the SUMO content team (probably Joni?) to get the page and in-product redirect set up, but that's likely blocked on having the content.

Using the placeholder link is fine for now, but I think we probably shouldn't be showing this (and other) UI yet. Can we put that behind a pref too (e.g. browser.eme.ui.enabled)?

Comment 13

[:Gijs (he/him)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #12)
> Using the placeholder link is fine for now, but I think we probably
> shouldn't be showing this (and other) UI yet.

Why not, and why was this not discussed prior to getting this implemented?

> Can we put that behind a pref
> too (e.g. browser.eme.ui.enabled)?

I will try to have a patch in an hour, considering that this has already landed...

Comment 14

[:Gijs (he/him)]

Attachment: use browser.eme.ui.enabled pref to hide EME UI for now,

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8556482 [details] [diff] [review]
use browser.eme.ui.enabled pref to hide EME UI for now,

stealing

Comment 16

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/4898e64585e9

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8556482 [details] [diff] [review]
use browser.eme.ui.enabled pref to hide EME UI for now,

Thanks for the patch, Gijs.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gijs Kruitbosch from comment #13)
> Why not, and why was this not discussed prior to getting this implemented?

I'm not confident about the state of the EME backend on Nightly or our plans for communicating this, and so it seems best to be conservative.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/3484b7d55720
https://hg.mozilla.org/mozilla-central/rev/4898e64585e9

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Redirecting to Sevaan, who has been doing most of the UI work for EME.
I assume that a user would still have the option to enable EME when on a page that includes protected content if he unchecks that box.

Comment 21

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #20)
> I assume that a user would still have the option to enable EME when on a
> page that includes protected content if he unchecks that box.

Yes, but that's not what this bug is about.

Comment 22

[Sevaan Franks [:sevaan]]

(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #20)
> Redirecting to Sevaan, who has been doing most of the UI work for EME.
> I assume that a user would still have the option to enable EME when on a
> page that includes protected content if he unchecks that box.

Yes, a notification bar will appear: https://bugzilla.mozilla.org/show_bug.cgi?id=1111153

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8556379 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8556379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, I think a 404 is fine for now. Would still be good to have UX feedback
> on the headers thing in the in-content prefs. If there isn't any by the end
> of this week, please do ping me so this doesn't fall through the cracks.

Sevaan, this was what the needinfo was originally about... we currently removed the "Popups" header above the popups part of the content prefs pane, and were wondering if that works, or if we need a separate header, or should replace it with "General", or something else. See comment #2 and comment #3.

Comment 24

[Sevaan Franks [:sevaan]]

I kind of feel like the headers are important to have. Especially for when search gets implemented...it will make search results much cleaner.

I don't think it's an issue to have a header with one Pref under it. It makes it easier for users to scan the page.

Comment 25

[Justin Dolske [:Dolske]]

Since this patch has already landed, we'll either need a followup patch or new bug to give each pref a header. Gijs, Dao's on PTO now, so can you take that?

Comment 26

[:Gijs (he/him)]

Yes, see bug 1127288.

Comment 27

[Javaun Moradi [:javaun]]

Joni says the redirect is in place. 

https://support.mozilla.org/en-US/kb/enable-drm

Can you verify this is correct? Thanks.

Comment 28

[:Gijs (he/him)]

(In reply to Javaun Moradi [:javaun] from comment #27)
> Joni says the redirect is in place. 
> 
> https://support.mozilla.org/en-US/kb/enable-drm
> 
> Can you verify this is correct? Thanks.

On Mac Nightly, the current in-product link points to:

https://support.mozilla.org/1/firefox/38.0a1/Darwin/en-US/drm-content

That still 404s. Note also that if I change the link to:

https://support.mozilla.org/1/firefox/38.0a1/Darwin/en-US/enable-drm

That does redirect (to https://support.mozilla.org/en-US/kb/watch-drm-content-firefox?as=u&utm_source=inproduct ) , but still shows a 404.

Joni, can you clarify what's going on and/or if this can be fixed on the SUMO side to work with the existing in-product https://support.mozilla.org/1/firefox/%VERSION%/%OS%/%LOCALE%/drm-content link, or if we need to do something on the Firefox side? Thanks!

Comment 29

[Joni Chan]

Hi Gijs, sorry, I think I might have broken the redirect when I changed the title. 

We've made a change on the SUMO side so that https://support.mozilla.org/1/firefox/38.0a1/Darwin/en-US/drm-content points to the article. It seems to work now, but please let me know if it doesn't work on your end.

Comment 30

[:Gijs (he/him)]

(In reply to Joni Savage from comment #29)
> Hi Gijs, sorry, I think I might have broken the redirect when I changed the
> title. 
> 
> We've made a change on the SUMO side so that
> https://support.mozilla.org/1/firefox/38.0a1/Darwin/en-US/drm-content points
> to the article. It seems to work now, but please let me know if it doesn't
> work on your end.

Works here, just in time for my trying it for the context menu changes. :-)

Comment 31

[Javaun Moradi [:javaun]]

That's on me, I told Joni that the file name string for that URL was enable-drm.Sounds like we're good.

Comment 32

[:Gijs (he/him)]

Uplifted in bug 1136165.

Comment 33

[Bogdan Maris, Desktop QA]

Did some exploratory testing around this on Aurora and Nightly under Windows 7 64-bit and Mac OS X 10.9.5 and found no new issues.

Comment 34

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

According to Lawrence: "EME will not be enabled for testing in 37 and no further EME related changes will be approved for 37. EME testing on Beta will now target 38 Beta 1.". Removing the qe-verify flag as this was already verified for Firefox 38 and won't be verified for Firefox 37.

Comment 35

[Joni Chan]

The SUMO article has been reviewed: https://support.mozilla.org/kb/enable-drm

It's still hidden from search and accessible only via the link.

Comment 36

[mcdavis941 (sporadically reading bugmail)]

- (2015-03-05) (Fx37) https://hg.mozilla.org/releases/mozilla-beta/rev/007cc5f2f96e