GN: Fix Android component build

GN: Fix Android component build

This change makes all libraries in the Android component build link
successfully (including fixing and enabling libchrome_shell). This also makes
all the libraries link in a component build for Linux, but without bot coverage
for that platform it'll surely regress.

This is almost entirely just fixing some missing/incorrect dependencies and
adding missing source files for Android.

Some targets were depending on an internal source_set/static_library when they
should have been (or already were) depending on the corresponding component. In
these cases, I added some visibility restrictions to those internal targets to
try to prevent those types of dependencies from coming back.

BUG=359249
Committed: https://crrev.com/09981a9eb8bff00e2644f3174e5651b5734eed9a
Cr-Commit-Position: refs/heads/master@{#301386}

[cjhopman, 2014-10-20 22:11:13]

Patchset #1 (id:1) has been deleted

[cjhopman, 2014-10-20 22:11:19]

Patchset #1 (id:20001) has been deleted

[cjhopman, 2014-10-20 22:11:27]

Patchset #1 (id:40001) has been deleted

[cjhopman, 2014-10-20 23:04:01]

cjhopman@chromium.org changed reviewers:
+ jamesr@chromium.org

[cjhopman, 2014-10-20 23:04:03]

jamesr: ptal for high-level + OWNERS of cc/ mojo/

This touches a lot of places so I can split it up a bit, but we can't enable it
on a bot until all of this lands. 

I don't know that it's any easier to review split up into one change/reviewer or
not, anyway, but I'm not particularly opposed to that.

[jamesr, 2014-10-20 23:11:24]

I don't think making component default is a good idea.

https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG...
build/config/BUILDCONFIG.gn:29: is_component_build = os == "android"
huh? why would this be the default? the production builds on android are not
component, nor is gyp

https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn#newcode570
chrome/BUILD.gn:570: "//content"
could you explain this? things in //chrome/ should only depend on the public
parts of //content, not the internals

[cjhopman, 2014-10-21 00:19:06]

https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG.gn
File build/config/BUILDCONFIG.gn (right):

https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG...
build/config/BUILDCONFIG.gn:29: is_component_build = os == "android"
On 2014/10/20 23:11:24, jamesr wrote:
> huh? why would this be the default? the production builds on android are not
> component, nor is gyp
Yeah, the production builds also have is_debug = false, and on linux (I believe)
have is_clang = false.

I think the defaults should be set up for the best developer experience, rather
than be set up to match production builds. We have long wanted to change the
default on Android to be component builds (and is_clang=true by default, too).

That being said, this isn't an important part of this change. I'll take it out
and try to do it in a different change sometime and then we can focus discussion
about it there. 

I think the bot should use the component build since it's more strict, though,
so we'll just have to update the recipe for that.

https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn
File chrome/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn#newcode570
chrome/BUILD.gn:570: "//content"
On 2014/10/20 23:11:24, jamesr wrote:
> could you explain this? things in //chrome/ should only depend on the public
> parts of //content, not the internals

This matches the gyp build: chrome/chrome_android.gypi:33

I'm no expert here, but it appears to be that, when chrome_multiple_dll is
false, //content/app should depend on //content/gpu. Making that change resolves
all the undefined references (but I don't know content/ well enough to say that
that is correct).

This also only actually affects the static_library build since in the component
build any dependency on the public parts of content becomes a dependency on
//content.

Also, //content is a bundling of all the public targets and //content/gpu, to
me, it doesn't look like an internal target (so maybe we should comment there
or, even better, restrict its visibility).

[jamesr, 2014-10-21 00:22:34]

On 2014/10/21 00:19:06, cjhopman wrote:
>
https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG.gn
> File build/config/BUILDCONFIG.gn (right):
> 
>
https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG...
> build/config/BUILDCONFIG.gn:29: is_component_build = os == "android"
> On 2014/10/20 23:11:24, jamesr wrote:
> > huh? why would this be the default? the production builds on android are not
> > component, nor is gyp
> Yeah, the production builds also have is_debug = false, and on linux (I
believe)
> have is_clang = false.
> 
> I think the defaults should be set up for the best developer experience,
rather
> than be set up to match production builds. We have long wanted to change the
> default on Android to be component builds (and is_clang=true by default, too).
> 
> That being said, this isn't an important part of this change. I'll take it out
> and try to do it in a different change sometime and then we can focus
discussion
> about it there. 

