[chromecast] Uses mock backend for MediaPipelineImplTest.

[chromecast] Uses mock backend for MediaPipelineImplTest.

In addition to making the tests more flexible, it removes the dependency
on the default backend which needs to change to act like a null sink
instead of a test interface.

BUG=internal b/33274894

Committed: https://crrev.com/3463f1b6dff127ccbc419f9a32e3edfc4989f875
Cr-Commit-Position: refs/heads/master@{#439264}

[alokp, 2016-12-15 22:36:54]

alokp@chromium.org changed reviewers:
+ byungchul@chromium.org, slan@chromium.org

[gfhuang, 2016-12-15 22:39:04]

On 2016/12/15 22:36:55, alokp wrote:

I don't think these belongs to public/

public/ is specifically reserved for OEM porting API headers.

[alokp, 2016-12-15 22:44:03]

On 2016/12/15 22:39:04, gfhuang wrote:
> On 2016/12/15 22:36:55, alokp wrote:
> 
> I don't think these belongs to public/
> 
> public/ is specifically reserved for OEM porting API headers.

And this is a mock interface for those headers. Putting them anywhere else will
make it harder to find. Do you have a suggestion on where to put them?

[halliwell, 2016-12-16 00:28:19]

On 2016/12/15 22:44:03, alokp wrote:
> On 2016/12/15 22:39:04, gfhuang wrote:
> > On 2016/12/15 22:36:55, alokp wrote:
> > 
> > I don't think these belongs to public/
> > 
> > public/ is specifically reserved for OEM porting API headers.
> 
> And this is a mock interface for those headers. Putting them anywhere else
will
> make it harder to find. Do you have a suggestion on where to put them?

Correct, these should absolutely not be in public.  They should be somewhere
under chromecast/media

[alokp, 2016-12-16 00:47:02]

On 2016/12/16 00:28:19, halliwell wrote:
> On 2016/12/15 22:44:03, alokp wrote:
> > On 2016/12/15 22:39:04, gfhuang wrote:
> > > On 2016/12/15 22:36:55, alokp wrote:
> > > 
> > > I don't think these belongs to public/
> > > 
> > > public/ is specifically reserved for OEM porting API headers.
> > 
> > And this is a mock interface for those headers. Putting them anywhere else
> will
> > make it harder to find. Do you have a suggestion on where to put them?
> 
> Correct, these should absolutely not be in public.  They should be somewhere
> under chromecast/media

What is wrong with them being in public? It is not adding any dependency to the
public target itself. All the dependencies will be added to test_support. Would
adding a separate public/test directory be preferable?

In this particular case we can probably move these mocks to
chromecast/media/cma/test (not ideal because it is not as readily discovered as
being right next to the interface), but in future if we need to mock other
interfaces in chromecast/public (not chromecast/public/media), where would you
add them?

[halliwell, 2016-12-16 01:43:37]

On 2016/12/16 00:47:02, alokp wrote:
> On 2016/12/16 00:28:19, halliwell wrote:
> > On 2016/12/15 22:44:03, alokp wrote:
> > > On 2016/12/15 22:39:04, gfhuang wrote:
> > > > On 2016/12/15 22:36:55, alokp wrote:
> > > > 
> > > > I don't think these belongs to public/
> > > > 
> > > > public/ is specifically reserved for OEM porting API headers.
> > > 
> > > And this is a mock interface for those headers. Putting them anywhere else
> > will
> > > make it harder to find. Do you have a suggestion on where to put them?
> > 
> > Correct, these should absolutely not be in public.  They should be somewhere
> > under chromecast/media
> 
> What is wrong with them being in public? It is not adding any dependency to
the
> public target itself. All the dependencies will be added to test_support.
Would
> adding a separate public/test directory be preferable?
> 
> In this particular case we can probably move these mocks to
> chromecast/media/cma/test (not ideal because it is not as readily discovered
as
> being right next to the interface), but in future if we need to mock other
> interfaces in chromecast/public (not chromecast/public/media), where would you
> add them?

Because public/ is by definition a directory just for headers to ship to
external partners.  Think of it like the common pattern of having an include/
directory for a library's headers.

Providing a clean set of headers to external partners is more important than
internal considerations, I don't think it's that hard to find files.

But if you want to keep mocks closer to the headers, I think we could consider
moving existing headers into something like public/include allowing something
like public/test for the mocks?

[alokp, 2016-12-16 05:59:32]

On 2016/12/16 01:43:37, halliwell wrote:
> On 2016/12/16 00:47:02, alokp wrote:
> > On 2016/12/16 00:28:19, halliwell wrote:
> > > On 2016/12/15 22:44:03, alokp wrote:
> > > > On 2016/12/15 22:39:04, gfhuang wrote:
> > > > > On 2016/12/15 22:36:55, alokp wrote:
> > > > > 
> > > > > I don't think these belongs to public/
> > > > > 
> > > > > public/ is specifically reserved for OEM porting API headers.
> > > > 
> > > > And this is a mock interface for those headers. Putting them anywhere
else
> > > will
> > > > make it harder to find. Do you have a suggestion on where to put them?
> > > 
> > > Correct, these should absolutely not be in public.  They should be
somewhere
> > > under chromecast/media
> > 
> > What is wrong with them being in public? It is not adding any dependency to
> the
> > public target itself. All the dependencies will be added to test_support.
> Would
> > adding a separate public/test directory be preferable?
> > 
> > In this particular case we can probably move these mocks to
> > chromecast/media/cma/test (not ideal because it is not as readily discovered
> as
> > being right next to the interface), but in future if we need to mock other
> > interfaces in chromecast/public (not chromecast/public/media), where would
you
> > add them?
> 
> Because public/ is by definition a directory just for headers to ship to
> external partners.  Think of it like the common pattern of having an include/
> directory for a library's headers.
> 
> Providing a clean set of headers to external partners is more important than
> internal considerations, I don't think it's that hard to find files.
> 
> But if you want to keep mocks closer to the headers, I think we could consider
> moving existing headers into something like public/include allowing something
> like public/test for the mocks?

I will move the mocks to chromecast/media/

[alokp, 2016-12-16 06:37:12]

alokp@chromium.org changed reviewers:
+ halliwell@chromium.org

[alokp, 2016-12-16 06:37:13]

PTAL

[halliwell, 2016-12-16 20:58:38]

On 2016/12/16 06:37:13, alokp wrote:
> PTAL

lgtm

[alokp, 2016-12-16 22:42:01]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-12-16 22:42:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 00:54:20]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481928121132750,
"parent_rev": "0b3bbaf627b76e2989dbdc18fb458669a89efa47", "commit_rev":
"ba7cc8da0f84c64137b772c20cc0c24a6c9fb70c"}

