content_shell: Add runtime resources into data_deps.

content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#data_deps_Non_linked-dependencies
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
Committed: https://crrev.com/08c74c994672d57ff4529c949097529e0f4c71d8
Cr-Commit-Position: refs/heads/master@{#429217}

[mithro, 2016-10-27 10:01:52]

Description was changed from

==========
content: Move runtime resources from deps into data_deps.

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and resources are not linked but rather just needed at runtime.

The following files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf

This is needed so isolate correctly identifies these files as runtime
dependencies for layout tests.

BUG=524758
==========

to

==========
content_shell: Move runtime resources from deps into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

[mithro, 2016-10-27 10:01:52]

tansell@chromium.org changed reviewers:
+ dpranke@chromium.org, jochen@chromium.org

[mithro, 2016-10-27 10:02:13]

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

[commit-bot: I haz the power, 2016-10-27 10:02:32]

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

[mithro, 2016-10-27 10:52:57]

Hi!

I'm in the process of trying to create a webkit_layout_tests target which can be
isolated (see https://codereview.chromium.org/2452313003/). This requires a
couple of small changes to other build targets which don't seem to be using
data_deps correctly. These are the changes needed for content_shell.

Tim 'mithro' Ansell

[commit-bot: I haz the power, 2016-10-27 12:40:04]

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

[commit-bot: I haz the power, 2016-10-27 12:40:04]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-10-28 00:25:56]

https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn#newc...
content/shell/BUILD.gn:508: data_deps = [
Are you sure this isn't also needed as a regular dependency, so that all of the
resources get compiled at the right time?

[jochen (gone - plz use gerrit), 2016-10-28 13:39:45]

will approve once dpranke approves

[Dirk Pranke, 2016-10-28 14:33:16]

lgtm

[mithro, 2016-10-31 01:46:47]

https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn
File content/shell/BUILD.gn (right):

https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn#newc...
content/shell/BUILD.gn:508: data_deps = [
On 2016/10/28 00:25:56, Dirk Pranke wrote:
> Are you sure this isn't also needed as a regular dependency, so that all of
the
> resources get compiled at the right time?

Does anything need the :pak at compile / linking time? How would I even find
this out?

The trybots all seem to pass...

[jochen (gone - plz use gerrit), 2016-10-31 10:40:08]

lgtm

[Dirk Pranke, 2016-10-31 17:36:10]

On 2016/10/31 01:46:47, mithro wrote:
> https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn
> File content/shell/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn#newc...
> content/shell/BUILD.gn:508: data_deps = [
> On 2016/10/28 00:25:56, Dirk Pranke wrote:
> > Are you sure this isn't also needed as a regular dependency, so that all of
> the
> > resources get compiled at the right time?
> 
> Does anything need the :pak at compile / linking time? How would I even find
> this out?

Good question. I'd try removing the data_dep and just building content_shell,
and
seeing if anything else builds :pak. If not (i.e., it never gets built), then it
clearly
can't be needed. If it is built, you might see what else is building it and why
...

`gn check` *might* catch issues, but I think it misses some for generated
headers
and I don't think it's enforced for everything in //content and below yet.

But this doesn't need to block landing this (or at least, a change to make it
also be a data dep),
you just get bonus points for double-checking.

[mithro, 2016-11-01 07:39:29]

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

[commit-bot: I haz the power, 2016-11-01 07:39:46]

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

[mithro, 2016-11-01 07:42:28]

On 2016/10/31 17:36:10, Dirk Pranke wrote:
> On 2016/10/31 01:46:47, mithro wrote:
> > https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn
> > File content/shell/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn#newc...
> > content/shell/BUILD.gn:508: data_deps = [
> > On 2016/10/28 00:25:56, Dirk Pranke wrote:
> > > Are you sure this isn't also needed as a regular dependency, so that all
of
> > the
> > > resources get compiled at the right time?
> > 
> > Does anything need the :pak at compile / linking time? How would I even find
> > this out?
> 
> Good question. I'd try removing the data_dep and just building content_shell,
> and
> seeing if anything else builds :pak. If not (i.e., it never gets built), then
it
> clearly
> can't be needed. If it is built, you might see what else is building it and
why
> ...
> 
> `gn check` *might* catch issues, but I think it misses some for generated
> headers
> and I don't think it's enforced for everything in //content and below yet.
> 
> But this doesn't need to block landing this (or at least, a change to make it
> also be a data dep),
> you just get bonus points for double-checking.

I commented out the //content/shell:pak in content/shell/BUILD.gn but the
content_shell pak was still being generated. I ended up using `gn desc --tree
//out/Release //content/shell:content_shell 2>&1 | less` to discover that it was
also a dependent of "test_support" in content/test/BUILD.gn -- Fixing that
reference seems to prevent the pak from being built.

As far as I can tell, the .pak files are *only* need at runtime and not needed
as part of the build process for anything. I have no idea how to actually prove
this result and I'm a bit worried they end up being needed for Mac bundles or
Android apks.

I'm going to take a look at splitting this CL into two;
 * One which adds the data_deps needed to get the correct runtime_deps output.
 * One which removes the duplicates from deps.

Tim 'mithro' Ansell

[commit-bot: I haz the power, 2016-11-01 07:43:11]

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

[commit-bot: I haz the power, 2016-11-01 07:43:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mithro, 2016-11-01 08:32:03]

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

[commit-bot: I haz the power, 2016-11-01 08:32:19]

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

[mithro, 2016-11-01 08:41:38]

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

[commit-bot: I haz the power, 2016-11-01 08:41:54]

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

[mithro, 2016-11-01 09:14:06]

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

[commit-bot: I haz the power, 2016-11-01 09:14:12]

Dry run: 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-11-01 10:34:19]

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

[commit-bot: I haz the power, 2016-11-01 10:34:20]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-11-01 16:23:02]

On 2016/11/01 07:42:28, mithro wrote:
> On 2016/10/31 17:36:10, Dirk Pranke wrote:
> > On 2016/10/31 01:46:47, mithro wrote:
> > > https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn
> > > File content/shell/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2460503002/diff/1/content/shell/BUILD.gn#newc...
> > > content/shell/BUILD.gn:508: data_deps = [
> > > On 2016/10/28 00:25:56, Dirk Pranke wrote:
> > > > Are you sure this isn't also needed as a regular dependency, so that all
> of
> > > the
> > > > resources get compiled at the right time?
> > > 
> > > Does anything need the :pak at compile / linking time? How would I even
find
> > > this out?
> > 
> > Good question. I'd try removing the data_dep and just building
content_shell,
> > and
> > seeing if anything else builds :pak. If not (i.e., it never gets built),
then
> it
> > clearly
> > can't be needed. If it is built, you might see what else is building it and
> why
> > ...
> > 
> > `gn check` *might* catch issues, but I think it misses some for generated
> > headers
> > and I don't think it's enforced for everything in //content and below yet.
> > 
> > But this doesn't need to block landing this (or at least, a change to make
it
> > also be a data dep),
> > you just get bonus points for double-checking.
> 
> I commented out the //content/shell:pak in content/shell/BUILD.gn but the
> content_shell pak was still being generated. I ended up using `gn desc --tree
> //out/Release //content/shell:content_shell 2>&1 | less` to discover that it
was
> also a dependent of "test_support" in content/test/BUILD.gn -- Fixing that
> reference seems to prevent the pak from being built.
> 
> As far as I can tell, the .pak files are *only* need at runtime and not needed
> as part of the build process for anything. I have no idea how to actually
prove
> this result and I'm a bit worried they end up being needed for Mac bundles or
> Android apks.
> 
> I'm going to take a look at splitting this CL into two;
>  * One which adds the data_deps needed to get the correct runtime_deps output.
>  * One which removes the duplicates from deps.

Sounds good.

[mithro, 2016-11-02 04:02:48]

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

[commit-bot: I haz the power, 2016-11-02 04:02:58]

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

[mithro, 2016-11-02 04:04:48]

Description was changed from

==========
content_shell: Move runtime resources from deps into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

to

==========
content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

[mithro, 2016-11-02 05:41:34]

The CQ bit was unchecked by tansell@chromium.org

[mithro, 2016-11-02 05:41:42]

The CQ bit was checked by tansell@chromium.org

[mithro, 2016-11-02 05:41:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2460503002/#ps100001
(title: "Moving the removal to another CL.")

[commit-bot: I haz the power, 2016-11-02 05:41:53]

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-11-02 05:46:13]

Description was changed from

==========
content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

to

==========
content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

[commit-bot: I haz the power, 2016-11-02 05:46:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-02 05:47:41]

Description was changed from

==========
content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf


https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758
==========

to

==========
content_shell: Add runtime resources into data_deps.

This is needed so isolate correctly identifies these files as runtime
dependencies for content shell (and thus webkit layout tests).

The following new files are now included in
`gn desc //out/Release //content/shell:content_shell runtime_deps`;

 * content_shell.pak
 * AHEM____.TTF
 * GardinerModBug.ttf
 * GardinerModCat.ttf
 * fonts.conf

https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...
> data_deps are non-linked runtime dependencies. They are built and available
> at runtime.

The .pak and font resources are not linked but rather just needed at
runtime.

BUG=524758

Committed: https://crrev.com/08c74c994672d57ff4529c949097529e0f4c71d8
Cr-Commit-Position: refs/heads/master@{#429217}
==========

[commit-bot: I haz the power, 2016-11-02 05:47:42]

Patchset 6 (id:??) landed as
https://crrev.com/08c74c994672d57ff4529c949097529e0f4c71d8
Cr-Commit-Position: refs/heads/master@{#429217}