Split bundle generation steps so that API registration is generated in browser, not common.

Split bundle generation steps so that API registration is generated in browser, not common.

We now have schema generation, schema bundle generation, and schema bundle registration generation; the last of these is generated in a separate build target and output to browser rather than common.

Fix a couple of dangling gyp issues also.

BUG=405226
TBR=sky@chromium.org

Committed: https://crrev.com/3b70ef14be986a3252428cc6d9553bf2c69deb70
Cr-Commit-Position: refs/heads/master@{#292057}

[Yoyo Zhou, 2014-08-21 00:54:26]

Please take a look at the gyp changes here. The equivalent gn changes are in
progress, but I'm currently blocked on this:

https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/7F-1aCFxKs4

[not at google - send to devlin, 2014-08-21 01:02:52]

json schema compiler changes lgtm

https://chromiumcodereview.appspot.com/489153003/diff/20001/tools/json_schema...
File tools/json_schema_compiler/compiler.py (right):

https://chromiumcodereview.appspot.com/489153003/diff/20001/tools/json_schema...
tools/json_schema_compiler/compiler.py:125: output_dir = os.path.join(destdir,
src_path)
nit: put this in an else condition.

[Yoyo Zhou, 2014-08-21 01:06:14]

https://chromiumcodereview.appspot.com/489153003/diff/20001/tools/json_schema...
File tools/json_schema_compiler/compiler.py (right):

https://chromiumcodereview.appspot.com/489153003/diff/20001/tools/json_schema...
tools/json_schema_compiler/compiler.py:125: output_dir = os.path.join(destdir,
src_path)
On 2014/08/21 01:02:52, kalman wrote:
> nit: put this in an else condition.

Done.

[Yoyo Zhou, 2014-08-21 17:44:48]

Got the GN build working! Scott, ready for your review.

[scottmg, 2014-08-21 18:17:17]

On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> Got the GN build working! Scott, ready for your review.

Thanks. I don't understand the finer points of the extensions parts, but this
fixes the linking problems so LGTM. :)

[scottmg, 2014-08-21 18:27:11]

On 2014/08/21 18:17:17, scottmg wrote:
> On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> > Got the GN build working! Scott, ready for your review.
> 
> Thanks. I don't understand the finer points of the extensions parts, but this
> fixes the linking problems so LGTM. :)

Oops, sorry I was mistaken (too many build settings >_<), let me see if I can
see what's still being linked in.

[scottmg, 2014-08-21 18:32:51]

On 2014/08/21 18:27:11, scottmg wrote:
> On 2014/08/21 18:17:17, scottmg wrote:
> > On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> > > Got the GN build working! Scott, ready for your review.
> > 
> > Thanks. I don't understand the finer points of the extensions parts, but
this
> > fixes the linking problems so LGTM. :)
> 
> Oops, sorry I was mistaken (too many build settings >_<), let me see if I can
> see what's still being linked in.

extensions/common/api/api.gyp includes build/json/schema_bundle_compile.gypi
which outputs generated_api_registration.cc and is process_outputs_as_sources:1,
so that .cc gets built causing, eg.

extensions_api.generated_api_registration.obj : error LNK2019: unresolved
external symbol "public: __thiscall
extensions::UsbFindDevicesFunction::UsbFindDevicesFunction(void)"
(??0UsbFindDevicesFunction@extensions@@QAE@XZ) referenced in function "class
ExtensionFunction * __cdecl NewExtensionFunction<class
extensions::UsbFindDevicesFunction>(void)"
(??$NewExtensionFunction@VUsbFindDevicesFunction@extensions@@@@YAPAVExtensionFunction@@XZ)

[Yoyo Zhou, 2014-08-21 22:01:03]

On 2014/08/21 18:32:51, scottmg wrote:
> On 2014/08/21 18:27:11, scottmg wrote:
> > On 2014/08/21 18:17:17, scottmg wrote:
> > > On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> > > > Got the GN build working! Scott, ready for your review.
> > > 
> > > Thanks. I don't understand the finer points of the extensions parts, but
> this
> > > fixes the linking problems so LGTM. :)
> > 
> > Oops, sorry I was mistaken (too many build settings >_<), let me see if I
can
> > see what's still being linked in.
> 
> extensions/common/api/api.gyp includes build/json/schema_bundle_compile.gypi
> which outputs generated_api_registration.cc and is
process_outputs_as_sources:1,
> so that .cc gets built causing, eg.
> 
> extensions_api.generated_api_registration.obj : error LNK2019: unresolved
> external symbol "public: __thiscall
> extensions::UsbFindDevicesFunction::UsbFindDevicesFunction(void)"
> (??0UsbFindDevicesFunction@extensions@@QAE@XZ) referenced in function "class
> ExtensionFunction * __cdecl NewExtensionFunction<class
> extensions::UsbFindDevicesFunction>(void)"
>
(??$NewExtensionFunction@VUsbFindDevicesFunction@extensions@@@@YAPAVExtensionFunction@@XZ)

Ok, back to the drawing board...

[scottmg, 2014-08-22 18:49:41]