[commit-bot: I haz the power, 2016-12-17 00:54:49]

Description was changed from

==========
[chromecast] Uses mock backend for MediaPipelineImplTest.

In addition to making the tests more flexible, it removes the dependency
on the default backend which needs to change to act like a null sink
instead of a test interface.

BUG=internal b/33274894
==========

to

==========
[chromecast] Uses mock backend for MediaPipelineImplTest.

In addition to making the tests more flexible, it removes the dependency
on the default backend which needs to change to act like a null sink
instead of a test interface.

BUG=internal b/33274894

Review-Url: https://codereview.chromium.org/2581013002
==========

[commit-bot: I haz the power, 2016-12-17 00:54:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-17 00:56:47]

Description was changed from

==========
[chromecast] Uses mock backend for MediaPipelineImplTest.

In addition to making the tests more flexible, it removes the dependency
on the default backend which needs to change to act like a null sink
instead of a test interface.

BUG=internal b/33274894

Review-Url: https://codereview.chromium.org/2581013002
==========

to

==========
[chromecast] Uses mock backend for MediaPipelineImplTest.

In addition to making the tests more flexible, it removes the dependency
on the default backend which needs to change to act like a null sink
instead of a test interface.

BUG=internal b/33274894

Committed: https://crrev.com/3463f1b6dff127ccbc419f9a32e3edfc4989f875
Cr-Commit-Position: refs/heads/master@{#439264}
==========

[commit-bot: I haz the power, 2016-12-17 00:56:47]

Patchset 2 (id:??) landed as
https://crrev.com/3463f1b6dff127ccbc419f9a32e3edfc4989f875
Cr-Commit-Position: refs/heads/master@{#439264}