I don't want to do it, and I definitely don't want to do it only for a certain
platform.  That's a recipe for horrible failure.

> 
> I think the bot should use the component build since it's more strict, though,
> so we'll just have to update the recipe for that.

The bots are component in debug by default.

> 
> https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn
> File chrome/BUILD.gn (right):
> 
>
https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn#newcode570
> chrome/BUILD.gn:570: "//content"
> On 2014/10/20 23:11:24, jamesr wrote:
> > could you explain this? things in //chrome/ should only depend on the public
> > parts of //content, not the internals
> 
> This matches the gyp build: chrome/chrome_android.gypi:33
> 
> I'm no expert here, but it appears to be that, when chrome_multiple_dll is
> false, //content/app should depend on //content/gpu. Making that change
resolves
> all the undefined references (but I don't know content/ well enough to say
that
> that is correct).

That seems more correct than having //chrome/ depend on //content(not-public)

> 
> This also only actually affects the static_library build since in the
component
> build any dependency on the public parts of content becomes a dependency on
> //content.
> 
> Also, //content is a bundling of all the public targets and //content/gpu, to
> me, it doesn't look like an internal target (so maybe we should comment there
> or, even better, restrict its visibility).

[cjhopman, 2014-10-21 01:31:42]

On 2014/10/21 00:22:34, jamesr wrote:
> On 2014/10/21 00:19:06, cjhopman wrote:
> >
>
https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG.gn
> > File build/config/BUILDCONFIG.gn (right):
> > 
> >
>
https://codereview.chromium.org/666813002/diff/80001/build/config/BUILDCONFIG...
> > build/config/BUILDCONFIG.gn:29: is_component_build = os == "android"
> > On 2014/10/20 23:11:24, jamesr wrote:
> > > huh? why would this be the default? the production builds on android are
not
> > > component, nor is gyp
> > Yeah, the production builds also have is_debug = false, and on linux (I
> believe)
> > have is_clang = false.
> > 
> > I think the defaults should be set up for the best developer experience,
> rather
> > than be set up to match production builds. We have long wanted to change the
> > default on Android to be component builds (and is_clang=true by default,
too).
> > 
> > That being said, this isn't an important part of this change. I'll take it
out
> > and try to do it in a different change sometime and then we can focus
> discussion
> > about it there. 
> 
> I don't want to do it, and I definitely don't want to do it only for a certain
> platform.  That's a recipe for horrible failure.
> 
> > 
> > I think the bot should use the component build since it's more strict,
though,
> > so we'll just have to update the recipe for that.
> 
> The bots are component in debug by default.

The gn android bot is not component (nor is the linux one). That's the one I
care about updating.

> 
> > 
> > https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn
> > File chrome/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/666813002/diff/80001/chrome/BUILD.gn#newcode570
> > chrome/BUILD.gn:570: "//content"
> > On 2014/10/20 23:11:24, jamesr wrote:
> > > could you explain this? things in //chrome/ should only depend on the
public
> > > parts of //content, not the internals
> > 
> > This matches the gyp build: chrome/chrome_android.gypi:33
> > 
> > I'm no expert here, but it appears to be that, when chrome_multiple_dll is
> > false, //content/app should depend on //content/gpu. Making that change
> resolves
> > all the undefined references (but I don't know content/ well enough to say
> that
> > that is correct).
> 
> That seems more correct than having //chrome/ depend on //content(not-public)

Done. In gyp, too.

> 
> > 
> > This also only actually affects the static_library build since in the
> component
> > build any dependency on the public parts of content becomes a dependency on
> > //content.
> > 
> > Also, //content is a bundling of all the public targets and //content/gpu,
to
> > me, it doesn't look like an internal target (so maybe we should comment
there
> > or, even better, restrict its visibility).

[cjhopman, 2014-10-21 01:48:13]

On 2014/10/21 01:31:42, cjhopman wrote:
> On 2014/10/21 00:22:34, jamesr wrote:
> > That seems more correct than having //chrome/ depend on
//content(not-public)
> 
> Done. In gyp, too.
> 

Undone in gyp. The linux gyp build is really unhappy and spits out a cycle of
some 150+ targets that don't actually seem to depend on each other.

[cjhopman, 2014-10-21 02:55:24]