On 2014/08/21 22:01:03, Yoyo Zhou wrote:
> On 2014/08/21 18:32:51, scottmg wrote:
> > On 2014/08/21 18:27:11, scottmg wrote:
> > > On 2014/08/21 18:17:17, scottmg wrote:
> > > > On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> > > > > Got the GN build working! Scott, ready for your review.
> > > > 
> > > > Thanks. I don't understand the finer points of the extensions parts, but
> > this
> > > > fixes the linking problems so LGTM. :)
> > > 
> > > Oops, sorry I was mistaken (too many build settings >_<), let me see if I
> can
> > > see what's still being linked in.
> > 
> > extensions/common/api/api.gyp includes build/json/schema_bundle_compile.gypi
> > which outputs generated_api_registration.cc and is
> process_outputs_as_sources:1,
> > so that .cc gets built causing, eg.
> > 
> > extensions_api.generated_api_registration.obj : error LNK2019: unresolved
> > external symbol "public: __thiscall
> > extensions::UsbFindDevicesFunction::UsbFindDevicesFunction(void)"
> > (??0UsbFindDevicesFunction@extensions@@QAE@XZ) referenced in function "class
> > ExtensionFunction * __cdecl NewExtensionFunction<class
> > extensions::UsbFindDevicesFunction>(void)"
> >
>
(??$NewExtensionFunction@VUsbFindDevicesFunction@extensions@@@@YAPAVExtensionFunction@@XZ)
> 
> Ok, back to the drawing board...

I think it's enough to just split the actions up into two files and only include
the appropriate one? I can try that and see if that makes any sense.

[Yoyo Zhou, 2014-08-22 19:12:21]

On 2014/08/22 18:49:41, scottmg wrote:
> On 2014/08/21 22:01:03, Yoyo Zhou wrote:
> > On 2014/08/21 18:32:51, scottmg wrote:
> > > On 2014/08/21 18:27:11, scottmg wrote:
> > > > On 2014/08/21 18:17:17, scottmg wrote:
> > > > > On 2014/08/21 17:44:48, Yoyo Zhou wrote:
> > > > > > Got the GN build working! Scott, ready for your review.
> > > > > 
> > > > > Thanks. I don't understand the finer points of the extensions parts,
but
> > > this
> > > > > fixes the linking problems so LGTM. :)
> > > > 
> > > > Oops, sorry I was mistaken (too many build settings >_<), let me see if
I
> > can
> > > > see what's still being linked in.
> > > 
> > > extensions/common/api/api.gyp includes
build/json/schema_bundle_compile.gypi
> > > which outputs generated_api_registration.cc and is
> > process_outputs_as_sources:1,
> > > so that .cc gets built causing, eg.
> > > 
> > > extensions_api.generated_api_registration.obj : error LNK2019: unresolved
> > > external symbol "public: __thiscall
> > > extensions::UsbFindDevicesFunction::UsbFindDevicesFunction(void)"
> > > (??0UsbFindDevicesFunction@extensions@@QAE@XZ) referenced in function
"class
> > > ExtensionFunction * __cdecl NewExtensionFunction<class
> > > extensions::UsbFindDevicesFunction>(void)"
> > >
> >
>
(??$NewExtensionFunction@VUsbFindDevicesFunction@extensions@@@@YAPAVExtensionFunction@@XZ)
> > 
> > Ok, back to the drawing board...
> 
> I think it's enough to just split the actions up into two files and only
include
> the appropriate one? I can try that and see if that makes any sense.

Yes, that's what I was hoping to avoid, but I got it working for the gyp
versions without a great deal of duplication, so that's good. I'm working on the
gn now and will have it back for review soon.

[Yoyo Zhou, 2014-08-23 01:15:03]

This patch should work in both gn and gyp. It's similar to the last one but
rather different too. Please take a look.

https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
File chrome/common/extensions/api/BUILD.gn (right):

https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
chrome/common/extensions/api/BUILD.gn:15:
generated_extensions_api("api_registration") {
Logically, this target should live in chrome/browser/extensions/api instead of
here, but the sources defined in schemas.gni are treated as paths relative to
the BUILD.gn (not to the include file), so that doesn't work as
straightforwardly. Hopefully it should be enough that this target is separate
from the api target.

[scottmg, 2014-08-25 17:09:01]

Thanks for doing this, LGTM once all the flavours of build are OK.

I confirmed for real this time that the dependency from renderer->browser is
fixed.

https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
File chrome/common/extensions/api/schemas.gni (right):

https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
chrome/common/extensions/api/schemas.gni:48: # deps += [
"<(DEPTH)/chrome/chrome.gyp:drive_proto" ]  TODO)GYP)
(GYP)

https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
File extensions/common/api/schemas.gypi (right):

https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
extensions/common/api/schemas.gypi:49: 'impl_dir_': 'extensions/browser/api',
I think this or the target it's included into needs an msvs_external_rule:1. I'm
getting this on a full build:

[7163->26/7222 ~33] IDL ..\..\extensions\common\api\app_runtime.idl
FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool midl-wrapper
environment.x86 obj\extensions\browser\api\extensions_api_registration.gen
app_runtime.tlb app_runtime.h app_runtime.dlldata.c app_runtime_i.c
app_runtime_p.c ..\..\extensions\common\api\app_runtime.idl /char signed /env
win32 /Oicf
..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2025 : syntax error :
expecting an interface name or DispatchInterfaceName or CoclassName or
ModuleName or LibraryName or ContractName or a type specification near
"namespace"
..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2026 : cannot recover
from earlier syntax errors; aborting compilation

[scottmg, 2014-08-25 19:47:12]

