Expose the sandbox related code through the content API. I did a bit of cleanup while I was doing...

Expose the sandbox related code through the content API. I did a bit of cleanup while I was doing this.

-got rid of SandboxInitWrapper, since I didn't see a need to expose given that we can just expose sandbox::SandboxInterfaceInfo
-got rid of the duplicated code to initialize the broker
-since I made MainFunctionParams only have the sandbox struct on Windows, I also made the mac specific auto release pool behind an ifdef as well. It seemed odd to make something so mac specific compile on all platforms to save some #ifdefs.

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

[Brad Chen (chromium), 2011-10-28 23:39:28]

On 2011/10/28 17:56:49, John Abd-El-Malek wrote:

Sadly, nacl_integration tests are off in the bots right now, so it would be very
helpful if you could test this manually against a nacl page or two before
commit. Or let me know if you want help some.

[jam, 2011-10-29 00:37:45]

On 2011/10/28 23:39:28, Brad Chen (chromium) wrote:
> On 2011/10/28 17:56:49, John Abd-El-Malek wrote:
> 
> Sadly, nacl_integration tests are off in the bots right now, so it would be
very
> helpful if you could test this manually against a nacl page or two before
> commit. Or let me know if you want help some.

yep, I didn't trust myself and verified against a few nacl pages beforehand, and
also that the sandbox is turned on.

[cpu_(ooo_6.6-7.5), 2011-10-29 01:02:35]

http://codereview.chromium.org/8414020/diff/1099/content/ppapi_plugin/ppapi_t...
File content/ppapi_plugin/ppapi_thread.cc (right):

http://codereview.chromium.org/8414020/diff/1099/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:204: if (!content::InitializeSandbox())
I normally use that if() macro, needs to be if () { macro ) or else it can be
broken if the macro expands to more than one line.

[cpu_(ooo_6.6-7.5), 2011-10-29 01:35:39]

first off nice refactoring. In general lgtm, some nits below, second, please
manually try --no-sandbox and verify with process explorer. And finally I am
rubberstamping the mac sandbox changes.

http://codereview.chromium.org/8414020/diff/1099/chrome/test/base/in_process_...
File chrome/test/base/in_process_browser_test.cc (right):

http://codereview.chromium.org/8414020/diff/1099/chrome/test/base/in_process_...
chrome/test/base/in_process_browser_test.cc:286: #if defined(OS_MACOSX)
I think the sprinkling of ifdefs on the test are a bit too much, how about
enhancing BrowserTestBase to do this so the derived classes are not cluttered?

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
File content/renderer/renderer_main_platform_delegate.h (right):

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
content/renderer/renderer_main_platform_delegate.h:15: #include
"base/basictypes.h"
why do you need basictypes here?

http://codereview.chromium.org/8414020/diff/1099/content/test/content_browser...
File content/test/content_browser_test.cc (right):

http://codereview.chromium.org/8414020/diff/1099/content/test/content_browser...
content/test/content_browser_test.cc:68: #endif
see my other comment about moving this.

http://codereview.chromium.org/8414020/diff/1099/sandbox/src/sandbox_types.h
File sandbox/src/sandbox_types.h (left):

http://codereview.chromium.org/8414020/diff/1099/sandbox/src/sandbox_types.h#...
sandbox/src/sandbox_types.h:63: void* legacy;
I already talked to John about this. I agree we need to remove this hack and do
a real fix.

[jam, 2011-10-29 01:49:10]

http://codereview.chromium.org/8414020/diff/1099/chrome/test/base/in_process_...
File chrome/test/base/in_process_browser_test.cc (right):

http://codereview.chromium.org/8414020/diff/1099/chrome/test/base/in_process_...
chrome/test/base/in_process_browser_test.cc:286: #if defined(OS_MACOSX)
On 2011/10/29 01:35:39, cpu wrote:
> I think the sprinkling of ifdefs on the test are a bit too much, how about
> enhancing BrowserTestBase to do this so the derived classes are not cluttered?

I agree this test suffered the most. I wasn't that concerned though because it's
testing code that people don't really look at since it's part of the
infrastructure. Enhancing BrowserTestBase to add platform specific hooks in all
these places, and implementing stub versions for each platform, doesn't seem to
be worth it IMO

http://codereview.chromium.org/8414020/diff/1099/content/ppapi_plugin/ppapi_t...
File content/ppapi_plugin/ppapi_thread.cc (right):

http://codereview.chromium.org/8414020/diff/1099/content/ppapi_plugin/ppapi_t...
content/ppapi_plugin/ppapi_thread.cc:204: if (!content::InitializeSandbox())
On 2011/10/29 01:02:35, cpu wrote:
> I normally use that if() macro, needs to be if () { macro ) or else it can be
> broken if the macro expands to more than one line.

ok. done. i think in this case it's fine though because we have this pattern all
over the code

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
File content/renderer/renderer_main_platform_delegate.h (right):

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
content/renderer/renderer_main_platform_delegate.h:15: #include
"base/basictypes.h"
On 2011/10/29 01:35:39, cpu wrote:
> why do you need basictypes here?

