Move content::MakeSharedMemorySegmentViaIPC into its own file

Move content::MakeSharedMemorySegmentViaIPC into its own file

The nacl_helper program uses content::MakeSharedMemorySegmentViaIPC but
none of the other proxy functions in the same source file.  Some of those
functions refer to symbols that bring in a great deal of other code, e.g.
most of v8.  We don't want all that code linked into nacl_helper--it bloats
the binary by a factor of ten or so.

Moving the function out to its own file prevents the other dependencies
coming in unnecessarily.

BUG= http://code.google.com/p/chromium/issues/detail?id=106986
TEST= still builds and nacl_helper binary is much smaller

R=jam@chromium.org,bradchen@google.com

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114539

[jam, 2011-12-10 00:29:47]

This sort of tricks are not maintainable. We used to have tons of ifdefs for the
windows binary, and they kept growing over time, hence why we built a minimal
target that doesn't depend on common. Randomly someone will make an existing
file that you need depend on another file, and it'll keep happening.

See
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/chrome_exe.gyp...,
which is roughly a windows equivalent for nacl_helper. Note that it doesn't
depend on the common target, but instead on self-contained cc files (they could
be moved to another target to be cleaner, but we haven't had the urgency to do
this).

[Brad Chen (chromium), 2011-12-10 14:12:47]

John, I don't quite understand your objection here. There aren't any ifdefs
involved here, only moving one function into a new file, right? We already have
a lot of source files so it's not obvious to me why this will negatively impact
maintainability.

On 2011/12/10 00:29:47, John Abd-El-Malek wrote:
> This sort of tricks are not maintainable. We used to have tons of ifdefs for
the
> windows binary, and they kept growing over time, hence why we built a minimal
> target that doesn't depend on common. Randomly someone will make an existing
> file that you need depend on another file, and it'll keep happening.
> 
> See
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/chrome_exe.gyp...,
> which is roughly a windows equivalent for nacl_helper. Note that it doesn't
> depend on the common target, but instead on self-contained cc files (they
could
> be moved to another target to be cleaner, but we haven't had the urgency to do
> this).

[jam, 2011-12-10 17:00:38]

My issue is with wanting to build a small target (nacl_helper), but having
it depend on a target (common) that could have a lot of dependencies.
The maintenance overhead is having to keep moving code around in order to
make sure that this target doesn't bring in more stuff that it needs.

This is exactly the situation that we used to have with the old nacl win64
target. It wasn't great because it placed a burden on all developers.
Seemingly innocent changes would suddenly cause that binary to balloon in
size.

I understand that this change isn't introducing that dependency, but it's a
bandaid for the existing problem. I would prefer that we fix the problem
that is exposed now rather than leave things fragile.

On Sat, Dec 10, 2011 at 4:12 PM, <bradchen@chromium.org> wrote:

> John, I don't quite understand your objection here. There aren't any ifdefs
> involved here, only moving one function into a new file, right? We already
> have
> a lot of source files so it's not obvious to me why this will negatively
> impact
> maintainability.
>
>
> On 2011/12/10 00:29:47, John Abd-El-Malek wrote:
>
>> This sort of tricks are not maintainable. We used to have tons of ifdefs
>> for
>>
> the
>
>> windows binary, and they kept growing over time, hence why we built a
>> minimal
>> target that doesn't depend on common. Randomly someone will make an
>> existing
>> file that you need depend on another file, and it'll keep happening.
>>
>
>  See
>>
>
> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
> chrome/chrome_exe.gypi&exact_**package=chromium&q=chrome_**
>
nacl_win64&type=cs&l=501<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/chrome_exe.gypi&exact_package=chromium&q=chrome_nacl_win64&type=cs&l=501>
> ,
>
>> which is roughly a windows equivalent for nacl_helper. Note that it
>> doesn't
>> depend on the common target, but instead on self-contained cc files (they
>>
> could
>
>> be moved to another target to be cleaner, but we haven't had the urgency
>> to do
>> this).
>>
>
>
>
>
http://codereview.chromium.**org/8893009/<http://codereview.chromium.org/8893...
>

[Brad Chen, 2011-12-10 17:29:02]