On 2014/08/25 17:09:01, scottmg wrote:
> Thanks for doing this, LGTM once all the flavours of build are OK.
> 
> I confirmed for real this time that the dependency from renderer->browser is
> fixed.
> 
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
> File chrome/common/extensions/api/schemas.gni (right):
> 
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
> chrome/common/extensions/api/schemas.gni:48: # deps += [
> "<(DEPTH)/chrome/chrome.gyp:drive_proto" ]  TODO)GYP)
> (GYP)
> 
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
> File extensions/common/api/schemas.gypi (right):
> 
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
> extensions/common/api/schemas.gypi:49: 'impl_dir_': 'extensions/browser/api',
> I think this or the target it's included into needs an msvs_external_rule:1.
I'm
> getting this on a full build:
> 
> [7163->26/7222 ~33] IDL ..\..\extensions\common\api\app_runtime.idl
> FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool midl-wrapper
> environment.x86 obj\extensions\browser\api\extensions_api_registration.gen
> app_runtime.tlb app_runtime.h app_runtime.dlldata.c app_runtime_i.c
> app_runtime_p.c ..\..\extensions\common\api\app_runtime.idl /char signed /env
> win32 /Oicf
> ..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2025 : syntax error
:
> expecting an interface name or DispatchInterfaceName or CoclassName or
> ModuleName or LibraryName or ContractName or a type specification near
> "namespace"
> ..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2026 : cannot
recover
> from earlier syntax errors; aborting compilation

Sorry, wrong magic gyp flag, that should be 'explicit_idl_action': 1 inside the
genapi_bundle_registration action to avoid the implicit midl thing.

[Yoyo Zhou, 2014-08-25 21:57:54]

On 2014/08/25 19:47:12, scottmg wrote:
> On 2014/08/25 17:09:01, scottmg wrote:
> > Thanks for doing this, LGTM once all the flavours of build are OK.
> > 
> > I confirmed for real this time that the dependency from renderer->browser is
> > fixed.
> > 
> >
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
> > File chrome/common/extensions/api/schemas.gni (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/chrome/common/ext...
> > chrome/common/extensions/api/schemas.gni:48: # deps += [
> > "<(DEPTH)/chrome/chrome.gyp:drive_proto" ]  TODO)GYP)
> > (GYP)
> > 
> >
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
> > File extensions/common/api/schemas.gypi (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/489153003/diff/80001/extensions/common...
> > extensions/common/api/schemas.gypi:49: 'impl_dir_':
'extensions/browser/api',
> > I think this or the target it's included into needs an msvs_external_rule:1.
> I'm
> > getting this on a full build:
> > 
> > [7163->26/7222 ~33] IDL ..\..\extensions\common\api\app_runtime.idl
> > FAILED: d:\src\depot_tools\python276_bin\python.exe gyp-win-tool
midl-wrapper
> > environment.x86 obj\extensions\browser\api\extensions_api_registration.gen
> > app_runtime.tlb app_runtime.h app_runtime.dlldata.c app_runtime_i.c
> > app_runtime_p.c ..\..\extensions\common\api\app_runtime.idl /char signed
/env
> > win32 /Oicf
> > ..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2025 : syntax
error
> :
> > expecting an interface name or DispatchInterfaceName or CoclassName or
> > ModuleName or LibraryName or ContractName or a type specification near
> > "namespace"
> > ..\..\extensions\common\api\app_runtime.idl(8) : error MIDL2026 : cannot
> recover
> > from earlier syntax errors; aborting compilation
> 
> Sorry, wrong magic gyp flag, that should be 'explicit_idl_action': 1 inside
the
> genapi_bundle_registration action to avoid the implicit midl thing.

I added explicit_idl_action to both of the bundle actions, since neither one
wants to be MIDL-compiled.

[Yoyo Zhou, 2014-08-25 22:45:42]

yoz@chromium.org changed reviewers:
+ sky@chromium.org

[Yoyo Zhou, 2014-08-25 22:45:42]

sky: TBR for the uninteresting gyp/gn changes in chrome/browser,
chrome/browser/devtools, chrome/browser/ui

[Yoyo Zhou, 2014-08-25 22:47:01]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-25 22:48:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/120001

[commit-bot: I haz the power, 2014-08-26 00:11:14]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/46056)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51266)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 00:13:48]

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

[commit-bot: I haz the power, 2014-08-26 00:13:49]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/46099)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yoyo Zhou, 2014-08-26 01:27:20]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:28:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/140001

[commit-bot: I haz the power, 2014-08-26 02:15:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51378)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 02:21:01]

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

[commit-bot: I haz the power, 2014-08-26 02:21:03]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Yoyo Zhou, 2014-08-26 02:44:49]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-26 02:46:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/160001

[commit-bot: I haz the power, 2014-08-26 03:38:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51425)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 03:48:30]

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

[commit-bot: I haz the power, 2014-08-26 03:48:31]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[scottmg, 2014-08-26 16:57:27]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-08-26 16:58:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/160001

[commit-bot: I haz the power, 2014-08-26 17:49:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51768)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 18:01:53]

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

[commit-bot: I haz the power, 2014-08-26 18:01:55]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[scottmg, 2014-08-26 20:04:36]

I'm confused by the compile errors.

Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
GYP_DEFINES=chromium_win_pch=0 component=static_library dcheck_always_on=1
fastbuild=1
ninja -C out\Release chrome_child.dll
builds fine.

As far as I can tell that's what failed here
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
(with the exception of not using goma, but I really don't think that'd be
relevant). Do you see any other difference I missed?

[Yoyo Zhou, 2014-08-26 20:15:37]

