Add blimp to root GN check_targets

Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This should only be added on linux though,
since the code that uses it is only used on linux, and adding it on
Windows when is_multi_dll_chrome is true would lead to both the browser
and the child process to get it, when only the child process should
have it. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
Committed: https://crrev.com/409b3366f93797658ebba8002564dd175bb49bd8
Cr-Commit-Position: refs/heads/master@{#390483}

[nyquist, 2016-04-25 23:34:34]

Description was changed from

==========
Add blimp to root GN check_targets

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

BUG=597830
==========

[nyquist, 2016-04-25 23:35:43]

nyquist@chromium.org changed reviewers:
+ dpranke@chromium.org, haibinlu@chromium.org, wez@chromium.org

[nyquist, 2016-04-25 23:35:44]

wez, haibinliu: PTAL //blimp
dpranke: PTAL //.gn and overall usage of GN.

[nyquist, 2016-04-25 23:36:12]

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

[commit-bot: I haz the power, 2016-04-25 23:36:48]

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

[commit-bot: I haz the power, 2016-04-25 23:42:02]

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

[commit-bot: I haz the power, 2016-04-25 23:42:03]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  linux_chromium_compile_dbg_ng 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, 2016-04-26 00:56:05]

lgtm

[Dirk Pranke, 2016-04-26 01:21:44]

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

[Dirk Pranke, 2016-04-26 01:21:45]

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:361: "//content",
Actually, thinking about this now, I might've gotten the direction backwards,
and you should actually unconditionally depend on //content/public/app:both ,
and we should modify that to forward the right configs in the component_build
case.

But, I can't remember for sure. jam@, brettw@, which way is right here?

[brettw, 2016-04-26 04:59:40]

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:361: "//content",
On 2016/04/26 01:21:44, Dirk Pranke wrote:
> Actually, thinking about this now, I might've gotten the direction backwards,
> and you should actually unconditionally depend on //content/public/app:both ,
> and we should modify that to forward the right configs in the component_build
> case.
> 
> But, I can't remember for sure. jam@, brettw@, which way is right here?

I think there is some context that I'm missing. But depending directly on
content is pretty suspicious and without context, app:both looks more correct.

[jam, 2016-04-26 16:27:06]

On 2016/04/26 04:59:40, brettw wrote:
> https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn
> File blimp/engine/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn#n...
> blimp/engine/BUILD.gn:361: "//content",
> On 2016/04/26 01:21:44, Dirk Pranke wrote:
> > Actually, thinking about this now, I might've gotten the direction
backwards,
> > and you should actually unconditionally depend on //content/public/app:both
,
> > and we should modify that to forward the right configs in the
component_build
> > case.
> > 
> > But, I can't remember for sure. jam@, brettw@, which way is right here?
> 
> I think there is some context that I'm missing. But depending directly on
> content is pretty suspicious and without context, app:both looks more correct.

I defer to Brett & Dirk

[nyquist, 2016-04-26 18:08:46]

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

[commit-bot: I haz the power, 2016-04-26 18:09:01]

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

[commit-bot: I haz the power, 2016-04-26 18:18:34]

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

[commit-bot: I haz the power, 2016-04-26 18:18:35]

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

[nyquist, 2016-04-26 21:41:33]

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

[commit-bot: I haz the power, 2016-04-26 21:41:57]

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

[nyquist, 2016-04-26 21:46:20]

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

[commit-bot: I haz the power, 2016-04-26 21:46:57]

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

[nyquist, 2016-04-26 21:47:50]

brettw, dpranke: PTAL after the new //content stuff.

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/40001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:361: "//content",
On 2016/04/26 04:59:40, brettw wrote:
> On 2016/04/26 01:21:44, Dirk Pranke wrote:
> > Actually, thinking about this now, I might've gotten the direction
backwards,
> > and you should actually unconditionally depend on //content/public/app:both
,
> > and we should modify that to forward the right configs in the
component_build
> > case.
> > 
> > But, I can't remember for sure. jam@, brettw@, which way is right here?
> 
> I think there is some context that I'm missing. But depending directly on
> content is pretty suspicious and without context, app:both looks more correct.

Updated https://codereview.chromium.org/1914933004/ with this change, and
updated all places in //blimp to never depend on //content, but instead always
depend on more specific targets within //content/public.

[commit-bot: I haz the power, 2016-04-26 22:13:24]

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

[commit-bot: I haz the power, 2016-04-26 22:13:25]

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

[haibinlu, 2016-04-26 23:11:33]

lgtm

[Dirk Pranke, 2016-04-26 23:26:39]

still lgtm.

[Wez, 2016-04-26 23:34:02]

https://codereview.chromium.org/1922613003/diff/140001/blimp/engine/common/bl...
File blimp/engine/common/blimp_user_agent.cc (right):

https://codereview.chromium.org/1922613003/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_user_agent.cc:17: std::string client_os_info = "Linux;
Android 5.1.1";
Won't this cause an initializer?

Why do we even need a global OS info - surely this is something that
BrowserMainParts or similar should hold?

