Fix missing dependencies for //components/crash.

Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org, jam@chromium.org, pkotwicz@chromium.org
BUG=547635
Committed: https://crrev.com/dd9600d94ae6f75a9e66fac0fb691207f005ecfd
Cr-Commit-Position: refs/heads/master@{#356993}

[Dirk Pranke, 2015-10-26 21:37:49]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2015-10-26 21:37:56]

Patchset #1 (id:20001) has been deleted

[Dirk Pranke, 2015-10-26 21:38:14]

Description was changed from

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org
BUG=547635
==========

to

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org
BUG=547635
==========

[Dirk Pranke, 2015-10-26 21:38:14]

dpranke@chromium.org changed reviewers:
+ jam@chromium.org

[Dirk Pranke, 2015-10-26 21:57:27]

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi
File components/crash.gypi (left):

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi#o...
components/crash.gypi:175: #
In theory, depending directly on content should now be safe.

[Dirk Pranke, 2015-10-26 21:57:46]

Description was changed from

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org
BUG=547635
==========

to

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org, jam@chromium.org
BUG=547635
==========

[Dirk Pranke, 2015-10-26 22:44:15]

dpranke@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[Dirk Pranke, 2015-10-26 22:44:26]

Description was changed from

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org, jam@chromium.org
BUG=547635
==========

to

==========
Fix missing dependencies for //components/crash.

This patch adds the dependencies that //components/crash that were
missing that was making `gn check` upset and, in at least one case,
causing us to actually miscompile things because the public_configs
were not being propagated from content properly.

R=rsesek@chromium.org, jam@chromium.org, pkotwicz@chromium.org
BUG=547635
==========

[pkotwicz, 2015-10-27 00:11:32]

lgtm

[Robert Sesek, 2015-10-27 21:11:17]

Sorry for the latency on this.

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi
File components/crash.gypi (right):

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi#n...
components/crash.gypi:200: '../content/content.gyp:content_common',
Is this the right target on which to add the dependency? It seems like this
should go on crash_component_non_mac.

[Dirk Pranke, 2015-10-27 21:30:48]

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi
File components/crash.gypi (right):

https://codereview.chromium.org/1423043002/diff/40001/components/crash.gypi#n...
components/crash.gypi:200: '../content/content.gyp:content_common',
On 2015/10/27 21:11:16, Robert Sesek wrote:
> Is this the right target on which to add the dependency? It seems like this
> should go on crash_component_non_mac.

I think you're probably right.

[Dirk Pranke, 2015-10-27 21:33:58]

okay, trying again now that the conflicting CLs have landed ...

https://codereview.chromium.org/1423043002/diff/60001/components/crash/conten...
File components/crash/content/app/BUILD.gn (right):

https://codereview.chromium.org/1423043002/diff/60001/components/crash/conten...
components/crash/content/app/BUILD.gn:68: "//components/crash/content/browser",
this is needed to make `gn check` happy, since crash_handler_host_linux.cc is
including breakpad_linux_impl.h.

I figured that that would be fixed as part of cleaning this up, so this change
would be ok. Let me know if you prefer a different approach.

[Dirk Pranke, 2015-10-27 21:42:57]

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

[commit-bot: I haz the power, 2015-10-27 21:44:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423043002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423043002/60001

[commit-bot: I haz the power, 2015-10-27 21:50:04]

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

[commit-bot: I haz the power, 2015-10-27 21:50:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Dirk Pranke, 2015-10-27 22:19:28]

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

[commit-bot: I haz the power, 2015-10-27 22:20:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423043002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423043002/80001

[commit-bot: I haz the power, 2015-10-27 22:37:11]

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

[commit-bot: I haz the power, 2015-10-27 22:37:11]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Dirk Pranke, 2015-10-27 23:11:24]

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

[commit-bot: I haz the power, 2015-10-27 23:12:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423043002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423043002/120001

[commit-bot: I haz the power, 2015-10-28 00:41:37]

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

[commit-bot: I haz the power, 2015-10-28 00:41:38]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Robert Sesek, 2015-10-28 15:31:46]

LGTM

[Dirk Pranke, 2015-10-28 23:10:36]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-10-28 23:10:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1423043002/#ps140001
(title: "fix win linkage issues, revert gypi change")

[commit-bot: I haz the power, 2015-10-28 23:11:09]

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

[commit-bot: I haz the power, 2015-10-28 23:15:16]

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

[commit-bot: I haz the power, 2015-10-28 23:15:16]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Dirk Pranke, 2015-10-29 00:24:10]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-10-29 00:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/1423043002/#ps160001
(title: "fix crash_keys dep")