Okay; so we could make nacl_helper depend on a bunch of cc files instead of
depending on the library. Better might be to factor common into a set of
smaller libraries, so we could only depend on what we actually needed. Even
so, we would still need to isolate the function needed by NaCl in its own
source file to avoid the bloat that is the objective of this CL. Understood?

The factoring of common into smaller libraries might be better done by
somebody with significant history working in that code, would you agree?
Given the historically static nature of Chrome, I fear it will not be
straightforward to disentangle the pieces into clean fragments. Sure, it
could be a great learning exercise for somebody, but how long do we want to
leave Chrome on Linux shipping with 6MB of extra bloat? Disclaimer; I
haven't tried to deconstruct things myself yet.

Brad

On Sat, Dec 10, 2011 at 9:00 AM, John Abd-El-Malek <jam@chromium.org> wrote:

> My issue is with wanting to build a small target (nacl_helper), but having
> it depend on a target (common) that could have a lot of dependencies.
> The maintenance overhead is having to keep moving code around in order to
> make sure that this target doesn't bring in more stuff that it needs.
>
> This is exactly the situation that we used to have with the old nacl win64
> target. It wasn't great because it placed a burden on all developers.
> Seemingly innocent changes would suddenly cause that binary to balloon in
> size.
>
> I understand that this change isn't introducing that dependency, but it's
> a bandaid for the existing problem. I would prefer that we fix the problem
> that is exposed now rather than leave things fragile.
>
>
> On Sat, Dec 10, 2011 at 4:12 PM, <bradchen@chromium.org> wrote:
>
>> John, I don't quite understand your objection here. There aren't any
>> ifdefs
>> involved here, only moving one function into a new file, right? We
>> already have
>> a lot of source files so it's not obvious to me why this will negatively
>> impact
>> maintainability.
>>
>>
>> On 2011/12/10 00:29:47, John Abd-El-Malek wrote:
>>
>>> This sort of tricks are not maintainable. We used to have tons of ifdefs
>>> for
>>>
>> the
>>
>>> windows binary, and they kept growing over time, hence why we built a
>>> minimal
>>> target that doesn't depend on common. Randomly someone will make an
>>> existing
>>> file that you need depend on another file, and it'll keep happening.
>>>
>>
>>  See
>>>
>>
>> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
>> chrome/chrome_exe.gypi&exact_**package=chromium&q=chrome_**
>>
nacl_win64&type=cs&l=501<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/chrome_exe.gypi&exact_package=chromium&q=chrome_nacl_win64&type=cs&l=501>
>> ,
>>
>>> which is roughly a windows equivalent for nacl_helper. Note that it
>>> doesn't
>>> depend on the common target, but instead on self-contained cc files (they
>>>
>> could
>>
>>> be moved to another target to be cleaner, but we haven't had the urgency
>>> to do
>>> this).
>>>
>>
>>
>>
>>
http://codereview.chromium.**org/8893009/<http://codereview.chromium.org/8893...
>>
>
>

[jam, 2011-12-11 00:17:07]

On Sat, Dec 10, 2011 at 7:28 PM, Brad Chen <bradchen@google.com> wrote:

> Okay; so we could make nacl_helper depend on a bunch of cc files instead
> of depending on the library. Better might be to factor common into a set of
> smaller libraries, so we could only depend on what we actually needed. Even
> so, we would still need to isolate the function needed by NaCl in its own
> source file to avoid the bloat that is the objective of this CL. Understood?


Yes. Making another library is the best. But for now, it seems like a small
set of self-contained cc files, instead of all common, is a step forward.


>
> The factoring of common into smaller libraries might be better done by
> somebody with significant history working in that code, would you agree?
> Given the historically static nature of Chrome, I fear it will not be
> straightforward to disentangle the pieces into clean fragments. Sure, it
> could be a great learning exercise for somebody, but how long do we want to
> leave Chrome on Linux shipping with 6MB of extra bloat? Disclaimer; I
> haven't tried to deconstruct things myself yet.
>

