[chromecast] Deletes libcast_media_1.0_default_core.

[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453
Committed: https://crrev.com/7820158d927c0128b492b1180707843d8e7193cf
Cr-Commit-Position: refs/heads/master@{#439322}

[alokp, 2016-12-14 19:41:45]

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

[kmackay, 2016-12-14 19:49:06]

kmackay@chromium.org changed reviewers:
+ byungchul@chromium.org

[alokp, 2016-12-14 22:20:53]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 22:21:32]

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

[alokp, 2016-12-14 22:25:53]

slan@: I followed your suggestion of not creating a separate target for default
backend.

kmackay@: I created a dummy video decoder for alsa to avoid depending on the
default backend.

PTAL

[commit-bot: I haz the power, 2016-12-14 23:25:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 23:25:05]

Dry run: This issue passed the CQ dry run.

[alokp, 2016-12-15 22:37:56]

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/BUILD.gn
File chromecast/media/BUILD.gn (right):

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/BUILD....
chromecast/media/BUILD.gn:62: # TODO(alokp): Remove this dependency by
refactoring
This TODO has now been implemented in a separate patch here:
https://codereview.chromium.org/2581013002/

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

Byungchul: Ping!

[byungchul, 2016-12-16 18:26:18]

Description was changed from

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453
==========

to

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453
==========

[byungchul, 2016-12-16 18:26:19]

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

[byungchul, 2016-12-16 18:26:29]

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/BUILD.gn (right):

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:28: sources += [ "cast_media_android.cc" ]
why doesn't android link libcast_media? In general, I prefer one shlib for atv
cast receiver (libcast_receiver.so), but libcast_media was already split for
atv.

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:37: shared_library("libcast_media_1.0") {
@Stepghen,

Doesn't it need a version script to expose only media shlib interface?

[alokp, 2016-12-16 18:29:37]

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/BUILD.gn (right):

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:28: sources += [ "cast_media_android.cc" ]
On 2016/12/16 18:26:29, byungchul wrote:
> why doesn't android link libcast_media? In general, I prefer one shlib for atv
> cast receiver (libcast_receiver.so), but libcast_media was already split for
> atv.

slan@ asked me to do this

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:37: shared_library("libcast_media_1.0") {
On 2016/12/16 18:26:29, byungchul wrote:
> @Stepghen,
> 
> Doesn't it need a version script to expose only media shlib interface?

We have scrapped the version script idea for now AFAIK

[slan, 2016-12-16 23:04:15]

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/BUILD.gn (right):

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:28: sources += [ "cast_media_android.cc" ]
On 2016/12/16 18:29:37, alokp wrote:
> On 2016/12/16 18:26:29, byungchul wrote:
> > why doesn't android link libcast_media? In general, I prefer one shlib for
atv
> > cast receiver (libcast_receiver.so), but libcast_media was already split for
> > atv.
> 
> slan@ asked me to do this

This was split only recently (https://codereview.chromium.org/2508803012), and I
don't think that it was the right change. Why deviate from our pattern, and add
a shlib if not necessary?

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:37: shared_library("libcast_media_1.0") {
On 2016/12/16 18:29:37, alokp wrote:
> On 2016/12/16 18:26:29, byungchul wrote:
> > @Stepghen,
> > 
> > Doesn't it need a version script to expose only media shlib interface?
> 
> We have scrapped the version script idea for now AFAIK

Byungchul: This is OK, because we are only linking in the extra symbols on the
cast_desktop build, where we will never swap out the library at runtime. So it
doesn't matter where the symbols come from. 

Non-desktop builds require us to swap the lib out at runtime. Therefore we can't
link anything except cast_media_dummy.

[byungchul, 2016-12-16 23:53:00]

lgtm

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
File chromecast/media/cma/backend/BUILD.gn (right):

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:28: sources += [ "cast_media_android.cc" ]
On 2016/12/16 23:04:14, slan wrote:
> On 2016/12/16 18:29:37, alokp wrote:
> > On 2016/12/16 18:26:29, byungchul wrote:
> > > why doesn't android link libcast_media? In general, I prefer one shlib for
> atv
> > > cast receiver (libcast_receiver.so), but libcast_media was already split
for
> > > atv.
> > 
> > slan@ asked me to do this
> 
> This was split only recently (https://codereview.chromium.org/2508803012), and
I
> don't think that it was the right change. Why deviate from our pattern, and
add
> a shlib if not necessary?
> 

I am also fine with this as long as Luke is fine.

https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
chromecast/media/cma/backend/BUILD.gn:37: shared_library("libcast_media_1.0") {
On 2016/12/16 23:04:14, slan wrote:
> On 2016/12/16 18:29:37, alokp wrote:
> > On 2016/12/16 18:26:29, byungchul wrote:
> > > @Stepghen,
> > > 
> > > Doesn't it need a version script to expose only media shlib interface?
> > 
> > We have scrapped the version script idea for now AFAIK
> 
> Byungchul: This is OK, because we are only linking in the extra symbols on the
> cast_desktop build, where we will never swap out the library at runtime. So it
> doesn't matter where the symbols come from. 
> 
> Non-desktop builds require us to swap the lib out at runtime. Therefore we
can't
> link anything except cast_media_dummy.

Okay, then production build still links libcast_media_dummy?

[slan, 2016-12-16 23:58:04]

On 2016/12/16 23:53:00, byungchul wrote:
> lgtm
> 
>
https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
> File chromecast/media/cma/backend/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
> chromecast/media/cma/backend/BUILD.gn:28: sources += [ "cast_media_android.cc"
]
> On 2016/12/16 23:04:14, slan wrote:
> > On 2016/12/16 18:29:37, alokp wrote:
> > > On 2016/12/16 18:26:29, byungchul wrote:
> > > > why doesn't android link libcast_media? In general, I prefer one shlib
for
> > atv
> > > > cast receiver (libcast_receiver.so), but libcast_media was already split
> for
> > > > atv.
> > > 
> > > slan@ asked me to do this
> > 
> > This was split only recently (https://codereview.chromium.org/2508803012),
and
> I
> > don't think that it was the right change. Why deviate from our pattern, and
> add
> > a shlib if not necessary?
> > 
> 
> I am also fine with this as long as Luke is fine.
> 
>
https://codereview.chromium.org/2577743002/diff/20001/chromecast/media/cma/ba...
> chromecast/media/cma/backend/BUILD.gn:37: shared_library("libcast_media_1.0")
{
> On 2016/12/16 23:04:14, slan wrote:
> > On 2016/12/16 18:29:37, alokp wrote:
> > > On 2016/12/16 18:26:29, byungchul wrote:
> > > > @Stepghen,
> > > > 
> > > > Doesn't it need a version script to expose only media shlib interface?
> > > 
> > > We have scrapped the version script idea for now AFAIK
> > 
> > Byungchul: This is OK, because we are only linking in the extra symbols on
the
> > cast_desktop build, where we will never swap out the library at runtime. So
it
> > doesn't matter where the symbols come from. 
> > 
> > Non-desktop builds require us to swap the lib out at runtime. Therefore we
> can't
> > link anything except cast_media_dummy.
> 
> Okay, then production build still links libcast_media_dummy?

Right.

[alokp, 2016-12-17 01:16:56]

Now that cast_media_unittests does not depend on the default backend, I was able
to further simplify libcast_media target.

Luke: PTAL

[halliwell, 2016-12-17 02:20:46]

On 2016/12/17 01:16:56, alokp wrote:
> Now that cast_media_unittests does not depend on the default backend, I was
able
> to further simplify libcast_media target.
> 
> Luke: PTAL

lgtm, thanks for sorting this out! :)

[alokp, 2016-12-17 03:08:09]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-12-17 03:08:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
byungchul@chromium.org
Link to the patchset: https://codereview.chromium.org/2577743002/#ps40001
(title: "rebase + simplify libcast_media target")

[commit-bot: I haz the power, 2016-12-17 03:08:38]

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 04:43:17]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481944089544440,
"parent_rev": "1261aec2d6acd8a56f834159e29af0b9e957936c", "commit_rev":
"35cce00163dc700085740e0045aa61af324dc14c"}

[commit-bot: I haz the power, 2016-12-17 04:43:43]

Description was changed from

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453
==========

to

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453

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

[commit-bot: I haz the power, 2016-12-17 04:43:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-17 04:48:16]

Description was changed from

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453

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

to

==========
[chromecast] Deletes libcast_media_1.0_default_core.

Links against libcast_media_1.0_default on desktop by default.

BUG=581453

Committed: https://crrev.com/7820158d927c0128b492b1180707843d8e7193cf
Cr-Commit-Position: refs/heads/master@{#439322}
==========

[commit-bot: I haz the power, 2016-12-17 04:48:17]

Patchset 3 (id:??) landed as
https://crrev.com/7820158d927c0128b492b1180707843d8e7193cf
Cr-Commit-Position: refs/heads/master@{#439322}