1129219 - Add a link to learn more about EME/CDMs from a CDMs entry in the add-ons manager

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Matthew N. [:MattN]]

Bug 1089882 wants an additional link for EME entries in the add-ons manager to provide information to the user:

(Quoting Kev Needham [:kev] from bug 1089882 comment #0)
> As a user, I am able to learn more about content protected by Encrypted
> Media Extensions - and the limitations it may place on the use of that
> content - through a link to more information in the CDM listing in the
> Addons->Plugins page so I can better understand the limitations of protected
> content, what entities place those limitations on that content, and what
> alternatives and/or recourse I have.

Bug 1089888 talks about possibly remotely controlling the URL.

Kev, is it fine to point to a static URL on a Mozilla webpage that we can change when required? If so, do we know what the URL should be yet?

Comment 1

[Justin Dolske [:Dolske]]

Static URL for the Learn More link is fine, see what we did in bug 1111146 comment 9 (app.support.baseURL + "drm-content").

Comment 2

[Chris Peterson [:cpeterson]]

The Learn More URL is in bug 1127290 comment 16. The URL isn't public yet.

Comment 3

[:Gijs (he/him)]

Taking per email discussion.

Comment 4

[:Gijs (he/him)]

Attachment: add learn more link for EME provider,

I couldn't find a toolkit-accessible string in a .properties file that wasn't scary (reusing the 'blocked plugin notification' strings seemed like a bad idea for l10n). For the branch patch, should I just get over my fear or hardcode things?

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8573881 [details] [diff] [review]
add learn more link for EME provider,

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

(In reply to :Gijs Kruitbosch from comment #4)
> I couldn't find a toolkit-accessible string in a .properties file that
> wasn't scary (reusing the 'blocked plugin notification' strings seemed like
> a bad idea for l10n). For the branch patch, should I just get over my fear
> or hardcode things?

In bug 1089867 it looks like we hard-coded the description string for uplift but I don't know if that's still the right thing to do as I don't know which locales this is getting released to. Maybe bsmedberg and/or l10n people would know better about the l10n situation.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +553,5 @@
> +    }
> +    if (aPlugin.licenseURL) {
> +      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
> +      rv.push("<xhtml:a href=\"" + aPlugin.licenseURL + "\" target=\"_blank\">" +
> +             licenseInfo + "</xhtml:a>.");

Nit: While you touching this stuff you could make them much nicer with template strings. Since it looks like the link structure is the same you could also use a loop to append to rv.

@@ +555,5 @@
> +      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
> +      rv.push("<xhtml:a href=\"" + aPlugin.licenseURL + "\" target=\"_blank\">" +
> +             licenseInfo + "</xhtml:a>.");
> +    }
> +    return rv ? rv.join('<xhtml:br /><xhtml:br />') : undefined;

rv is always truthy here so I think you want to check .length but are you sure we won't end up stringifying to "undefined"? Perhaps it's safer to return an empty string?

Nit: Use double quotes

Comment 6

[Justin Dolske [:Dolske]]

For uplift to 37 we should hard code the string, as in the other patches.

38 is an interesting question, since this is Aurora. I would ask L10N if they'd prefer a hard-coded string or to break string freeze. (I'd sorta hope the latter, since it's small and similar to other things we have, so low-effort for localizers.)

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #5)
> ::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
> @@ +553,5 @@
> > +    }
> > +    if (aPlugin.licenseURL) {
> > +      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
> > +      rv.push("<xhtml:a href=\"" + aPlugin.licenseURL + "\" target=\"_blank\">" +
> > +             licenseInfo + "</xhtml:a>.");
> 
> Nit: While you touching this stuff you could make them much nicer with
> template strings. Since it looks like the link structure is the same you
> could also use a loop to append to rv.

Done.

> @@ +555,5 @@
> > +      let licenseInfo = pluginsBundle.GetStringFromName(GMP_LICENSE_INFO);
> > +      rv.push("<xhtml:a href=\"" + aPlugin.licenseURL + "\" target=\"_blank\">" +
> > +             licenseInfo + "</xhtml:a>.");
> > +    }
> > +    return rv ? rv.join('<xhtml:br /><xhtml:br />') : undefined;
> 
> rv is always truthy here so I think you want to check .length but are you
> sure we won't end up stringifying to "undefined"? Perhaps it's safer to
> return an empty string?

