Closed Bug 1284809 Opened 4 years ago Closed 3 years ago

Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(1 file)

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.
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 on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review59622
Attachment #8768359 - Flags: review?(jwwang)
Summary: Move CDMCallbackProxy to be a innerclass of CDMProxy → Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use
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/
Attachment #8768359 - Attachment description: Bug 1284809 - Move CDMCallbackProxy to be a innerclass of CDMProxy. → Bug-1284809 Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.
Attachment #8768359 - Flags: review?(jwwang)
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
Attachment #8768359 - Flags: review?(jwwang) → review-
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/
Attachment #8768359 - Attachment description: Bug-1284809 Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use. → [mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.
Attachment #8768359 - Flags: review- → review?(jwwang)
Attachment #8768359 - Flags: review?(jwwang) → review+
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 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?
Attachment #8768359 - Flags: review?(cpearce)
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.
(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.
(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 on attachment 8768359 [details]
[mq]: Bug 1284809 - Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use.

https://reviewboard.mozilla.org/r/62632/#review61112
Attachment #8768359 - Flags: review?(cpearce) → review+
Thanks for your reviewing :)

attach treeherder result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d157220010d
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3d7cad6c85
Rename CDMCallbackProxy to GMPCDMCallbackProxy to make it GMP specific use. r=jwwang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d3d7cad6c85
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.