[nyquist, 2016-04-27 00:56:39]

https://codereview.chromium.org/1922613003/diff/140001/blimp/engine/common/bl...
File blimp/engine/common/blimp_user_agent.cc (right):

https://codereview.chromium.org/1922613003/diff/140001/blimp/engine/common/bl...
blimp/engine/common/blimp_user_agent.cc:17: std::string client_os_info = "Linux;
Android 5.1.1";
On 2016/04/26 23:34:02, Wez wrote:
> Won't this cause an initializer?
> 
> Why do we even need a global OS info - surely this is something that
> BrowserMainParts or similar should hold?

It's part of EngineSettingsMessage, so we could move it somewhere else (the call
to SetClientOSInfo below is where we're handling the proto that contains this
data). I'll file a bug to do this as a follow-up, since this CL is just moving
stuff around.

[Wez, 2016-04-27 01:01:49]

LGTM

[nyquist, 2016-04-27 18:49:45]

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

[commit-bot: I haz the power, 2016-04-27 18:50:05]

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

[commit-bot: I haz the power, 2016-04-27 19:40:35]

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

[commit-bot: I haz the power, 2016-04-27 19:40:36]

Dry run: This issue passed the CQ dry run.

[nyquist, 2016-04-27 19:46:40]

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

[commit-bot: I haz the power, 2016-04-27 19:47:25]

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

[commit-bot: I haz the power, 2016-04-27 19:56:01]

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

[commit-bot: I haz the power, 2016-04-27 19:56:03]

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

[nyquist, 2016-04-27 20:00:57]

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

[commit-bot: I haz the power, 2016-04-27 20:02:04]

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

[commit-bot: I haz the power, 2016-04-27 21:56:44]

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

[commit-bot: I haz the power, 2016-04-27 21:56:47]

Dry run: This issue passed the CQ dry run.

[nyquist, 2016-04-27 22:09:27]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-04-27 22:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, wez@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1922613003/#ps240001
(title: "Add missing visibility entries to ppapi_plugin_sources")

[commit-bot: I haz the power, 2016-04-27 22:09:55]

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

[commit-bot: I haz the power, 2016-04-27 22:19:37]

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

[commit-bot: I haz the power, 2016-04-27 22:19:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nyquist, 2016-04-27 22:19:50]

brettw: PTAL //.gn and //content

[brettw, 2016-04-27 22:40:22]

I don't understand the ppapi_plugin changes in content. Can the CL description
address those? I don't see any code in content/app using ppapi_plugin.

[brettw, 2016-04-27 22:40:36]

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:36: "//blimp/common:blimp_common",
As a followup, this should be renamed just "//blimp/common" with an output_name
of "blimp_common" as per GN style.

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:54: defines = [ "BLIMP_CLIENT_IMPLEMENTATION=1" ]
This define won't do anything since no sources are being compiled as part of
this source set. You can just delete it.

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:141: "//components/url_formatter:url_formatter",
Just use "//components/url_formatter" (expands to same thing)

[nyquist, 2016-04-27 22:47:58]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:13:37]

brettw: Updated CL description, and incorporated the changes you suggested. I'll
do the output_name change later like you suggested.

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn
File blimp/client/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:36: "//blimp/common:blimp_common",
On 2016/04/27 22:40:36, brettw wrote:
> As a followup, this should be renamed just "//blimp/common" with an
output_name
> of "blimp_common" as per GN style.

Wow. Thanks! I'll definitely clean this up in a follow-up (in addition to a few
other targets where we've done the same thing).

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:54: defines = [ "BLIMP_CLIENT_IMPLEMENTATION=1" ]
On 2016/04/27 22:40:36, brettw wrote:
> This define won't do anything since no sources are being compiled as part of
> this source set. You can just delete it.

Done.

https://codereview.chromium.org/1922613003/diff/240001/blimp/client/BUILD.gn#...
blimp/client/BUILD.gn:141: "//components/url_formatter:url_formatter",
On 2016/04/27 22:40:36, brettw wrote:
> Just use "//components/url_formatter" (expands to same thing)

Done.

[Dirk Pranke, 2016-04-27 23:23:22]

lgtm, but update your CL description to reflect the deletion of
blimp_client_export (and any other changes that might be needed, I didn't stare
at it).

[nyquist, 2016-04-27 23:24:38]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:24:54]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:25:14]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

A similar trick was done for //blimp/client/blimp_client_export.h
which is used by both the //blimp/client:blimp_client target and
//blimp/client:feature.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:27:54]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, since //content/app/content_main_runner.cc refers to
PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content, since //content/app/content_main_runner.cc refers
to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:28:26]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content, since //content/app/content_main_runner.cc refers
to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc refers
to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:28:50]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc refers
to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a missing
dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-27 23:30:00]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[brettw, 2016-04-27 23:31:36]

The content/app thing is wrong. It took me a few minutes to figure out how this
works actually.

On Windows we have a different DLL for the browser process and the child
processes. The plugin stuff needs to be in the child one, and your patch will
link it into both. This is what the is_multi_dll_chrome flag tells us.