On 2014/10/21 01:48:13, cjhopman wrote:
> On 2014/10/21 01:31:42, cjhopman wrote:
> > On 2014/10/21 00:22:34, jamesr wrote:
> > > That seems more correct than having //chrome/ depend on
> //content(not-public)
> > 
> > Done. In gyp, too.
> > 
> 
> Undone in gyp. The linux gyp build is really unhappy and spits out a cycle of
> some 150+ targets that don't actually seem to depend on each other.

With this change: https://codereview.chromium.org/664253005/ we can actually get
useful output from gyp. The cycle is content -> content_gpu -> content, which,
is, of course, due to the way that the content component is defined in gyp
(content_gpu depends on content which then includes content_app.gypi directly in
the target) 

In fact, you effectively cannot specify dependencies between parts of content
the way you specify them from content to other places. Those inter-content
dependencies need to be specified on the static_library-specific target
definitions.

[jamesr, 2014-10-21 19:47:50]

Could you update the patch description?  It still says it's making component be
default on android

[cjhopman, 2014-10-21 19:50:38]

On 2014/10/21 19:47:50, jamesr wrote:
> Could you update the patch description?  It still says it's making component
be
> default on android

Done. I had updated the first line of the description but not the title (wish
they would just actually be linked).

[jamesr, 2014-10-21 19:51:24]

Thanks, I'll try to get to this later today.

[jamesr, 2014-10-22 00:28:19]

https://codereview.chromium.org/666813002/diff/180001/base/third_party/nspr/B...
File base/third_party/nspr/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/base/third_party/nspr/B...
base/third_party/nspr/BUILD.gn:6: visibility = [ "//base:base" ]
//base is the same thing as //base:base

https://codereview.chromium.org/666813002/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/666813002/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:364: #if
defined(FULL_SAFE_BROWSING)
i don't really understand this - could you get a chrome/browser/.../ owner to
check this too?

https://codereview.chromium.org/666813002/diff/180001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/666813002/diff/180001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:275: # GN: //chrome/common:common
//chrome/common means the same thing and is shorter

https://codereview.chromium.org/666813002/diff/180001/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/666813002/diff/180001/components/bookmarks.gy...
components/bookmarks.gypi:63: # GN version:
//components/bookmarks/common/android
"GN version" comments are normally on target definitions - what is this
referring to?

https://codereview.chromium.org/666813002/diff/180001/mojo/services/native_vi...
File mojo/services/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/mojo/services/native_vi...
mojo/services/native_viewport/BUILD.gn:66: libs = []
nit: you don't need this, although it'll be nice if more things are added

https://codereview.chromium.org/666813002/diff/180001/third_party/cld/BUILD.gn
File third_party/cld/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/third_party/cld/BUILD.g...
third_party/cld/BUILD.gn:11: set_sources_assignment_filter([])
why do you need this? do we use the windows code on non-windows platforms?

[cjhopman, 2014-10-23 00:15:49]

https://codereview.chromium.org/666813002/diff/180001/base/third_party/nspr/B...
File base/third_party/nspr/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/base/third_party/nspr/B...
base/third_party/nspr/BUILD.gn:6: visibility = [ "//base:base" ]
On 2014/10/22 00:28:18, jamesr wrote:
> //base is the same thing as //base:base

Done.

https://codereview.chromium.org/666813002/diff/180001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/666813002/diff/180001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:364: #if
defined(FULL_SAFE_BROWSING)
On 2014/10/22 00:28:18, jamesr wrote:
> i don't really understand this - could you get a chrome/browser/.../ owner to
> check this too?

Acknowledged.

https://codereview.chromium.org/666813002/diff/180001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/666813002/diff/180001/chrome/chrome_common.gy...
chrome/chrome_common.gypi:275: # GN: //chrome/common:common
On 2014/10/22 00:28:18, jamesr wrote:
> //chrome/common means the same thing and is shorter
For these pointers in GYP to the GN target, I've been avoiding the implicit
target to make it easier for people who are editing gyp and need to update the
gn file but might not know about the implicit target.

Maybe we can assume that people know that //chrome/common:common ==
//chrome/common, though. I don't really feel too strongly about it, anyway, so
let me know if you think we should stick to the shorter one.

https://codereview.chromium.org/666813002/diff/180001/components/bookmarks.gypi
File components/bookmarks.gypi (right):