I did at one point, but I had removed that code. done.

[jam, 2011-10-29 02:13:59]

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
File content/renderer/renderer_main_platform_delegate.h (right):

http://codereview.chromium.org/8414020/diff/1099/content/renderer/renderer_ma...
content/renderer/renderer_main_platform_delegate.h:15: #include
"base/basictypes.h"
On 2011/10/29 01:49:10, John Abd-El-Malek wrote:
> On 2011/10/29 01:35:39, cpu wrote:
> > why do you need basictypes here?
> 
> I did at one point, but I had removed that code. done.

ok actually it turns out it's still needed for the DISALLOW_COPY_AND_ASSIGN. now
main_function_params.h doesn't include it anymore through sandbox_init_wrapper.h

[Mark Mentovai, 2012-03-26 16:23:16]

It looks like you masked unwanted “cleanup” in a larger change, and didn’t go
through the proper review channels, like consulting an OWNER in a directory
where there was no “set noparent.”

You’ve made the point in the past that you (and others) would defer to
appropriate platform reviewers as needed. Not only did you fail to do that here,
you didn’t even seek any Mac developer’s input.

http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
File base/mac/scoped_nsautorelease_pool.h (left):

http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
base/mac/scoped_nsautorelease_pool.h:12: #if defined(OS_MACOSX)
Why did you get rid of this?

The intent was for this class to be usable without ugly #if guards on other
platforms.

http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc
File base/message_pump_default.cc (right):

http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc...
base/message_pump_default.cc:24: #if defined(OS_MACOSX)
Like here, for example.

[Mark Mentovai, 2012-03-26 17:06:28]

John Abd-El-Malek wrote:
> (trimming public list, since I don't want the justification here to be taken
> out of context and misapplied in other cases)

Really? Why? I think that this is important. Why do you want to hide
it? If there’s missing context, provide it, but others can learn from
this, too.

> when owners was first added, it was agreed that the owners system we were
> starting with wouldn't work well with large refactorings. in google3, with
> large refactorings like this the developers would be put in the top-level
> owners file until they're done. since we had so many "set noparents" at that
> time, it was agreed with ben/darin that we (content) would --force, while of
> course using our judgment.

And the specific issue I’ve raised is also about seeking the advice
and review of platform experts, whether or not “set noparent” is
present. The issue isn’t really about “noparent” especially now that
it’s going away, it’s about discussing your change with the right
people. base/mac did not have “noparent” at the time of your change
and you did not seek review from any base/mac reviewer. base did and
still does, and since the API you were cleaning up was a base API,
“judgment” would seem to indicate that you ought to talk to someone
competent in that directory. You were not changing base as a result of
an API changing elsewhere in the codebase, you were changing a base
API, and the effects were felt elsewhere in the codebase. --forcing
past a “noparent” for purposes of cleanup was intended to ease the
latter, not a way to evade review by the API owners.

> regarding the scoped_nsautorelease_pool stuff, that is something I did as
> part of the cleanup. From this email it's obvious that you disagree that the
> change makes things better. I had talked to a number of people before doing
> this (inc mac folks) in-person, and they were fine with it. IMO, the way
> that we included a platform specific object in all platforms to reduce
> ifdefs is something that we don't do anywhere else, and i wouldn't want this
> pattern to spread. i.e. no where else does linux/win code include files from
> base/mac, or vice versa for other combinations of platforms.

“Talking to a number of people in-person” is just another way to hide.
We’re a distributed team, and people outside of your own local office
can and do have opinions, and might even be the right people to
discuss your changes with. In-person back-room discussions are
exclusionary and not auditable. The review process that we have exists
for a reason. In fact, it exists for several reasons, and nobody is
free to flout it for matters of convenience or expediency.

Furthermore, I maintain that hiding “cleanup” inside a larger change
is a bad practice, and it’s frowned upon at Google and by this team.

> On Mon, Mar 26, 2012 at 9:23 AM, <mark@chromium.org> wrote:
>>
>> It looks like you masked unwanted “cleanup” in a larger change, and didn’t
>> go
>> through the proper review channels, like consulting an OWNER in a
>> directory
>> where there was no “set noparent.”
>>
>> You’ve made the point in the past that you (and others) would defer to
>> appropriate platform reviewers as needed. Not only did you fail to do that
>> here,
>> you didn’t even seek any Mac developer’s input.
>>
>>
>>
>>
http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
>> File base/mac/scoped_nsautorelease_pool.h (left):
>>
>>
>>
http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
>> base/mac/scoped_nsautorelease_pool.h:12: #if defined(OS_MACOSX)
>> Why did you get rid of this?
>>
>> The intent was for this class to be usable without ugly #if guards on
>> other platforms.
>>
>>
>> http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc
>> File base/message_pump_default.cc (right):
>>
>>
>>
http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc...
>> base/message_pump_default.cc:24: #if defined(OS_MACOSX)
>> Like here, for example.
>>
>> http://codereview.chromium.org/8414020/
>
>

