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)
Core
Audio/Video: Playback
Tracking
()
VERIFIED
FIXED
mozilla51
People
(Reporter: hsivonen, Assigned: kikuo)
References
(Blocks 1 open bug, )
Details
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
florian
:
review+
ritu
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release-
|
Details |
58 bytes,
text/x-review-board-request
|
florian
:
review+
ritu
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release-
|
Details |
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.
Reporter | ||
Updated•9 years ago
|
Comment 1•9 years ago
|
||
Also, when I right-click on this Clear Key video, the context menu includes the "Learn more about DRM…" menu item.
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Reporter | ||
Comment 2•8 years ago
|
||
Moving to P1 per discussion with ajones and cpearce in London.
Priority: -- → P1
Kilik - can you take this bug?
Flags: needinfo?(kikuo)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #3) > Kilik - can you take this bug? No problem !
Flags: needinfo?(kikuo)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kikuo
Assignee | ||
Comment 5•8 years ago
|
||
(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)
Reporter | ||
Comment 6•8 years ago
|
||
(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)
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69320/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69320/
Attachment #8777912 -
Flags: review?(florian)
Attachment #8777913 -
Flags: review?(florian)
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/69322/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/69322/
Assignee | ||
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
Hi florian, I found Gijs is on PTO recently, could you help to review these patches for me ? Thanks.
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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-
Assignee | ||
Comment 13•8 years ago
|
||
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 :)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
mozreview-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/#review67142
Attachment #8777912 -
Flags: review?(florian) → review+
Comment 17•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
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 20•8 years ago
|
||
mozreview-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 ::: 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-
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 22•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
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 25•8 years ago
|
||
mozreview-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/#review67578
Attachment #8777913 -
Flags: review?(florian) → review+
Assignee | ||
Comment 26•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 27•8 years ago
|
||
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
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7e175504e408 https://hg.mozilla.org/mozilla-central/rev/5d2faa33e1fa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 29•8 years ago
|
||
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?
Assignee | ||
Comment 30•8 years ago
|
||
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?
status-firefox49:
--- → affected
status-firefox50:
--- → affected
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+
Reporter | ||
Comment 33•8 years ago
|
||
Verified on Nightly.
Status: RESOLVED → VERIFIED
Flags: needinfo?(hsivonen)
Comment 34•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/b68ca93e59e2 https://hg.mozilla.org/releases/mozilla-beta/rev/ec45633dde2a
Comment 35•8 years ago
|
||
We only have a couple of weeks till 50 moves to release. I think this can wait for 50.
Updated•8 years ago
|
Attachment #8777912 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•8 years ago
|
Attachment #8777913 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Blocks: 1311166
You need to log in
before you can comment on or make changes to this bug.
Description
•