https://codereview.chromium.org/666813002/diff/180001/components/bookmarks.gy...
components/bookmarks.gypi:63: # GN version:
//components/bookmarks/common/android
On 2014/10/22 00:28:18, jamesr wrote:
> "GN version" comments are normally on target definitions - what is this
> referring to?
Added clarification.

https://codereview.chromium.org/666813002/diff/180001/mojo/services/native_vi...
File mojo/services/native_viewport/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/mojo/services/native_vi...
mojo/services/native_viewport/BUILD.gn:66: libs = []
On 2014/10/22 00:28:19, jamesr wrote:
> nit: you don't need this, although it'll be nice if more things are added

With this change (and previous ones), it's easier (slightly) to make
configuration-specific changes to targets that follow this pattern. I also like
that the stuff in the conditions is consistent in that it is modifying the
existing lists instead of assigning its own.

Also, to support two non-mutually-exclusive conditions that both add to libs
(or, more commonly deps), you need to define the list outside the conditions
(though it will just quietly work if you aren't in the configuration that hits
both conditions).

https://codereview.chromium.org/666813002/diff/180001/third_party/cld/BUILD.gn
File third_party/cld/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/180001/third_party/cld/BUILD.g...
third_party/cld/BUILD.gn:11: set_sources_assignment_filter([])
On 2014/10/22 00:28:19, jamesr wrote:
> why do you need this? do we use the windows code on non-windows platforms?
We do.

In gyp, we actually only apply the filename filters where chromium_code == 1 and
so I've run into this problem of filtering too much in GN several times now.

[cjhopman, 2014-10-24 02:08:38]

cjhopman@chromium.org changed reviewers:
+ andrewhayden@chromium.org, avi@chromium.org, brettw@chromium.org,
ddorwin@chromium.org, dtrainor@chromium.org, jochen@chromium.org,
mattm@chromium.org, raymes@chromium.org, thestig@chromium.org

[cjhopman, 2014-10-24 02:08:39]

Ok, adding in OWNERS for everything.

Here's the breakdown:
jamesr:
  cc/surfaces/BUILD.gn
  mojo/converters/surfaces/BUILD.gn
  mojo/gles2/BUILD.gn
  mojo/services/html_viewer/BUILD.gn
  mojo/services/native_viewport/BUILD.gn
avi:
  content/app/BUILD.gn
  content/common/BUILD.gn
  ui/base/BUILD.gn
  ui/gfx/BUILD.gn
  ui/gfx/ipc/BUILD.gn
  ui/gfx/x/BUILD.gn
jochen:
  chrome/browser/BUILD.gn
  chrome/common/BUILD.gn
  chrome/renderer/BUILD.gn
  components/bookmarks.gypi
  components/bookmarks/common/BUILD.gn
  components/bookmarks/common/android/BUILD.gn
  components/dom_distiller/core/BUILD.gn
  components/keyed_service/content/BUILD.gn
  components/sessions/BUILD.gn
willchan:
  base/BUILD.gn
  base/third_party/nspr/BUILD.gn
andrewhayden:
  third_party/cld/BUILD.gn
dtrainor:
  chrome/android/BUILD.gn
  third_party/android_opengl/etc1/BUILD.gn
  third_party/android_opengl/etc1/etc1.gyp
raymes:
  ppapi/BUILD.gn
brettw:
  build/config/BUILD.gn
ddorwin:
  media/audio/BUILD.gn
  media/base/BUILD.gn
  media/base/android/BUILD.gn
  media/blink/BUILD.gn
  media/media.gyp
mattm:
  chrome/browser/safe_browsing/safe_browsing_service.cc

[ddorwin, 2014-10-24 02:14:18]

media/ rs lgtm
+dalecurtis FYI

[jamesr, 2014-10-24 02:16:23]

mojo/ lgtm

cc/ looks fishy

https://codereview.chromium.org/666813002/diff/190001/cc/surfaces/BUILD.gn
File cc/surfaces/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/190001/cc/surfaces/BUILD.gn#ne...
cc/surfaces/BUILD.gn:35: "//ui/events:events_base",
hmm, cc/surfaces doesn't use anything in events. why do you need this dep?

[cjhopman, 2014-10-24 02:26:33]