Looking briefly at this, I don't think this is too complicated. I think it
should be done by someone working on NaCl, because as in the previous case
of the win64 nacl binary, something was hacked up which worked but it added
a long-term maintenance cost for all chrome developers, which we should try
to avoid here (when I refactored the windows one I think there around 30
ifdefs/gyp changes at that point to support it, mostly found by developers
when they broke the build). Looking at content/common includes:
nacl_helper_linux.cc needs content/common/unix_domain_socket_posix.h
and chrome/nacl/nacl_listener.h
unix_domain_socket_posix.cc has no dependencies
nacl_listener.cc needs common/nacl_messages.cc and
content/public/common/child_process_sandbox_support_linux.h (only for
MakeSharedMemorySegmentViaIPC, which should be split up like this) change.


>
> Brad
>
>
> On Sat, Dec 10, 2011 at 9:00 AM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> My issue is with wanting to build a small target (nacl_helper), but
>> having it depend on a target (common) that could have a lot of
>> dependencies. The maintenance overhead is having to keep moving code around
>> in order to make sure that this target doesn't bring in more stuff that it
>> needs.
>>
>> This is exactly the situation that we used to have with the old nacl
>> win64 target. It wasn't great because it placed a burden on all developers.
>> Seemingly innocent changes would suddenly cause that binary to balloon in
>> size.
>>
>> I understand that this change isn't introducing that dependency, but it's
>> a bandaid for the existing problem. I would prefer that we fix the problem
>> that is exposed now rather than leave things fragile.
>>
>>
>> On Sat, Dec 10, 2011 at 4:12 PM, <bradchen@chromium.org> wrote:
>>
>>> John, I don't quite understand your objection here. There aren't any
>>> ifdefs
>>> involved here, only moving one function into a new file, right? We
>>> already have
>>> a lot of source files so it's not obvious to me why this will negatively
>>> impact
>>> maintainability.
>>>
>>>
>>> On 2011/12/10 00:29:47, John Abd-El-Malek wrote:
>>>
>>>> This sort of tricks are not maintainable. We used to have tons of
>>>> ifdefs for
>>>>
>>> the
>>>
>>>> windows binary, and they kept growing over time, hence why we built a
>>>> minimal
>>>> target that doesn't depend on common. Randomly someone will make an
>>>> existing
>>>> file that you need depend on another file, and it'll keep happening.
>>>>
>>>
>>>  See
>>>>
>>>
>>> http://codesearch.google.com/**codesearch#OAMlx_jo-ck/src/**
>>> chrome/chrome_exe.gypi&exact_**package=chromium&q=chrome_**
>>>
nacl_win64&type=cs&l=501<http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/chrome_exe.gypi&exact_package=chromium&q=chrome_nacl_win64&type=cs&l=501>
>>> ,
>>>
>>>> which is roughly a windows equivalent for nacl_helper. Note that it
>>>> doesn't
>>>> depend on the common target, but instead on self-contained cc files
>>>> (they
>>>>
>>> could
>>>
>>>> be moved to another target to be cleaner, but we haven't had the
>>>> urgency to do
>>>> this).
>>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/8893009/<http://codereview.chromium.org/8893...
>>>
>>
>>
>

[Use chromium.org instead, 2011-12-12 17:51:10]

I'm already working on doing John's initial suggestion of listing the
individual source files.
It's not a big deal.  I'll get that working and then leave the
small-libraries cleanup for later.

[Roland McGrath, 2011-12-12 19:06:34]

I've changed the build rules to use minimal dependencies.
PTAL

[jam, 2011-12-14 01:14:28]

http://codereview.chromium.org/8893009/diff/6002/content/common/child_process...
File content/common/child_process_sandbox_support_impl_shm_linux.cc (right):

http://codereview.chromium.org/8893009/diff/6002/content/common/child_process...
content/common/child_process_sandbox_support_impl_shm_linux.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
child_process_sandbox_support_impl_shm_linux.cc is (like the original file) not
very informative (that name means nothing to me). can we pick something more
sane? also, this should have its own header, to make it look like less of a
workaround for a build problem.

actually, looking at this more, there's only one caller of this in the nacl
code. why not just put this file in it directly and avoid this new file
altogether?

http://codereview.chromium.org/8893009/diff/6002/content/common/child_process...
content/common/child_process_sandbox_support_impl_shm_linux.cc:13: return
kSandboxIPCChannel + base::GlobalDescriptors::kBaseDescriptor;
we shouldn't duplicate this, can we share it somehow?

