Turn //mojo/public/cpp/bindings and //mojo/public/cpp/system into components

Turn //mojo/public/cpp/bindings and //mojo/public/cpp/system into components

//mojo/public/cpp/bindings has global state: putting it in a source set
is trouble in a component build, since it's easy to end up with
multiple copies of globals in different components.

//mojo/public/cpp/system is a dependency of bindings, so change it to
a component as well to avoid multiple definitions in different
components that depend on bindings.

BUG=648397
Committed: https://crrev.com/9c3e65898eceec294d486b1968ec0e2b7ee8f120
Cr-Commit-Position: refs/heads/master@{#420561}

[dcheng, 2016-09-21 23:21:02]

dcheng@chromium.org changed reviewers:
+ yzshen@chromium.org

[dcheng, 2016-09-21 23:21:03]

yzshen@ verified that it fixed another issue he was seeing; I'm going to be
testing on my Blink CL shortly to verify it fixes the problem there as well.

The process for writing this patch was pretty simple: on Windows and Linux, try
to build a component build. When linking fails, add EXPORT macros and repeat.
I'm hoping there's not too many config differences between platforms and the
trybots will be OK with this patch...

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/BUILD.gn:145: defines = [
"MOJO_CPP_BINDINGS_IMPLEMENTATION" ]
Originally, I only componentized bindings, but it depends on
//mojo/public/cpp/system, which is a source_set.

I made system a component as well, but since we already have a
MOJO_SYSTEM_EXPORT macro (for //mojo/public/c/system), I used
MOJO_CPP_SYSTEM_IMPLEMENTATION (and thus MOJO_CPP_BINDINGS_IMPLEMENTATION here
for consistency)

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/interfaces/...
File mojo/public/interfaces/bindings/BUILD.gn (right):

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/interfaces/...
mojo/public/interfaces/bindings/BUILD.gn:14: export_define =
"MOJO_CPP_BINDINGS_IMPLEMENTATION"
I think we really only care about export_define here, but the mojom template
requires us to define all three.

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2358133002/diff/60001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:209: defines += [ invoker.export_define ]
I'm not actually sure this is the best fix. The problem is that
//mojo/public/cpp/bindings depends on
//mojo/public/interfaces/bindings:bindings_cpp_sources. This depends on the
shared sources, which use some code from //mojo/public/cpp/bindings. Since it's
all built into the same component, we need to make sure the EXPORT macros agree
on whether or not this is importing or exporting, otherwise you get a bunch of
linker warnings on Windows. I'm open to other ideas.

[dcheng, 2016-09-21 23:21:09]

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

[commit-bot: I haz the power, 2016-09-21 23:21:38]

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

[commit-bot: I haz the power, 2016-09-21 23:33:44]

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

[commit-bot: I haz the power, 2016-09-21 23:33:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2016-09-21 23:49:36]

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

[commit-bot: I haz the power, 2016-09-21 23:50:03]

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

[commit-bot: I haz the power, 2016-09-22 01:09:09]

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

[commit-bot: I haz the power, 2016-09-22 01:09:10]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-09-22 17:38:45]

I've confirmed that this fixes the test hang in
https://codereview.chromium.org/2345423002/

[yzshen1, 2016-09-22 18:26:27]

Thanks!

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:50: MOJO_CPP_BINDINGS_EXPORT bool
ValidateStructHeaderAndClaimMemory(
I notice that some places put MOJO_CPP_BINDINGS_EXPORT at the beginning of a
function signature; while the others put it between the return type and the
function name (For example, ReportBadMessage in message.h). Please make them
consistent.

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/system/...
File mojo/public/cpp/system/system_export.h (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/system/...
mojo/public/cpp/system/system_export.h:5: #ifndef
MOJO_PUBLIC_CPP_SYSTEM_BINDINGS_EXPORT_H_
This should match the file path

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:209: defines += [ invoker.export_define ]
Why this is needed?

[yzshen1, 2016-09-22 18:34:10]

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:209: defines += [ invoker.export_define ]
On 2016/09/22 18:26:27, yzshen1 (OOO Sep26-Oct30) wrote:
> Why this is needed?

Just saw your comments. Let me think about it some more...

[yzshen1, 2016-09-22 19:39:36]

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:209: defines += [ invoker.export_define ]
On 2016/09/22 18:34:10, yzshen1 (OOO Sep26-Oct30) wrote:
> On 2016/09/22 18:26:27, yzshen1 (OOO Sep26-Oct30) wrote:
> > Why this is needed?
> 
> Just saw your comments. Let me think about it some more...

It seems a little hacky because invoker.export_define is for the "chromium
variant" while this shared_cpp_sources is used by both the "chromium variant"
and "blink variant". 

I think another possible solution is to make mojo/public/cpp/bindings depends on
mojo/public/interfaces/bindings:bindings__generator (which just generates
files), and then explicitly adds those generated files into the sources section
of mojo/public/cpp/bindings.
Because this is a very special case, I think maybe it is fine to be verbose.

WDYT? Thanks!

[yzshen1, 2016-09-22 19:40:02]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org

[yzshen1, 2016-09-22 19:40:02]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-09-22 19:40:03]

lgtm

[commit-bot: I haz the power, 2016-09-22 19:40:21]

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

[yzshen1, 2016-09-22 19:42:30]

(Mistakenly hit the l-g-t-m button.)

[yzshen1, 2016-09-22 19:43:06]

The CQ bit was unchecked by yzshen@chromium.org

[dcheng, 2016-09-22 22:18:22]

PTAL, I hope I listed the sources correctly...

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/validation_util.h (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/validation_util.h:50: MOJO_CPP_BINDINGS_EXPORT bool
ValidateStructHeaderAndClaimMemory(
On 2016/09/22 18:26:27, yzshen1 (OOO Sep26-Oct30) wrote:
> I notice that some places put MOJO_CPP_BINDINGS_EXPORT at the beginning of a
> function signature; while the others put it between the return type and the
> function name (For example, ReportBadMessage in message.h). Please make them
> consistent.

I thought I fixed them all but I guessed I missed a couple. Done... I hope.

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/system/...
File mojo/public/cpp/system/system_export.h (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/cpp/system/...
mojo/public/cpp/system/system_export.h:5: #ifndef
MOJO_PUBLIC_CPP_SYSTEM_BINDINGS_EXPORT_H_
On 2016/09/22 18:26:27, yzshen1 (OOO Sep26-Oct30) wrote:
> This should match the file path

Copy-paste fail >_<

Done

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
File mojo/public/tools/bindings/mojom.gni (right):

https://codereview.chromium.org/2358133002/diff/80001/mojo/public/tools/bindi...
mojo/public/tools/bindings/mojom.gni:209: defines += [ invoker.export_define ]
On 2016/09/22 19:39:36, yzshen1 (OOO Sep26-Oct30) wrote:
> On 2016/09/22 18:34:10, yzshen1 (OOO Sep26-Oct30) wrote:
> > On 2016/09/22 18:26:27, yzshen1 (OOO Sep26-Oct30) wrote:
> > > Why this is needed?
> > 
> > Just saw your comments. Let me think about it some more...
> 
> It seems a little hacky because invoker.export_define is for the "chromium
> variant" while this shared_cpp_sources is used by both the "chromium variant"
> and "blink variant". 
> 
> I think another possible solution is to make mojo/public/cpp/bindings depends
on
> mojo/public/interfaces/bindings:bindings__generator (which just generates
> files), and then explicitly adds those generated files into the sources
section
> of mojo/public/cpp/bindings.
> Because this is a very special case, I think maybe it is fine to be verbose.
> 
> WDYT? Thanks!
> 
> 

It's very hacky. I like your solution better. =)

[yzshen1, 2016-09-22 22:31:15]

LGTM

Thanks!

https://codereview.chromium.org/2358133002/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/2358133002/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/BUILD.gn:12: # in Windows components build, this target
depends on the generated .cc
nit: please consider changing ".cc" to "C++", because we have both .cc and .h
files here.

[dcheng, 2016-09-23 01:43:44]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-09-23 01:43:45]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2358133002/#ps140001
(title: "Fix wording")

[commit-bot: I haz the power, 2016-09-23 01:44:05]

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

[commit-bot: I haz the power, 2016-09-23 03:11:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-23 03:13:34]

Description was changed from

==========
Turn //mojo/public/cpp/bindings and //mojo/public/cpp/system into components

//mojo/public/cpp/bindings has global state: putting it in a source set
is trouble in a component build, since it's easy to end up with
multiple copies of globals in different components.

//mojo/public/cpp/system is a dependency of bindings, so change it to
a component as well to avoid multiple definitions in different
components that depend on bindings.

BUG=648397
==========

to

==========
Turn //mojo/public/cpp/bindings and //mojo/public/cpp/system into components

//mojo/public/cpp/bindings has global state: putting it in a source set
is trouble in a component build, since it's easy to end up with
multiple copies of globals in different components.

//mojo/public/cpp/system is a dependency of bindings, so change it to
a component as well to avoid multiple definitions in different
components that depend on bindings.

BUG=648397

Committed: https://crrev.com/9c3e65898eceec294d486b1968ec0e2b7ee8f120
Cr-Commit-Position: refs/heads/master@{#420561}
==========

[commit-bot: I haz the power, 2016-09-23 03:13:35]

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

[dcheng, 2016-09-24 01:06:45]

(Sending out drafts that I forgot to publish)

https://codereview.chromium.org/2358133002/diff/120001/mojo/public/cpp/bindin...
File mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/2358133002/diff/120001/mojo/public/cpp/bindin...
mojo/public/cpp/bindings/BUILD.gn:12: # in Windows components build, this target
depends on the generated .cc
On 2016/09/22 22:31:15, yzshen1 (OOO Sep26-Oct30) wrote:
> nit: please consider changing ".cc" to "C++", because we have both .cc and .h
> files here. 

Done.