Closed Bug 1150371 Opened 9 years ago Closed 8 years ago

DRM UI shows up when the Clear Key CDM is in use

Categories

(Core :: Audio/Video: Playback, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: hsivonen, Assigned: kikuo)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

Steps to reproduce:
 1) Use Nightly on Windows Vista or later (without e10s for the time being).
 2) Navigate to http://media.junglecode.net/test/dash-eme/sintel.html
 3) Look at the space between the back button and the location in the location bar.
 4) Click the icon shown there.
 5) Right-click the video.

Actual results:
There is an icon of links of a chain on a gray circle between the back button and the location in the location bar. Clicking this icon drops doorhanger that claims that some video or audio on this site use DRM software. The last item of the context menu is "Learn more about DRM…"

Expected results:
Since Clear Key is not DRM (obtaining the keys is not conditional on demonstrating adherence to a DRM robustness and compliance regime), the DRM UI should not be shown when the Clear Key CDM is the only CDM in use.

Additional info:
Filing as a Core bug, because I'm assuming the Gecko is responsible for informing the Firefox UI about the  presence of DRM. 

Marking as "major", because this UI showing up for Clear Key substantially misrepresents the situation on a controversial topic.
Also, when I right-click on this Clear Key video, the context menu includes the "Learn more about DRM…" menu item.
Component: Audio/Video → Audio/Video: Playback
Moving to P1 per discussion with ajones and cpearce in London.
Priority: -- → P1
Kilik - can you take this bug?
Flags: needinfo?(kikuo)
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3)
> Kilik - can you take this bug?

No problem !
Flags: needinfo?(kikuo)
Assignee: nobody → kikuo
(In reply to Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) from comment #0)
> Steps to reproduce:
>  1) Use Nightly on Windows Vista or later (without e10s for the time being).
>  2) Navigate to http://media.junglecode.net/test/dash-eme/sintel.html

I got 404 error on the site "http://media.junglecode.net/test/dash-eme/sintel.html", so I cannot really know how that website works exactly.

>  3) Look at the space between the back button and the location in the
> location bar.
>  4) Click the icon shown there.
>  5) Right-click the video.
> 
> Actual results:
> There is an icon of links of a chain on a gray circle between the back
> button and the location in the location bar. Clicking this icon drops
> doorhanger that claims that some video or audio on this site use DRM
> software. The last item of the context menu is "Learn more about DRM…"
> 
> Expected results:
> Since Clear Key is not DRM (obtaining the keys is not conditional on
> demonstrating adherence to a DRM robustness and compliance regime), the DRM
> UI should not be shown when the Clear Key CDM is the only CDM in use.

Current behaviour in nightly, 
for those websites with multi-DRM scheme, i.e. http://shaka-player-demo.appspot.com/,
If "Play DRM Content" is unchecked in preferences, a yellow notification bar("Enable DRM") will show up (due to "api-disabled" event for proprietary DRMs) but soon disappear because a "cdm-created" event is notified for clearkey. Though there should be a potential timing issue here, but user is able to modify the pref by clicking the icon of links of chains.

I suppose the fix is to not show the icon of links of chains in location bar when the website "only" requests for clearkey keysystem, and remove the "Learn more about Drm..." item from that video element's Rigtl-Click context menu.  Do I understand it correctly ? 

Or not to show the DRM chained-icon if Clearkey is the only one CDM which is created successfully on that website ?

