1256186 - [EME] Show "Enable EME" notification box only when MediaKeys requested for supported keysystem

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

A commenter in bug 1252168 pointed out that if script requests a DRM keysystem that we don't support and EME is disabled, we show the "click here to enable EME" notification box. We should only show that if the user requested a keysystem that we support, so that users only see the banner in cases where they can click the "make it work" button and have the site work.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Blake can someone on your team take this bug?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

We need to not dispatch the "mediakeys-request" message in MediaKeySystemAccess::NotifyObservers when we don't support the key system.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

I can mentor this bug if someone likes to make an attempt.

Comment 4

[Blake Wu [:bwu][:blakewu]]

Kilik, 
You should be interested in this bug. :-)

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #2)
> We need to not dispatch the "mediakeys-request" message in
> MediaKeySystemAccess::NotifyObservers when we don't support the key system.

I'm a little bit confused between the bug title and Comment 2.
Referring to the message of notification bar [1], these notifications are triggered by the topic "mediakeys-request", and browser still needs to show pop-up "Unsupported/Unknown CDM". 

If we don't dispatch "mediakeys-request" when we don't support the keysystem (no matter what error it is), then user cannot know what happened, and in that case, we don't even need to handle "cdm-not-supported" in chrome [2].

I think what I can do is to refactor/reorder these condition code in MediaKeySystemAccessManager::Request() [3] to make sure the MediaKeySystemAccess::NotifyObservers() is called only when we've done every check of capability and the status is not "cdm-not-supported".
Does it make sense ?




[1] Bug 1111153 Attachment 8566525 [details]
[2] https://dxr.mozilla.org/mozilla-central/rev/b942c98f56c4c2926b8b81b98425072a091bbf7b/browser/base/content/browser-eme.js#67
[3] https://dxr.mozilla.org/mozilla-central/rev/b942c98f56c4c2926b8b81b98425072a091bbf7b/dom/media/eme/MediaKeySystemAccessManager.cpp#76

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

We want to change the behaviour so that whenever a CDM is requested, we only show the notification box if there's something that the user can do to rectify the problem. In the case of an unsupported keysystem, we want to never show the notification box, as there's nothing the user can do to fix the problem, except switch browsers.

The bug here is that if EME is disabled via toggling "Play DRM Content" in about:preferences#content, then if the script requests an unsupported keysystem, we will show the "EME is disabled" notification box, and if the user clicks on the "enable EME" button, EME still won't work because the request is for an unsupported keysystem. So we should not dispatch the "mediakeys-request" topic in this case, as there's nothing the user can do to make EME work.

We should also remove the chrome handler on this message. That should happen in another patch and/or bug.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

As Chris said, we should show "click here to enable EME" only when the key system is supported. If the key system is not supported, there is no way to "enable EME".

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Thanks for the explanation, Chris, JW ! Now I have a clear mind :)

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Notify user to enable EME after checking supportabiliyt to target keysystem.

Hi JW, could you help review this patch ?

I'd also like to remove the handler in chrome for status "Cdm_not_supported" in another patch, but it seems that we still need to notify users on Windows (lower than 6.0) and OSX (lower than 10.7) [1].  Or we considered that won't be a matter from now ?

[1] https://dxr.mozilla.org/mozilla-central/rev/d5d53a3b4e50b94cdf85d20690526e5a00d5b63e/dom/media/eme/MediaKeySystemAccess.cpp#296

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8736139 [details] [diff] [review]
Notify user to enable EME after checking supportabiliyt to target keysystem.

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

::: dom/media/eme/MediaKeySystemAccessManager.cpp
@@ +94,5 @@
> +                          NS_LITERAL_CSTRING("Key system is not supported"));
> +    return;
> +  }
> +
> +  MediaKeySystemConfiguration config;

The order of checks seem wrong to me, it should be:
1. return if key system not supported.
2. return if "media.eme.enabled" is preffed off.
3. ...

MediaKeySystemStatus should be checked before MediaKeySystemConfiguration.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

