Create top-level separate targets for browser and child dlls

Create top-level separate targets for browser and child dlls

The general idea is that there's top level targets chrome and chrome_child,
and corresponding content_app and content_app_child that depend on only
the subtargets that should be included in the appropriate dll.

Pull bluetooth_utils from bluetooth_device into separate common target
as it's referenced from chrome/common/extensions.

Currently (probably) Windows-only and requires setting chrome_multiple_dll=1
for gyp.

Links, but Blink is still included in browser.

Single-process mode is currently disabled when chrome_multiple_dll is set.

Current graph is at: http://commondatastorage.googleapis.com/chromelinkgraph/deps.html
generated by "python tools\win\split_link\graph_dependencies.py deps.html"

Remove the previous hacky-er attempt at this that was named "split dll".

TBR=jam@chromium.org

BUG=237249, 256965
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212230

[scottmg, 2013-06-25 20:53:31]

just fyi if you want try it locally. got kind of ugly, i'm not sure if there's a
cleaner way to cut things up at the top level. i'm sure browser is still linking
blink too.

[scottmg, 2013-07-16 22:34:32]

jam: How do you feel about this tactic for splitting things up? It links now
that the importer code has been wrangled.

[jamesr, 2013-07-16 22:45:43]

nifty! some comments from the peanut gallery

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
content/app/content_main_runner.cc:411: { "",                           
BrowserMain },
shouldn't this be restricted to only appear in CHROME_MULTIPLE_DLL_BROWSER?

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
content/app/content_main_runner.cc:423: { switches::kGpuProcess,         GpuMain
},
aside: why's the GPU process code in both DLLs? are we just not sure which type
this should be?

[scottmg, 2013-07-16 23:08:32]

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
content/app/content_main_runner.cc:411: { "",                           
BrowserMain },
On 2013/07/16 22:45:43, jamesr wrote:
> shouldn't this be restricted to only appear in CHROME_MULTIPLE_DLL_BROWSER?

Good point. Done.

https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
content/app/content_main_runner.cc:423: { switches::kGpuProcess,         GpuMain
},
On 2013/07/16 22:45:43, jamesr wrote:
> aside: why's the GPU process code in both DLLs? are we just not sure which
type
> this should be?

Yeah, I wasn't sure if it shared more with one or the other. I was leaning
towards browser. Any opinion?

[jamesr, 2013-07-16 23:09:41]

On 2013/07/16 23:08:32, scottmg wrote:
>
https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
> content/app/content_main_runner.cc:423: { switches::kGpuProcess,        
GpuMain
> },
> On 2013/07/16 22:45:43, jamesr wrote:
> > aside: why's the GPU process code in both DLLs? are we just not sure which
> type
> > this should be?
> 
> Yeah, I wasn't sure if it shared more with one or the other. I was leaning
> towards browser. Any opinion?

I don't think it'll share all that much either way.  Let's toss it in whichever
DLL is currently smaller?

[scottmg, 2013-07-16 23:22:19]

On 2013/07/16 23:09:41, jamesr wrote:
> On 2013/07/16 23:08:32, scottmg wrote:
> >
>
https://codereview.chromium.org/17619005/diff/21001/content/app/content_main_...
> > content/app/content_main_runner.cc:423: { switches::kGpuProcess,        
> GpuMain
> > },
> > On 2013/07/16 22:45:43, jamesr wrote:
> > > aside: why's the GPU process code in both DLLs? are we just not sure which
> > type
> > > this should be?
> > 
> > Yeah, I wasn't sure if it shared more with one or the other. I was leaning
> > towards browser. Any opinion?
> 
> I don't think it'll share all that much either way.  Let's toss it in
whichever
> DLL is currently smaller?

Not sure which is smaller until blink is out of browser. I put it in child for
now since.. it's not the browser. :)

[scottmg, 2013-07-17 00:33:51]

