Closed Bug 1111153 Opened 9 years ago Closed 9 years ago

[EME] Show a notification bar when EME content cannot be played

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox37 --- fixed
firefox38 --- verified

People

(Reporter: Dolske, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 5 obsolete files)

See user stories in bug 1089885 (CDM unsupported) and bug 1089883 (CDM disabled).

If the user attempts to play DRM content, but that fails due the the relevant (required) CDM being unsupported or disabled, we should show UI (like a notification bar) to indicate what the problem is (and allow fixing it, when the CDM is simply disabled).

This should probably also handle the special case of a CDM being required which the browser is still in the process of trying to install (either because you just installed Firefox and the CDMs are still downloading, or if some issue is preventing the CDM install).
sevaan: This was underspecified in the UX summary, which seemed to assume we'd always be using Firefox's default video controls. See bug 1089883 comment 3. Assuming this is what we want, we'll need icons for the notification bar, and a spec for its text / button actions.
Flags: needinfo?(sfranks)
Depends on: 1111160
Blocks: 1083662
Flags: qe-verify+
Flags: firefox-backlog+
Hi Matej, can you help us craft some strings here? Some helpful background:

Playback of DRM content requires a Content Decryption Module (CDM) which gets downloaded and activated once a user installs/updates Firefox. Some people don't like DRM so they may disable this afterwards. In some cases, those users may go to a page that uses DRM and see the page does not work. So we want to present them with a notification bar to help them fix/understand the error.

There will be two notification bars, one for a CDM that has been disabled ala the example above, and one for CDMs that are not supported.
I've started with some strings below and would appreciate your craftsmanship.

Note: Added alternate word options to the string below


*DISABLED CDM*

String Option #1:
The content/media on this page/site/website is locked/protected by DRM. Would you like to turn on/re-activate/activate playback of DRM-protected content? Learn more.

Buttons:
- [Play DRM-locked/protected content]
- *[No thanks]* ...(default selection)
-- Possible "Don't show this again"?

String Option #2:
The media on this page is locked by DRM. Would you like to play this content? Learn more.

Buttons:
-[Play DRM-locked/protected content]
- *[No thanks]* ...(default selection)
-- Possible "Don't show this again"?

Q: Do we want to make mention here somehow that the user turned off DRM in the past?


*UNSUPPORTED CDM*

String Option #2:
Firefox is unable to play this content as it is locked by DRM that we do not currently support. Learn more.

Button:
- [Dismiss]

String Option #2: 
The content/media on this page/site is locked/protected by DRM that Firefox does not support. Learn more.

