1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

In current code base,

CDMCallbackProxy(private cstr) is only allowed to be use by CDMProxy(CDMCallbackProxy's friend).

I think it is better to put CDMCallbackProxy into CDMProxy protected field.

The advantage is
1. Get rid of |friend modifier|.
2. It only allows derived class(Bug 1284192 intended to do) to use this class.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: [mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

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

Comment 2

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

I assume CDMCallbackProxy is strongly coupled with GMP because it inherits GMPDecryptorProxyCallback. The chance for it to be reusable with other CDMProxy subclass should be low.

I would suggest just rename CDMCallbackProxy to GMPCDMCallbackProxy which will be used by GMPCDMProxy only.

Comment 3

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

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review59622

Comment 4

[James Cheng[:JamesCheng]]

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

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

Comment 5

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

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review60228

1. Use "hg mv" unless you want to keep CDMCallbackProxy.cpp
2. GMPCDMCallbackProxy.cpp should be put under dom/media/gmp

Comment 6

[James Cheng[:JamesCheng]]

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

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

Comment 7

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

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review60242

Looks good to me. But the bug title is different from the description of comment 0. Please explain the purpose of this bug again.

Comment 8

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

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

GMP changes should go through Chris.

Hi Chris,
Do you have concerns about incoming changes to CDMProxy (bug 1284192) and related files?

Comment 9

[James Cheng[:JamesCheng]]

For bug 1284192, we are planning to make cdmproxy to be a base class. This bug is trying to make CDMCallbackproxy to be a gmp-used only class, so I moved it to gmp folder and renamed.

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

(In reply to JW Wang [:jwwang] from comment #8)
> Comment on attachment 8768359 [details]
> [mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make
> it GMP specific use.
> 
> GMP changes should go through Chris.
> 
> Hi Chris,
> Do you have concerns about incoming changes to CDMProxy (bug 1284192) and
> related files?

Should add, the reason we want this change is to build another FennecCDMProxy which will encapsulate the code logic & function calls related to MediaDrm/MediaCrypto. By doing so, we can also make the session-related IPC calls obscure once we have a separated process(service) to decode video.

Comment 11

[James Cheng[:JamesCheng]]

(In reply to Kilik Kuo [:kikuo] from comment #10)
> (In reply to JW Wang [:jwwang] from comment #8)
> > Comment on attachment 8768359 [details]
> > [mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make
> > it GMP specific use.
> > 
> > GMP changes should go through Chris.
> > 
> > Hi Chris,
> > Do you have concerns about incoming changes to CDMProxy (bug 1284192) and
> > related files?
> 
> Should add, the reason we want this change is to build another
> FennecCDMProxy which will encapsulate the code logic & function calls
> related to MediaDrm/MediaCrypto. By doing so, we can also make the
> session-related IPC calls obscure once we have a separated process(service)
> to decode video.
Yes, 
Bug 1284434 is intended to do this.

Comment 12

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review61112

Comment 13

[James Cheng[:JamesCheng]]

Thanks for your reviewing ：)

attach treeherder result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d157220010d

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3d7cad6c85
Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use. r=jwwang

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d3d7cad6c85