https://codereview.chromium.org/666813002/diff/190001/cc/surfaces/BUILD.gn
File cc/surfaces/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/190001/cc/surfaces/BUILD.gn#ne...
cc/surfaces/BUILD.gn:35: "//ui/events:events_base",
On 2014/10/24 02:16:23, jamesr wrote:
> hmm, cc/surfaces doesn't use anything in events. why do you need this dep?

I can check for sure if you want, but I'm pretty sure this is from cc's use of
ui::LatencyInfo (almost every case where a dep on events_base was for
LatencyInfo, and cc/ definitely uses it).

[Lei Zhang, 2014-10-24 02:27:44]

https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn
File build/config/BUILD.gn (left):

https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn#o...
build/config/BUILD.gn:35: "ENABLE_BACKGROUND=1",
This is out of date. I just moved this into chrome/

https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:93: "//extensions/common",
I just removed all extensions code from Android today. Why are you adding it
back in?

[cjhopman, 2014-10-24 02:31:11]

On 2014/10/24 02:27:44, Lei Zhang wrote:
> https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn
> File build/config/BUILD.gn (left):
> 
>
https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn#o...
> build/config/BUILD.gn:35: "ENABLE_BACKGROUND=1",
> This is out of date. I just moved this into chrome/
> 
> https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn
> File chrome/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn#...
> chrome/common/BUILD.gn:93: "//extensions/common",
> I just removed all extensions code from Android today. Why are you adding it
> back in?

Ah, right, both those changes just landed earlier today. I'll sync past that and
update this.

[mattm, 2014-10-24 02:34:56]

safe_browsing_service.cc lgtm

[cjhopman, 2014-10-24 02:48:02]

I've confirmed that the extensions-y things we pull in are extension_constants
and component_extensions_resources (in gn and gyp the resources are picked up
through chrome extra_resources).

https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn
File build/config/BUILD.gn (left):

https://codereview.chromium.org/666813002/diff/190001/build/config/BUILD.gn#o...
build/config/BUILD.gn:35: "ENABLE_BACKGROUND=1",
On 2014/10/24 02:27:44, Lei Zhang wrote:
> This is out of date. I just moved this into chrome/

Done.

https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/190001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:93: "//extensions/common",
On 2014/10/24 02:27:44, Lei Zhang wrote:
> I just removed all extensions code from Android today. Why are you adding it
> back in?

Done.

[David Trainor- moved to gerrit, 2014-10-24 02:49:04]

chrome/android lgtm
third_party/android_opengl/etc1/ lgtm

[Lei Zhang, 2014-10-24 02:55:51]

https://codereview.chromium.org/666813002/diff/210001/base/third_party/nspr/B...
File base/third_party/nspr/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/base/third_party/nspr/B...
base/third_party/nspr/BUILD.gn:6: visibility = [ "//base" ]
willchan is on leave, and I don't understand visibility enough to say whether
this is correct or not. Maybe brettw can chime in?

https://codereview.chromium.org/666813002/diff/210001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:498: "//components/web_contents_delegate_android",
nit: alphabetical order please.

https://codereview.chromium.org/666813002/diff/210001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:109: "//extensions/common",
Revert this change, it was fine as is.

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:43: "//components/plugins/renderer",
Can you explain why this is being added to the build?

[jamesr, 2014-10-24 03:08:24]

Ah yes, that's right about latency info.

[Andrew Hayden (chromium.org), 2014-10-24 07:35:15]

third_party/cld LGTM, but hopefully we'll be off of it in the very near future
when the dynamic CLD stuff lands and we can upgrade to CLD2 :)

[Avi (use Gerrit), 2014-10-24 15:24:58]

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

https://codereview.chromium.org/666813002/diff/210001/content/app/BUILD.gn#ne...
content/app/BUILD.gn:70: content_app_deps += [ "//content/gpu" ]
I'm not sure how to confirm this. For all the other GN files, I could see the
corresponding item in the gyp, but not this one. Can you give me a hand here?

[cjhopman, 2014-10-24 21:41:12]

https://codereview.chromium.org/666813002/diff/210001/base/third_party/nspr/B...
File base/third_party/nspr/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/base/third_party/nspr/B...
base/third_party/nspr/BUILD.gn:6: visibility = [ "//base" ]
On 2014/10/24 02:55:51, Lei Zhang wrote:
> willchan is on leave, and I don't understand visibility enough to say whether
> this is correct or not. Maybe brettw can chime in?

