Closed Bug 1332530 Opened 4 years ago Closed 4 years ago

Remove GMP device binding

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

We can remove the device binding code now that we're dropping Adobe EME, and move the GMP loading code back into xul.
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 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 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 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 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.
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.