[jam, 2012-03-26 17:34:57]

On Mon, Mar 26, 2012 at 10:06 AM, Mark Mentovai <mark@chromium.org> wrote:

> John Abd-El-Malek wrote:
> > (trimming public list, since I don't want the justification here to be
> taken
> > out of context and misapplied in other cases)
>
> Really? Why? I think that this is important. Why do you want to hide
> it? If there’s missing context, provide it, but others can learn from
> this, too.
>

I think cases like this end up as a judgement call. From my experience with
these kind of threads, trying to come up with one rigid rule for it isn't
possible, and having everyone chip in and +1 with their preference isn't
productive.


> > when owners was first added, it was agreed that the owners system we were
> > starting with wouldn't work well with large refactorings. in google3,
> with
> > large refactorings like this the developers would be put in the top-level
> > owners file until they're done. since we had so many "set noparents" at
> that
> > time, it was agreed with ben/darin that we (content) would --force,
> while of
> > course using our judgment.
>
> And the specific issue I’ve raised is also about seeking the advice
> and review of platform experts, whether or not “set noparent” is
> present. The issue isn’t really about “noparent” especially now that
> it’s going away, it’s about discussing your change with the right
> people. base/mac did not have “noparent” at the time of your change
> and you did not seek review from any base/mac reviewer. base did and
> still does, and since the API you were cleaning up was a base API,
> “judgment” would seem to indicate that you ought to talk to someone
> competent in that directory. You were not changing base as a result of
> an API changing elsewhere in the codebase, you were changing a base
> API, and the effects were felt elsewhere in the codebase. --forcing
> past a “noparent” for purposes of cleanup was intended to ease the
> latter, not a way to evade review by the API owners.
>
> > regarding the scoped_nsautorelease_pool stuff, that is something I did as
> > part of the cleanup. From this email it's obvious that you disagree that
> the
> > change makes things better. I had talked to a number of people before
> doing
> > this (inc mac folks) in-person, and they were fine with it. IMO, the way
> > that we included a platform specific object in all platforms to reduce
> > ifdefs is something that we don't do anywhere else, and i wouldn't want
> this
> > pattern to spread. i.e. no where else does linux/win code include files
> from
> > base/mac, or vice versa for other combinations of platforms.
>
> “Talking to a number of people in-person” is just another way to hide.
> We’re a distributed team, and people outside of your own local office
> can and do have opinions, and might even be the right people to
> discuss your changes with. In-person back-room discussions are
> exclusionary and not auditable. The review process that we have exists
> for a reason. In fact, it exists for several reasons, and nobody is
> free to flout it for matters of convenience or expediency.
>
> Furthermore, I maintain that hiding “cleanup” inside a larger change
> is a bad practice, and it’s frowned upon at Google and by this team.
>

I don't agree that this is hiding a cleanup inside a larger change. (from
the cl desc) I had refactored the startup code so that now we have a struct
that had to have a windows specific struct (behind an ifdef). since the mac
specific member object was moving to that struct as well, i moved it to an
ifdef to match the windows one. Again, to make the usage of the mac class
match the usage of every other platform specific class that is only
included for that platform, in the process I updated the comment in base
and the othe one test/non-test cc files to add #ifdefs as well. As I said
earlier, I had run these by mac people. In retrospect I should have run it
by you since you're the only mac person in base/OWNERS. I guess I didn't
think this was worth a ping specifically to, but in the future I'll make
sure to ping you. As you've seen my other changes where I add you, I
usually add mac owners.


>
> > On Mon, Mar 26, 2012 at 9:23 AM, <mark@chromium.org> wrote:
> >>
> >> It looks like you masked unwanted “cleanup” in a larger change, and
> didn’t
> >> go
> >> through the proper review channels, like consulting an OWNER in a
> >> directory
> >> where there was no “set noparent.”
> >>
> >> You’ve made the point in the past that you (and others) would defer to
> >> appropriate platform reviewers as needed. Not only did you fail to do
> that
> >> here,
> >> you didn’t even seek any Mac developer’s input.
> >>
> >>
> >>
> >>
>
http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
> >> File base/mac/scoped_nsautorelease_pool.h (left):
> >>
> >>
> >>
>
http://codereview.chromium.org/8414020/diff/5115/base/mac/scoped_nsautoreleas...
> >> base/mac/scoped_nsautorelease_pool.h:12: #if defined(OS_MACOSX)
> >> Why did you get rid of this?
> >>
> >> The intent was for this class to be usable without ugly #if guards on
> >> other platforms.
> >>
> >>
> >>
> http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc
> >> File base/message_pump_default.cc (right):
> >>
> >>
> >>
>
http://codereview.chromium.org/8414020/diff/5115/base/message_pump_default.cc...
> >> base/message_pump_default.cc:24: #if defined(OS_MACOSX)
> >> Like here, for example.
> >>
> >> http://codereview.chromium.org/8414020/
> >
> >
>