(In reply to JW Wang [:jwwang] from comment #10)
> Comment on attachment 8736139 [details] [diff] [review]
> Notify user to enable EME after checking supportabiliyt to target keysystem.
> 
> Review of attachment 8736139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/eme/MediaKeySystemAccessManager.cpp
> @@ +94,5 @@
> > +                          NS_LITERAL_CSTRING("Key system is not supported"));
> > +    return;
> > +  }
> > +
> > +  MediaKeySystemConfiguration config;
> 
> The order of checks seem wrong to me, it should be:
> 1. return if key system not supported.
> 2. return if "media.eme.enabled" is preffed off.
> 3. ...
> 
> MediaKeySystemStatus should be checked before MediaKeySystemConfiguration.

If the keysystem is supported, but the configuration is not supported, do we still notify user to Enable EME ?  Because users still cannot see anything, can they ?

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

No, they are different cases.

EME preffed off -> Tell the user the EME is preffed. The user needs to turn on the pref and try again.

Configuration not supported -> Tell the user this configuration is not supported. The user might try again with another configuration.

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Attachment: MozReview Request: Bug 1256186 - [EME] Show 'Enable EME' notification box only when MediaKeys requested for supported keysystem.; r=jwwang

Review commit: https://reviewboard.mozilla.org/r/43245/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43245/

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Remove status handler in chrome to avoid confusing users.

Hi Chris,

I removed that part that handling the "cdm_not_supported" status, and also remove the check-return you added in MediaKeySystemAccess::NotifyObservers.
Is it good to you ?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8736398 [details] [diff] [review]
Remove status handler in chrome to avoid confusing users.

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

We'll need someone in the Firefox team to review this. I think Gijs reviews browser-eme.js.

::: browser/base/content/browser-eme.js
@@ +73,5 @@
>          notificationId = "drmContentCDMInstalling";
>          params = [this._brandShortName];
>          break;
>  
> +      case "cdm-not-supported":

We shouldn't get these notifications any more right?

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8736394 [details]
MozReview Request: Bug 1256186 - [EME] Show 'Enable EME' notification box only when MediaKeys requested for supported keysystem.; r=jwwang

https://reviewboard.mozilla.org/r/43245/#review39979