This restricts visibility so that only "//base:base" can depend on this target.
The issue is that this is meant to be part of that shared library and it uses
things in that shared library that base doesn't export. Then, if some other
target depends directly on this (like base_unittests was) it won't be able to
link because it can't access the non-exported parts of base.

We could loosen this visibility to allow other source_sets/static_libraries that
are bundled into libbase.so to depend on this target, but that's uglier and not
currently necessary.

I'm not particularly attached to this more restricted visibility, it just makes
it harder for people to do the wrong thing.

https://codereview.chromium.org/666813002/diff/210001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:498: "//components/web_contents_delegate_android",
On 2014/10/24 02:55:51, Lei Zhang wrote:
> nit: alphabetical order please.

Done.

https://codereview.chromium.org/666813002/diff/210001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:109: "//extensions/common",
On 2014/10/24 02:55:51, Lei Zhang wrote:
> Revert this change, it was fine as is.

Done.

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:43: "//components/plugins/renderer",
On 2014/10/24 02:55:51, Lei Zhang wrote:
> Can you explain why this is being added to the build?

chrome/chrome_renderer.gypi also has this dependency. It is needed because:

chrome_plugin_placeholder.cc has a bunch of references to
plugins::PluginPlaceholder
chrome_content_renderer_client.cc has several references to MobileYouTubePlugin

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

https://codereview.chromium.org/666813002/diff/210001/content/app/BUILD.gn#ne...
content/app/BUILD.gn:70: content_app_deps += [ "//content/gpu" ]
On 2014/10/24 15:24:58, Avi wrote:
> I'm not sure how to confirm this. For all the other GN files, I could see the
> corresponding item in the gyp, but not this one. Can you give me a hand here?

Yeah, this isn't in gyp. In earlier patch sets I tried to fix this in gyp too,
but the way that content is built is more complicated there. I believe that
dependencies between content targets can only be added to the static_library
specific content target definitions (the shared_library specific ones will get a
dependency cycle if there are intra-content dependencies there). In the earliest
patch sets I did what we do in gyp, but james didn't like it (neither did I). In
gyp, we have higher levels just depend on all of content, instead of just
content_app.

This is needed because content_main_runner.cc has several references to
content_gpu stuff:
CreateInProcessGpuThread and GpuMain

[cjhopman, 2014-10-24 21:44:06]

cjhopman@chromium.org changed reviewers:
+ blundell@chromium.org
- jochen@chromium.org

[cjhopman, 2014-10-24 21:44:07]

blundell for components/ since jochen is OOO

[Avi (use Gerrit), 2014-10-24 21:45:49]

On 2014/10/24 21:44:07, cjhopman wrote:
> blundell for components/ since jochen is OOO

OK then.

LGTM

[Lei Zhang, 2014-10-24 21:57:04]

chrome/ and base/ lgtm

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/210001/chrome/renderer/BUILD.g...
chrome/renderer/BUILD.gn:43: "//components/plugins/renderer",
On 2014/10/24 21:41:12, cjhopman wrote:
> On 2014/10/24 02:55:51, Lei Zhang wrote:
> > Can you explain why this is being added to the build?
> 
> chrome/chrome_renderer.gypi also has this dependency. It is needed because:
> 
> chrome_plugin_placeholder.cc has a bunch of references to
> plugins::PluginPlaceholder
> chrome_content_renderer_client.cc has several references to
MobileYouTubePlugin
> 
> 

Fair enough, I suspect they are both wrong though. :)

https://codereview.chromium.org/666813002/diff/230001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/230001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:173: ldflags = [ "-Wl,--gc-sections" ]
Should this reference http://crbug.com/159847 ?

[cjhopman, 2014-10-24 22:12:10]

cjhopman@chromium.org changed reviewers:
+ sievers@chromium.org

[cjhopman, 2014-10-24 22:12:11]

sievers@ for gpu/BUILD.gn

[no sievers, 2014-10-24 22:25:07]

https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn#newcode126
gpu/BUILD.gn:126: libs += [ "android" ]
Should this move to native_test_native_code?

Don't all native test targets need this?

[cjhopman, 2014-10-24 22:27:58]

https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn
File gpu/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn#newcode126
gpu/BUILD.gn:126: libs += [ "android" ]
On 2014/10/24 22:25:07, sievers wrote:
> Should this move to native_test_native_code?
> 
> Don't all native test targets need this?

No, they don't. base_unittests, for example, doesn't need it.