Button:
- [Dismiss]
Flags: needinfo?(sfranks) → needinfo?(matej)
(In reply to Sevaan Franks [:sevaan] from comment #2)
> 
> *DISABLED CDM*
> 
> String Option #2:
> The media on this page is locked by DRM. Would you like to play this
> content? Learn more.
> 
> Buttons:
> -[Play DRM-locked/protected content]
> - *[No thanks]* ...(default selection)
> -- Possible "Don't show this again"?

I would use this one, with minor tweaks:

The media on this page is protected by DRM. Would you like to play it? Learn more.

-[Play media]
- *[No thanks]* ...(default selection)
-- Possible "Don't show this again"?


> Q: Do we want to make mention here somehow that the user turned off DRM in
> the past?

I get the feeling that this kind of user will know they did it, so I don't think it's necessary.


> *UNSUPPORTED CDM*
> 
> String Option #2: 
> The content/media on this page/site is locked/protected by DRM that Firefox
> does not support. Learn more.
> 
> Button:
> - [Dismiss]

I like this one, which the following options:

The media on this page is protected by DRM that Firefox does not support. Learn more.

Button:
- [Dismiss]
Flags: needinfo?(matej)
Attached image Notification Bars (obsolete) —
Mockup of notification bars attached.
Attachment #8551846 - Flags: ui-review?(philipp)
Attachment #8551846 - Flags: feedback?(dolske)
Comment on attachment 8551846 [details]
Notification Bars

Did you consider giving the default action button a slightly different appearance? Or is that not possible with our current notification bar code?
Flags: needinfo?(sfranks)
Attachment #8551846 - Flags: ui-review?(philipp) → ui-review+
(In reply to Philipp Sackl [:phlsa] please use needinfo to make me respond from comment #5)
> Comment on attachment 8551846 [details]
> Notification Bars
> 
> Did you consider giving the default action button a slightly different
> appearance? Or is that not possible with our current notification bar code?

While we /can/ add styling for default buttons, it's not generally done.

Interestingly/ironically, I don't think we can currently put the little dropdown arrow on the options part of the notification bar (see also e.g. the default browser notification bar). I'm guessing that's not actually a showstopper, although this isn't the only notification bar that has a dropdown menu and we should really just be getting the UI right at least for that bit. :-\
Phlsa: I played around with some ideas and chatted with Shorlander about it. In the end we decided to work within our existing parameters.

Is it your preference to have the default action be difference? I had a hard time coming up with a style that worked. Would need to defer to shorlander or mmaslaney for their input.

> Interestingly/ironically, I don't think we can currently put the little
> dropdown arrow on the options part of the notification bar (see also e.g.
> the default browser notification bar). I'm guessing that's not actually a
> showstopper, although this isn't the only notification bar that has a
> dropdown menu and we should really just be getting the UI right at least for
> that bit. :-\

Yes, it's not a showstopper, and we can add the "Don't show this again" when we get our new notification bar UI going. I think it's an important piece of functionality to have going forward though.
Flags: needinfo?(sfranks)
Just came across this: http://cl.ly/image/1M0n2t3r1w00

Couldn't we just include a down arrow as a character?
(In reply to Sevaan Franks [:sevaan] from comment #8)
> Just came across this: http://cl.ly/image/1M0n2t3r1w00
> 
> Couldn't we just include a down arrow as a character?

I don't gamble, but if I did I would bet you 10 dollars that localizers will either have issues with using it correctly, or the fonts in use on differently localized versions of Windows will not have glyphs for it and the alignment will go haywire (due to using glyphs from different fonts) or we'll show the dreaded unicode character box.

IOW: no, not "just". We should just fix it properly, quite orthogonal to this bug. It's not super hard. Please file a separate bug, CC me, mark fx-backlog+, and stick it in the backlog generation spreadsheet for the iteration that starts tomorrow. :-)
(In reply to Matej Novak [:matej] from comment #3)

> I would use this one, with minor tweaks:
> 
> The media on this page is protected by DRM. Would you like to play it? Learn
> more.

I do worry a bit that this is too oblique about its effect (i.e. it re-enables the disabled CDM or global setting). And so we should be a little more explicit about it. In other cases I think it's great to have a "fine, whatever, just make it work" button, but given that users who have disabled this may have done so for principled and/or emotional reasons, I wouldn't want them to feel like they've been tricked into turning it back on. Although the message does clearly say "DRM", so maybe I'm just being overly paranoid.

But I don't have a suggestion I feel is much better. The variations I toyed with all felt wordy and complex, due to trying to make a distinction between what the page wants, why it's not working, and what the fix is.

Best attempt is "The media on this page is protected by DRM you've disabled in Firefox. Would you like to play it?", with a button to "Reenable DRM" (or "Enable DRM"). 


Also, small aside for the record:

"DRM" is jargon, but it's succinct and avoids getting into the perceived doublespeak around what "digital rights management" means (and it's still jargony). I assume we're just going to ignore this issue, and let the "Learn More" page deal with explaining what DRM is. Especially given that most users won't ever see this.
Comment on attachment 8551846 [details]
Notification Bars

I'd agree with Gijs that we should punt the down-arrow bit to a separate bug. We already ship the popup blocker notification bar this way.

I think it's also fine to punt on "don't show this again" entirely, at least for now... I'd expect that a few users are only going to see this at all, and basically only on pages whose primary purpose is playing DRM media (like Netflix). If you disable DRM, you're quickly not going to be visiting those pages very often, and so the problem solves itself.
Attachment #8551846 - Flags: feedback?(dolske) → feedback+
I've been working on some related copy in another bug. Due to the sensitivities around the word "protected" in this context, as well as the suggestions in comment 10, here are another couple of options:

You can not play the media on this page because you have previously disabled DRM. Would you like re-enable it now? Learn more.

You must re-enable DRM to play the media on the page. Would you like to play it now? Learn more.
Points: --- → 5
I like this a lot

> You must enable DRM to play the media on the page. Would you like to play
> it now? Learn more.

I don't feel we need to say "re-enable", I believe "enable" is fine and addresses both the use cases of someone turning it off and never turning it on. For someone who turned it off out of principle, in their minds they may never have consented to "enable" it, even if it was on a while. 

The word "DRM" in there is adequate signal to the knowledgeable as to why they never "enabled" it before. 

Perhaps we could also shorten the second line:

   "Play it now?", or 

   "Enable it now?"
(In reply to Javaun Moradi [:javaun] from comment #13)
> I like this a lot
> 
> > You must enable DRM to play the media on the page. Would you like to play
> > it now? Learn more.
> 
> I don't feel we need to say "re-enable", I believe "enable" is fine and
> addresses both the use cases of someone turning it off and never turning it
> on. For someone who turned it off out of principle, in their minds they may
> never have consented to "enable" it, even if it was on a while. 
> 
> The word "DRM" in there is adequate signal to the knowledgeable as to why
> they never "enabled" it before. 
> 
> Perhaps we could also shorten the second line:
> 
>    "Play it now?", or 
> 
>    "Enable it now?"

How about "Would you like to proceed?" It's such a short line. It would be great to not have to repeat "enable" or "play" again.
Sounds great. I'll send this up the chain if there are no objections.
(In reply to Javaun Moradi [:javaun] from comment #15)
> Sounds great. I'll send this up the chain if there are no objections.

Great. Here's the whole thing:

You must enable DRM to play the media on this page. Would you like to proceed? Learn more.
WFM.
Taking for the next iteration...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
(In reply to Matej Novak [:matej] from comment #16)

> Great. Here's the whole thing:
> 
> You must enable DRM to play the media on this page. Would you like to
> proceed? Learn more.

Hmm, so I think we might have missed a few details while getting to this particular final string. To summarize the cases and make sure we' know what to implement... There are 4 flavors of notifications this bug is looking to handle:

1) DRM is globally disabled
2) A specific CDM is disabled
3) The content uses an unsupported/unknown CDM
4) Firefox is still in the middle of downloading the CDM to use

* Do we want to use the same string for #1 and #2? ("You must enable DRM to play the media on this page. Would you like to proceed?")

* Are the buttons still "Play media" and "No thanks"? (per comment 3 / attachment 8551846 [details])

* What string to use for #3? The last string for this was in comment 3, "The media on this page is protected by DRM that Firefox does not support. Learn more. [Dismiss]" But comment 12 notes that we want to avoid using the word "protected". Perhaps just "The media on this page is using DRM that..."?

* We don't have a proposed string for case #4. It wasn't part of the original requirements, and is a bit edge-casey, but it seems useful and would be wise to have a string landed so we can use it. Strawman: "Firefox is still downloading components needed to play media in this page. Please try again later. [Dismiss]"
Flags: needinfo?(matej)
(In reply to Justin Dolske [:Dolske] from comment #19)

Here's an option for each case:


> 1) DRM is globally disabled

You must enable DRM to play the audio or video on this page. Would you like to proceed? Learn more.


> 2) A specific CDM is disabled

You must enable DRM to play some audio or video on this page. Would you like to proceed? Learn more.


> 3) The content uses an unsupported/unknown CDM

