Closed Bug 1111148 Opened 9 years ago Closed 9 years ago

[EME] Add doorhanger to notify the user when DRM content is being played

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox37 --- fixed
firefox38 --- verified

People

(Reporter: Dolske, Assigned: Gijs)

References

Details

Attachments

(3 files, 4 obsolete files)

See user stories in bug 1089869 and bug 1089871. Initial wordsmithing in bug 1095734.

When a page is playing DRM content, the browser should allow the user to discover that fact. This shouldn't be intrusive -- an icon-only doorhanger is sufficient. (i.e., an icon appears in the URL bar, but no doorhanger is shown unless the user clicks it.)

There are two variations: one for the first time the user encounters DRM (1089869), and one for future encounters (1089871).
Sevaan: We'll need an icon (for the URL bar, and for the doorhanger) and final wording. What's the button and action? Opening up the Plugins section in the Addon Manager?
Flags: needinfo?(sfranks)
Looping in Michael Maslaney re: the icon.

Kev, has work begun on the messaging, and if so, who is doing that?

> What's the button and action?

The action takes users to the Content section in the user prefs that contains our "Play DRM Content" checkbox.
Flags: needinfo?(sfranks)
Flags: needinfo?(mmaslaney)
Flags: needinfo?(kev)
Blocks: 1083662
Work has not commenced. Will ping Sevaan and Henri separately on this in the next two days, it's something I tried (and did not succeed) at wrangling prior to the break.