[commit-bot: I haz the power, 2015-10-29 00:26:56]

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

[commit-bot: I haz the power, 2015-10-29 00:38:51]

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

[commit-bot: I haz the power, 2015-10-29 00:38:52]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dirk Pranke, 2015-10-29 00:43:30]

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

[commit-bot: I haz the power, 2015-10-29 00:45:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423043002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423043002/180001

[commit-bot: I haz the power, 2015-10-29 01:31:02]

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

[commit-bot: I haz the power, 2015-10-29 01:31:03]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2015-10-29 01:34:35]

On 2015/10/29 01:31:03, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

jam@, can you take a look and make sure this LGTY (and give the OWNERS approval
for the .gn change as well)?

[jam, 2015-10-29 16:11:44]

https://codereview.chromium.org/1423043002/diff/180001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/1423043002/diff/180001/content/public/common/...
content/public/common/BUILD.gn:34: "content_descriptors.h",
can you explain to me what this accomplishes?

i.e. from our chat, you mentioned "the problem is that breakpad_linux.cc
actually includes content/public/common/content_descriptors.h but doesn't
specify a dependency on it, and so it doesn't get the right set of settings
propagated to it"

How does having a target with one header propagate settings? is it because of
the import(s) above?

[Dirk Pranke, 2015-10-29 21:27:47]

https://codereview.chromium.org/1423043002/diff/180001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/1423043002/diff/180001/content/public/common/...
content/public/common/BUILD.gn:34: "content_descriptors.h",
On 2015/10/29 16:11:44, jam wrote:
> can you explain to me what this accomplishes?
> 
> i.e. from our chat, you mentioned "the problem is that breakpad_linux.cc
> actually includes content/public/common/content_descriptors.h but doesn't
> specify a dependency on it, and so it doesn't get the right set of settings
> propagated to it"
> 
> How does having a target with one header propagate settings? is it because of
> the import(s) above?

Argh! You're right. This change is wrong. This needs to be

    source_set("content_descriptors") {
      sources = [ "content_descriptors.h" ]
      public_configs = [ "//v8:external_startup_data" ] 
    }

so that the V8 define gets propagated up to anyone depending on this file.

Originally I had this fixed by making the crash components depend on 
//content/public/common (which does have the public_config as I added it 
earlier) but that doesn't actually work as when you link chrome it caused
content_switches.cc to get linked in twice. I changed content_common to the
new target to prevent content_switches from getting sucked in, but forgot
to propagate the config over.

Good catch! I would've felt stupid landing this and realizing it was wrong
afterwards.

[Dirk Pranke, 2015-10-29 21:30:14]

fix uploaded. 

jam, please take another look?

[jam, 2015-10-29 23:17:26]

lgtm

[Dirk Pranke, 2015-10-29 23:18:07]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-10-29 23:18:07]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/1423043002/#ps200001
(title: "double-sigh. Add v8 config to content_descriptors target")

[commit-bot: I haz the power, 2015-10-29 23:19:13]

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

[commit-bot: I haz the power, 2015-10-30 00:05:31]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-10-30 00:06:38]

Patchset 9 (id:??) landed as
https://crrev.com/dd9600d94ae6f75a9e66fac0fb691207f005ecfd
Cr-Commit-Position: refs/heads/master@{#356993}