::: dom/media/eme/MediaKeySystemAccessManager.cpp:90
(Diff revision 1)
>    // Parse keysystem, split it out into keySystem prefix, and version suffix.
>    nsAutoString keySystem;
>    int32_t minCdmVersion = NO_CDM_VERSION;
> -  if (!ParseKeySystem(aKeySystem,
> -                      keySystem,
> -                      minCdmVersion)) {
> +  bool keySystemSupported = ParseKeySystem(aKeySystem, keySystem, minCdmVersion);
> +
> +  if (!keySystemSupported) {

You don't need this temp. Just say if (!ParseKeySystem(...)) {}.

::: dom/media/eme/MediaKeySystemAccessManager.cpp:94
(Diff revision 1)
> -                      keySystem,
> -                      minCdmVersion)) {
> -    // Invalid keySystem string, or unsupported keySystem. Send notification
> -    // to chrome to show a failure notice.
> -    MediaKeySystemAccess::NotifyObservers(mWindow, aKeySystem, MediaKeySystemStatus::Cdm_not_supported);
> +
> +  if (!keySystemSupported) {
> +    // Not to inform user, because nothing to do if the keySystem is not
> +    // supported.
> +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
> +                          NS_LITERAL_CSTRING("Key system is not supported"));

Use the original wording "Key system string is invalid, or key system is unsupported" which is more informative.

::: dom/media/eme/MediaKeySystemAccessManager.cpp:98
(Diff revision 1)
> -    MediaKeySystemAccess::NotifyObservers(mWindow, aKeySystem, MediaKeySystemStatus::Cdm_not_supported);
> +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
> +                          NS_LITERAL_CSTRING("Key system is not supported"));
> +    return;
> +  }
> +
> +  if (!emePrefEnabled) {

Ditto. Remove the temp.

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8736394 [details]
MozReview Request: Bug 1256186 - [EME] Show 'Enable EME' notification box only when MediaKeys requested for supported keysystem.; r=jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43245/diff/1-2/

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

(In reply to JW Wang [:jwwang] from comment #16)
> Comment on attachment 8736394 [details]
> MozReview Request: Bug 1256186 - [EME] Show 'Enable EME' notification box
> only when MediaKeys requested for supported keysystem.; r=jwwang
> 
> https://reviewboard.mozilla.org/r/43245/#review39979
> 
> ::: dom/media/eme/MediaKeySystemAccessManager.cpp:90
> (Diff revision 1)
> >    // Parse keysystem, split it out into keySystem prefix, and version suffix.
> >    nsAutoString keySystem;
> >    int32_t minCdmVersion = NO_CDM_VERSION;
> > -  if (!ParseKeySystem(aKeySystem,
> > -                      keySystem,
> > -                      minCdmVersion)) {
> > +  bool keySystemSupported = ParseKeySystem(aKeySystem, keySystem, minCdmVersion);
> > +
> > +  if (!keySystemSupported) {
> 
> You don't need this temp. Just say if (!ParseKeySystem(...)) {}.
> 
> ::: dom/media/eme/MediaKeySystemAccessManager.cpp:94
> (Diff revision 1)
> > -                      keySystem,
> > -                      minCdmVersion)) {
> > -    // Invalid keySystem string, or unsupported keySystem. Send notification
> > -    // to chrome to show a failure notice.
> > -    MediaKeySystemAccess::NotifyObservers(mWindow, aKeySystem, MediaKeySystemStatus::Cdm_not_supported);
> > +
> > +  if (!keySystemSupported) {
> > +    // Not to inform user, because nothing to do if the keySystem is not
> > +    // supported.
> > +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
> > +                          NS_LITERAL_CSTRING("Key system is not supported"));
> 
> Use the original wording "Key system string is invalid, or key system is
> unsupported" which is more informative.
> 
> ::: dom/media/eme/MediaKeySystemAccessManager.cpp:98
> (Diff revision 1)
> > -    MediaKeySystemAccess::NotifyObservers(mWindow, aKeySystem, MediaKeySystemStatus::Cdm_not_supported);
> > +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,
> > +                          NS_LITERAL_CSTRING("Key system is not supported"));
> > +    return;
> > +  }
> > +
> > +  if (!emePrefEnabled) {
> 
> Ditto. Remove the temp.

Thanks for the review, I fixed these issues and pushed a new MozRequest with r+ carried.

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Chris Pearce (:cpearce) from comment #15)
> Comment on attachment 8736398 [details] [diff] [review]
> Remove status handler in chrome to avoid confusing users.
> 
> Review of attachment 8736398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We'll need someone in the Firefox team to review this. I think Gijs reviews
> browser-eme.js.
> 
> ::: browser/base/content/browser-eme.js
> @@ +73,5 @@
> >          notificationId = "drmContentCDMInstalling";
> >          params = [this._brandShortName];
> >          break;
> >  
> > +      case "cdm-not-supported":
> 
> We shouldn't get these notifications any more right?

Thanks for the feedback and sorry about the late response, I was in PTO these days.
Though we tend not to handle this notification anymore, but we still notify this information to chrome on Windows XP/OS-X Leopard in MediaKeySystemAccess::GetKeySystemStatus(), that's why I left the case entry with a default handler as below in case we may re-use it in the future.

default:
  let typeOfIssue = status == "error" ? "error" : "message ('" + status + "')";
  Cu.reportError("Unknown " + typeOfIssue + " dealing with EME key request: " + data);

I think it should be fine and informative for developers.

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

Attachment: MozReview Request: Bug 1256186 - Part 2 - Remove the handler in chrome against MediaKeySystemStatus 'cdm-not-supported' and related strings to avoid confusing users.; r=Gijs.

Remove the popup notificaiton for status "cdm-not-supported" to not confuse users.
Leave the case entry for reference.

Review commit: https://reviewboard.mozilla.org/r/44033/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44033/

Comment 21

[:Gijs (he/him)]

Comment on attachment 8737607 [details]
MozReview Request: Bug 1256186 - Part 2 - Remove the handler in chrome against MediaKeySystemStatus 'cdm-not-supported' and related strings to avoid confusing users.; r=Gijs.

https://reviewboard.mozilla.org/r/44033/#review40599

::: browser/base/content/browser-eme.js
(Diff revision 1)
> -      case "cdm-not-supported":
> -        notificationId = "drmContentCDMNotSupported";
> -        params = [this._brandShortName, this.getLearnMoreLink(notificationId)];
> -        break;
> -

This should also remove all the relevant strings.

::: browser/base/content/browser-eme.js
(Diff revision 1)
> -    if (notificationId == "drmContentCDMNotSupported" &&
> -        keySystem.startsWith("com.adobe")) {
> -      let os = Services.appinfo.OS.toLowerCase();
> -      if (os.startsWith("linux") || os.startsWith("darwin")) {
> -        msgId = msgPrefix + "unsupportedOS.message";
> -        labelParams.splice(1, 0, os.startsWith("linux") ? "Linux" : "Mac OS X");
> -      }
> -    }

So now we don't tell users on Linux or OS X anything? That doesn't seem right.

::: dom/media/eme/MediaKeySystemAccess.cpp
(Diff revision 1)
> -  if (aStatus == MediaKeySystemStatus::Cdm_not_supported) {
> -    // Ignore, since there's nothing the user can do to rectify this, and we
> -    // don't want the prompt to confuse them.
> -    // TODO: Remove places that call with this entirely.
> -    return;
> -  }

I can't review this change, and I don't really understand why we're now notifying when we weren't before, and at the same time are removing the handlers for this...

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

Hi Gijs,

Sorry for confusion, I didn't elaborate well.
Please see the reply below.

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8737607 [details]
> MozReview Request: Bug 1256186 - Remove the handler in chrome against
> MediaKeySystemStatus 'cdm-not-supported' to avoid confusing users.; r?Gijs.
> 
> https://reviewboard.mozilla.org/r/44033/#review40599
> 
> ::: browser/base/content/browser-eme.js
> (Diff revision 1)
> > -      case "cdm-not-supported":
> > -        notificationId = "drmContentCDMNotSupported";
> > -        params = [this._brandShortName, this.getLearnMoreLink(notificationId)];
> > -        break;
> > -
> 
> This should also remove all the relevant strings.

IIUC, do you mean all the strings related to the notificationId "drmContentCDMNotSupported" around here [1] should be removed as well ?

[1] https://dxr.mozilla.org/mozilla-central/rev/55d557f4d73ee58664bdf2fa85aaab555224722e/browser/locales/en-US/chrome/browser/browser.properties#645

> 
> ::: browser/base/content/browser-eme.js
> (Diff revision 1)
> > -    if (notificationId == "drmContentCDMNotSupported" &&
> > -        keySystem.startsWith("com.adobe")) {
> > -      let os = Services.appinfo.OS.toLowerCase();
> > -      if (os.startsWith("linux") || os.startsWith("darwin")) {
> > -        msgId = msgPrefix + "unsupportedOS.message";
> > -        labelParams.splice(1, 0, os.startsWith("linux") ? "Linux" : "Mac OS X");
> > -      }
> > -    }
> 
> So now we don't tell users on Linux or OS X anything? That doesn't seem
> right.
> 

This issue is a follow-up of Bug 1252168, and the original behaviour in Firefox is popping up a notification "The audio or video on this page requires DRM software that Firefox does not support" to users if the DRM key system is not supported, but user may see a notification asking them to enable EME first if he/she pref off "media.eme.enabled".
This may result to a user frustration. They still cannot watch anything after enabling EME.

So what we're gonna do is to ...
1) notify user to enable EME only when the key system is supported and the pref is off.
2) remove all the cdm-not-supported notification to avoid user confusion/frustration because nothing can be done about it.

