1397123 - [EME] Decouple ChromiumCDMProxy from ChromiumCDMParent.

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

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

Comment 1

[James Cheng[:JamesCheng]]

Attachment: Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

1. Make aMessage in CDMProxy::OnSessionMessage be const reference.
2. Pass ChromiumCDMCallback interface to ChromiumCDMParent instead of ChromiumCDMProxy directly.
3. Wrap dispatching to main thread function to clean up the redundant code.

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

Comment 2

[James Cheng[:JamesCheng]]

Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

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

Comment 3

[James Cheng[:JamesCheng]]

Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

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

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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/

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.

1. Pass ChromiumCDMCallback interface to ChromiumCDMParent instead of ChromiumCDMProxy directly.
2. Wrap dispatching to main thread function to clean up the redundant code.

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

Comment 6

[James Cheng[:JamesCheng]]

Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

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

Comment 7

[James Cheng[:JamesCheng]]

Fixed the review feedback,

Thank you and I will do mach clang-format from now on.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

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;

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

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

Comment 10

[James Cheng[:JamesCheng]]

Comment on attachment 8906467 [details]
Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.

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

Comment 11

[James Cheng[:JamesCheng]]

(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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/99f47bca403a for Android build bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=130246546&repo=autoland

Comment 14

[James Cheng[:JamesCheng]]

Comment on attachment 8905390 [details]
Bug 1397123 - [Part1] Make aMessage of CDMProxy::OnSessionMessage const.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177170/diff/4-5/

Comment 15

[James Cheng[:JamesCheng]]

Comment on attachment 8906467 [details]
Bug 1397123 - [Part2] Decouple ChromiumCDMProxy from ChromiumCDMParent.

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

Comment 16

[James Cheng[:JamesCheng]]

Updated the patch for Fennec-only code.
Try looks good,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=512f688e4c55ac41824b9ca227cbef1b91835d9d

Re-push it again.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7d56316ea722
https://hg.mozilla.org/mozilla-central/rev/a359e9bd31c7