Split cc/base into separate GN source_set and clean deps

Split cc/base into separate GN source_set and clean deps

//cc/base is intended to be a set of low level utilities used by the rest
of cc (and consumers of cc) and as such shouldn't contain dependencies
on other parts of //cc. This splits the //cc/base files into a separate
GN source_set so 'gn check' can examine dependencies and moves a few
types that have dependencies on other types out:

swap_promise is conceptually about swapping frames, so it goes into
cc/output

*swap_promise_monitor is dealing with swaps on trees, so it goes in
cc/trees

R=danakj@chromium.org

Committed: https://crrev.com/f313a215a789688cff2d56cef89c60e2de416220
Cr-Commit-Position: refs/heads/master@{#320796}

[jamesr, 2015-03-12 22:41:23]

https://codereview.chromium.org/1002073002/diff/1/cc/trees/swap_promise_monit...
File cc/trees/swap_promise_monitor.h (right):

https://codereview.chromium.org/1002073002/diff/1/cc/trees/swap_promise_monit...
cc/trees/swap_promise_monitor.h:30: SwapPromiseMonitor(LayerTreeHost*
layer_tree_host,
Dana - you mentioned in IRC that this could instead take pointers to an
interface and be decoupled from LayerTreeHost{,Impl}, which seems true (since
all it does is add/remove monitors) but looking at the users of this class it
seems they all have to know a lot about trees anyway so this seemed like the
right place to put it.  The class level comment talks a lot about the behavior
of particular trees.  Let me know if you'd prefer to have this in cc/output/
with an interface in between it and LayerTreeHost{,Impl} and I'll see if I can
make that work.

[jamesr, 2015-03-12 22:47:37]

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

[jamesr, 2015-03-12 22:47:37]

sievers@ - once Dana's happy with this patch could you approve for content/
owners? It's a bunch of #include updates due to file moves, no (intentional)
behavior changes.

[no sievers, 2015-03-13 00:41:55]

content lgtm

[danakj, 2015-03-13 17:30:50]

I think this makes good sense. LGTM except..

https://codereview.chromium.org/1002073002/diff/20001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1002073002/diff/20001/cc/cc.gyp#newcode67
cc/cc.gyp:67: 'base/completion_event.h',
I think it'll be confusing to split this into a separate target in one build
type but not the other.

[jamesr, 2015-03-13 18:02:21]

It's not possible to split a component into chunks in GYP since GYP doesn't have
the notion of source_set.  cc/base/ contains definitions of many symbols that
are exported from the cc component, but if we linked cc/base into a static
library then when that static library was linked into the cc component object
files not referenced by other parts of cc would be dropped by the linker.  This
means any symbol defined in cc/base in an object file referred to by things
*outside* of //cc would be undefined symbols and a real mystery of a linker
error for some poor sap to try to figure out.

GN has the source_set construct which was explicitly designed to handle this
case of wanting to break up things that link into one logical component.  It
links the component against the individual object files from the source_set, not
an intermediate static lib, so the linker includes all the object files.

The closest thing syntactically to this in GYP would be to have a gypi for the
files in cc/base and smash it into the cc/cc.gyp:cc component target via an
include.  However, the rules for how things combine when doing a gyp include are
pretty darn subtle and places where I've seen this done are just endlessly
confusing to every involved (//webkit used to have targets like this), so I
think it'd be better not to do that.

[danakj, 2015-03-13 20:00:47]

On Fri, Mar 13, 2015 at 11:02 AM, <jamesr@chromium.org> wrote:

> It's not possible to split a component into chunks in GYP since GYP
> doesn't have
> the notion of source_set.  cc/base/ contains definitions of many symbols
> that
> are exported from the cc component, but if we linked cc/base into a static
> library then when that static library was linked into the cc component
> object
> files not referenced by other parts of cc would be dropped by the linker.
> This
> means any symbol defined in cc/base in an object file referred to by things
> *outside* of //cc would be undefined symbols and a real mystery of a linker
> error for some poor sap to try to figure out.
>
> GN has the source_set construct which was explicitly designed to handle
> this
> case of wanting to break up things that link into one logical component.
> It
> links the component against the individual object files from the
> source_set, not
> an intermediate static lib, so the linker includes all the object files.
>
> The closest thing syntactically to this in GYP would be to have a gypi for
> the
> files in cc/base and smash it into the cc/cc.gyp:cc component target via an
> include.  However, the rules for how things combine when doing a gyp
> include are
> pretty darn subtle and places where I've seen this done are just endlessly
> confusing to every involved (//webkit used to have targets like this), so I
> think it'd be better not to do that.
>

Hmm.. can we use DEPS to keep people who aren't using GN having the same
errors as GN? It'd be a lot nicer to get an error at upload time than from
gn trybots.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2015-03-16 18:11:25]

On 2015/03/13 20:00:47, danakj wrote:
> Hmm.. can we use DEPS to keep people who aren't using GN having the same
> errors as GN? It'd be a lot nicer to get an error at upload time than from
> gn trybots.

I can try (DEPS is a bit less precise since it doesn't understand targets).

[jamesr, 2015-03-16 18:26:04]

PTAL

[danakj, 2015-03-16 18:27:27]

LGTM

[jamesr, 2015-03-16 19:30:20]

The CQ bit was checked by jamesr@chromium.org

[jamesr, 2015-03-16 19:30:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1002073002/#ps40001
(title: "DEPS")

[commit-bot: I haz the power, 2015-03-16 19:31:18]

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

[commit-bot: I haz the power, 2015-03-16 19:53:11]

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

[commit-bot: I haz the power, 2015-03-16 19:53:12]

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...)

[jamesr, 2015-03-16 20:10:38]

The CQ bit was checked by jamesr@chromium.org

[jamesr, 2015-03-16 20:10:39]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1002073002/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2015-03-16 20:11:16]

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

[commit-bot: I haz the power, 2015-03-16 21:28:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-16 21:28:44]

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