The audio or video on this page requires DRM software that Firefox does not support. Learn more.


> 4) Firefox is still in the middle of downloading the CDM to use

Firefox is installing components needed to play the audio or video on this page. Please try again later. [Dismiss]

(I changed "downloading" to "installing" since that feels more all encompassing.)


> * Do we want to use the same string for #1 and #2?

I used "the audio or video" for #1 and "some audio or video" for #2. Does that make sense?


> * Are the buttons still "Play media" and "No thanks"?

I think these work, but I wonder if "Enable DRM" makes it more explicit about what the user is about to do.
Flags: needinfo?(matej)
Attached image Notification Bars (Updated) (obsolete) —
Updated mockups with new text/icon.
Having seen Sevaan's revised comps, Philipp suggested that we could drop the "Would you like to proceed?" text in scenarios 1,2 above, since we have a button to "Play Media". 

What do you think?
attachment 8563013 [details]
Flags: needinfo?(matej)
Attached image Notification Bars (Update #2) (obsolete) —
Removed "Would you like to proceed".
(In reply to Javaun Moradi [:javaun] from comment #22)
> Having seen Sevaan's revised comps, Philipp suggested that we could drop the
> "Would you like to proceed?" text in scenarios 1,2 above, since we have a
> button to "Play Media". 
> 
> What do you think?
> attachment 8563013 [details]

That sounds right to me, but should be button be "Play Media" or "Enable DRM"?

I think I'd be more likely to click the former if I weren't paying attention. The latter would probably get me to think about it and read the line if I skipped it. I guess it depends which outcome is preferable.
Flags: needinfo?(matej)
My vote is for the latter, "Enable DRM". The user in step 1 or 2 has taken an deliberate, advanced action to opt-out by going into preferences. Going straight to "Play Media?" glosses over the prior choice and might be construed as unclear or misleading.
Flags: needinfo?(sfranks)
(In reply to Javaun Moradi [:javaun] from comment #25)
> My vote is for the latter, "Enable DRM". The user in step 1 or 2 has taken
> an deliberate, advanced action to opt-out by going into preferences. Going
> straight to "Play Media?" glosses over the prior choice and might be
> construed as unclear or misleading.

That sounds good if the user has previously opted out.
For the unsuspecting user who just came across this on Netflix, it is probably too technical a term.
I don't believe the user who just came to Netflix would see this message though. These notifications (1,2) are just for users who have disabled a DRM CDM in add-ons or globally turned it off in prefs. It should "Just Work"(TM) for everyone else unless they take those actions to opt-out.
(In reply to Sevaan Franks [:sevaan] from comment #21)
> Created attachment 8563013 [details]
> Notification Bars (Updated)
> 
> Updated mockups with new text/icon.

Do we need the "dismiss" button on the last two notifications? That's what the [x] is for, I would argue...

Also, should the CDM disabled message have the name of the DRM software involved? (IE "You must enable Adobe DRM software" or "You must enable ClearKey DRM software" or whatever?)
Depends on: 1133583
Attached image Notification Bars (Update #3) (obsolete) —
Updated the notification bar mockups to show "Enable DRM" and I have removed the Dismiss buttons ala Gijs' suggestion (good catch).
Attachment #8551846 - Attachment is obsolete: true
Attachment #8563013 - Attachment is obsolete: true
Attachment #8563579 - Attachment is obsolete: true
Flags: needinfo?(sfranks)
> Also, should the CDM disabled message have the name of the DRM software
> involved? (IE "You must enable Adobe DRM software" or "You must enable
> ClearKey DRM software" or whatever?)

Right now, we only have the brand name (Adobe) in the doorhanger. I can see us needing CDM name in the the notifications as soon as we support multiple CDMs to help avoid user confusion. Given the door hangers, notifications, context menu, etc. we might want to revisit a comprehensive messaging approach as to where we use vendor name and where we don't.

For v1 though, I'm in favor of punting and keeping it simple.
So if a site tries to start EME but fails, we can detect that and show UI, but what if the site also detects that failure and falls back to use Silverlight or Flash, or some other plugin? Then we'll be showing the notification bar saying the site requires something in order to work, but the site is still working...
(In reply to Chris Pearce (:cpearce) from comment #31)
> So if a site tries to start EME but fails, we can detect that and show UI,
> but what if the site also detects that failure and falls back to use
> Silverlight or Flash, or some other plugin? Then we'll be showing the
> notification bar saying the site requires something in order to work, but
> the site is still working...

I don't think we'd be able to do much for this case except hiding the notification again if we detect replacement plugin content (cue discussion about how exactly to detect replacement plugin content and distinguish it from other content).

Sounds to me like this should be a followup.
If we're (going to be) downloading a new version (per bug 1124031), should we be posting the same "Installing/Downloading" message we currently post for the CDM, potentially with added "Firefox needs to update the DRM software you have installed in order for this website to work" disclaimer, or something?
Flags: needinfo?(matej)
(In reply to Matej Novak [:matej] from comment #20)
> (In reply to Justin Dolske [:Dolske] from comment #19)
> 
> Here's an option for each case:
> 
> 
> > 1) DRM is globally disabled
> 
> You must enable DRM to play the audio or video on this page. Would you like
> to proceed? Learn more.
> 
> 
> > 2) A specific CDM is disabled
> 
> You must enable DRM to play some audio or video on this page. Would you like
> to proceed? Learn more.

So to be clear, when we say "A specific CDM is disabled", this means that DRM as a whole is enabled, but a specific type of DRM (ie the DRM specifically provided by $DRMVENDOR) is disabled.

> > * Do we want to use the same string for #1 and #2?
> 
> I used "the audio or video" for #1 and "some audio or video" for #2. Does
> that make sense?

This sounds like you think "the page has N audios/videos, and we can only play some of them". In reality, what happens is that the page will usually have 1 audio/video, which isn't playable because the DRM provided by $DRMVENDOR is disabled.

So, no, I think this should either say exactly the same (if we want to gloss over the difference) or mention something about the "type" of the DRM or mention the vendor by name.
(In reply to :Gijs Kruitbosch from comment #33)
> If we're (going to be) downloading a new version (per bug 1124031), should
> we be posting the same "Installing/Downloading" message we currently post
> for the CDM, potentially with added "Firefox needs to update the DRM
> software you have installed in order for this website to work" disclaimer,
> or something?

It could be something like:

Firefox is installing updates needed to play the audio or video on this page. Please try again later. [Dismiss]
Flags: needinfo?(matej)
Updated notification bar mockup after discussion with Gijs. Replaced "No Thanks" button with "Options" as we do not support split-buttons in our current notification bar UI.
Attachment #8565507 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #34)
> (In reply to Matej Novak [:matej] from comment #20)
> > (In reply to Justin Dolske [:Dolske] from comment #19)
> 
> > > * Do we want to use the same string for #1 and #2?
> > 
> > I used "the audio or video" for #1 and "some audio or video" for #2. Does
> > that make sense?
> 
> This sounds like you think "the page has N audios/videos, and we can only
> play some of them". In reality, what happens is that the page will usually
> have 1 audio/video, which isn't playable because the DRM provided by
> $DRMVENDOR is disabled.
> 
> So, no, I think this should either say exactly the same (if we want to gloss
> over the difference) or mention something about the "type" of the DRM or
> mention the vendor by name.

So there's no case where there would be two videos on a page, for example, but only one of them would be subject to DRM?
Per discussion out-of-band with Matej, using the same string for CDM/API disabled for now, both using 'some' because there could be non-DRM video on the same page. We can iterate on this afterwards if necessary. Also incorporating messaging for the update case that cpearce is still implementing. You can test the harder-to-get messages by firing off gEMEHandler.receiveMessage with {target: gBrowser.selectedBrowser, data: JSON.stringify(...)} as its param, and the clearkey stuff should be working on both OSX and Windows with a recent build with http://people.mozilla.org/~cpearce/mse-clearkey/
Attachment #8566551 - Flags: review?(florian)
Oh, and I bit the bullet and added a little bit of code to toolkit to support a documentfragment for the description. We've had that issue before where we want other content in the description...

Assuming you agree, we will need to update https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/notificationbox .

AFAICT other code (sync migration, for instance) has all created their own bindings instead, which I thought was ugly and annoying (a plethora of bindings that do very similar things but are named (and in browser/) for overly specific purposes like sync/translation/whatever isn't particularly useful), so I just went with this.
(Oh, lastly, I'm not updating the styling of the learn more link to match the design in attachment 8566525 [details] as we /just/ landed some for the sync migration bar (bug 1120716), and a lot of work (and !important force) went into making them look this way, and that seems like something we shouldn't bother messing with in this bug - we can deal with it in a followup if it's deemed very important)
Comment on attachment 8566551 [details] [diff] [review]
show error notifications for broken EME content,

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

Adding support for using a document fragment instead of just a string seems reasonable to me, but I don't think that would be enough for the case we had for translation.

Should we add a test for this new feature of the notification binding?

::: browser/base/content/browser-eme.js
@@ +69,5 @@
> +        break;
> +
> +      case "cdm-not-supported":
> +        notificationId = "drmContentCDMNotSupported";
> +        params = [document.getElementById("bundle_brand").getString("brandShortName")];

This is duplicated 4 times across this patch. Should we have a getter for it somewhere?

@@ +141,5 @@
> +      fragment.appendChild(descriptionContainer.childNodes[0]);
> +    }
> +
> +    // Ensure the link works:
> +    let labelLink = fragment.querySelector("label.text-link");

This seems fragile (a typo in a localized string and the link will be displayed but not work).

@@ +163,5 @@
> +    let btnAccessKeyId = msgPrefix + "button.accesskey";
> +
> +    let drmProvider = this.getDRMLabel(keySystem);
> +    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
> +    let message = gNavigatorBundle.getFormattedString(msgId, [drmProvider, brandName], 2);

The third parameter ('2') doesn't seem needed.

@@ +169,5 @@
> +
> +    let mainAction = {
> +      label: gNavigatorBundle.getString(btnLabelId),
> +      accessKey: gNavigatorBundle.getString(btnAccessKeyId),
> +      callback: function() { openPreferences("paneContent") },

nit: ';' after the openPreferences call.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +595,5 @@
>  emeNotifications.drmContentPlaying.message = Some audio or video on this site uses %1$S DRM software, which may limit what %2$S can let you do with it.
>  emeNotifications.drmContentPlaying.button.label = Configure…
>  emeNotifications.drmContentPlaying.button.accesskey = C
>  
> +emeNotifications.drmContentDisabled.message = You must enable DRM to play some audio or video on this page. <label class="text-link">Learn More</label>

Having "<label class="text-link">" in the localizable string doesn't seem good.

I would really prefer having the "Learn More" string be a separate string, and generating the label element with the JS code. I'm not sure if you would need to insert a placeholder in the strings so that localizers can decide where the "learn more" button will be inserted or if we can just put the string and then the button. I don't think our other UIs with a 'learn more' link offer to display some text after it for localized builds; so hopefully we can keep this simple.

::: toolkit/content/widgets/notification.xml
@@ +103,5 @@
>              var newitem = document.createElementNS(XULNS, "notification");
> +            // Can't use instanceof in case this was created from a different document:
> +            let labelIsDocFragment = aLabel && typeof aLabel == "object" && aLabel.nodeType &&
> +                                     aLabel.nodeType == aLabel.DOCUMENT_FRAGMENT_NODE;
> +            if (!labelIsDocFragment) {

nit: The surrounding code doesn't seem to use {} for one line if statements; I think we should be consistent.

@@ +153,5 @@
>                newitem.style.opacity = "0";
>              }
>              this.insertBefore(newitem, insertPos);
> +            // Can only insert the document fragment after the item has been created because
> +            // otherwise the XBL structure isn't there yet:

Are we sure the XBL structure is immediately there after inserting? I thought that was asynchronous.

@@ +156,5 @@
> +            // Can only insert the document fragment after the item has been created because
> +            // otherwise the XBL structure isn't there yet:
> +            if (labelIsDocFragment) {
> +              var d = document.getAnonymousElementByAttribute(newitem, "anonid", "messageText");
> +              d.appendChild(aLabel);

nit: I don't see what 'd' means here, so I think this would be as clear and shorted if you just did:
  document.getAnonymous...
          .appendChild(aLabel);
Attachment #8566551 - Flags: review?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #41)
> Comment on attachment 8566551 [details] [diff] [review]
> show error notifications for broken EME content,
> 
> Review of attachment 8566551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Adding support for using a document fragment instead of just a string seems
> reasonable to me, but I don't think that would be enough for the case we had
> for translation.
> 
> Should we add a test for this new feature of the notification binding?

Sounds fair, but can I follow-up bug that? :-)
 
> ::: browser/base/content/browser-eme.js
> @@ +69,5 @@
> > +        break;
> > +
> > +      case "cdm-not-supported":
> > +        notificationId = "drmContentCDMNotSupported";
> > +        params = [document.getElementById("bundle_brand").getString("brandShortName")];
> 
> This is duplicated 4 times across this patch. Should we have a getter for it
> somewhere?

Can do.

> @@ +141,5 @@
> > +      fragment.appendChild(descriptionContainer.childNodes[0]);
> > +    }
> > +
> > +    // Ensure the link works:
> > +    let labelLink = fragment.querySelector("label.text-link");
> 
> This seems fragile (a typo in a localized string and the link will be
> displayed but not work).

If the localizer gets either of these wrong (not a label, or not with this class) it will also not work / have the right styling. So I don't think this is a problem - otherwise, we'd set the href but it would still not work or not have the right styling... 

> @@ +163,5 @@
> > +    let btnAccessKeyId = msgPrefix + "button.accesskey";
> > +
> > +    let drmProvider = this.getDRMLabel(keySystem);
> > +    let brandName = document.getElementById("bundle_brand").getString("brandShortName");
> > +    let message = gNavigatorBundle.getFormattedString(msgId, [drmProvider, brandName], 2);
> 
> The third parameter ('2') doesn't seem needed.
> 
> @@ +169,5 @@
> > +
> > +    let mainAction = {
> > +      label: gNavigatorBundle.getString(btnLabelId),
> > +      accessKey: gNavigatorBundle.getString(btnAccessKeyId),
> > +      callback: function() { openPreferences("paneContent") },
> 
> nit: ';' after the openPreferences call.
> 
> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +595,5 @@
> >  emeNotifications.drmContentPlaying.message = Some audio or video on this site uses %1$S DRM software, which may limit what %2$S can let you do with it.
> >  emeNotifications.drmContentPlaying.button.label = Configure…
> >  emeNotifications.drmContentPlaying.button.accesskey = C
> >  
> > +emeNotifications.drmContentDisabled.message = You must enable DRM to play some audio or video on this page. <label class="text-link">Learn More</label>
> 
> Having "<label class="text-link">" in the localizable string doesn't seem
> good.
> 
> I would really prefer having the "Learn More" string be a separate string,
> and generating the label element with the JS code. I'm not sure if you would
> need to insert a placeholder in the strings so that localizers can decide
> where the "learn more" button will be inserted or if we can just put the
> string and then the button.

I'm pretty sure we would need to use a replacement pattern.

> I don't think our other UIs with a 'learn more'
> link offer to display some text after it for localized builds; so hopefully
> we can keep this simple.

> ::: toolkit/content/widgets/notification.xml
> @@ +103,5 @@
> >              var newitem = document.createElementNS(XULNS, "notification");
> > +            // Can't use instanceof in case this was created from a different document:
> > +            let labelIsDocFragment = aLabel && typeof aLabel == "object" && aLabel.nodeType &&
> > +                                     aLabel.nodeType == aLabel.DOCUMENT_FRAGMENT_NODE;
> > +            if (!labelIsDocFragment) {
> 
> nit: The surrounding code doesn't seem to use {} for one line if statements;
> I think we should be consistent.
> 
> @@ +153,5 @@
> >                newitem.style.opacity = "0";
> >              }
> >              this.insertBefore(newitem, insertPos);
> > +            // Can only insert the document fragment after the item has been created because
> > +            // otherwise the XBL structure isn't there yet:
> 
> Are we sure the XBL structure is immediately there after inserting? I
> thought that was asynchronous.

I'm pretty sure there are bazillions of things that depend on this being sync (or at least forced to be sync if you ask for things in it straight after), so I would be very surprised.

> 
> @@ +156,5 @@
> > +            // Can only insert the document fragment after the item has been created because
> > +            // otherwise the XBL structure isn't there yet:
> > +            if (labelIsDocFragment) {
> > +              var d = document.getAnonymousElementByAttribute(newitem, "anonid", "messageText");
> > +              d.appendChild(aLabel);
> 
> nit: I don't see what 'd' means here

description, because it's a <xul:description> element that I'm getting.

>, so I think this would be as clear and
> shorted if you just did:
>   document.getAnonymous...
>           .appendChild(aLabel);

Meh. This makes me think that you're appending to the document. In any case, I can single-line it if you think that makes more sense.
In particular, regarding sync binding construction, see:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/notification.xml#70
This should address your feedback, I believe. :-)
Attachment #8566764 - Flags: review?(florian)
Attachment #8566551 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #42)

> > Should we add a test for this new feature of the notification binding?
> 
> Sounds fair, but can I follow-up bug that? :-)

Fair enough :-)

> > @@ +141,5 @@
> > > +      fragment.appendChild(descriptionContainer.childNodes[0]);
> > > +    }
> > > +
> > > +    // Ensure the link works:
> > > +    let labelLink = fragment.querySelector("label.text-link");
> > 
> > This seems fragile (a typo in a localized string and the link will be
> > displayed but not work).
> 
> If the localizer gets either of these wrong (not a label, or not with this
> class) it will also not work / have the right styling. So I don't think this
> is a problem - otherwise, we'd set the href but it would still not work or
> not have the right styling...

Right. My point is I would prefer if a localizer typo-ing on a single character couldn't break something. Especially as this UI won't be visible very frequently, so I don't expect localizers dogfooding their work to actually notice if it's broken.

> > Having "<label class="text-link">" in the localizable string doesn't seem
> > good.
> > 
> > I would really prefer having the "Learn More" string be a separate string,
> > and generating the label element with the JS code. I'm not sure if you would
> > need to insert a placeholder in the strings so that localizers can decide
> > where the "learn more" button will be inserted or if we can just put the
> > string and then the button.
> 
> I'm pretty sure we would need to use a replacement pattern.
> 
> > I don't think our other UIs with a 'learn more'
> > link offer to display some text after it for localized builds; so hopefully
> > we can keep this simple.

If you would like a second opinion we can double check with flod :-).

> > Are we sure the XBL structure is immediately there after inserting? I
> > thought that was asynchronous.
> 
> I'm pretty sure there are bazillions of things that depend on this being
> sync (or at least forced to be sync if you ask for things in it straight
> after), so I would be very surprised.

Yeah, I didn't fully think this through, sorry.
Comment on attachment 8566764 [details] [diff] [review]
show error notifications for broken EME content,

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

::: browser/base/content/browser-eme.js
@@ +23,5 @@
> +    let text = gNavigatorBundle.getString("emeNotifications." + msgId + ".learnMoreLabel");
> +    let label = document.createElement("label");
> +    label.textContent = text;
> +    let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
> +    // This because outerHTML produces an unnecessary xmlns attribute

The comment should possibly mention too that this dance is needed to escape HTML special chars that may exist once the string is localized (it took me a few seconds to realize this is what you are doing here).

@@ +188,5 @@
> +  return document.getElementById("bundle_brand").getString("brandShortName");
> +});
> +
> +XPCOMUtils.defineLazyGetter(gEMEHandler, "_learnMoreLink", function() {
> +});

dead code?

::: browser/base/content/browser.xul
@@ +571,5 @@
> +    <menupopup id="emeNotificationsPopup">
> +      <menuitem id="emeNotificationsNotNow"
> +                label="&emeNotificationsNotNow.label;"
> +                acceskey="&emeNotificationsNotNow.accesskey;"
> +                oncommand="gEMEHandler.onNotNow(this)"/>

nit: missing ';' in the 2 oncommand handlers here.

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +595,5 @@
>  emeNotifications.drmContentPlaying.message = Some audio or video on this site uses %1$S DRM software, which may limit what %2$S can let you do with it.
>  emeNotifications.drmContentPlaying.button.label = Configure…
>  emeNotifications.drmContentPlaying.button.accesskey = C
>  
> +# LOCALIZATION NOTE(emeNotifications.drmContentDisabled.message): %S will be the 'learn more' link

I hope localizers won't introduce < or & characters when translating this.
Attachment #8566764 - Flags: review?(florian) → review+
Blocks: 1135010
Blocks: 1135013
Blocks: 1135014
https://hg.mozilla.org/mozilla-central/rev/80bd1ae9dd0a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Blocks: 1136165
Uplifted in bug 1136165.
Depends on: 1140377
Did some exploratory testing around this on Aurora and Nightly under Windows 7 64-bit and Mac OS X 10.9.5 and found a new issues that is a duplicate.
Status: RESOLVED → VERIFIED
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: