Closed Bug 1314445 Opened 7 years ago Closed 7 years ago

Remove GMPService::GetPluginVersionForAPI

Categories

(Core :: Audio/Video: GMP, defect, P3)

Product:
Core
Component:
Audio/Video: GMP
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(11 files)

Bug 1314445 - Don't pass CDM version to MediaKey*.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314445 - Make MediaKeySystemAccess use GMPService::HasPluginForAPI.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314445 - Remove GMPService::GetPluginVersionForAPI.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314445 - Remove unused ParseKeySystem().
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314445 - Remove unused enum values from MediaKeySystemStatus.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1314445 - Remove unused EME notification handlers from browser-media.js.
58 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1314445 - Remove unused EME notification handlers from GMPProvider.jsm.
58 bytes, text/x-review-board-request
spohl
: review+
cpearce
: review+
Details
Bug 1314445 - Remove tests for CDM versions specified in keysystem string.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1314445 - Update tests for CDM request notifications to reflect new behaviour.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Fix test_eme_request_notifications to not fail on WinXP
2.39 KB, patch
Details | Diff | Splinter Review
Fix test_eme_request_notifications to not fail on WinXP
1.73 KB, patch
Details | Diff | Splinter Review 
We no longer need the callers of GMPService::GetPluginVersionForAPI and the code itself.
Attachment #8806583 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8806577 [details]
Bug 1314445 - Don't pass CDM version to MediaKey*.

https://reviewboard.mozilla.org/r/89968/#review89488
Attachment #8806577 - Flags: review?(gsquelart) → review+
Comment on attachment 8806578 [details]
Bug 1314445 - Make MediaKeySystemAccess use GMPService::HasPluginForAPI.

https://reviewboard.mozilla.org/r/89970/#review89492
Attachment #8806578 - Flags: review?(gsquelart) → review+
Comment on attachment 8806579 [details]
Bug 1314445 - Remove GMPService::GetPluginVersionForAPI.

https://reviewboard.mozilla.org/r/89972/#review89494
Attachment #8806579 - Flags: review?(gsquelart) → review+
Comment on attachment 8806580 [details]
Bug 1314445 - Remove unused ParseKeySystem().

https://reviewboard.mozilla.org/r/89974/#review89496
Attachment #8806580 - Flags: review?(gsquelart) → review+
Comment on attachment 8806584 [details]
Bug 1314445 - Remove tests for CDM versions specified in keysystem string.

https://reviewboard.mozilla.org/r/89982/#review89498
Attachment #8806584 - Flags: review?(gsquelart) → review+
Comment on attachment 8806585 [details]
Bug 1314445 - Update tests for CDM request notifications to reflect new behaviour.

https://reviewboard.mozilla.org/r/89984/#review89500
Attachment #8806585 - Flags: review?(gsquelart) → review+
Comment on attachment 8806581 [details]
Bug 1314445 - Remove unused enum values from MediaKeySystemStatus.

https://reviewboard.mozilla.org/r/89976/#review89620

::: dom/webidl/MediaKeysRequestStatus.webidl:6
(Diff revision 1)
>  /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  

Would be nice if somewhere in this file there was a comment that this isn't exposed to the web, but is gecko internal thing only.
Attachment #8806581 - Flags: review?(bugs) → review+
Comment on attachment 8806582 [details]
Bug 1314445 - Remove unused EME notification handlers from browser-media.js.

https://reviewboard.mozilla.org/r/89978/#review89638

::: browser/base/content/browser-media.js:94
(Diff revision 1)
>        case "cdm-not-supported":
>          // Not to pop up user-level notification because they cannot do anything
>          // about it.

This looks like it should be an early return? I realize this isn't something introduced by your patch, but we might as well fix it while we're here.

::: browser/base/content/browser-media.js:98
(Diff revision 1)
>          // Not to pop up user-level notification because they cannot do anything
>          // about it.
> -      case "error":
> -        // Fall through and do the same for unknown messages:
>        default:
> -        let typeOfIssue = status == "error" ? "error" : "message ('" + status + "')";
> +        Cu.reportError("Unknown message ('" + status + "') dealing with EME key request: " + data);

For bonus points, please use `new Error()` instead of passing a string to `Cu.reportError`.

::: browser/base/content/browser-media.js:144
(Diff revision 1)
>                             buttons);
>    },
>    showPopupNotificationForSuccess: function(browser, keySystem) {
>      // We're playing EME content! Remove any "we can't play because..." messages.
>      var box = gBrowser.getNotificationBox(browser);
> -    ["drmContentDisabled",
> +    ["drmContentDisabled", "drmContentCDMInstalling"].forEach(function (value) {

Personally, I'd prefer to keep this multilined so it's easier to add new items if we add different ones, and so we don't have to touch blame.
Attachment #8806582 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8806583 [details]
Bug 1314445 - Remove unused EME notification handlers from GMPProvider.jsm.

https://reviewboard.mozilla.org/r/89980/#review89872
Attachment #8806583 - Flags: review+
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84a6f16feabd
Don't pass CDM version to MediaKey*. r=gerald
https://hg.mozilla.org/integration/autoland/rev/7fbf411102d8
Make MediaKeySystemAccess use GMPService::HasPluginForAPI. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ce3ab5d4e29d
Remove GMPService::GetPluginVersionForAPI. r=gerald
https://hg.mozilla.org/integration/autoland/rev/06b9f9ad45ab
Remove unused ParseKeySystem(). r=gerald
https://hg.mozilla.org/integration/autoland/rev/06fd311735c7
Remove unused enum values from MediaKeySystemStatus. r=smaug
https://hg.mozilla.org/integration/autoland/rev/18558217586a
Remove unused EME notification handlers from browser-media.js. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/bd3344c860a0
Remove unused EME notification handlers from GMPProvider.jsm. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/ba37b01751a2
Remove tests for CDM versions specified in keysystem string. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4eb93831e7d3
Update tests for CDM request notifications to reflect new behaviour. r=gerald
(In reply to Chris Pearce (:cpearce) from comment #29)
> Comment on attachment 8806583 [details]
> Bug 1314445 - Remove unused EME notification handlers from GMPProvider.jsm.
> 
> https://reviewboard.mozilla.org/r/89980/#review89872

This is me converting the r+ spohl applied to Attachment #8806583 [details] into a format mozreview can understand so I can autoland.
Comment on attachment 8806583 [details]
Bug 1314445 - Remove unused EME notification handlers from GMPProvider.jsm.

https://reviewboard.mozilla.org/r/89980/#review89874
Attachment #8806583 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8891610c7c7b
Fix test_eme_request_notifications to not fail on WinXP. r=bustage
Depends on: 1431153
You need to log in before you can comment on or make changes to this bug.