GN: Make clearkeycdm a data_dep for clearkeycdmadapter

GN: Use forward_variables_from and remove unnecessary ldflags for cdm_adapter.gni

This is just a simplification, no behavioural change.

Committed: https://crrev.com/348d96f86b50db70c45928c93a2f5d6255d3475a
Cr-Commit-Position: refs/heads/master@{#342818}

[agrieve, 2015-08-10 18:21:25]

agrieve@chromium.org changed reviewers:
+ brettw@chromium.org

[agrieve, 2015-08-10 18:21:25]

Another spot where loadable_module is subtly different. In this case it seems
nicer the GN way though.

[brettw, 2015-08-10 19:57:36]

https://codereview.chromium.org/1276973003/diff/1/media/cdm/ppapi/BUILD.gn
File media/cdm/ppapi/BUILD.gn (left):

https://codereview.chromium.org/1276973003/diff/1/media/cdm/ppapi/BUILD.gn#ol...
media/cdm/ppapi/BUILD.gn:71: # Because clearkeycdm has type 'loadable_module',
we must explicitly
This old code is pretty confusing.

I *think* you can actually delete this block (at least the ldflags thing). This
is trying to work around the fact that clearkeycdm is a loadable module and link
to it by manually specifying the generated library on the link line.

But the fact that it's in the deps above will make it link to that. I don't
think we even need the data deps.

I'm not sure what the "rt" lib is for.

[agrieve, 2015-08-10 20:04:14]

https://codereview.chromium.org/1276973003/diff/1/media/cdm/ppapi/BUILD.gn
File media/cdm/ppapi/BUILD.gn (left):

https://codereview.chromium.org/1276973003/diff/1/media/cdm/ppapi/BUILD.gn#ol...
media/cdm/ppapi/BUILD.gn:71: # Because clearkeycdm has type 'loadable_module',
we must explicitly
On 2015/08/10 19:57:36, brettw wrote:
> This old code is pretty confusing.
> 
> I *think* you can actually delete this block (at least the ldflags thing).
This
> is trying to work around the fact that clearkeycdm is a loadable module and
link
> to it by manually specifying the generated library on the link line.
> 
> But the fact that it's in the deps above will make it link to that. I don't
> think we even need the data deps.
> 
> I'm not sure what the "rt" lib is for.

The presubmit failed, so could be that's what the "rt" is for. However, my
attempts to make it fail locally have all failed to fail so far.

The .so is listed in the .isolate file, so for that reason it makes sense as a
data_dep I think.

[brettw, 2015-08-10 23:55:32]

> The .so is listed in the .isolate file, so for that reason it makes sense as a
> data_dep I think.

It looks like we do need "rt" for the clock_gettime function. So we'll need to
put that back.

GN will count shared libraries in the dependency tree when generating it's
equivalent of an isolate since a shared library is always needed at runtime, in
addition to being linked to. Since it already appears in the "deps" above, I
still think we can remove the data_deps line.

[agrieve, 2015-08-11 00:16:55]

On 2015/08/10 23:55:32, brettw wrote:
> > The .so is listed in the .isolate file, so for that reason it makes sense as
a
> > data_dep I think.
> 
> It looks like we do need "rt" for the clock_gettime function. So we'll need to
> put that back.
> 
> GN will count shared libraries in the dependency tree when generating it's
> equivalent of an isolate since a shared library is always needed at runtime,
in
> addition to being linked to. Since it already appears in the "deps" above, I
> still think we can remove the data_deps line.

Removed data_dep, put back lib (but put it in the same file as the source that
requires it exists), added in forward_variables_from, and updated commit
message.

[brettw, 2015-08-11 01:28:02]

lgtm

https://codereview.chromium.org/1276973003/diff/20001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.gni (right):

https://codereview.chromium.org/1276973003/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.gni:42: } else if (is_posix) {
Just making sure you meant to remove the mac condition here.

[agrieve, 2015-08-11 13:53:04]

https://codereview.chromium.org/1276973003/diff/20001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.gni (right):

https://codereview.chromium.org/1276973003/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.gni:42: } else if (is_posix) {
On 2015/08/11 01:28:02, brettw wrote:
> Just making sure you meant to remove the mac condition here.

Yep - it's redundant since there's "if (is_mac) {" as the first condition.

[agrieve, 2015-08-11 13:53:08]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-08-11 13:53:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1276973003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1276973003/20001

[commit-bot: I haz the power, 2015-08-11 13:59:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-08-11 14:00:15]

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