On 2014/08/26 20:04:36, scottmg wrote:
> I'm confused by the compile errors.
> 
> Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> GYP_DEFINES=chromium_win_pch=0 component=static_library dcheck_always_on=1
> fastbuild=1
> ninja -C out\Release chrome_child.dll
> builds fine.
> 
> As far as I can tell that's what failed here
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> (with the exception of not using goma, but I really don't think that'd be
> relevant). Do you see any other difference I missed?

Your gyp defines look like they match. The list of targets built is:
'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
'compositor_unittests', 'events_unittests', 'views_unittests', 'wm_unittests'

Do you get any errors when building 'content_app_child'? What about 'chrome'?

[scottmg, 2014-08-26 20:23:11]

On 2014/08/26 20:15:37, Yoyo Zhou wrote:
> On 2014/08/26 20:04:36, scottmg wrote:
> > I'm confused by the compile errors.
> > 
> > Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> > GYP_DEFINES=chromium_win_pch=0 component=static_library dcheck_always_on=1
> > fastbuild=1
> > ninja -C out\Release chrome_child.dll
> > builds fine.
> > 
> > As far as I can tell that's what failed here
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> > (with the exception of not using goma, but I really don't think that'd be
> > relevant). Do you see any other difference I missed?
> 
> Your gyp defines look like they match. The list of targets built is:
> 'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
> 'compositor_unittests', 'events_unittests', 'views_unittests', 'wm_unittests'
> 
> Do you get any errors when building 'content_app_child'? What about 'chrome'?

No such luck, content_app_child and chrome are fine, all in progress, but I
suspect it'll be fine.

[scottmg, 2014-08-26 21:12:15]

I have no idea. I know this is silly, but I'm going to click it again, just in
case.

[scottmg, 2014-08-26 21:12:21]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-08-26 21:13:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/160001

[sky, 2014-08-26 21:23:12]

On 2014/08/26 20:23:11, scottmg wrote:
> On 2014/08/26 20:15:37, Yoyo Zhou wrote:
> > On 2014/08/26 20:04:36, scottmg wrote:
> > > I'm confused by the compile errors.
> > > 
> > > Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> > > GYP_DEFINES=chromium_win_pch=0 component=static_library dcheck_always_on=1
> > > fastbuild=1
> > > ninja -C out\Release chrome_child.dll
> > > builds fine.
> > > 
> > > As far as I can tell that's what failed here
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> > > (with the exception of not using goma, but I really don't think that'd be
> > > relevant). Do you see any other difference I missed?
> > 
> > Your gyp defines look like they match. The list of targets built is:
> > 'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
> > 'compositor_unittests', 'events_unittests', 'views_unittests',
'wm_unittests'
> > 
> > Do you get any errors when building 'content_app_child'? What about
'chrome'?
> 
> No such luck, content_app_child and chrome are fine, all in progress, but I
> suspect it'll be fine.

Seems like content_app has the file content/app/content_main_runner.cc, which
includes content/public/browser/content_browser_client.h, which is on
content_browser, yet content_app doesn't depend upon content_browser.

[Yoyo Zhou, 2014-08-26 22:05:31]

On 2014/08/26 21:23:12, sky wrote:
> On 2014/08/26 20:23:11, scottmg wrote:
> > On 2014/08/26 20:15:37, Yoyo Zhou wrote:
> > > On 2014/08/26 20:04:36, scottmg wrote:
> > > > I'm confused by the compile errors.
> > > > 
> > > > Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> > > > GYP_DEFINES=chromium_win_pch=0 component=static_library
dcheck_always_on=1
> > > > fastbuild=1
> > > > ninja -C out\Release chrome_child.dll
> > > > builds fine.
> > > > 
> > > > As far as I can tell that's what failed here
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> > > > (with the exception of not using goma, but I really don't think that'd
be
> > > > relevant). Do you see any other difference I missed?
> > > 
> > > Your gyp defines look like they match. The list of targets built is:
> > > 'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
> > > 'compositor_unittests', 'events_unittests', 'views_unittests',
> 'wm_unittests'
> > > 
> > > Do you get any errors when building 'content_app_child'? What about
> 'chrome'?
> > 
> > No such luck, content_app_child and chrome are fine, all in progress, but I
> > suspect it'll be fine.
> 
> Seems like content_app has the file content/app/content_main_runner.cc, which
> includes content/public/browser/content_browser_client.h, which is on
> content_browser, yet content_app doesn't depend upon content_browser.

I'm not really sure how content_main_runner is set up (it's possible that
something dubious is going on). But if that was the issue, I expect it would
have failed before my patch.

[commit-bot: I haz the power, 2014-08-26 22:06:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51951)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[scottmg, 2014-08-26 22:08:00]

On 2014/08/26 21:23:12, sky wrote:
> On 2014/08/26 20:23:11, scottmg wrote:
> > On 2014/08/26 20:15:37, Yoyo Zhou wrote:
> > > On 2014/08/26 20:04:36, scottmg wrote:
> > > > I'm confused by the compile errors.
> > > > 
> > > > Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> > > > GYP_DEFINES=chromium_win_pch=0 component=static_library
dcheck_always_on=1
> > > > fastbuild=1
> > > > ninja -C out\Release chrome_child.dll
> > > > builds fine.
> > > > 
> > > > As far as I can tell that's what failed here
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> > > > (with the exception of not using goma, but I really don't think that'd
be
> > > > relevant). Do you see any other difference I missed?
> > > 
> > > Your gyp defines look like they match. The list of targets built is:
> > > 'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
> > > 'compositor_unittests', 'events_unittests', 'views_unittests',
> 'wm_unittests'
> > > 
> > > Do you get any errors when building 'content_app_child'? What about
> 'chrome'?
> > 
> > No such luck, content_app_child and chrome are fine, all in progress, but I
> > suspect it'll be fine.
> 
> Seems like content_app has the file content/app/content_main_runner.cc, which
> includes content/public/browser/content_browser_client.h, which is on
> content_browser, yet content_app doesn't depend upon content_browser.