(In reply to Sevaan Franks [:sevaan] from comment #2)
> Looping in Michael Maslaney re: the icon.
> 
> Kev, has work begun on the messaging, and if so, who is doing that?
Flags: needinfo?(kev)
Flags: qe-verify+
Flags: firefox-backlog+
Attached file DRM_Icon.zip
SVG Icon assets, attached.
Flags: needinfo?(mmaslaney)
Comment on attachment 8550511 [details]
DRM_Icon.zip

Looks great, Michael! The chain concept is a fantastic one when it comes to DRM.

I have seen the chain used to signifying creating a hyperlink too though (like this in Wordpress: http://cl.ly/image/1i2g3I27433c). Not sure how prevalent that is, and if it will be confusing.
Attachment #8550511 - Flags: feedback?(shorlander)
Comment on attachment 8550511 [details]
DRM_Icon.zip

(In reply to Sevaan Franks [:sevaan] from comment #5)
> Comment on attachment 8550511 [details]
> DRM_Icon.zip
> 
> Looks great, Michael! The chain concept is a fantastic one when it comes to
> DRM.
> 
> I have seen the chain used to signifying creating a hyperlink too though
> (like this in Wordpress: http://cl.ly/image/1i2g3I27433c). Not sure how
> prevalent that is, and if it will be confusing.

It's a good metaphor here. I agree that it is also frequently used for "link", but I think it should be fine. We don't internally use it that way, that I know of.

Most of the other obvious metaphors for this also have other connotations. Frequently related to security, which I think we should avoid.

A more neutral approach would be to indicate that media is playing with a play icon and leave the rest to the messaging.
Attachment #8550511 - Flags: feedback?(shorlander) → feedback+
I think the Play icon is a great, neutral indicator. Michael, Kev, Dolske? Thoughts on this? Let's wrap this icon up.
(In reply to Sevaan Franks [:sevaan] from comment #7)
> I think the Play icon is a great, neutral indicator. Michael, Kev, Dolske?
> Thoughts on this? Let's wrap this icon up.

Personally, I don't think we should be neutral here; we have a message for the user (cf. bug 1095734) and the icon should match that message.

I also think a play icon would have the downside that it'll confuse users who try to click it in order to start the media, which won't work (and which I'd be unwilling to suggest investing the time/energy/code complexity into making work, nor would I think that logical considering the message next to it and the fact that we don't do that in any of our other doorhangers).
The chainlinks are fine with me. Let's go with that. It's not perfect (similar to hyperlink, possible connotation with security), but I don't think those are a big deal and I don't have a better proposal.

A neutral indicator would also be fine in principle, but the "play" overloading that Gijs notes seems more likely to be an actual issue. It's more frequently used as either an actual button, or as an indicator for something playing (eg the way YouTube puts it in the title of tabs playing videos). And worth noting it would be nice (but not required) to use the same/similar icon for other UI (like bug 1111153), where the media is actually _not_ playing. :)

Icon changes are easy, so it's pretty trivial to revisit this if we decide there's a better icon.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
Points: --- → 8
Dumb question time: how do we detect (on the Firefox chrome JS side) that EME media is playing? From bug 1111160 comment 1 it seems there's an "encrypted" event sent to the video element (so presumably bubbling up to the document) - is that the right thing to key off, or is there something else we should be using? From reading http://mxr.mozilla.org/mozilla-central/source/dom/media/fmp4/MP4Reader.cpp#379 that won't fire if the user has turned off EME entirely, so that sounds right, I guess?
Flags: needinfo?(cpearce)
(In reply to :Gijs Kruitbosch from comment #10)
> Dumb question time: how do we detect (on the Firefox chrome JS side) that
> EME media is playing?

https://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLMediaElement.webidl#54

And if you want to know if it's loaded (i.e. decryption was setup successfully), also check if the readyState is >= HAVE_METADATA and the element's mediaKeys attribute is non-null.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #11)
> (In reply to :Gijs Kruitbosch from comment #10)
> > Dumb question time: how do we detect (on the Firefox chrome JS side) that
> > EME media is playing?
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLMediaElement.
> webidl#54
> 
> And if you want to know if it's loaded (i.e. decryption was setup
> successfully), also check if the readyState is >= HAVE_METADATA and the
> element's mediaKeys attribute is non-null.

Sorry for not being clear. This tells us whether a media element is encrypted, given that media element, but what I want to know is, how do we know when (and for which tabs) to start showing the doorhanger (icon) ? IOW, is there some kind of event we can listen for? (constantly checking for media elements that have the requisite properties would be very inefficient...)

The easiest for us would be something in the observer service that passes the media element or document as the subject.
Flags: needinfo?(cpearce)
The events won't bubble out. We'll need to add something.

I think the easiest thing would be what you suggest; for Gecko to dispatch an observer service notification when an encrypted media first successfully starts up. I think on start up makes more sense than on playing.

Please file a bug in 'Core :: Video/Audio' describing exactly what you need (i.e. what subject you want, notification name), ni? me, and I'll get it assigned to someone on our team.
Flags: needinfo?(cpearce)
The event doesn't bubble? That's unfortunate. Looks like other media events don't bubble either -- is that just for performance and consistency?

Maybe we use the "requestMediaKeySystemSucceeded" event proposed in bug 1111160 comment 4?
The events fired at media elements are specified to be "simple events", which are specified to not bubble.
(In reply to Justin Dolske [:Dolske] from comment #14)
> Maybe we use the "requestMediaKeySystemSucceeded" event proposed in bug
> 1111160 comment 4?

A "requestMediaKeySystemSucceeded" message just means that a CDM *could* be created and talked to by JS.

There are some use cases where script would want to request the CDM without actually wanting to play a video.

So I think it makes more sense to add a new message that fires to chrome on successful EME playback, as then users can associate the UX resulting from that message with playing the EME video. If not, we could end up showing the UI when it's not obvious that the page is doing something with DRM video.
Depends on: 1127416
Attached file DRM_Icon_v2.zip
Attached, the updated icons.
Depends on: 1128492
This works for now; the icon will go away if you navigate. Bug 1128492 tracks getting an event for the unloading of the video element instead. Note that I've already had a go at doing bookkeeping of the number of video elements per page; I'm assuming metadata is only loaded once per element (and/or the unload event will fire if we reload it within the video element). Dolske and Florian, can you give initial feedback on the approach here?
Attachment #8557936 - Flags: feedback?(florian)
Attachment #8557936 - Flags: feedback?(dolske)
(In reply to :Gijs Kruitbosch from comment #18)
> Created attachment 8557936 [details] [diff] [review]
> show doorhanger for EME being played back,
> 
> This works for now; the icon will go away if you navigate. Bug 1128492
> tracks getting an event for the unloading of the video element instead. Note
> that I've already had a go at doing bookkeeping of the number of video
> elements per page; I'm assuming metadata is only loaded once per element
> (and/or the unload event will fire if we reload it within the video
> element)

... unfinished thought alert!

... the idea being that we'd decrement said counter when we hit the other event, and remove the notification.

See also the currently hardcoded vendor, which is passed by the content event and therefore should be easy to update to pass the 'real' info from the video element (which the content observer has access to).

Finally, we really need some shared CSS infra, for both jar.mn and for browser.css. I just realized I only added css for OS X...
Now with extra windows/linux CSS (untested, as I'm in the London office and don't have access to my Windows machine right now).
Attachment #8557940 - Flags: feedback?(florian)
Attachment #8557940 - Flags: feedback?(dolske)
Attachment #8557936 - Attachment is obsolete: true
Attachment #8557936 - Flags: feedback?(florian)
Attachment #8557936 - Flags: feedback?(dolske)
Comment on attachment 8557940 [details] [diff] [review]
show doorhanger for EME being played back,

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

::: browser/base/content/browser-eme.js
@@ +6,5 @@
> +let gBrowserEMEData = new WeakMap();
> +function gEMEListener(msg /*{target: browser, data: data} */) {
> +  let browser = msg.target;
> +  if (msg.name == "EMEVideo:MetadataLoaded") {
> +    let data = gBrowserEMEData.get(browser);

This variable name is a little bit confusing, at first I assumed it contained the value of msg.data.

@@ +11,5 @@
> +    if (!data) {
> +      data = {count: 1};
> +      gBrowserEMEData.set(browser, data);
> +    } else {
> +      data.count++;

Why isn't this counting done in the C++ code that triggers the observer notification? Assuming this first question has a reasonable answer, shouldn't this counting be done in the content process rather than in the chrome one?

@@ +19,5 @@
> +  // so as to remove the notification if the video element goes away
> +
> +  let notificationId = "drmContentPlaying";
> +  // Don't need to show if disabled, nor reshow if it's already there
> +  if (!Services.prefs.getBoolPref("browser.eme.ui.enabled") ||

Do we need to remove existing notifications if the pref is false? Or are we confident enough that users will restart after touching it from about:config?

@@ +35,5 @@
> +    callback: function() { openPreferences("paneContent"); },
> +    dismiss: true
> +  };
> +  PopupNotifications.show(browser, notificationId, message, anchorId,
> +                          mainAction, null, {dismissed: true, persistence: 0});

Should this notification be preserved when the tab is teared-off? If the answer is yes you can do it by adding:
  eventCallback: aTopic => aTopic == "swapping"
in the options object.

I don't see what |persistence: 0| does here, 0 seems to have the same effect as not setting that value at all.

::: browser/base/content/content.js
@@ +1068,5 @@
> +  switch (topic) {
> +    case "media-eme-metadataloaded":
> +      let win = subject.ownerDocument.defaultView.top;
> +      if (win != content) {
> +        return;

What is this actually doing?

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +591,5 @@
>  # origin for the sharing menu if no readable origin could be deduced from the URL.
>  getUserMedia.sharingMenuUnknownHost = Unknown origin
>  
> +# LOCALIZATION NOTE(emeNotifications.drmContentPlaying.message): %S is the vendor name of the DRM that's in use
> +emeNotifications.drmContentPlaying.message = Some audio or video on this site uses %S DRM software, which may limit what Firefox can let you do with it.

Should "Firefox" be &brandShortName; here?

::: browser/themes/linux/jar.mn
@@ +30,5 @@
>    skin/classic/browser/content-contextmenu.svg
>    skin/classic/browser/dots.png                             (../shared/dots.png)
>    skin/classic/browser/dots@2x.png                          (../shared/dots@2x.png)
> +  skin/classic/browser/drm-icon.svg                         (../shared/drm-icon.svg)
> +  skin/classic/browser/drm-icon-pressed.svg                 (../shared/drm-icon-pressed.svg)

From a quick look, these 2 files seem identical minus the colors used. Is there any chance they could be merged into only one, using the trick used at http://hg.mozilla.org/mozilla-central/annotate/3bf7ed413e87/browser/themes/shared/aboutSessionRestore.css#l26 / http://hg.mozilla.org/mozilla-central/annotate/3bf7ed413e87/toolkit/themes/shared/in-content/check.svg ?
Attachment #8557940 - Flags: feedback?(florian) → feedback+
(In reply to :Gijs Kruitbosch from comment #18)

> This works for now; the icon will go away if you navigate. Bug 1128492
> tracks getting an event for the unloading of the video element instead.

Hmm, I don't think that's necessary. I think it's adequate to show the doorhanger when DRM is activated, and leave it there until navigation. Trying to make it reflect the current state of the page isn't so useful, and risks becoming UI that appears and disappears at random.
(In reply to Justin Dolske [:Dolske] from comment #22)
> (In reply to :Gijs Kruitbosch from comment #18)
> 
> > This works for now; the icon will go away if you navigate. Bug 1128492
> > tracks getting an event for the unloading of the video element instead.
> 
> Hmm, I don't think that's necessary. I think it's adequate to show the
> doorhanger when DRM is activated, and leave it there until navigation.
> Trying to make it reflect the current state of the page isn't so useful, and
> risks becoming UI that appears and disappears at random.

OK. Marked bug 1128492 as WONTFIX accordingly.


(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> Comment on attachment 8557940 [details] [diff] [review]
> show doorhanger for EME being played back,
> 
> Review of attachment 8557940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-eme.js
> @@ +6,5 @@
> > +let gBrowserEMEData = new WeakMap();
> > +function gEMEListener(msg /*{target: browser, data: data} */) {
> > +  let browser = msg.target;
> > +  if (msg.name == "EMEVideo:MetadataLoaded") {
> > +    let data = gBrowserEMEData.get(browser);
> 
> This variable name is a little bit confusing, at first I assumed it
> contained the value of msg.data.
> 
> @@ +11,5 @@
> > +    if (!data) {
> > +      data = {count: 1};
> > +      gBrowserEMEData.set(browser, data);
> > +    } else {
> > +      data.count++;
> 
> Why isn't this counting done in the C++ code that triggers the observer
> notification? Assuming this first question has a reasonable answer,
> shouldn't this counting be done in the content process rather than in the
> chrome one?

We're going to get rid of this per the last comment, so *shrug*, but, the counting was meant to work per-browser element. None of the C++ or content process JS has any idea about browsers, AFAICT, although I guess the content process JS could theoretically do it per top-level document. Not sure if that'd be better/worse.

> @@ +19,5 @@
> > +  // so as to remove the notification if the video element goes away
> > +
> > +  let notificationId = "drmContentPlaying";
> > +  // Don't need to show if disabled, nor reshow if it's already there
> > +  if (!Services.prefs.getBoolPref("browser.eme.ui.enabled") ||
> 
> Do we need to remove existing notifications if the pref is false? Or are we
> confident enough that users will restart after touching it from about:config?

We're planning to get rid of the pref RSN, basically as soon as we're happy with the implementation of EME. It's not intended to be permanent and was basically a stopgap to ensure we don't accidentally end up exposing UI before we've fully implemented the functionality or... something. See bug 1111146.

In other words, I don't think we care about turning off the pref while the notification is in use.

> @@ +35,5 @@
> > +    callback: function() { openPreferences("paneContent"); },
> > +    dismiss: true
> > +  };
> > +  PopupNotifications.show(browser, notificationId, message, anchorId,
> > +                          mainAction, null, {dismissed: true, persistence: 0});
> 
> Should this notification be preserved when the tab is teared-off? If the
> answer is yes you can do it by adding:
>   eventCallback: aTopic => aTopic == "swapping"
> in the options object.

I don't know? Depends on if the document is preserved and video keeps playing. If it's restarted/reloaded, it doesn't need to - it'll come back anyway. I'll try testing.

> I don't see what |persistence: 0| does here, 0 seems to have the same effect
> as not setting that value at all.

Good to know. I just read the docs, which don't state that it's optional. I'll try to remember to fix them.

> ::: browser/base/content/content.js
> @@ +1068,5 @@
> > +  switch (topic) {
> > +    case "media-eme-metadataloaded":
> > +      let win = subject.ownerDocument.defaultView.top;
> > +      if (win != content) {
> > +        return;
> 
> What is this actually doing?

Checking that the notification (which is observer service, so process-global, so will fire in every window's content.js!) is actually for a window/document we care about? Do you think there should be a comment? Is the check wrong? Not sure why you were asking this. The other use of the observer service in this file does the same kind of thing, but with docshells.

I'm assuming we get 1 content.js instance for every content window. Is that assumption wrong? (There are no docs that I've been able to find, so...)

> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +591,5 @@
> >  # origin for the sharing menu if no readable origin could be deduced from the URL.
> >  getUserMedia.sharingMenuUnknownHost = Unknown origin
> >  
> > +# LOCALIZATION NOTE(emeNotifications.drmContentPlaying.message): %S is the vendor name of the DRM that's in use
> > +emeNotifications.drmContentPlaying.message = Some audio or video on this site uses %S DRM software, which may limit what Firefox can let you do with it.
> 
> Should "Firefox" be &brandShortName; here?

Good catch! :-)

> ::: browser/themes/linux/jar.mn
> @@ +30,5 @@
> >    skin/classic/browser/content-contextmenu.svg
> >    skin/classic/browser/dots.png                             (../shared/dots.png)
> >    skin/classic/browser/dots@2x.png                          (../shared/dots@2x.png)
> > +  skin/classic/browser/drm-icon.svg                         (../shared/drm-icon.svg)
> > +  skin/classic/browser/drm-icon-pressed.svg                 (../shared/drm-icon-pressed.svg)
> 
> From a quick look, these 2 files seem identical minus the colors used. Is
> there any chance they could be merged into only one, using the trick used at
> http://hg.mozilla.org/mozilla-central/annotate/3bf7ed413e87/browser/themes/
> shared/aboutSessionRestore.css#l26 /
> http://hg.mozilla.org/mozilla-central/annotate/3bf7ed413e87/toolkit/themes/
> shared/in-content/check.svg ?

Good idea, I'd not looked at the contents. I'll have a look.

needinfo for the content.js check thing...
Flags: needinfo?(florian)
(In reply to :Gijs Kruitbosch from comment #23)

> > ::: browser/base/content/content.js
> > @@ +1068,5 @@
> > > +  switch (topic) {
> > > +    case "media-eme-metadataloaded":
> > > +      let win = subject.ownerDocument.defaultView.top;
> > > +      if (win != content) {
> > > +        return;
> > 
> > What is this actually doing?
> 
> Checking that the notification (which is observer service, so
> process-global, so will fire in every window's content.js!) is actually for
> a window/document we care about? Do you think there should be a comment? Is
> the check wrong? Not sure why you were asking this. The other use of the
> observer service in this file does the same kind of thing, but with
> docshells.

The keyword-uri-fixup observer seems to be doing this indeed.

This seems wasteful to me. It means if you have 100 tabs and one starts playing a DRM'ed video, you'll make 99 unneeded calls to the getters in subject.ownerDocument.defaultView.top.

For the webrtc notifications, I imported a js module from the content.js file and ensured from content.js that it was initialized. Then when receiving a notification, there's a way to get to the message manager for the relevant browser (http://hg.mozilla.org/mozilla-central/annotate/7c87286ce969/browser/modules/ContentWebRTC.jsm#l238) and send it an async message.
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> This seems wasteful to me. It means if you have 100 tabs and one starts
> playing a DRM'ed video, you'll make 99 unneeded calls to the getters in
> subject.ownerDocument.defaultView.top.

This is cheap, they're just some webidl bindings and getters which return local variables. The compiler will have optimized the ... out of this, all being well.

What's probably significantly less cheap is the 100 xpconnect C++ -> JS jumps we make because of the observer service.

Can you file a bug on the uri-fixup case? I'll fix the patch here to use a (content process) module.
Flags: needinfo?(florian)
(In reply to :Gijs Kruitbosch from comment #25)

> Can you file a bug on the uri-fixup case?

Filed bug 1128937.
Flags: needinfo?(florian)
Comment on attachment 8557940 [details] [diff] [review]
show doorhanger for EME being played back,

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +590,5 @@
>  # LOCALIZATION NOTE(getUserMedia.sharingMenuUnknownHost): this is used for the website
>  # origin for the sharing menu if no readable origin could be deduced from the URL.
>  getUserMedia.sharingMenuUnknownHost = Unknown origin
>  
> +# LOCALIZATION NOTE(emeNotifications.drmContentPlaying.message): %S is the vendor name of the DRM that's in use

Is it possible to have 2 different pieces of DRM software used by the same page? If so, how will that be displayed?
(In reply to Florian Quèze [:florian] [:flo] from comment #27)
> Comment on attachment 8557940 [details] [diff] [review]
> show doorhanger for EME being played back,
> 
> Review of attachment 8557940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/browser.properties
> @@ +590,5 @@
> >  # LOCALIZATION NOTE(getUserMedia.sharingMenuUnknownHost): this is used for the website
> >  # origin for the sharing menu if no readable origin could be deduced from the URL.
> >  getUserMedia.sharingMenuUnknownHost = Unknown origin
> >  
> > +# LOCALIZATION NOTE(emeNotifications.drmContentPlaying.message): %S is the vendor name of the DRM that's in use
> 
> Is it possible to have 2 different pieces of DRM software used by the same
> page? If so, how will that be displayed?

Theoretically, yes, practically, I don't think it's at all likely to happen (as this would require the server to support and use various types of CDMs, which we would also have to support; I don't see any good reason why a single page would use multiple different CDMs).

Considering the time and energy it took to even converge on this string (bug 1095734), I'm not going to spend effort on trying to optimize this here.
(In reply to :Gijs Kruitbosch from comment #28)

> > Is it possible to have 2 different pieces of DRM software used by the same
> > page? If so, how will that be displayed?
> 
> Theoretically, yes, practically, I don't think it's at all likely to happen
> (as this would require the server to support and use various types of CDMs,
> which we would also have to support; I don't see any good reason why a
> single page would use multiple different CDMs).

The case I had in mind was a webpage showing videos from other websites using iframes.
(In reply to Florian Quèze [:florian] [:flo] from comment #29)
> (In reply to :Gijs Kruitbosch from comment #28)
> 
> > > Is it possible to have 2 different pieces of DRM software used by the same
> > > page? If so, how will that be displayed?
> > 
> > Theoretically, yes, practically, I don't think it's at all likely to happen
> > (as this would require the server to support and use various types of CDMs,
> > which we would also have to support; I don't see any good reason why a
> > single page would use multiple different CDMs).
> 
> The case I had in mind was a webpage showing videos from other websites
> using iframes.

I'll file a followup bug for this, but I still don't think it needs to be a priority for now.
(In reply to :Gijs Kruitbosch from comment #28)

> > Is it possible to have 2 different pieces of DRM software used by the same
> > page? If so, how will that be displayed?
> 
> Theoretically, yes, practically, I don't think it's at all likely to happen
> (as this would require the server to support and use various types of CDMs,
> which we would also have to support; I don't see any good reason why a
> single page would use multiple different CDMs).
> 
> Considering the time and energy it took to even converge on this string (bug
> 1095734), I'm not going to spend effort on trying to optimize this here.

I think this is fine, the primary purpose of this is to let the user known DRM is being used, and the particular DRM vendor is just helping to provide some extra context/detail. So it's not really important to precisely indicate all flavors that might be in use (and I'd expect this case to be exceptionally rare).
Agreed, we can draw this out-of-scope for this bug and for launch.

Florian's right to raise it, it could surface with embeds. There are other string conversations that will happen as well.
Comment on attachment 8557940 [details] [diff] [review]
show doorhanger for EME being played back,

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

::: browser/base/content/content.js
@@ +1071,5 @@
> +      if (win != content) {
> +        return;
> +      }
> +      sendAsyncMessage("EMEVideo:MetadataLoaded", {
> +        // FIXME: this should be the actual DRM provider passed in the |data|

s/FIXME/bug #/
Attachment #8557940 - Flags: feedback?(dolske) → feedback+
Depends on: 1129362
Depends on: 1129370
This should address all the feedback so far, but for the tab dnd thing. I can't figure out how to actually get EME to work locally (I test with one of the testcases in dom/media/test/test_eme*, but they finish too quickly to try the dnd thing), and all the public testcases I've found by scouring BMO are broken by API changes in the EME APIs. Otherwise, I think this is ready for review.
Attachment #8559224 - Flags: review?(florian)
Attachment #8557940 - Attachment is obsolete: true
(In reply to :Gijs Kruitbosch from comment #34)
> I can't figure out how to actually get EME to work locally 

Point Nightly/mozilla-central at:
http://people.mozilla.org/~cpearce/mse-clearkey/
(In reply to Chris Pearce (:cpearce) from comment #35)
> (In reply to :Gijs Kruitbosch from comment #34)
> > I can't figure out how to actually get EME to work locally 
> 
> Point Nightly/mozilla-central at:
> http://people.mozilla.org/~cpearce/mse-clearkey/

I see:

EVENT: encrypted
video got encrypted event
Failed to request key system access. NotSupportedError: Operation is not supported
created MediaKeys object ok
video ensured MediaKeys available on HTMLMediaElement
video Failed to generate request. TypeError: video.mediaKeys is null
EVENT: encrypted
video got encrypted event
video ensured MediaKeys available on HTMLMediaElement
video Failed to generate request. TypeError: video.mediaKeys is null
EVENT: progress

on both self-built fx-team tip + this patch, and nightly (from the 3rd, which claims to be up-to-date for reasons I don't immediately understand). The automated dom/media/ EME tests pass locally, though...
EME only reliably works on Windows on mse-clearkey, I haven't had time to dive into why it doesn't work on Mac.
Comment on attachment 8559224 [details] [diff] [review]
show doorhanger for EME being played back,

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

::: browser/base/content/browser-eme.js
@@ +6,5 @@
> +function gEMEListener(msg /*{target: browser, data: data} */) {
> +  let browser = msg.target;
> +  let notificationId = "drmContentPlaying";
> +  // Don't need to show if disabled, nor reshow if it's already there
> +  // bug 1129362 covers supporting multiple CDM vendors here (which would

If this is wontfix I guess you can drop this comment.

@@ +26,5 @@
> +    callback: function() { openPreferences("paneContent"); },
> +    dismiss: true
> +  };
> +  PopupNotifications.show(browser, notificationId, message, anchorId,
> +                          mainAction, null, {dismissed: true});

I think it's safe to assume a video will continue playing when the tab is teared off.

@@ +29,5 @@
> +  PopupNotifications.show(browser, notificationId, message, anchorId,
> +                          mainAction, null, {dismissed: true});
> +};
> +
> +window.messageManager.addMessageListener("EMEVideo:MetadataLoaded", gEMEListener);

Do we need a matching removeMessageListener call somewhere? It seems most message listener added from browser.js are removed somewhere.

::: browser/base/content/content.js
@@ +31,5 @@
>    "resource://gre/modules/AboutReader.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode",
>    "resource://gre/modules/ReaderMode.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ContentObservers",
> +  "resource:///modules/ContentObservers.jsm");

ContentObservers is used unconditionally when the content.js file is loaded, so I think making this lazy just adds overhead.

::: browser/modules/ContentObservers.jsm
@@ +18,5 @@
> +let {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let gEMEUIObserver = function(subject, topic, data) {

Given that you are using a switch/case here and that you are planning for this file to be generic, I wonder if this shouldn't be just gObserver instead of gEMEUIObserver.

@@ +55,5 @@
> +      return;
> +    }
> +    _inited = true;
> +    Services.obs.addObserver(gEMEUIObserver, "media-eme-metadataloaded", false);
> +    contentGlobal.addEventListener("unload", this, false);

I don't think you'll leak anything if you never remove the observer as I think you want it for the whole duration of the content process. It looks to me like the way you are removing the observer here will make us stop observing media-eme-metadataloaded notifications once the first tab is closed.

@@ +64,5 @@
> +    }
> +  },
> +  uninit: function() {
> +    Services.obs.removeObserver(gEMEUIObserver, "media-eme-metadataloaded");
> +  },

If you really don't need to remove the observer, the 'init' method can be simplified to just the addObserver call. Or we could even just have the addObserver call not wrapped in any function/object; it would be executed the first time the module is imported (ie. no need for the _inited variable and the ContentObservers object anymore).

::: browser/themes/shared/drm-icon.svg
@@ +1,3 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
> +	 viewBox="0 0 16 16" enable-background="new 0 0 16 16" xml:space="preserve">

nit: mis-indented line.

@@ +15,5 @@
> +  </style>
> +  <defs>
> +    <linearGradient id="baseGradient" gradientUnits="userSpaceOnUse" x1="8" x2="8" y1="16" y2="0">
> +      <stop  offset="0" style="stop-color:#808080"/>
> +      <stop  offset="1" style="stop-color:#999999"/>

nit: double space between "<stop" and "offset" on these 2 lines.
Attachment #8559224 - Flags: review?(florian) → feedback+
(In reply to Florian Quèze [:florian] [:flo] from comment #38)
> Comment on attachment 8559224 [details] [diff] [review]
> show doorhanger for EME being played back,
> 
> Review of attachment 8559224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser-eme.js
> @@ +6,5 @@
> > +function gEMEListener(msg /*{target: browser, data: data} */) {
> > +  let browser = msg.target;
> > +  let notificationId = "drmContentPlaying";
> > +  // Don't need to show if disabled, nor reshow if it's already there
> > +  // bug 1129362 covers supporting multiple CDM vendors here (which would
> 
> If this is wontfix I guess you can drop this comment.
> 
> @@ +26,5 @@
> > +    callback: function() { openPreferences("paneContent"); },
> > +    dismiss: true
> > +  };
> > +  PopupNotifications.show(browser, notificationId, message, anchorId,
> > +                          mainAction, null, {dismissed: true});
> 
> I think it's safe to assume a video will continue playing when the tab is
> teared off.
> 
> @@ +29,5 @@
> > +  PopupNotifications.show(browser, notificationId, message, anchorId,
> > +                          mainAction, null, {dismissed: true});
> > +};
> > +
> > +window.messageManager.addMessageListener("EMEVideo:MetadataLoaded", gEMEListener);
> 
> Do we need a matching removeMessageListener call somewhere? It seems most
> message listener added from browser.js are removed somewhere.
> 
> ::: browser/base/content/content.js
> @@ +31,5 @@
> >    "resource://gre/modules/AboutReader.jsm");
> >  XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode",
> >    "resource://gre/modules/ReaderMode.jsm");
> > +XPCOMUtils.defineLazyModuleGetter(this, "ContentObservers",
> > +  "resource:///modules/ContentObservers.jsm");
> 
> ContentObservers is used unconditionally when the content.js file is loaded,
> so I think making this lazy just adds overhead.
> 
> ::: browser/modules/ContentObservers.jsm
> @@ +18,5 @@
> > +let {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> > +
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +let gEMEUIObserver = function(subject, topic, data) {
> 
> Given that you are using a switch/case here and that you are planning for
> this file to be generic, I wonder if this shouldn't be just gObserver
> instead of gEMEUIObserver.

No, because the reason I'm using a switch/case is that I expect there to be other EME-related observer notifications, see bug 1111160. I think individual "areas" of observing should still have their own observer.


> @@ +55,5 @@
> > +      return;
> > +    }
> > +    _inited = true;
> > +    Services.obs.addObserver(gEMEUIObserver, "media-eme-metadataloaded", false);
> > +    contentGlobal.addEventListener("unload", this, false);
> 
> I don't think you'll leak anything if you never remove the observer as I
> think you want it for the whole duration of the content process. It looks to
> me like the way you are removing the observer here will make us stop
> observing media-eme-metadataloaded notifications once the first tab is
> closed.

No, see bug 1048618 comment 11 through 14.
Ok, then isn't this keeping a reference to the global of the first tab until the content process is shutdown? Is there actually a need for removing this observer?
Actually, if I drag out a tab on my Windows machine that has an EME video playing, the video stops playing and seems not usable until I reload the tab (at which point, of course, the notification comes back anyway). I'm going to assume that's a bug and ideally the video should continue playing.
Attachment #8559900 - Flags: review?(florian)
Attachment #8559224 - Attachment is obsolete: true
(In reply to Florian Quèze [:florian] [:flo] from comment #40)
> Ok, then isn't this keeping a reference to the global of the first tab until
> the content process is shutdown?

Why?
Comment on attachment 8559900 [details] [diff] [review]
show doorhanger for EME being played back,

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

::: browser/base/content/content.js
@@ +1064,5 @@
>      dataURL: canvas.toDataURL("image/jpeg", ""),
>    });
>  });
> +
> +ContentObservers.init(this);

The 'this' you pass in here is the content global of the first tab, isn't it?

::: browser/modules/ContentObservers.jsm
@@ +55,5 @@
> +      return;
> +    }
> +    _inited = true;
> +    Services.obs.addObserver(gEMEUIObserver, "media-eme-metadataloaded", false);
> +    contentGlobal.addEventListener("unload", this, false);

so either it's GC'ed when the first tab is closed and this is never called. Or it's not, and it's kept around for the whole duration of the process.

Am I missing something?

@@ +63,5 @@
> +      this.uninit();
> +    }
> +  },
> +  uninit: function() {
> +    Services.obs.removeObserver(gEMEUIObserver, "media-eme-metadataloaded");

I'm still not entirely sure we need the removeObserver call here. For bug 973001 we didn't remove the content observers, and I don't think anything is leaked (or if it is, it's not leaked in a way that I could notice).
(In reply to Florian Quèze [:florian] [:flo] from comment #44)
> Comment on attachment 8559900 [details] [diff] [review]
> show doorhanger for EME being played back,
> 
> Review of attachment 8559900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/content.js
> @@ +1064,5 @@
> >      dataURL: canvas.toDataURL("image/jpeg", ""),
> >    });
> >  });
> > +
> > +ContentObservers.init(this);
> 
> The 'this' you pass in here is the content global of the first tab, isn't it?

As best I understand content.js is loaded exactly once per content process, and |this| will point to 
the ContentFrameMessageManager (which should stay alive while the process is alive), but our documentation is terrible so maybe I misunderstand. However, it isn't actually relevant because...

> ::: browser/modules/ContentObservers.jsm
> @@ +55,5 @@
> > +      return;
> > +    }
> > +    _inited = true;
> > +    Services.obs.addObserver(gEMEUIObserver, "media-eme-metadataloaded", false);
> > +    contentGlobal.addEventListener("unload", this, false);
> 
> so either it's GC'ed when the first tab is closed and this is never called.
> Or it's not, and it's kept around for the whole duration of the process.

This shouldn't (won't) keep a reference to contentGlobal. The function runs to completion, then the argument here is GC'd. There's nothing that requires it to stay around. It'd be different if I used an inline event handler (which by its scope would reference contentGlobal) and if that function then outlived the contentGlobal otherwise, that'd be keeping contentGlobal alive. But I'm not.

> Am I missing something?

Either that or I am. :-)

> @@ +63,5 @@
> > +      this.uninit();
> > +    }
> > +  },
> > +  uninit: function() {
> > +    Services.obs.removeObserver(gEMEUIObserver, "media-eme-metadataloaded");
> 
> I'm still not entirely sure we need the removeObserver call here. For bug
> 973001 we didn't remove the content observers, and I don't think anything is
> leaked (or if it is, it's not leaked in a way that I could notice).

I'm confused why you're concerned about leaking something that tries to remove leaks, but not concerned about removing the initial leak of an observer.


I was going to do a try push but then I realized I messed up both of them, canceled their builds, and then realized the tree was closed so now I can't do a new push. :-\
Per IRC discussion.
Attachment #8560455 - Flags: review?(florian)
Attachment #8559900 - Attachment is obsolete: true
Attachment #8559900 - Flags: review?(florian)
Comment on attachment 8560455 [details] [diff] [review]
show doorhanger for EME being played back,

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

Looks good, thanks! I added one suggestion that could remove another 4 lines of code, but I don't care strongly about it, r+ either way.

::: browser/modules/ContentObservers.jsm
@@ +20,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let gEMEUIObserver = function(subject, topic, data) {
> +  switch (topic) {
> +    case "media-eme-metadataloaded":

I think you added this switch/case when we expected to remove the notification as soon as the EME content stops playing. If we are no longer expecting to have another EME related notification soon, testing the topic isn't useful here.
Attachment #8560455 - Flags: review?(florian) → review+
w/ the switch removed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/2903cea3975e
Flags: in-testsuite-
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2903cea3975e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
QA Contact: bogdan.maris
We have a 95% certainty on the name of the Adobe product, it is "Adobe Primetime". https://www.adobe.com/solutions/primetime.html. 

Can we updated the variable for DRM provider name from "Adobe" > "Adobe Primetime"?

Also, do you want to use this bug or should I open another?
sorry, comment 51 meant for Gijs!
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Javaun Moradi [:javaun] from comment #51)
> We have a 95% certainty on the name of the Adobe product, it is "Adobe
> Primetime". https://www.adobe.com/solutions/primetime.html. 
> 
> Can we updated the variable for DRM provider name from "Adobe" > "Adobe
> Primetime"?
> 
> Also, do you want to use this bug or should I open another?

I will stick this into the patch for bug 1111153 which I'm going to be landing on fx-team as soon as I finish going through bugmail.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1136165
Uplifted in bug 1136165.
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.
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+
Depends on: 1145039
Depends on: 1162198
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: