Remove GMP device binding

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(6 attachments)

We can remove the device binding code now that we're dropping Adobe EME, and move the GMP loading code back into xul.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8831013 [details]
Bug 1332530 - Remove librlz and EME/GMP device binding code.

https://reviewboard.mozilla.org/r/107658/#review108824
Attachment #8831013 - Flags: review?(gsquelart) → review+

Comment 9

2 years ago
mozreview-review
Comment on attachment 8831014 [details]
Bug 1332530 - Move GMPLoader code out of plugin-container and back into XUL.

https://reviewboard.mozilla.org/r/107660/#review108830

r+ with style nit:

::: dom/media/gmp/GMPLoader.cpp:207
(Diff revision 1)
>    }
>  }
>  #endif
> +
> +#if defined(XP_WIN) && defined(MOZ_SANDBOX)
> +class WinSandboxStarter : public mozilla::gmp::SandboxStarter {

I see you've moved this code from other files, but this file uses a different (and more correct!) style with class/function braces '{' on their own line, so could you please make the imported code consistent with its new home?
Attachment #8831014 - Flags: review?(gsquelart) → review+

Comment 10

2 years ago
mozreview-review
Comment on attachment 8831015 [details]
Bug 1332530 - Flatten GMPLoader and GMPLoaderImpl.

https://reviewboard.mozilla.org/r/107662/#review108832

r+ with style nits:

::: dom/media/gmp/GMPLoader.h:57
(Diff revision 1)
> -//
>  // Load() takes an optional GMPAdapter which can be used to adapt non-GMPs
>  // to adhere to the GMP API.
>  class GMPLoader {
>  public:
> -  virtual ~GMPLoader() {}
> +  explicit GMPLoader();

'explicit' is meaningless with a no-argument constructor. But it's harmless, so it's fine to keep if you prefer having it.

::: dom/media/gmp/GMPLoader.cpp:243
(Diff revision 1)
> -UniquePtr<GMPLoader> CreateGMPLoader() {
> -  return MakeUnique<GMPLoaderImpl>(MakeSandboxStarter());
> +GMPLoader::GMPLoader()
> +{
> +  mSandboxStarter = MakeSandboxStarter();

Could you please move this initialization into the constructor's member initializer list?
Attachment #8831015 - Flags: review?(gsquelart) → review+

Comment 11

2 years ago
mozreview-review
Comment on attachment 8831017 [details]
Bug 1332530 - Remove code to pass nodeId to GMP process.

https://reviewboard.mozilla.org/r/107666/#review108836
Attachment #8831017 - Flags: review?(gsquelart) → review+

Comment 12

2 years ago
mozreview-review
Comment on attachment 8831018 [details]
Bug 1332530 - Remove GMP enum storage names.

https://reviewboard.mozilla.org/r/107668/#review108838

r+ as it does what you've said in the short commit description...
But would you mind adding why you're removing this seemingly-useful API? (Is it just actually not that useful?)
Attachment #8831018 - Flags: review?(gsquelart) → review+
Comment on attachment 8831016 [details]
Bug 1332530 - Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined.

https://reviewboard.mozilla.org/r/107664/#review108916
Attachment #8831016 - Flags: review?(jld) → review+
(In reply to Gerald Squelart [:gerald] from comment #12)
> Comment on attachment 8831018 [details]
> Bug 1332530 - Remove GMP enum storage names.
> 
> https://reviewboard.mozilla.org/r/107668/#review108838
> 
> r+ as it does what you've said in the short commit description...
> But would you mind adding why you're removing this seemingly-useful API? (Is
> it just actually not that useful?)

There is no analogous API in the Chromium CDM API. So since we've removed the Adobe Primetime CDM, and move ClearKey to the Chromium API, there's no callers for this API. So no point keeping it around.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14c5ba1485bd
Remove librlz and EME/GMP device binding code. r=gerald
https://hg.mozilla.org/integration/autoland/rev/57c613d80dc3
Move GMPLoader code out of plugin-container and back into XUL. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6b3ca23424dc
Flatten GMPLoader and GMPLoaderImpl. r=gerald
https://hg.mozilla.org/integration/autoland/rev/6a31f295cd81
Don't require a SandboxStarter if MOZ_GMP_SANDBOX is not defined. r=jld
https://hg.mozilla.org/integration/autoland/rev/b599c9fa4080
Remove code to pass nodeId to GMP process. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8977e2dfe179
Remove GMP enum storage names. r=gerald
See Also: → 1422669
You need to log in before you can comment on or make changes to this bug.