[Roland McGrath, 2011-12-14 18:37:54]

PTAL

On 2011/12/14 01:14:28, John Abd-El-Malek wrote:
> actually, looking at this more, there's only one caller of this in the nacl
> code. why not just put this file in it directly and avoid this new file
> altogether?

Done.  I suspect that the reason it was not done this way originally was
just to keep uniformity among all the Linux sandbox proxy functions.

>
http://codereview.chromium.org/8893009/diff/6002/content/common/child_process...
> content/common/child_process_sandbox_support_impl_shm_linux.cc:13: return
> kSandboxIPCChannel + base::GlobalDescriptors::kBaseDescriptor;
> we shouldn't duplicate this, can we share it somehow?

Done.

[jam, 2011-12-14 23:14:53]

Roland: I apologize, but when I made my earlier comments, I completely forgot
that the code where the function would move to is in chrome. chrome can only
consume content through content/public, and moving this function like I
suggested would necessitate pulling a bunch of other implementation details to
public, which isn't good.

so something like patchset 3, but without duplicating GetSandboxFD() (i.e. how
you added a definition in the header of child_process_sandbox_support_linux.h)
lgtm

[jam, 2011-12-15 01:08:31]

http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi
File chrome/nacl.gypi (right):

http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi#newcode146
chrome/nacl.gypi:146:
'../content/common/child_process_sandbox_support_impl_shm_linux.cc',
nit: ". cc"

http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
File content/common/child_process_sandbox_support_impl_linux.cc (right):

http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
content/common/child_process_sandbox_support_impl_linux.cc:12: #include
"content/common/child_process_sandbox_support_impl_linux.h"
nit: redundant

[Roland McGrath, 2011-12-15 18:06:38]

Note this was already landed and reverted because of some obscure build
issues with nonstandard configurations that I'll be looking into today.  So
I'll be sending a new version separately sometime soon, and you can review
it freshly then.

On 2011/12/15 01:08:31, John Abd-El-Malek wrote:
> http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi
> File chrome/nacl.gypi (right):
> 
> http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi#newcode146
> chrome/nacl.gypi:146:
> '../content/common/child_process_sandbox_support_impl_shm_linux.cc',
> nit: ". cc"

Say what?

>
http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
> File content/common/child_process_sandbox_support_impl_linux.cc (right):
> 
>
http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
> content/common/child_process_sandbox_support_impl_linux.cc:12: #include
> "content/common/child_process_sandbox_support_impl_linux.h"
> nit: redundant

Oops!  I didn't notice the first one because it was out of order.
Is there some style guideline that says the header directly for the
thing being implemented is supposed to be in its own stanza and before
the system (<...>) header #includes?


Thanks,
Roland

[jam, 2011-12-15 23:46:11]

On 2011/12/15 18:06:38, Roland McGrath wrote:
> Note this was already landed and reverted because of some obscure build
> issues with nonstandard configurations that I'll be looking into today.  So
> I'll be sending a new version separately sometime soon, and you can review
> it freshly then.
> 
> On 2011/12/15 01:08:31, John Abd-El-Malek wrote:
> > http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi
> > File chrome/nacl.gypi (right):
> > 
> >
http://codereview.chromium.org/8893009/diff/18001/chrome/nacl.gypi#newcode146
> > chrome/nacl.gypi:146:
> > '../content/common/child_process_sandbox_support_impl_shm_linux.cc',
> > nit: ". cc"
> 
> Say what?

argh, i'm behind a funky mobile wireless proxy that's changing html. ignore this
:)
> 
> >
>
http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
> > File content/common/child_process_sandbox_support_impl_linux.cc (right):
> > 
> >
>
http://codereview.chromium.org/8893009/diff/18001/content/common/child_proces...
> > content/common/child_process_sandbox_support_impl_linux.cc:12: #include
> > "content/common/child_process_sandbox_support_impl_linux.h"
> > nit: redundant
> 
> Oops!  I didn't notice the first one because it was out of order.
> Is there some style guideline that says the header directly for the
> thing being implemented is supposed to be in its own stanza and before
> the system (<...>) header #includes?

yep:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

this helps avoid sudden compile errors when someone else includes a header,
because it doesn't include everything that it needs
> 
> 
> Thanks,
> Roland