Closed Bug 1331484 Opened 3 years ago Closed 3 years ago

Remove traces of Adobe from DecoderDoctor

Categories

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

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: gerald, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Spawned from bug 1329543 comment 40.
There are still mentions of the Adobe CDM in Decoder Doctor and related browser-media.js, they should be exterminated.
There was actually nothing related to Adobe in DecoderDoctorDiagnostics.
Handled what was left in browser-media.js and its tests.
Comment on attachment 8832771 [details]
Bug 1331484 - Removed mentions of Adobe from browser-media.js -

https://reviewboard.mozilla.org/r/109016/#review110262

We need to make a decision about the string here. Holding off on r+ for now because of that. :-)

::: browser/base/content/browser-media.js
(Diff revision 1)
>  
>  let gDecoderDoctorHandler = {
>    getLabelForNotificationBox(type) {
> -    if (type == "adobe-cdm-not-found" &&
> -        AppConstants.platform == "win") {
> -      return gNavigatorBundle.getString("decoder.noCodecs.message");

It seems this message reads:

"To play video, you may need to install Microsoft’s Media Feature Pack."

And has no other consumers besides the two you're removing here - though the decoder.noHWAcceleration.message string looks eerily similar.

Do we need to either add another case here so we still display this message to users who are on e.g. Win10 KN or N? Or does the other string take care of that already, in which case, should we remove https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/browser/locales/en-US/chrome/browser/browser.properties#842 ?
Attachment #8832771 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8832771 [details]
Bug 1331484 - Removed mentions of Adobe from browser-media.js -

https://reviewboard.mozilla.org/r/109016/#review110262

> It seems this message reads:
> 
> "To play video, you may need to install Microsoft’s Media Feature Pack."
> 
> And has no other consumers besides the two you're removing here - though the decoder.noHWAcceleration.message string looks eerily similar.
> 
> Do we need to either add another case here so we still display this message to users who are on e.g. Win10 KN or N? Or does the other string take care of that already, in which case, should we remove https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/browser/locales/en-US/chrome/browser/browser.properties#842 ?

I intentionally left it there, because it seems like it could be a useful message, even if not used today. It's already been translated, so I didn't want to waste that effort in case we need it again one day.

Now, looking at the other message (starting with "to improve video quality"), it seems to be used in cases where we actually cannot play the requested media, so in fact we should probably be displaying the noCodecs "To play video..." messages!

So thank you for making me look, it seems I need to put more thought into this! However this is becoming off-topic with the current bug, which was just about removing Adobe-handling code.

How about:
- You r+ this, as (I think) it does the job of removing these Adobe bits, as per the bug description.
- I open a separate bug to review the whole Decoder Doctor decision tree, in which useless messages can be pruned.
Comment on attachment 8832771 [details]
Bug 1331484 - Removed mentions of Adobe from browser-media.js -

(Please see comment 4)
Attachment #8832771 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8832771 [details]
Bug 1331484 - Removed mentions of Adobe from browser-media.js -

https://reviewboard.mozilla.org/r/109016/#review110432

r=me given a followup bug to re-evaluate the strings and/or decision tree.
Attachment #8832771 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1336256
Thank you Gijs, I've opened bug 1336256 to follow up on the issue vs user report mismatch.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4290c796975c
Removed mentions of Adobe from browser-media.js - r=Gijs
https://hg.mozilla.org/mozilla-central/rev/4290c796975c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.