Oh shoot, you're right. I'm not sure why I can't repro that locally.

I guess it never came up before because something's always pulled in that
dependency accidentally. (?) I'm uncertain because I can't repro, but I think

diff --git a/content/app/content_main_runner.cc
b/content/app/content_main_runner.cc
index 5827ae2..f84644e 100644
--- a/content/app/content_main_runner.cc
+++ b/content/app/content_main_runner.cc
@@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);

 namespace content {

+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
 base::LazyInstance<ContentBrowserClient>
     g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
+#endif
+
 #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
 base::LazyInstance<ContentPluginClient>
     g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;


would probably fix the link error then. (That file is weird, it's built in 2
different ways)

[Yoyo Zhou, 2014-08-26 22:15:07]

On 2014/08/26 22:08:00, scottmg wrote:
> On 2014/08/26 21:23:12, sky wrote:
> > On 2014/08/26 20:23:11, scottmg wrote:
> > > On 2014/08/26 20:15:37, Yoyo Zhou wrote:
> > > > On 2014/08/26 20:04:36, scottmg wrote:
> > > > > I'm confused by the compile errors.
> > > > > 
> > > > > Compiling with at 7a32920fbe0a3 with this patch git cl patch'd in
> > > > > GYP_DEFINES=chromium_win_pch=0 component=static_library
> dcheck_always_on=1
> > > > > fastbuild=1
> > > > > ninja -C out\Release chrome_child.dll
> > > > > builds fine.
> > > > > 
> > > > > As far as I can tell that's what failed here
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...
> > > > > (with the exception of not using goma, but I really don't think that'd
> be
> > > > > relevant). Do you see any other difference I missed?
> > > > 
> > > > Your gyp defines look like they match. The list of targets built is:
> > > > 'all', 'ash_unittests', 'aura_unittests', 'chrome', 'chrome_run',
> > > > 'compositor_unittests', 'events_unittests', 'views_unittests',
> > 'wm_unittests'
> > > > 
> > > > Do you get any errors when building 'content_app_child'? What about
> > 'chrome'?
> > > 
> > > No such luck, content_app_child and chrome are fine, all in progress, but
I
> > > suspect it'll be fine.
> > 
> > Seems like content_app has the file content/app/content_main_runner.cc,
which
> > includes content/public/browser/content_browser_client.h, which is on
> > content_browser, yet content_app doesn't depend upon content_browser.
> 
> Oh shoot, you're right. I'm not sure why I can't repro that locally.
> 
> I guess it never came up before because something's always pulled in that
> dependency accidentally. (?) I'm uncertain because I can't repro, but I think
> 
> diff --git a/content/app/content_main_runner.cc
> b/content/app/content_main_runner.cc
> index 5827ae2..f84644e 100644
> --- a/content/app/content_main_runner.cc
> +++ b/content/app/content_main_runner.cc
> @@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);
> 
>  namespace content {
> 
> +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>  base::LazyInstance<ContentBrowserClient>
>      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
> +#endif
> +
>  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>  base::LazyInstance<ContentPluginClient>
>      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
> 
> 
> would probably fix the link error then. (That file is weird, it's built in 2
> different ways)

Looks promising, given RunNamedProcessTypeMain. I'll give it a try.

[Yoyo Zhou, 2014-08-26 22:15:15]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-26 22:17:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/180001

[scottmg, 2014-08-26 22:33:16]

Sorry, that's not quite enough. Here's another random inline diff to try. :/

diff --git a/content/app/content_main_runner.cc
b/content/app/content_main_runner.cc
index 5827ae2..faf7797e8 100644
--- a/content/app/content_main_runner.cc
+++ b/content/app/content_main_runner.cc
@@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);

 namespace content {

+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
 base::LazyInstance<ContentBrowserClient>
     g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
+#endif
+
 #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
 base::LazyInstance<ContentPluginClient>
     g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
@@ -242,12 +245,14 @@ class ContentClientInitializer {
   static void Set(const std::string& process_type,
                   ContentMainDelegate* delegate) {
     ContentClient* content_client = GetContentClient();
+#if !defined(CHROME_MULTIPLE_DLL_CHILD)
     if (process_type.empty()) {
       if (delegate)
         content_client->browser_ = delegate->CreateContentBrowserClient();
       if (!content_client->browser_)
         content_client->browser_ = &g_empty_content_browser_client.Get();
     }
+#endif

 #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
     if (process_type == switches::kPluginProcess ||
@@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
main_function_params,

 #if !defined(OS_IOS)
 static void RegisterMainThreadFactories() {
-#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
+#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
!defined(CHROME_MULTIPLE_DLL_CHILD)
   UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
       CreateInProcessUtilityThread);
   RenderProcessHostImpl::RegisterRendererMainThreadFactory(

[sky, 2014-08-26 22:55:46]

Shouldn't we just fix the gyp* files and add the dependency if it
needs to be there?

On Tue, Aug 26, 2014 at 3:33 PM,  <scottmg@chromium.org> wrote:
> Sorry, that's not quite enough. Here's another random inline diff to try. :/
>
> diff --git a/content/app/content_main_runner.cc
> b/content/app/content_main_runner.cc
> index 5827ae2..faf7797e8 100644
>
> --- a/content/app/content_main_runner.cc
> +++ b/content/app/content_main_runner.cc
> @@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);
>
>  namespace content {
>
> +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>  base::LazyInstance<ContentBrowserClient>
>      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
> +#endif
> +
>  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>  base::LazyInstance<ContentPluginClient>
>      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
> @@ -242,12 +245,14 @@ class ContentClientInitializer {
>    static void Set(const std::string& process_type,
>                    ContentMainDelegate* delegate) {
>      ContentClient* content_client = GetContentClient();
> +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>      if (process_type.empty()) {
>        if (delegate)
>          content_client->browser_ = delegate->CreateContentBrowserClient();
>        if (!content_client->browser_)
>          content_client->browser_ = &g_empty_content_browser_client.Get();
>      }
> +#endif
>
>  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>      if (process_type == switches::kPluginProcess ||
> @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
> main_function_params,
>
>  #if !defined(OS_IOS)
>  static void RegisterMainThreadFactories() {
> -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
> +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
> !defined(CHROME_MULTIPLE_DLL_CHILD)
>    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
>        CreateInProcessUtilityThread);
>    RenderProcessHostImpl::RegisterRendererMainThreadFactory(
>
>
> https://chromiumcodereview.appspot.com/489153003/

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

[commit-bot: I haz the power, 2014-08-26 23:14:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/51981)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Yoyo Zhou, 2014-08-26 23:15:46]

The CQ bit was unchecked by yoz@chromium.org

[Yoyo Zhou, 2014-08-26 23:17:46]

On 2014/08/26 22:33:16, scottmg wrote:
> Sorry, that's not quite enough. Here's another random inline diff to try. :/
> 
> diff --git a/content/app/content_main_runner.cc
> b/content/app/content_main_runner.cc
> index 5827ae2..faf7797e8 100644
> --- a/content/app/content_main_runner.cc
> +++ b/content/app/content_main_runner.cc
> @@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);
> 
>  namespace content {
> 
> +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>  base::LazyInstance<ContentBrowserClient>
>      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
> +#endif
> +
>  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>  base::LazyInstance<ContentPluginClient>
>      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
> @@ -242,12 +245,14 @@ class ContentClientInitializer {
>    static void Set(const std::string& process_type,
>                    ContentMainDelegate* delegate) {
>      ContentClient* content_client = GetContentClient();
> +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>      if (process_type.empty()) {
>        if (delegate)
>          content_client->browser_ = delegate->CreateContentBrowserClient();
>        if (!content_client->browser_)
>          content_client->browser_ = &g_empty_content_browser_client.Get();
>      }
> +#endif
> 
>  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>      if (process_type == switches::kPluginProcess ||
> @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
> main_function_params,
> 
>  #if !defined(OS_IOS)
>  static void RegisterMainThreadFactories() {
> -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
> +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
> !defined(CHROME_MULTIPLE_DLL_CHILD)
>    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
>        CreateInProcessUtilityThread);
>    RenderProcessHostImpl::RegisterRendererMainThreadFactory(

I'll try this, although I'm not sure the change at line 349 is necessary here.

[Yoyo Zhou, 2014-08-26 23:18:49]

The CQ bit was checked by yoz@chromium.org

[commit-bot: I haz the power, 2014-08-26 23:19:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoz@chromium.org/489153003/200001

[scottmg, 2014-08-26 23:42:53]

On 2014/08/26 22:55:46, sky wrote:
> Shouldn't we just fix the gyp* files and add the dependency if it
> needs to be there?

No, content_app_child (i.e. renderer) shouldn't be linking against any browser
code, that's what this CL is trying to fix.

> 
> On Tue, Aug 26, 2014 at 3:33 PM,  <mailto:scottmg@chromium.org> wrote:
> > Sorry, that's not quite enough. Here's another random inline diff to try. :/
> >
> > diff --git a/content/app/content_main_runner.cc
> > b/content/app/content_main_runner.cc
> > index 5827ae2..faf7797e8 100644
> >
> > --- a/content/app/content_main_runner.cc
> > +++ b/content/app/content_main_runner.cc
> > @@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);
> >
> >  namespace content {
> >
> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
> >  base::LazyInstance<ContentBrowserClient>
> >      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
> > +#endif
> > +
> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
> >  base::LazyInstance<ContentPluginClient>
> >      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
> > @@ -242,12 +245,14 @@ class ContentClientInitializer {
> >    static void Set(const std::string& process_type,
> >                    ContentMainDelegate* delegate) {
> >      ContentClient* content_client = GetContentClient();
> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
> >      if (process_type.empty()) {
> >        if (delegate)
> >          content_client->browser_ = delegate->CreateContentBrowserClient();
> >        if (!content_client->browser_)
> >          content_client->browser_ = &g_empty_content_browser_client.Get();
> >      }
> > +#endif
> >
> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
> >      if (process_type == switches::kPluginProcess ||
> > @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
> > main_function_params,
> >
> >  #if !defined(OS_IOS)
> >  static void RegisterMainThreadFactories() {
> > -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
> > +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
> > !defined(CHROME_MULTIPLE_DLL_CHILD)
> >    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
> >        CreateInProcessUtilityThread);
> >    RenderProcessHostImpl::RegisterRendererMainThreadFactory(
> >
> >
> > https://chromiumcodereview.appspot.com/489153003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-08-26 23:49:57]

Then why do we want ContentBrowserClient used at all in
content/app/content_main_runner.cc?

On Tue, Aug 26, 2014 at 4:42 PM,  <scottmg@chromium.org> wrote:
> On 2014/08/26 22:55:46, sky wrote:
>>
>> Shouldn't we just fix the gyp* files and add the dependency if it
>> needs to be there?
>
>
> No, content_app_child (i.e. renderer) shouldn't be linking against any
> browser
> code, that's what this CL is trying to fix.
>
>
>> On Tue, Aug 26, 2014 at 3:33 PM,  <mailto:scottmg@chromium.org> wrote:
>> > Sorry, that's not quite enough. Here's another random inline diff to
>> > try. :/
>> >
>> > diff --git a/content/app/content_main_runner.cc
>> > b/content/app/content_main_runner.cc
>> > index 5827ae2..faf7797e8 100644
>> >
>> > --- a/content/app/content_main_runner.cc
>> > +++ b/content/app/content_main_runner.cc
>> > @@ -125,8 +125,11 @@ extern int UtilityMain(const MainFunctionParams&);
>> >
>> >  namespace content {
>> >
>> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >  base::LazyInstance<ContentBrowserClient>
>> >      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
>> > +#endif
>> > +
>> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> >  base::LazyInstance<ContentPluginClient>
>> >      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
>> > @@ -242,12 +245,14 @@ class ContentClientInitializer {
>> >    static void Set(const std::string& process_type,
>> >                    ContentMainDelegate* delegate) {
>> >      ContentClient* content_client = GetContentClient();
>> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >      if (process_type.empty()) {
>> >        if (delegate)
>> >          content_client->browser_ =
>> > delegate->CreateContentBrowserClient();
>> >        if (!content_client->browser_)
>> >          content_client->browser_ =
>> > &g_empty_content_browser_client.Get();
>> >      }
>> > +#endif
>> >
>> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> >      if (process_type == switches::kPluginProcess ||
>> > @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
>> > main_function_params,
>> >
>> >  #if !defined(OS_IOS)
>> >  static void RegisterMainThreadFactories() {
>> > -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> > +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
>> > !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
>> >        CreateInProcessUtilityThread);
>> >    RenderProcessHostImpl::RegisterRendererMainThreadFactory(
>> >
>> >
>> > https://chromiumcodereview.appspot.com/489153003/
>
>
>> To unsubscribe from this group and stop receiving emails from it, send an
>
> email
>>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
>
> https://chromiumcodereview.appspot.com/489153003/

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

[scottmg, 2014-08-27 00:10:41]

content_main_runner.cc is in content_app.gypi

content_app.gypi is included into two targets, content_app_browser and
content_app_child. Those are the content entry points in the two DLLs. i.e.
content_main_runner.cc is compiled two different ways, and only sometimes
has a ContentBrowserClient dependency.

It seems like it should be split up into two files, but I think it was
tricky because it's cross platform and we only do browser/child split on
Windows.




On Tue, Aug 26, 2014 at 4:49 PM, Scott Violet <sky@chromium.org> wrote:

> Then why do we want ContentBrowserClient used at all in
> content/app/content_main_runner.cc?
>
> On Tue, Aug 26, 2014 at 4:42 PM,  <scottmg@chromium.org> wrote:
> > On 2014/08/26 22:55:46, sky wrote:
> >>
> >> Shouldn't we just fix the gyp* files and add the dependency if it
> >> needs to be there?
> >
> >
> > No, content_app_child (i.e. renderer) shouldn't be linking against any
> > browser
> > code, that's what this CL is trying to fix.
> >
> >
> >> On Tue, Aug 26, 2014 at 3:33 PM,  <mailto:scottmg@chromium.org> wrote:
> >> > Sorry, that's not quite enough. Here's another random inline diff to
> >> > try. :/
> >> >
> >> > diff --git a/content/app/content_main_runner.cc
> >> > b/content/app/content_main_runner.cc
> >> > index 5827ae2..faf7797e8 100644
> >> >
> >> > --- a/content/app/content_main_runner.cc
> >> > +++ b/content/app/content_main_runner.cc
> >> > @@ -125,8 +125,11 @@ extern int UtilityMain(const
> MainFunctionParams&);
> >> >
> >> >  namespace content {
> >> >
> >> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
> >> >  base::LazyInstance<ContentBrowserClient>
> >> >      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
> >> > +#endif
> >> > +
> >> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
> >> >  base::LazyInstance<ContentPluginClient>
> >> >      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
> >> > @@ -242,12 +245,14 @@ class ContentClientInitializer {
> >> >    static void Set(const std::string& process_type,
> >> >                    ContentMainDelegate* delegate) {
> >> >      ContentClient* content_client = GetContentClient();
> >> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
> >> >      if (process_type.empty()) {
> >> >        if (delegate)
> >> >          content_client->browser_ =
> >> > delegate->CreateContentBrowserClient();
> >> >        if (!content_client->browser_)
> >> >          content_client->browser_ =
> >> > &g_empty_content_browser_client.Get();
> >> >      }
> >> > +#endif
> >> >
> >> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
> >> >      if (process_type == switches::kPluginProcess ||
> >> > @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
> >> > main_function_params,
> >> >
> >> >  #if !defined(OS_IOS)
> >> >  static void RegisterMainThreadFactories() {
> >> > -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
> >> > +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
> >> > !defined(CHROME_MULTIPLE_DLL_CHILD)
> >> >    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
> >> >        CreateInProcessUtilityThread);
> >> >    RenderProcessHostImpl::RegisterRendererMainThreadFactory(
> >> >
> >> >
> >> > https://chromiumcodereview.appspot.com/489153003/
> >
> >
> >> To unsubscribe from this group and stop receiving emails from it, send
> an
> >
> > email
> >>
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> >
> > https://chromiumcodereview.appspot.com/489153003/
>

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

[commit-bot: I haz the power, 2014-08-27 00:13:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-27 00:51:45]

Committed patchset #10 (200001) as 596ff54335b2a1b393af10657bc4945114f3beed

[sky, 2014-08-27 14:34:28]

Tricky. I like breaking into two files to make it clear which parts
are used where.

  -Scott

On Tue, Aug 26, 2014 at 5:10 PM, Scott Graham <scottmg@chromium.org> wrote:
> content_main_runner.cc is in content_app.gypi
>
> content_app.gypi is included into two targets, content_app_browser and
> content_app_child. Those are the content entry points in the two DLLs. i.e.
> content_main_runner.cc is compiled two different ways, and only sometimes
> has a ContentBrowserClient dependency.
>
> It seems like it should be split up into two files, but I think it was
> tricky because it's cross platform and we only do browser/child split on
> Windows.
>
>
>
>
> On Tue, Aug 26, 2014 at 4:49 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> Then why do we want ContentBrowserClient used at all in
>> content/app/content_main_runner.cc?
>>
>> On Tue, Aug 26, 2014 at 4:42 PM,  <scottmg@chromium.org> wrote:
>> > On 2014/08/26 22:55:46, sky wrote:
>> >>
>> >> Shouldn't we just fix the gyp* files and add the dependency if it
>> >> needs to be there?
>> >
>> >
>> > No, content_app_child (i.e. renderer) shouldn't be linking against any
>> > browser
>> > code, that's what this CL is trying to fix.
>> >
>> >
>> >> On Tue, Aug 26, 2014 at 3:33 PM,  <mailto:scottmg@chromium.org> wrote:
>> >> > Sorry, that's not quite enough. Here's another random inline diff to
>> >> > try. :/
>> >> >
>> >> > diff --git a/content/app/content_main_runner.cc
>> >> > b/content/app/content_main_runner.cc
>> >> > index 5827ae2..faf7797e8 100644
>> >> >
>> >> > --- a/content/app/content_main_runner.cc
>> >> > +++ b/content/app/content_main_runner.cc
>> >> > @@ -125,8 +125,11 @@ extern int UtilityMain(const
>> >> > MainFunctionParams&);
>> >> >
>> >> >  namespace content {
>> >> >
>> >> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >> >  base::LazyInstance<ContentBrowserClient>
>> >> >      g_empty_content_browser_client = LAZY_INSTANCE_INITIALIZER;
>> >> > +#endif
>> >> > +
>> >> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> >> >  base::LazyInstance<ContentPluginClient>
>> >> >      g_empty_content_plugin_client = LAZY_INSTANCE_INITIALIZER;
>> >> > @@ -242,12 +245,14 @@ class ContentClientInitializer {
>> >> >    static void Set(const std::string& process_type,
>> >> >                    ContentMainDelegate* delegate) {
>> >> >      ContentClient* content_client = GetContentClient();
>> >> > +#if !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >> >      if (process_type.empty()) {
>> >> >        if (delegate)
>> >> >          content_client->browser_ =
>> >> > delegate->CreateContentBrowserClient();
>> >> >        if (!content_client->browser_)
>> >> >          content_client->browser_ =
>> >> > &g_empty_content_browser_client.Get();
>> >> >      }
>> >> > +#endif
>> >> >
>> >> >  #if !defined(OS_IOS) && !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> >> >      if (process_type == switches::kPluginProcess ||
>> >> > @@ -349,7 +354,7 @@ int RunZygote(const MainFunctionParams&
>> >> > main_function_params,
>> >> >
>> >> >  #if !defined(OS_IOS)
>> >> >  static void RegisterMainThreadFactories() {
>> >> > -#if !defined(CHROME_MULTIPLE_DLL_BROWSER)
>> >> > +#if !defined(CHROME_MULTIPLE_DLL_BROWSER) &&
>> >> > !defined(CHROME_MULTIPLE_DLL_CHILD)
>> >> >    UtilityProcessHostImpl::RegisterUtilityMainThreadFactory(
>> >> >        CreateInProcessUtilityThread);
>> >> >    RenderProcessHostImpl::RegisterRendererMainThreadFactory(
>> >> >
>> >> >
>> >> > https://chromiumcodereview.appspot.com/489153003/
>> >
>> >
>> >> To unsubscribe from this group and stop receiving emails from it, send
>> >> an
>> >
>> > email
>> >>
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >
>> >
>> >
>> >
>> > https://chromiumcodereview.appspot.com/489153003/
>
>

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

[commit-bot: I haz the power, 2014-09-10 02:48:20]

Patchset 10 (id:??) landed as
https://crrev.com/3b70ef14be986a3252428cc6d9553bf2c69deb70
Cr-Commit-Position: refs/heads/master@{#292057}