Closed Bug 1397123 Opened 6 years ago Closed 6 years ago

[EME] Decouple ChromiumCDMProxy from ChromiumCDMParent.

Categories

(Core :: Audio/Video: GMP, enhancement, P1)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(2 files)

Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
When I'm trying to fix Bug 1396493, in TestGMPCrossOrigin, I get the CDM by 

'service->GetCDM(' and it will resolve the promise with ChromiumCDMParent instance, then I need to call its Init function, but it needs a ChromiumCDMProxy[1] which I couldn't create it in gtest.

I think I should follow the idea from [2] to loose the dependency between classes.

[1]
http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/dom/media/gmp/ChromiumCDMParent.cpp#43
[2]
https://dxr.mozilla.org/mozilla-beta/rev/72a20e86b67a02fe180bf32f5526998e9fd52d55/dom/media/gmp/GMPCDMCallbackProxy.h#18
Summary: Decouple ChromiumCDMProxy from ChromiumCDMParent. → [EME] Decouple ChromiumCDMProxy from ChromiumCDMParent.
Blocks: 1396493, 1392506
Rank: 15
Priority: -- → P1
Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

https://reviewboard.mozilla.org/r/177170/#review183056

::: commit-message-d8e23:3
(Diff revision 3)
> +Bug 1397123 - [EME] Decouple ChromiumCDMProxy from ChromiumCDMParent. r?cpearce
> +
> +1. Make aMessage in CDMProxy::OnSessionMessage be const reference.

Making the aMessage argument of OnSessionMessage() const should be a separate changeset. It is logically a separate and unrelated change.

One changeset per idea.

::: dom/media/gmp/ChromiumCDMCallbackProxy.cpp:126
(Diff revision 3)
> +                                            nsTArray<mozilla::gmp::CDMKeyInformation> && aKeysInfo)
> +{
> +  bool keyStatusesChange = false;
> +  {
> +    CDMCaps::AutoLock caps(mProxy->Capabilites());
> +    for (size_t i = 0; i < aKeysInfo.Length(); i++) {

This would be nicer as a range-based for loop.

::: dom/media/gmp/ChromiumCDMParent.h:22
(Diff revision 3)
>  #include "PlatformDecoderModule.h"
>  #include "ImageContainer.h"
>  #include "mozilla/Span.h"
>  #include "ReorderQueue.h"
>  
> +

You added an extra line break here. Please run `./mach clang-format` on your patches before requesting review.

::: dom/media/gmp/ChromiumCDMParent.cpp:1084
(Diff revision 3)
> -        mProxy,
> -        &ChromiumCDMProxy::Shutdown),
> -      NS_DISPATCH_NORMAL);
>    }
>  
> -  // We may be called from a task holding the last reference to the proxy, so
> +  // We may be called from a task holding the last reference to the cdmcallback, so

s/cdmcallback/CDM callback/
Attachment #8905390 - Flags: review?(cpearce) → review-
Fixed the review feedback,

Thank you and I will do mach clang-format from now on.
Comment on attachment 8906467 [details]
Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.

https://reviewboard.mozilla.org/r/178218/#review183542

::: dom/media/gmp/ChromiumCDMParent.h:9
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef ChromiumCDMParent_h_
>  #define ChromiumCDMParent_h_
>  
> +#include "ChromiumCDMCallback.h"

Where possible, you should predeclare classes where the compiler doesn't yet need to know the size of the object. That way you don't need to #include the file. This will speed up compilation.

I think that applies here. So instead of including ChromiumCDMCallback, pre-declare with:

class ChromiumCDMCallback;
Attachment #8906467 - Flags: review?(cpearce) → review+
Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

https://reviewboard.mozilla.org/r/177170/#review183544
Attachment #8905390 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8906467 [details]
> Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.
> 
> https://reviewboard.mozilla.org/r/178218/#review183542
> 
> ::: dom/media/gmp/ChromiumCDMParent.h:9
> (Diff revision 1)
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  #ifndef ChromiumCDMParent_h_
> >  #define ChromiumCDMParent_h_
> >  
> > +#include "ChromiumCDMCallback.h"
> 
> Where possible, you should predeclare classes where the compiler doesn't yet
> need to know the size of the object. That way you don't need to #include the
> file. This will speed up compilation.
> 
> I think that applies here. So instead of including ChromiumCDMCallback,
> pre-declare with:
> 
> class ChromiumCDMCallback;

Yes, I miss this one.

Thank you.
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d41a6eb4df4
[Part1] Make aMessage of CDMProxy::OnSessionMessage const. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/803c2d6f4be4
[Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent. r=cpearce
Updated the patch for Fennec-only code.
Try looks good,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=512f688e4c55ac41824b9ca227cbef1b91835d9d

Re-push it again.
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d56316ea722
[Part1] Make aMessage of CDMProxy::OnSessionMessage const. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/a359e9bd31c7
[Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent. r=cpearce
You need to log in before you can comment on or make changes to this bug.