For 2), I don't know if there's a Firefox browser PM who can help to comment if the behaviour change is ok or not. But IMHO, I suggest to remove only the trigger point for the pop-up in case we need to notify it again in the future when Firefox supports more key systems.
Does it make sense to you ?

> ::: dom/media/eme/MediaKeySystemAccess.cpp
> (Diff revision 1)
> > -  if (aStatus == MediaKeySystemStatus::Cdm_not_supported) {
> > -    // Ignore, since there's nothing the user can do to rectify this, and we
> > -    // don't want the prompt to confuse them.
> > -    // TODO: Remove places that call with this entirely.
> > -    return;
> > -  }
> 
> I can't review this change, and I don't really understand why we're now
> notifying when we weren't before, and at the same time are removing the
> handlers for this...

This should be removed because it's like kinda workaround here.
I'm gonna separate it to another review request and find a correct reviewer, thanks.

Comment 23

[:Gijs (he/him)]

(In reply to Kilik Kuo [:kikuo] from comment #22)

> > This should also remove all the relevant strings.
> 
> IIUC, do you mean all the strings related to the notificationId
> "drmContentCDMNotSupported" around here [1] should be removed as well ?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 55d557f4d73ee58664bdf2fa85aaab555224722e/browser/locales/en-US/chrome/
> browser/browser.properties#645

Yes.

> > ::: browser/base/content/browser-eme.js
> > (Diff revision 1)
> > > -    if (notificationId == "drmContentCDMNotSupported" &&
> > > -        keySystem.startsWith("com.adobe")) {
> > > -      let os = Services.appinfo.OS.toLowerCase();
> > > -      if (os.startsWith("linux") || os.startsWith("darwin")) {
> > > -        msgId = msgPrefix + "unsupportedOS.message";
> > > -        labelParams.splice(1, 0, os.startsWith("linux") ? "Linux" : "Mac OS X");
> > > -      }
> > > -    }
> > 
> > So now we don't tell users on Linux or OS X anything? That doesn't seem
> > right.
> > 
> 
> This issue is a follow-up of Bug 1252168, and the original behaviour in
> Firefox is popping up a notification "The audio or video on this page
> requires DRM software that Firefox does not support" to users if the DRM key
> system is not supported, but user may see a notification asking them to
> enable EME first if he/she pref off "media.eme.enabled".
> This may result to a user frustration. They still cannot watch anything
> after enabling EME.
> 
> So what we're gonna do is to ...
> 1) notify user to enable EME only when the key system is supported and the
> pref is off.