It used to be undefined before my changes, but also, yes:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#2805

checks if it's falsy, which |undefined| will satisfy.


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

Comment 8

[:Gijs (he/him)]

Boo at broken tests, fix pushed:

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


(thanks to Mossop for telling me how to fix!)

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/ef482a54a912
https://hg.mozilla.org/mozilla-central/rev/d164b59f3b94

Comment 10

[Chris Peterson [:cpeterson]]

Gijs: I think we need to uplift this "Learn more about EME" link to Aurora 38. Adobe plans to start public beta testing in Firefox Beta 38.

Comment 11

[:Gijs (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #10)
> Gijs: I think we need to uplift this "Learn more about EME" link to Aurora
> 38. Adobe plans to start public beta testing in Firefox Beta 38.

Ugh, sorry, I dropped the ball on this. Axel, per comment #6, what do we want to do for uplift here?

Comment 12

[Axel Hecht [:Pike]]

We're two days past end-of-sluggy for l10n on aurora, I'd like to understand the actual impact. I have a hard time figuring out if EME is going to touch people in any significant amount any time soon. And if it was, how this string works in practice.

Comment 13

[Justin Dolske [:Dolske]]

The actual impact is that we are planning to ship this feature in Firefox 38, live and exposed for everyone, and with one or more major sites making use of it.

As for how the string works: look at the OpenH264 provider in the addons manager (under plugins), and click the "More" link in its row to expand it to full-page. CDMs will look similar to this, and this patch adds a "Learn More" link above the "License Information" link. The link goes to a SUMO page.

Comment 14

[Axel Hecht [:Pike]]

For l10n on 38, this is really bad timing.

We looked at a few options, including just uplifting, using an existing string, and hard-coding. Hard-coding at this point is ugh, re-using an existing string is tricky. There's a few "More Information" link titles in extensions.properties, maybe use one of those?

I'd reraise if we really need to uplift this, too. Reading bug 1127290 comment 14, we're launching this for 32bit windows only. Then we're having the folks that click on about:addons, find the plugins tab, expand a single plugin, and then click on a link to sumo. Having a page on sumo is great, but this doesn't look like the canonical way to find it.

Kev, as you filed the original user story, is this one good enough to ride the trains?

Comment 15

[Justin Dolske [:Dolske]]

(In reply to Axel Hecht [:Pike] from comment #14)

> There's a few "More Information" link titles in
> extensions.properties, maybe use one of those?

That's not a terrible idea. Other than it usually being you telling us that reusing strings is a bad idea. ;-) But maybe it's a least-worst option.

> Having a page on sumo is great,
> but this doesn't look like the canonical way to find it.

I agree that's it's fairly buried in the UI, and not something users will commonly be seeing, and reasonable to consider cutting. The decision for Product is if the sensitivity around DRM and desire to over-communicate makes this ok to cut.

Javaun is currently chasing this down, so moving the NI over to him for an answer.

Comment 16

[Javaun Moradi [:javaun]]

Thanks, just ran this one down.  

We don't need this "Learn More" link in there in 38. In fact, we may pull it out of 39, having a link in the add-ons manager to go to our SUMO page isn't a firm requirement. It's not in the final, approved UX spec. We already have 3 inbound links in UI (context menu, Preference>Content global "Play DRM" pref, door hanger). We don't do it for OpenH264.

We'll make a call whether this stays in 39 or we pull it out. 

But, definitively, we don't need to take any action for 38.

Comment 17

[Chris Peterson [:cpeterson]]

OK. I'm marking this bug as fixed in 39 (unless we back it out) and wontfix in 38.

Comment 18

[Bogdan Maris, Desktop QA]

Verified that 'Link More' is present in latest Nightly using Windows 7 64-bit and Windows XP 64-bit, and it redirects to https://support.mozilla.org/de/kb/enable-drm?as=u&utm_source=inproduct.