[no sievers, 2014-10-24 22:34:51]

On 2014/10/24 22:27:58, cjhopman wrote:
> https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn
> File gpu/BUILD.gn (right):
> 
> https://codereview.chromium.org/666813002/diff/230001/gpu/BUILD.gn#newcode126
> gpu/BUILD.gn:126: libs += [ "android" ]
> On 2014/10/24 22:25:07, sievers wrote:
> > Should this move to native_test_native_code?
> > 
> > Don't all native test targets need this?
> 
> No, they don't. base_unittests, for example, doesn't need it.

lgtm

[raymes, 2014-10-26 23:58:09]

rubberstamp lgtm for ppapi/BUILD.gn

[blundell, 2014-10-27 07:10:40]

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
components/dom_distiller/core/BUILD.gn:6: source_set("core") {
why this change?

[jamesr, 2014-10-27 07:14:46]

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
components/dom_distiller/core/BUILD.gn:6: source_set("core") {
On 2014/10/27 07:10:39, blundell wrote:
> why this change?

static_library is nearly always wrong in GN in chromium unless there's a very
specific need for it, which there doesn't appear to be here.

[blundell, 2014-10-27 07:31:47]

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
components/dom_distiller/core/BUILD.gn:6: source_set("core") {
On 2014/10/27 07:14:46, jamesr wrote:
> On 2014/10/27 07:10:39, blundell wrote:
> > why this change?
> 
> static_library is nearly always wrong in GN in chromium unless there's a very
> specific need for it, which there doesn't appear to be here.

IIUC, source_set means "fold the .o's in this target into the library of the
target that depends on this target." Is that correct? I would have expected to
see this replaced by "component". I'm not super-familiar with gn yet though.

[cjhopman, 2014-10-27 07:32:08]

https://codereview.chromium.org/666813002/diff/230001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/230001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:173: ldflags = [ "-Wl,--gc-sections" ]
On 2014/10/24 21:57:04, Lei Zhang wrote:
> Should this reference http://crbug.com/159847 ?
Yes, it should. Done.

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
File components/dom_distiller/core/BUILD.gn (right):

https://codereview.chromium.org/666813002/diff/250001/components/dom_distille...
components/dom_distiller/core/BUILD.gn:6: source_set("core") {
On 2014/10/27 07:14:46, jamesr wrote:
> On 2014/10/27 07:10:39, blundell wrote:
> > why this change?
> 
> static_library is nearly always wrong in GN in chromium unless there's a very
> specific need for it, which there doesn't appear to be here.

Yeah, as james said.

This isn't actually needed to make linking work... I just reflexively fixed it
as I was changing this target.

[blundell, 2014-10-27 07:40:34]

LGTM

As I said, I don't understand the usage of "source_set" instead of "component",
but it appears it's done in lots of BUILD.gn files under //components, so
there's probably just something incorrect in my limited understanding of gn.

[cjhopman, 2014-10-27 15:30:21]

The CQ bit was checked by cjhopman@chromium.org

[commit-bot: I haz the power, 2014-10-27 15:30:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/666813002/250001

[commit-bot: I haz the power, 2014-10-27 17:11:29]

Committed patchset #11 (id:250001)

[commit-bot: I haz the power, 2014-10-27 17:12:09]

Patchset 11 (id:??) landed as
https://crrev.com/09981a9eb8bff00e2644f3174e5651b5734eed9a
Cr-Commit-Position: refs/heads/master@{#301386}

[jamesr, 2014-10-27 17:33:50]

On 2014/10/27 07:40:34, blundell wrote:
> As I said, I don't understand the usage of "source_set" instead of
"component",
> but it appears it's done in lots of BUILD.gn files under //components, so
> there's probably just something incorrect in my limited understanding of gn.

Component vs not doesn't have to do with GN, it has to do with how the code is
linked.  This code is not a component because it does not link as a separate DLL
in the component build.  It has no _export header and no _IMPLEMENTATION macro,
which are used to set up dll exports / symbol visibility for things that do link
as DLLs in the component build.  It's the same as GYP in this aspect.

GYP doesn't have the notion of source_sets, so it uses static_library in some
places but it causes issues with symbols getting dropped when linked into
intermediate targets which requires somewhat nasty workarounds with gypi files. 
See the things that link into content for an example.  GN does have source_sets,
so we use them.