Right, this makes sense.

> 2) remove all the cdm-not-supported notification to avoid user
> confusion/frustration because nothing can be done about it.

Well, they can contact the site and/or use a different browser. I don't know if saying nothing is really preferable over saying "The audio or video on this page requires DRM software that Firefox does not support.", which at least tells them why the page doesn't work...

> For 2), I don't know if there's a Firefox browser PM who can help to comment
> if the behaviour change is ok or not. But IMHO, I suggest to remove only the
> trigger point for the pop-up in case we need to notify it again in the
> future when Firefox supports more key systems.
> Does it make sense to you ?

It sounds like the removal of such a notification should get UX review. Maybe Sevaan can help? Or Javaun?

If we're removing the notification now, it seems like we should remove the string as well. If we then later want to go back to adding a user-level notification about what is going on, we can come up with a better string then.

Comment 24

[Sevaan Franks [:sevaan]]

Thanks. Makes sense to me not have a broken experience! Thanks!

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

Attachment: MozReview Request: Bug 1256186 - Part 3 - Remove the temporary solution which ignores MediaKeySystemStatus::Cdm_not_supported.; r?jwwang

Review commit: https://reviewboard.mozilla.org/r/44367/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44367/

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8737607 [details]
MozReview Request: Bug 1256186 - Part 2 - Remove the handler in chrome against MediaKeySystemStatus 'cdm-not-supported' and related strings to avoid confusing users.; r=Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44033/diff/1-2/

Comment 27

[:Gijs (he/him)]

Comment on attachment 8737607 [details]
MozReview Request: Bug 1256186 - Part 2 - Remove the handler in chrome against MediaKeySystemStatus 'cdm-not-supported' and related strings to avoid confusing users.; r=Gijs.

https://reviewboard.mozilla.org/r/44033/#review41063

with the below addressed, r=me

::: browser/base/content/browser-eme.js
(Diff revision 2)
>          notificationId = "drmContentDisabled";
>          buttonCallback = gEMEHandler.ensureEMEEnabled.bind(gEMEHandler, browser, keySystem)
>          params = [this.getLearnMoreLink(notificationId)];
>          break;
>  
> -      case "cdm-not-supported":

The previous patch ensured we still reported an error in the console. I think that would continue to make sense to do for web developers who might otherwise be confused as to why their content is not working in Firefox.

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/44033/#review40599

> This should also remove all the relevant strings.

All related strings are removed in the new review request.

> So now we don't tell users on Linux or OS X anything? That doesn't seem right.

I suggest removing this notification too, because we're not handling "cdm-not-supported" now, there seems no need to notify user because switching to other browsers is easier than switching os :(

> I can't review this change, and I don't really understand why we're now notifying when we weren't before, and at the same time are removing the handlers for this...

Move this part into another request.

Comment 29

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8737607 [details]
MozReview Request: Bug 1256186 - Part 2 - Remove the handler in chrome against MediaKeySystemStatus 'cdm-not-supported' and related strings to avoid confusing users.; r=Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44033/diff/2-3/

Comment 30

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8738184 [details]
MozReview Request: Bug 1256186 - Part 3 - Remove the temporary solution which ignores MediaKeySystemStatus::Cdm_not_supported.; r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44367/diff/1-2/