> 
> Additional info:
> Filing as a Core bug, because I'm assuming the Gecko is responsible for
> informing the Firefox UI about the  presence of DRM. 
> 
> Marking as "major", because this UI showing up for Clear Key substantially
> misrepresents the situation on a controversial topic.
Flags: needinfo?(hsivonen)
(In reply to Kilik Kuo [:kikuo] from comment #5)
> If "Play DRM Content" is unchecked in preferences, a yellow notification
> bar("Enable DRM") will show up (due to "api-disabled" event for proprietary
> DRMs) but soon disappear because a "cdm-created" event is notified for
> clearkey. Though there should be a potential timing issue here, but user is
> able to modify the pref by clicking the icon of links of chains.
> 
> I suppose the fix is to not show the icon of links of chains in location bar
> when the website "only" requests for clearkey keysystem, and remove the
> "Learn more about Drm..." item from that video element's Rigtl-Click context
> menu.  Do I understand it correctly ? 
> 
> Or not to show the DRM chained-icon if Clearkey is the only one CDM which is
> created successfully on that website ?

When only the Clear Key key system is in use, I think we shouldn't do any of the following:
 * Observe the state of the pref.
 * Show the chain icon in the location bar.
 * Show an info bar.
 * Show the chain icon in the context menu.

That is, this bug is requesting that Gecko inform the front end of DRM only if a non-Clear Key (i.e. Widevine or Primetime) key system is in use (or usage is attempted).
Flags: needinfo?(hsivonen)
https://reviewboard.mozilla.org/r/69322/#review66450

::: browser/base/content/browser-media.js:73
(Diff revision 1)
>      let buttonCallback;
>      let params = [];
>      switch (status) {
>        case "available":
>        case "cdm-created":
> +        if (this.isProprietaryKeySystem(keySystem)) {

"cdm-created" is the event that will be fired from gecko everytime a cdm is created successfully. Now we tread Clearkey as an always-enabled built-in cdm.

I'd prefer to handle the UI behaviour specifically here rather than blocking the event from gecko [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/eme/MediaKeys.cpp#418-420
Hi florian, I found Gijs is on PTO recently, could you help to review these patches for me ?
Thanks.
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review66722

::: browser/base/content/browser-media.js:44
(Diff revision 1)
>          Services.prefs.getPrefType("media.gmp-widevinecdm.visible")) {
>        return Services.prefs.getBoolPref("media.gmp-widevinecdm.visible");
>      }
>      return true;
>    },
> +  isProprietaryKeySystem: function(keySystem) {

If this method is going to be used only once, you could as well inline it.

Currently making this a method only seems to be used to make the check have an explicit name; you can get the same result by adding a comment above the test.

::: browser/base/content/browser-media.js:45
(Diff revision 1)
>        return Services.prefs.getBoolPref("media.gmp-widevinecdm.visible");
>      }
>      return true;
>    },
> +  isProprietaryKeySystem: function(keySystem) {
> +	return keySystem != "org.w3.clearkey";

no tabs for the indent please.

::: browser/base/content/browser-media.js:154
(Diff revision 1)
>      ["drmContentDisabled",
>       "drmContentCDMInsufficientVersion",
>       "drmContentCDMInstalling"
>       ].forEach(function (value) {
>          var notification = box.getNotificationWithValue(value);
> -        if (notification)
> +        if (notification) {

This change doesn't seem related and is far away from the code you really changed; please revert.
Attachment #8777913 - Flags: review?(florian) → review-
Comment on attachment 8777912 [details]
Bug 1150371 - [Part1] Remove 'Learn more about Drm...' from r-click context menu if it's a MediaElement for clearkey content.

https://reviewboard.mozilla.org/r/69320/#review66724

::: browser/base/content/nsContextMenu.js:972
(Diff revision 1)
>      // Otherwise make sure that nothing in the parent chain disables spellchecking
>      return aNode.spellcheck;
>    },
>  
> +  _isProprietaryDRM: function() {
> +    if (this.target.isEncrypted && "mediaKeys" in this.target &&

You don't need the |"mediaKeys" in this.target| check, doing a simple null check is enough and won't cause a warning if the property doesn't exist.

::: browser/base/content/nsContextMenu.js:973
(Diff revision 1)
>      return aNode.spellcheck;
>    },
>  
> +  _isProprietaryDRM: function() {
> +    if (this.target.isEncrypted && "mediaKeys" in this.target &&
> +        this.target.mediaKeys != null &&

You don't need the "!= null" part.

::: browser/base/content/nsContextMenu.js:975
(Diff revision 1)
>  
> +  _isProprietaryDRM: function() {
> +    if (this.target.isEncrypted && "mediaKeys" in this.target &&
> +        this.target.mediaKeys != null &&
> +        this.target.mediaKeys.keySystem != "org.w3.clearkey") {
> +      return true;

if (condition)
  return true;
return false;

Should be simplified to:
return condition;
Attachment #8777912 - Flags: review?(florian) → review-
https://reviewboard.mozilla.org/r/69322/#review66722

> If this method is going to be used only once, you could as well inline it.
> 
> Currently making this a method only seems to be used to make the check have an explicit name; you can get the same result by adding a comment above the test.

Good point ! Thanks :)
Comment on attachment 8777912 [details]
Bug 1150371 - [Part1] Remove 'Learn more about Drm...' from r-click context menu if it's a MediaElement for clearkey content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69320/diff/1-2/
Attachment #8777912 - Flags: review- → review?(florian)
Attachment #8777913 - Flags: review- → review?(florian)
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69322/diff/1-2/
Comment on attachment 8777912 [details]
Bug 1150371 - [Part1] Remove 'Learn more about Drm...' from r-click context menu if it's a MediaElement for clearkey content.

https://reviewboard.mozilla.org/r/69320/#review67142
Attachment #8777912 - Flags: review?(florian) → review+
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67144

::: browser/base/content/browser-media.js:70
(Diff revision 2)
>      let buttonCallback;
>      let params = [];
>      switch (status) {
>        case "available":
>        case "cdm-created":
> +        // The chain icon shows up only when there's proprietary CDM can be used.

This comment doesn't seem grammatically correct.

Maybe you meant:
// Show the chain icon only for proprietary CDMs. Clear Key is not proprietary.
Attachment #8777913 - Flags: review?(florian) → review-
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67144

> This comment doesn't seem grammatically correct.
> 
> Maybe you meant:
> // Show the chain icon only for proprietary CDMs. Clear Key is not proprietary.

Thanks for your reminder, I modified the sentence : )
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67532

::: browser/base/content/browser-media.js:70
(Diff revision 3)
>      let buttonCallback;
>      let params = [];
>      switch (status) {
>        case "available":
>        case "cdm-created":
> +        // Only show the chain icon when proprietary CDMs are available.

It's not when they are available, it's when they are being used.
Attachment #8777913 - Flags: review?(florian) → review-
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67532

> It's not when they are available, it's when they are being used.

Oops, forgive my impresice expression, "avaiable" meant "cdm is created successfully and is good for use."
I'll rephrase it.
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67532

> Oops, forgive my impresice expression, "avaiable" meant "cdm is created successfully and is good for use."
> I'll rephrase it.

I should explained more, the reason I use "available" is, although the cdm is created due to the creation of MediaKeys, but it is not in operation until the playback starts.
Do you think "cdm is created successfully and is good for use." is ok ? Or I could use the sentence "Only show the chain icon for proprietary CDMs. Clearkey is not one."
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67532

> I should explained more, the reason I use "available" is, although the cdm is created due to the creation of MediaKeys, but it is not in operation until the playback starts.
> Do you think "cdm is created successfully and is good for use." is ok ? Or I could use the sentence "Only show the chain icon for proprietary CDMs. Clearkey is not one."

I chose the later. It's more straightforward.
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

https://reviewboard.mozilla.org/r/69322/#review67578
Attachment #8777913 - Flags: review?(florian) → review+
try run is good,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64de2e0b05a7&selectedJob=25454994 

Except 'bc6' on Linux debug (Bug 1293014), and it should not be related.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e175504e408
[Part1] - Remove 'Learn more about Drm...' from r-click context menu if it's a MediaElement for clearkey content. r=florian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2faa33e1fa
[Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully. r=florian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e175504e408
https://hg.mozilla.org/mozilla-central/rev/5d2faa33e1fa
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8777912 [details]
Bug 1150371 - [Part1] Remove 'Learn more about Drm...' from r-click context menu if it's a MediaElement for clearkey content.

Approval Request Comment
[Feature/regressing bug #]: Bug 1311166
[User impact if declined]: Not enough simplified visual experience.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a20454fddd1
[Risks and why]: Very low, only UI logic is involed. And it avoids the confusion. 
[String/UUID change made/needed]: NONE
Attachment #8777912 - Flags: approval-mozilla-release?
Attachment #8777912 - Flags: approval-mozilla-beta?
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

Approval Request Comment
[Feature/regressing bug #]: Bug 1311166
[User impact if declined]: Not enough simplified visual experience.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low, only UI logic is involed. And it avoids the confusion. 
[String/UUID change made/needed]: NONE
Attachment #8777913 - Flags: approval-mozilla-release?
Attachment #8777913 - Flags: approval-mozilla-beta?
Hello Henri, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(hsivonen)
Comment on attachment 8777913 [details]
Bug 1150371 - [Part2] DRM UI should not show up when Clearkey is the only one CDM created successfully.

Fixes a recent regression that provides confusing messaging to end-user, Beta50+
Attachment #8777913 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8777912 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified on Nightly.
Status: RESOLVED → VERIFIED
Flags: needinfo?(hsivonen)
We only have a couple of weeks till 50 moves to release. I think this can wait for 50.
Attachment #8777912 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #8777913 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.