(see also http://commondatastorage.googleapis.com/chromelinkgraph/deps.html)

[jam, 2013-07-17 16:23:37]

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc
File chrome/app/child_stubs.cc (right):

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc...
chrome/app/child_stubs.cc:5: // TODO(scottmg): http://crbug.com/237249 This is
called from common, but
i have a very strong aversion to stub files. can we fix this first?

https://codereview.chromium.org/17619005/diff/28002/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/17619005/diff/28002/chrome/chrome_dll.gypi#ne...
chrome/chrome_dll.gypi:233: ['chrome_multiple_dll!=1', {
nit: I think this would be readable the other way around, i.e.

['chrome_multiple_dll==1', {
  'defines': [
    'CHROME_MULTIPLE_DLL_BROWSER',
  ],
}, {
  'dependencies': [
    'CHROME_MULTIPLE_DLL_BROWSER',

https://codereview.chromium.org/17619005/diff/28002/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/17619005/diff/28002/content/app/content_main_...
content/app/content_main_runner.cc:421: #if !defined(CHROME_MULTIPLE_DLL) ||
defined(CHROME_MULTIPLE_DLL_CHILD)
nit: perhaps only have this condition once, i.e. in line 414 and put the
enabled_plugins ones inside of it?

https://codereview.chromium.org/17619005/diff/28002/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/17619005/diff/28002/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:507: // Single-process
mode not supported in multiple-dll mode currently.
you probably want the same thing in utility_process_host_impl?

https://codereview.chromium.org/17619005/diff/28002/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/17619005/diff/28002/content/content.gyp#newco...
content/content.gyp:46: 'target_name': 'content_all',
why do we need content_all? it seems redundant compared to just "content"

[scottmg, 2013-07-17 16:49:34]

thanks

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc
File chrome/app/child_stubs.cc (right):

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc...
chrome/app/child_stubs.cc:5: // TODO(scottmg): http://crbug.com/237249 This is
called from common, but
On 2013/07/17 16:23:37, jam wrote:
> i have a very strong aversion to stub files. can we fix this first?

pulled required bluetooth into device_bluetooth_common and removed
child_stubs.cc. it turns out one of the stubs is no longer required, so your
aversion is probably warranted.

https://codereview.chromium.org/17619005/diff/28002/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/17619005/diff/28002/chrome/chrome_dll.gypi#ne...
chrome/chrome_dll.gypi:233: ['chrome_multiple_dll!=1', {
On 2013/07/17 16:23:37, jam wrote:
> nit: I think this would be readable the other way around, i.e.
> 
> ['chrome_multiple_dll==1', {
>   'defines': [
>     'CHROME_MULTIPLE_DLL_BROWSER',
>   ],
> }, {
>   'dependencies': [
>     'CHROME_MULTIPLE_DLL_BROWSER',

Done.

https://codereview.chromium.org/17619005/diff/28002/content/app/content_main_...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/17619005/diff/28002/content/app/content_main_...
content/app/content_main_runner.cc:421: #if !defined(CHROME_MULTIPLE_DLL) ||
defined(CHROME_MULTIPLE_DLL_CHILD)
On 2013/07/17 16:23:37, jam wrote:
> nit: perhaps only have this condition once, i.e. in line 414 and put the
> enabled_plugins ones inside of it?

Done.

https://codereview.chromium.org/17619005/diff/28002/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/17619005/diff/28002/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:507: // Single-process
mode not supported in multiple-dll mode currently.
On 2013/07/17 16:23:37, jam wrote:
> you probably want the same thing in utility_process_host_impl?

i started that in a follow up here https://codereview.chromium.org/19502003/ but
i'll merge if you don't mind this CL getting a bit bigger.

https://codereview.chromium.org/17619005/diff/28002/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/17619005/diff/28002/content/content.gyp#newco...
content/content.gyp:46: 'target_name': 'content_all',
On 2013/07/17 16:23:37, jam wrote:
> why do we need content_all? it seems redundant compared to just "content"

it was mostly so that when i was renaming i got an error when on all references
to 'content' (meaning everything) that ought to be content_browser or
content_child instead. renamed back to 'content'.

[scottmg, 2013-07-17 16:54:15]

On 2013/07/17 16:49:34, scottmg wrote:
>
> it was mostly so that when i was renaming i got an error when on all
references
> to 'content' (meaning everything) that ought to be content_browser or
> content_child instead. renamed back to 'content'.

(well, that sentence made no sense, but... "wanted errors while renaming,
undone")

[jam, 2013-07-17 18:09:58]

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc
File chrome/app/child_stubs.cc (right):

https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc...
chrome/app/child_stubs.cc:5: // TODO(scottmg): http://crbug.com/237249 This is
called from common, but
On 2013/07/17 16:49:35, scottmg wrote:
> On 2013/07/17 16:23:37, jam wrote:
> > i have a very strong aversion to stub files. can we fix this first?
> 
> pulled required bluetooth into device_bluetooth_common and removed
> child_stubs.cc. it turns out one of the stubs is no longer required, so your
> aversion is probably warranted.

device/bluetooth seems like it's code that should only be used by the browser
process, so it seems like it's working around the problem by propagating
multi-process knowledge into low-level components that shouldn't know about it.

looking at it more, BluetoothPermissionData::Check is actually not called by any
code. so perhaps remove that unused function now so that we don't have to create
new targets in src/device/

[scottmg, 2013-07-17 19:06:31]

On 2013/07/17 18:09:58, jam wrote:
> https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc
> File chrome/app/child_stubs.cc (right):
> 
>
https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc...
> chrome/app/child_stubs.cc:5: // TODO(scottmg): http://crbug.com/237249 This is
> called from common, but
> On 2013/07/17 16:49:35, scottmg wrote:
> > On 2013/07/17 16:23:37, jam wrote:
> > > i have a very strong aversion to stub files. can we fix this first?
> > 
> > pulled required bluetooth into device_bluetooth_common and removed
> > child_stubs.cc. it turns out one of the stubs is no longer required, so your
> > aversion is probably warranted.
> 
> device/bluetooth seems like it's code that should only be used by the browser
> process, so it seems like it's working around the problem by propagating
> multi-process knowledge into low-level components that shouldn't know about
it.
> 
> looking at it more, BluetoothPermissionData::Check is actually not called by
any
> code. so perhaps remove that unused function now so that we don't have to
create
> new targets in src/device/

it seems it's called via templated code in
chrome\common\extensions\permissions\set_disjunction_permission.h.

i notice
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_comm...
that common already depends on device_usb, so maybe just depending on
device_bluetooth, but not making a common target is less bad. the linker should
(hopefully) drop most of the unused code anyway i guess.

[scottmg, 2013-07-17 19:46:08]

On 2013/07/17 19:06:31, scottmg wrote:
> On 2013/07/17 18:09:58, jam wrote:
> >
https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc
> > File chrome/app/child_stubs.cc (right):
> > 
> >
>
https://codereview.chromium.org/17619005/diff/28002/chrome/app/child_stubs.cc...
> > chrome/app/child_stubs.cc:5: // TODO(scottmg): http://crbug.com/237249 This
is
> > called from common, but
> > On 2013/07/17 16:49:35, scottmg wrote:
> > > On 2013/07/17 16:23:37, jam wrote:
> > > > i have a very strong aversion to stub files. can we fix this first?
> > > 
> > > pulled required bluetooth into device_bluetooth_common and removed
> > > child_stubs.cc. it turns out one of the stubs is no longer required, so
your
> > > aversion is probably warranted.
> > 
> > device/bluetooth seems like it's code that should only be used by the
browser
> > process, so it seems like it's working around the problem by propagating
> > multi-process knowledge into low-level components that shouldn't know about
> it.
> > 
> > looking at it more, BluetoothPermissionData::Check is actually not called by
> any
> > code. so perhaps remove that unused function now so that we don't have to
> create
> > new targets in src/device/
> 
> it seems it's called via templated code in
> chrome\common\extensions\permissions\set_disjunction_permission.h.
> 
> i notice
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome_comm...
> that common already depends on device_usb, so maybe just depending on
> device_bluetooth, but not making a common target is less bad. the linker
should
> (hopefully) drop most of the unused code anyway i guess.

Did that.

[jam, 2013-07-17 19:58:24]

lgtm

[scottmg, 2013-07-18 03:46:07]

tbr nduca for re-enabling tracing in base/debug/
tbr jamesr for re-enabling in webkit/glue/

[scottmg, 2013-07-18 03:47:45]

Committed patchset #17 manually as r212230 (presubmit successful).

[nduca, 2013-07-18 05:20:59]

thanks scott lgtm

[jamesr, 2013-07-18 05:48:25]

lgtm