Comment 31

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/44033/#review41063

Thanks for the review.

> The previous patch ensured we still reported an error in the console. I think that would continue to make sense to do for web developers who might otherwise be confused as to why their content is not working in Firefox.

Addressed.

Comment 32

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8738184 [details]
MozReview Request: Bug 1256186 - Part 3 - Remove the temporary solution which ignores MediaKeySystemStatus::Cdm_not_supported.; r?jwwang

https://reviewboard.mozilla.org/r/44367/#review41501

Comment 33

[Kilik Kuo [:kikuo] (inactive)]

https://reviewboard.mozilla.org/r/44367/#review41501

Thanks for the review.

Comment 34

[Kilik Kuo [:kikuo] (inactive)]

Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=109e43954724&selectedJob=19138243

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8923987bb55b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f774205be94
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a98e60aa2d

Comment 36

[Chris Peterson [:cpeterson]]

cpearce, do we want to uplift Kilik's fix to Aurora 47?

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8923987bb55b
https://hg.mozilla.org/mozilla-central/rev/7f774205be94
https://hg.mozilla.org/mozilla-central/rev/e6a98e60aa2d

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

I don't think we need to rush to uplift this.

Comment 39

[b1883900]

Will this still be fixed eventually?

Comment 40

[Kilik Kuo [:kikuo] (inactive)]

(In reply to b1883900 from comment #39)
> Will this still be fixed eventually?

This should be fixed already since FF48, or do you mean the bug still exists ?

Comment 41

[b1883900]

Still exists for me on Hulu.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1252168#c28 starting with comment 28.  

With the EME checkbox unchecked (Hamburger menu ("☰") > Preferences > Content and "Play DRM Content"), Hulu started displaying the same "You must enable DRM to play some audio or video on this page" banner notification again since June.

Comment 42

[:Gijs (he/him)]

(In reply to b1883900 from comment #41)
> Still exists for me on Hulu.  See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1252168#c28 starting with
> comment 28.  
> 
> With the EME checkbox unchecked (Hamburger menu ("☰") > Preferences >
> Content and "Play DRM Content"), Hulu started displaying the same "You must
> enable DRM to play some audio or video on this page" banner notification
> again since June.

File a new bug with more details about what version of Firefox you're seeing this on (I presume something later than 48?), whether you have EME enabled on that version, and if it reproduces in a clean profile (potentially with the EME pref flipped).

Comment 43

[b1883900]

Are you sure you want a new bug filed for this as it is the same as this one?

It has been constant on every version of Firefox for Hulu since that June 22nd post for me.  It didn't start appearing as part of a Firefox version upgrade as I was using version 47 for about two weeks before the banner started again.  The banner has been present every single Hulu visit since that period over versions 47, 47.01, 48.0, 48.0.1, 48.0.2, 49.0, 49.0.1, 49.0.2, 50.0, 50.0.1 and 50.0.2.

The banner appears with the Play DRM Content unchecked.  Goes away with it checked as detailed in this bug.  Same on a clean profile.

Comment 44

[:Gijs (he/him)]

(In reply to b1883900 from comment #43)
> Are you sure you want a new bug filed for this as it is the same as this one?

It isn't - in this bug Hulu asks for a keysystem that we don't support. In your case, apparently Hulu tries to use DRM and because you've disabled it entirely, the notification pops up. It'd be worth having a separate bug for this.

> It has been constant on every version of Firefox for Hulu since that June
> 22nd post for me.  It didn't start appearing as part of a Firefox version
> upgrade as I was using version 47 for about two weeks before the banner
> started again.  The banner has been present every single Hulu visit since
> that period over versions 47, 47.01, 48.0, 48.0.1, 48.0.2, 49.0, 49.0.1,
> 49.0.2, 50.0, 50.0.1 and 50.0.2.

OK.
 
> The banner appears with the Play DRM Content unchecked.  Goes away with it
> checked as detailed in this bug.  Same on a clean profile.

OK. So please file a new bug. It's possible we'll just end up contacting Hulu to avoid them requesting DRM if they don't need it, but even so it'd be good to have an open bug on file.

Note that both this and bug 1252168 have patches that landed, so we don't generally reopen those bugs after patches have been landed, to avoid getting confused about which patches landed on which branches. That's another reason why a new bug is a good idea.