I'm not 100% sure how this "should" work since the same is obviously true of all
other sub-processes and I don't see those deps in content/app. Did something
particularly break for you? If you can tell me what exactly is broken I might be
able to make a better suggestion. If you just noticed this omission and were
trying to fix something, I would recommend not touching it for now :)

[brettw, 2016-04-27 23:40:57]

We talked in person about reverting the changes in content and adding a
dependency from blimp to content/public/child. LGTM with that (hopefully it
works).

[Dirk Pranke, 2016-04-28 00:25:40]

https://codereview.chromium.org/1922613003/diff/260001/content/app/BUILD.gn
File content/app/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/260001/content/app/BUILD.gn#n...
content/app/BUILD.gn:56: if (enable_plugins) {
For posterity, I think in order to address brettw's comments in #59 and #60, I
think we should change this to if (is_linux && enable_plugins), and not add the
dependency from blimp to //content/public/child. I think if we do what I
suggest, we keep the dependencies in blimp "right" and make content/app
"slightly more right", whereas if we do brettw's suggestion in #60, we make
blimp more confusing and don't make content any less confusing.

[nyquist, 2016-04-28 00:46:58]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This should only be added on linux though,
since the code that uses it is only used on linux, and adding it on
Windows when is_multi_dll_chrome is true would lead to both the browser
and the child process to get it, when only the child process should
have it. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

[nyquist, 2016-04-28 00:54:50]

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

[commit-bot: I haz the power, 2016-04-28 00:55:02]

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

[nyquist, 2016-04-28 00:56:01]

https://codereview.chromium.org/1922613003/diff/260001/content/app/BUILD.gn
File content/app/BUILD.gn (right):

https://codereview.chromium.org/1922613003/diff/260001/content/app/BUILD.gn#n...
content/app/BUILD.gn:56: if (enable_plugins) {
On 2016/04/28 00:25:40, Dirk Pranke wrote:
> For posterity, I think in order to address brettw's comments in #59 and #60, I
> think we should change this to if (is_linux && enable_plugins), and not add
the
> dependency from blimp to //content/public/child. I think if we do what I
> suggest, we keep the dependencies in blimp "right" and make content/app
> "slightly more right", whereas if we do brettw's suggestion in #60, we make
> blimp more confusing and don't make content any less confusing. 

I implemented both... So, in patch set 17 below you'll find the latest and
greatest, which for now is your suggestion of adding is_linux.

[commit-bot: I haz the power, 2016-04-28 02:00:13]

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

[commit-bot: I haz the power, 2016-04-28 02:00:15]

Dry run: This issue passed the CQ dry run.

[brettw, 2016-04-28 20:37:45]

LGTM, I'm OK with whatever at this point.

[nyquist, 2016-04-28 21:03:25]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2016-04-28 21:03:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
haibinlu@chromium.org, wez@chromium.org, dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1922613003/#ps310001
(title: "Flipped the order of the conditional in //content/app/BUILD.gn")

[commit-bot: I haz the power, 2016-04-28 21:03:56]

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

[commit-bot: I haz the power, 2016-04-28 21:10:20]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-04-30 17:21:28]

Patchset 17 (id:??) landed as
https://crrev.com/409b3366f93797658ebba8002564dd175bb49bd8
Cr-Commit-Position: refs/heads/master@{#390483}

[commit-bot: I haz the power, 2016-04-30 17:57:35]

Description was changed from

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This should only be added on linux though,
since the code that uses it is only used on linux, and adding it on
Windows when is_multi_dll_chrome is true would lead to both the browser
and the child process to get it, when only the child process should
have it. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830
==========

to

==========
Add blimp to root GN check_targets

//blimp has not been checked for GN issues until now, which has caused
incorrect dependencies to creep in, as well as several missing
dependencies.

This CL adds //blimp/* to the list of targets that should always be
checked by GN, which should prevent further issues.

As part of this change, several parts of //blimp/engine:app had to be
split out into their own source_sets since otherwise there would be
cyclic dependencies between different targets in //blimp/engine/*.

The functionality related to getting the user agent was also moved
from the blimp_content_client.h header into its own file, also to
remove a cyclic dependency.

The blimp_client_export.h file is not needed anymore, since no symbols
ever need to be exported from the client, so it has thus been removed.

Lastly, all deps that used to be //content have now been changed to
more specific ones, such as //content/public/browser. This lead to an
issue with //content/app, since //content/app/content_main_runner.cc
refers to PpapiPluginMain in RunZygote if ENABLE_PLUGINS is defined, a
missing dependency was added to the content_app_deps in
//content/app/BUILD.gn. This should only be added on linux though,
since the code that uses it is only used on linux, and adding it on
Windows when is_multi_dll_chrome is true would lead to both the browser
and the child process to get it, when only the child process should
have it. This also required updating the visibility
of the //content/ppapi:ppapi_plugin_sources target to allow for this.

BUG=597830

Committed: https://crrev.com/409b3366f93797658ebba8002564dd175bb49bd8
Cr-Commit-Position: refs/heads/master@{#390483}
==========