Load mac sandbox definitions from resources instead of the bundle.

Load mac sandbox definitions from resources instead of the bundle.

Also, move all mac sandbox unittests to content

BUG=90443
TEST=content_unittests

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

[jochen (gone - plz use gerrit), 2011-11-16 22:00:53]

plz review

avi, mac bits
joi, grd bits
john, content/ approval

[Avi (use Gerrit), 2011-11-16 22:02:59]

Looks plausible, but the Mac sandbox master is Jeremy.

[jam, 2011-11-17 01:50:46]

content lgtm

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.m...
content/common/sandbox_mac.mm:33: #include "grit/content_resources.h"
initially when separating content from chrome, we couldn't include files from
grit/ because of circular dependencies with chrome. i guess this now solved
because of your earlier patch to create content_resources?

i'm a little worried that people will start including chrome resources now
though. can we make content/DEPS only allow content_resources.h and
webkit_strings.h?

[jeremy, 2011-11-17 13:34:14]

High level comments:
1)It's really important that we don't lose coverage on the Sandbox tests - could
you double check that they are still running with this CL?
3) The terminology is "Sandbox profile" not "Sandbox definition", could you
change all instances of definition to "profile" in this cl?
4) Looks like this CL changes all references to Sandbox types in the code to
resource IDs. IMHO the notion of process type needs to be decoupled from the
actual Sandbox profile used (these used to be filenames) since things need to be
flexible enough to easily use the same sandbox profile for 2 different process
types. So rather than deleting the current Sandbox enum, augment that with a
resource enum and then have the function that loads the sandbox translate
between the process type and the actual resource to load.

[jeremy, 2011-11-17 13:36:11]

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_init_...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_init_...
content/common/sandbox_init_mac.cc:16: bool InitializeSandbox(int
sandbox_definition_resource_id) {
I would refactor this into 3 functions:
InitializeSandbox(sandbox_type, filepath)
ReadSandboxDefsFromCommandLine(sandbox_type*,fiepath*) <- not happy with this
name, feel free to pick a better one.
InitializeSandboxFromCommandLine() which calls the above 2 functions.

[Jói, 2011-11-17 13:50:21]

Overall looks good, but it sounds like Jeremy should have the last word here.

I'll take another look once you've addressed everybody's comments.

Cheers,
Jói

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.h...
content/common/sandbox_mac.h:66: // |sandbox_definition_resource_id| - type of
Sandbox to use.
type of -> resource ID of definition of
?

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac_u...
File content/common/sandbox_mac_unittest_helper.h (right):

http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac_u...
content/common/sandbox_mac_unittest_helper.h:48: // Runs a test specified by
|test_name| in a sandbox of the type specified by
type specified by -> as defined by the resource with ID

[jochen (gone - plz use gerrit), 2011-11-19 12:24:04]

On 2011/11/17 13:34:14, jeremy wrote:
> High level comments:
> 1)It's really important that we don't lose coverage on the Sandbox tests -
could
> you double check that they are still running with this CL?
> 3) The terminology is "Sandbox profile" not "Sandbox definition", could you
> change all instances of definition to "profile" in this cl?
> 4) Looks like this CL changes all references to Sandbox types in the code to
> resource IDs. IMHO the notion of process type needs to be decoupled from the
> actual Sandbox profile used (these used to be filenames) since things need to
be
> flexible enough to easily use the same sandbox profile for 2 different process
> types. So rather than deleting the current Sandbox enum, augment that with a
> resource enum and then have the function that loads the sandbox translate
> between the process type and the actual resource to load.

This won't work.

The sandbox code lives in content/ so it must not know about all possible
sandbox types, esp. the NACL one.

[jochen (gone - plz use gerrit), 2011-11-19 13:40:13]

On 2011/11/17 01:50:46, John Abd-El-Malek wrote:
> content lgtm
> 
> http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.mm
> File content/common/sandbox_mac.mm (right):
> 
>
http://codereview.chromium.org/8589001/diff/4001/content/common/sandbox_mac.m...
> content/common/sandbox_mac.mm:33: #include "grit/content_resources.h"
> initially when separating content from chrome, we couldn't include files from
> grit/ because of circular dependencies with chrome. i guess this now solved
> because of your earlier patch to create content_resources?

To be able to include a file in grit/ you'll need to depend on the grid target
in gyp that generates the file.

> 
> i'm a little worried that people will start including chrome resources now
> though. can we make content/DEPS only allow content_resources.h and
> webkit_strings.h?

That should be possible

[jam, 2011-11-21 17:11:54]

On 2011/11/19 12:24:04, jochen wrote:
> On 2011/11/17 13:34:14, jeremy wrote:
> > High level comments:
> > 1)It's really important that we don't lose coverage on the Sandbox tests -
> could
> > you double check that they are still running with this CL?
> > 3) The terminology is "Sandbox profile" not "Sandbox definition", could you
> > change all instances of definition to "profile" in this cl?
> > 4) Looks like this CL changes all references to Sandbox types in the code to
> > resource IDs. IMHO the notion of process type needs to be decoupled from the
> > actual Sandbox profile used (these used to be filenames) since things need
to
> be
> > flexible enough to easily use the same sandbox profile for 2 different
process
> > types. So rather than deleting the current Sandbox enum, augment that with a
> > resource enum and then have the function that loads the sandbox translate
> > between the process type and the actual resource to load.
> 
> This won't work.
> 
> The sandbox code lives in content/ so it must not know about all possible
> sandbox types, esp. the NACL one.

I agree it would be nicer if we used an enum instead of resource id.

note that with a lot of enums that used to be formally just in content, we have
split them up into two enums. the enum in chrome starts at the last value from
content. see ViewTypeValues, NotificationType, ResultCode

[jochen (gone - plz use gerrit), 2011-11-22 18:13:16]

Ok, so what about the following structure

in content/public..

RegisterSandboxPolicy(int sandbox_type, int sandbox_policy_resource_id);
InitializeSandbox(int sandbox_type, const FilePath& allowed_dir);

and in content/common

InitializeSandbox() which will detect the sandbox_type and allowed_dir based on
the process type, and invoke InitializeSandbox(.., ...) with the result?

[jochen (gone - plz use gerrit), 2011-11-23 00:53:26]

PTAL

I've tried another approach that maintains the separation between process type
and policy

[jeremy, 2011-11-23 07:02:16]

In general this approach seems OK to me, although we need to figure out a way to
not lose coverage of the nacl loader sandbox type in the unit tests.

http://codereview.chromium.org/8589001/diff/11001/chrome/app/chrome_main_dele...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/8589001/diff/11001/chrome/app/chrome_main_dele...
chrome/app/chrome_main_delegate.cc:285: // Mac needs them to for scrollbar
related images and for the sandbox.
nit: *too
and for the sandbox -> and for sandbox profiles.

http://codereview.chromium.org/8589001/diff/11001/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/8589001/diff/11001/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:38: virtual int
GetSandboxPolicyForSandboxType(int sandbox_type) const OVERRIDE;
Can you add a comment for this function?

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:23: bool InitializeSandbox() {
I think this would be a bit more readable if you split this function further:

void GetSandboxTypeFromCommandLine(int* sandboxtype, FilePath* allowed_dir);

Then InitializeSandbox becomes 3 lines...

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:341: NSString* LoadSandboxTemplate(int
sandbox_type) {
Can you add a CHECK() here or in an appropriate place to make sure we aren't
passing in bogus values for sandbox_type.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:346: for (size_t i = 0;
// Find resource id for sandbox profile to use for the specific sandbox type.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:357: // See if the embedder knows about this
sandbox process type.
*See->Check

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:362: DLOG(FATAL) << "Unknown sandbox type " <<
sandbox_type;
LOG_IF()
errors here should always be fatal, I know you're doing the same as the old
code, but IMHO failing hard is the right approach.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:369: DLOG(FATAL) << "Failed to load the sandbox
profile (resource id "
*DLOG -> LOG

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:471: if (sandbox_type <
content::SANDBOX_AFTER_TYPE_LAST_TYPE &&
CHECK(sandbox_type < content::SANDBOX_AFTER_TYPE_LAST_TYPE);
if(...)

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac_...
File content/common/sandbox_mac_unittest_helper.mm (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac_...
content/common/sandbox_mac_unittest_helper.mm:60: ++i) {
Don't we lose coverage here since we won't be checking the nacl loader sandbox?

http://codereview.chromium.org/8589001/diff/11001/content/content_resources.grd
File content/content_resources.grd (right):

http://codereview.chromium.org/8589001/diff/11001/content/content_resources.g...
content/content_resources.grd:24: common/sandbox_mac_unittest_helper.mm -->
Not clear on what's to be updated in helper.mm? Do you mean
sandbox_process_type_mac.h ?

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
content/public/common/sandbox_init.h:40: //
content::sandbox::SANDBOX_AFTER_TYPE_LAST_TYPE.
We need the unit tests to run on all sandbox types, how is this achieved if
embedders can add their own out-of-band sandbox types?

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:9: namespace content {
Need a header comment explaining what this file does.

[jochen (gone - plz use gerrit), 2011-11-23 10:57:27]

http://codereview.chromium.org/8589001/diff/11001/chrome/app/chrome_main_dele...
File chrome/app/chrome_main_delegate.cc (right):

http://codereview.chromium.org/8589001/diff/11001/chrome/app/chrome_main_dele...
chrome/app/chrome_main_delegate.cc:285: // Mac needs them to for scrollbar
related images and for the sandbox.
On 2011/11/23 07:02:17, jeremy wrote:
> nit: *too
> and for the sandbox -> and for sandbox profiles.

Done.

http://codereview.chromium.org/8589001/diff/11001/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/8589001/diff/11001/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:38: virtual int
GetSandboxPolicyForSandboxType(int sandbox_type) const OVERRIDE;
On 2011/11/23 07:02:17, jeremy wrote:
> Can you add a comment for this function?

There's a comment in the interface definition. We usually don't repeat them in
the implementation.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:23: bool InitializeSandbox() {
On 2011/11/23 07:02:17, jeremy wrote:
> I think this would be a bit more readable if you split this function further:
> 
> void GetSandboxTypeFromCommandLine(int* sandboxtype, FilePath* allowed_dir);
> 
> Then InitializeSandbox becomes 3 lines...

Done.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:341: NSString* LoadSandboxTemplate(int
sandbox_type) {
On 2011/11/23 07:02:17, jeremy wrote:
> Can you add a CHECK() here or in an appropriate place to make sure we aren't
> passing in bogus values for sandbox_type.

Done.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:346: for (size_t i = 0;
On 2011/11/23 07:02:17, jeremy wrote:
> // Find resource id for sandbox profile to use for the specific sandbox type.

Done.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:357: // See if the embedder knows about this
sandbox process type.
On 2011/11/23 07:02:17, jeremy wrote:
> *See->Check

Done.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:362: DLOG(FATAL) << "Unknown sandbox type " <<
sandbox_type;
On 2011/11/23 07:02:17, jeremy wrote:
> LOG_IF()
> errors here should always be fatal, I know you're doing the same as the old
> code, but IMHO failing hard is the right approach.

I turned this into a CHECK

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:369: DLOG(FATAL) << "Failed to load the sandbox
profile (resource id "
On 2011/11/23 07:02:17, jeremy wrote:
> *DLOG -> LOG

Done.

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac....
content/common/sandbox_mac.mm:471: if (sandbox_type <
content::SANDBOX_AFTER_TYPE_LAST_TYPE &&
On 2011/11/23 07:02:17, jeremy wrote:
> CHECK(sandbox_type < content::SANDBOX_AFTER_TYPE_LAST_TYPE);
> if(...)

That's not necessarily true, as the embedder might register arbitrary other
sandbox types which might allow directories

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac_...
File content/common/sandbox_mac_unittest_helper.mm (right):

http://codereview.chromium.org/8589001/diff/11001/content/common/sandbox_mac_...
content/common/sandbox_mac_unittest_helper.mm:60: ++i) {
On 2011/11/23 07:02:17, jeremy wrote:
> Don't we lose coverage here since we won't be checking the nacl loader
sandbox?

Yes and no.

The NaCL sandbox is exercised by the PPAPINaClTest and the nacl integration
tests

As discussed off-line, I've added a comment pointing out that the embedder will
need to test its own sandboxes.

http://codereview.chromium.org/8589001/diff/11001/content/content_resources.grd
File content/content_resources.grd (right):

http://codereview.chromium.org/8589001/diff/11001/content/content_resources.g...
content/content_resources.grd:24: common/sandbox_mac_unittest_helper.mm -->
On 2011/11/23 07:02:17, jeremy wrote:
> Not clear on what's to be updated in helper.mm? Do you mean
> sandbox_process_type_mac.h ?
That was an old comment, I removed it

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
content/public/common/sandbox_init.h:40: //
content::sandbox::SANDBOX_AFTER_TYPE_LAST_TYPE.
On 2011/11/23 07:02:17, jeremy wrote:
> We need the unit tests to run on all sandbox types, how is this achieved if
> embedders can add their own out-of-band sandbox types?

The embedder will have to test their own definitions themselves

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/11001/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:9: namespace content {
On 2011/11/23 07:02:17, jeremy wrote:
> Need a header comment explaining what this file does.

Done.

[Jói, 2011-11-23 12:16:36]

LGTM with a couple of nits

http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:36:
SANDBOX_AFTER_TYPE_LAST_TYPE,  // Placeholder to ease iteration.
-> SANDBOX_TYPE_LAST_TYPE ? (skip the _AFTER part)
alternatively
-> SANDBOX_AFTER_LAST_TYPE

http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS
File content/shell/DEPS (right):

http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS#newcode4
content/shell/DEPS:4: "+grit",
can this be narrowed down at all?

[jochen (gone - plz use gerrit), 2011-11-23 19:24:23]

http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:36:
SANDBOX_AFTER_TYPE_LAST_TYPE,  // Placeholder to ease iteration.
What about SANDBOX_TYPE_AFTER_LAST_TYPE? I think that's the most clear option

http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS
File content/shell/DEPS (right):

http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS#newcode4
content/shell/DEPS:4: "+grit",
On 2011/11/23 12:16:37, Jói wrote:
> can this be narrowed down at all?

Done.

[Jói, 2011-11-23 19:56:36]

LGTM

On Wed, Nov 23, 2011 at 7:24 PM,  <jochen@chromium.org> wrote:
>
>
http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
> File content/public/common/sandbox_process_type_mac.h (right):
>
>
http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
> content/public/common/sandbox_process_type_mac.h:36:
> SANDBOX_AFTER_TYPE_LAST_TYPE,  // Placeholder to ease iteration.
> What about SANDBOX_TYPE_AFTER_LAST_TYPE? I think that's the most clear
> option
>
>
> http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS
> File content/shell/DEPS (right):
>
> http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS#newcode4
> content/shell/DEPS:4: "+grit",
> On 2011/11/23 12:16:37, Jói wrote:
>>
>> can this be narrowed down at all?
>
>
> Done.
>
> http://codereview.chromium.org/8589001/

[Jói, 2011-11-23 19:56:36]

LGTM

On Wed, Nov 23, 2011 at 7:24 PM,  <jochen@chromium.org> wrote:
>
>
http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
> File content/public/common/sandbox_process_type_mac.h (right):
>
>
http://codereview.chromium.org/8589001/diff/19044/content/public/common/sandb...
> content/public/common/sandbox_process_type_mac.h:36:
> SANDBOX_AFTER_TYPE_LAST_TYPE,  // Placeholder to ease iteration.
> What about SANDBOX_TYPE_AFTER_LAST_TYPE? I think that's the most clear
> option
>
>
> http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS
> File content/shell/DEPS (right):
>
> http://codereview.chromium.org/8589001/diff/19044/content/shell/DEPS#newcode4
> content/shell/DEPS:4: "+grit",
> On 2011/11/23 12:16:37, Jói wrote:
>>
>> can this be narrowed down at all?
>
>
> Done.
>
> http://codereview.chromium.org/8589001/

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[jam, 2011-11-23 20:29:17]

lgtm with nits

great cleanup!

http://codereview.chromium.org/8589001/diff/21001/content/DEPS
File content/DEPS (right):

http://codereview.chromium.org/8589001/diff/21001/content/DEPS#newcode62
content/DEPS:62: "-grit",
can we just allow the ones that content can include right here? i.e.

"+grit/content_resources.h",
"+grit/webkit_strings.h",
"+grit/webkit_chromium_resources.h"

that way there's just one place that shows which grit files content can use
(other than content/shell or tests, which is a separate story)

http://codereview.chromium.org/8589001/diff/21001/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/21001/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:15: SANDBOX_TYPE_FIRST_TYPE =
0,  // Placeholder to ease iteration.
nit: content API (like WebKit API) has a convention that the enum values start
with the name of the enum. so either rename the enum (and file) to SandboxType,
or change the values to SANDBOX_PROCESS_TYPE...

[jochen (gone - plz use gerrit), 2011-11-23 21:51:00]

http://codereview.chromium.org/8589001/diff/21001/content/DEPS
File content/DEPS (right):

http://codereview.chromium.org/8589001/diff/21001/content/DEPS#newcode62
content/DEPS:62: "-grit",
On 2011/11/23 20:29:17, John Abd-El-Malek wrote:
> can we just allow the ones that content can include right here? i.e.
> 
> "+grit/content_resources.h",
> "+grit/webkit_strings.h",
> "+grit/webkit_chromium_resources.h"
> 
> that way there's just one place that shows which grit files content can use
> (other than content/shell or tests, which is a separate story)

Done.

http://codereview.chromium.org/8589001/diff/21001/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/21001/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:15: SANDBOX_TYPE_FIRST_TYPE =
0,  // Placeholder to ease iteration.
I picked SANDBOX_PROCESS_TYPE...

[jeremy, 2011-11-24 12:20:19]

Code looks good, mostly naming issues and sprucing up comments.

Since all the sandbox functions now take int's , could you add a note about
which enums provide legal values for these?

http://codereview.chromium.org/8589001/diff/21002/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8589001/diff/21002/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:410: int
ChromeContentClient::GetSandboxPolicyForSandboxType(
Policy -> Profile here and in the enum.

http://codereview.chromium.org/8589001/diff/21002/chrome/nacl/nacl_main_platf...
File chrome/nacl/nacl_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/8589001/diff/21002/chrome/nacl/nacl_main_platf...
chrome/nacl/nacl_main_platform_delegate_mac.mm:60:
CHROME_SANDBOX_PROCESS_TYPE_NACL_LOADER, FilePath()))
imho this should be indented 4 from the start of "content::..."

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:24: FilePath* allowed_dir) {
IMHO this should return a bool rather than using the magic value of -1 for
sandbox_process_type .

Also, could you add a comment for the function?

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.h:62: // is turned on. |sandbox_type| is the type of
sandbox to warm up.
Could you add a reference in the comment to the enum that needs to be used for
sandbox_type.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:39: struct SandboxProcessTypePolicyMapping {
IMHO SandboxTypeToResourceIDMapping would be a better name, also open to other
options.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:344: int sandbox_policy_resource_id = -1;
*policy->profile also in the rest of the function.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:359: sandbox_policy_resource_id =
content::GetContentClient()->
No chance that GetContentClient() can be null, right?

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac_...
File content/common/sandbox_mac_unittest_helper.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac_...
content/common/sandbox_mac_unittest_helper.h:61: // these tests.
nit: I'd make this more prominent, something like:
// DANGER WILL ROBINSON: Additional sandbox types defined by the embedder (e.g.
the NaCL sandbox) won't be covered by these tests.

http://codereview.chromium.org/8589001/diff/21002/content/content_resources.grd
File content/content_resources.grd (right):

http://codereview.chromium.org/8589001/diff/21002/content/content_resources.g...
content/content_resources.grd:23: <include name="IDR_GPU_SANDBOX_POLICY"
file="browser/gpu.sb" type="BINDATA" />
POLICY -> PROFILE

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
content/public/common/content_client.h:119: // sandbox policy given by a
resource ID. Returns -1 if no sandbox policy for
*policy -> profile

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
content/public/common/content_client.h:120: // the given |sandbox_type| exists.
* exists -> exists or the resource id corresponding to the sandbox profile to
use.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:33: // Initialize the sandbox of the given
|sandbox_type|, optionally specifying a
could you add a pointer to the enum that contains legal values for sandbox_type.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:35: // policy associated with the given
|sandbox_type|.
policy -> profile

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:37: // The sandbox policy to use for this
|sandbox_type| is queried using
You should note something along the lines of  "If the sandbox_type isn't one of
the ones defined by content then the embedder is queried.." .

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:16:
SANDBOX_PROCESS_TYPE_FIRST_TYPE = 0,  // Placeholder to ease iteration.
I'd remove the word PROCESS from all these defines e.g.
SANDBOX_TYPE_FIRST_TYPE .

I'm also fine renaming the enum to SandboxType if you want but don't feel
strongly about it.

[jochen (gone - plz use gerrit), 2011-11-24 16:23:21]

tl;dr all comments addressed

http://codereview.chromium.org/8589001/diff/21002/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/8589001/diff/21002/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:410: int
ChromeContentClient::GetSandboxPolicyForSandboxType(
On 2011/11/24 12:20:19, jeremy wrote:
> Policy -> Profile here and in the enum.

Done.

http://codereview.chromium.org/8589001/diff/21002/chrome/nacl/nacl_main_platf...
File chrome/nacl/nacl_main_platform_delegate_mac.mm (right):

http://codereview.chromium.org/8589001/diff/21002/chrome/nacl/nacl_main_platf...
chrome/nacl/nacl_main_platform_delegate_mac.mm:60:
CHROME_SANDBOX_PROCESS_TYPE_NACL_LOADER, FilePath()))
On 2011/11/24 12:20:19, jeremy wrote:
> imho this should be indented 4 from the start of "content::..."

After renaming the constant, everything fitted into one line

Done

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:24: FilePath* allowed_dir) {
On 2011/11/24 12:20:19, jeremy wrote:
> IMHO this should return a bool rather than using the magic value of -1 for
> sandbox_process_type .
> 
> Also, could you add a comment for the function?

Done.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.h:62: // is turned on. |sandbox_type| is the type of
sandbox to warm up.
On 2011/11/24 12:20:19, jeremy wrote:
> Could you add a reference in the comment to the enum that needs to be used for
> sandbox_type.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:39: struct SandboxProcessTypePolicyMapping {
On 2011/11/24 12:20:19, jeremy wrote:
> IMHO SandboxTypeToResourceIDMapping would be a better name, also open to other
> options.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:344: int sandbox_policy_resource_id = -1;
On 2011/11/24 12:20:19, jeremy wrote:
> *policy->profile also in the rest of the function.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac....
content/common/sandbox_mac.mm:359: sandbox_policy_resource_id =
content::GetContentClient()->
On 2011/11/24 12:20:19, jeremy wrote:
> No chance that GetContentClient() can be null, right?

Right

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac_...
File content/common/sandbox_mac_unittest_helper.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/common/sandbox_mac_...
content/common/sandbox_mac_unittest_helper.h:61: // these tests.
On 2011/11/24 12:20:19, jeremy wrote:
> nit: I'd make this more prominent, something like:
> // DANGER WILL ROBINSON: Additional sandbox types defined by the embedder
(e.g.
> the NaCL sandbox) won't be covered by these tests.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/content_resources.grd
File content/content_resources.grd (right):

http://codereview.chromium.org/8589001/diff/21002/content/content_resources.g...
content/content_resources.grd:23: <include name="IDR_GPU_SANDBOX_POLICY"
file="browser/gpu.sb" type="BINDATA" />
On 2011/11/24 12:20:19, jeremy wrote:
> POLICY -> PROFILE

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
content/public/common/content_client.h:119: // sandbox policy given by a
resource ID. Returns -1 if no sandbox policy for
On 2011/11/24 12:20:19, jeremy wrote:
> *policy -> profile

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/conte...
content/public/common/content_client.h:120: // the given |sandbox_type| exists.
On 2011/11/24 12:20:19, jeremy wrote:
> * exists -> exists or the resource id corresponding to the sandbox profile to
> use.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:33: // Initialize the sandbox of the given
|sandbox_type|, optionally specifying a
On 2011/11/24 12:20:19, jeremy wrote:
> could you add a pointer to the enum that contains legal values for
sandbox_type.

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:35: // policy associated with the given
|sandbox_type|.
On 2011/11/24 12:20:19, jeremy wrote:
> policy -> profile

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_init.h:37: // The sandbox policy to use for this
|sandbox_type| is queried using
On 2011/11/24 12:20:19, jeremy wrote:
> You should note something along the lines of  "If the sandbox_type isn't one
of
> the ones defined by content then the embedder is queried.." .

Done.

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
File content/public/common/sandbox_process_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/21002/content/public/common/sandb...
content/public/common/sandbox_process_type_mac.h:16:
SANDBOX_PROCESS_TYPE_FIRST_TYPE = 0,  // Placeholder to ease iteration.
On 2011/11/24 12:20:19, jeremy wrote:
> I'd remove the word PROCESS from all these defines e.g.
> SANDBOX_TYPE_FIRST_TYPE .
> 
> I'm also fine renaming the enum to SandboxType if you want but don't feel
> strongly about it.

Done.

[jeremy, 2011-11-24 16:42:04]

Looks very good, a few minor comments - hopefully this will be the last round :)

Thanks for your patience!

http://codereview.chromium.org/8589001/diff/24003/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/8589001/diff/24003/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:38: virtual int
GetSandboxProfileForSandboxType(int sandbox_type) const OVERRIDE;
Can you add a comment on legal values for |sandbox_type|.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:24: // process type. Returns false, if the
current process type isn't sandboxed.
How about:
Fill in |sandbox_type| and |allowed_dir| based on the command line,  returns
false if the current process type doesn't need to be sandboxed or if the sandbox
was disabled from the command line.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.h:62: // Sandbox is turned on. |sandbox_type| is the
type of sandbox to warm up.
nit: sandbox with a small 's'.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.h:69: // |sandbox_type| - type of Sandbox to use.
- see SandboxWarmup() above for legal values.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.mm:360:
GetSandboxProfileForSandboxType(sandbox_type);
Can we make this function return true if it knows about sandbox_type and fill in
sandbox_profile_resource_id as a parameter?

I'm concerned about the case where an embedder returns some bogus value other
than -1 when it can't load a profile.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.mm:469: // being passed in.
nit: rewrap comment.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:118: // Allows the embedder to define a
new |sandbox_type| by mapping it to a
Can you point back to the enum of sandbox types in content ?

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:119: // sandbox profile given by a
resource ID. Returns -1 if no sandbox profile
*by mapping it to the resource id corresponding to the sandbox profile to use.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:122: virtual int
GetSandboxProfileForSandboxType(int sandbox_type) const = 0;
Per previous comment I think this would be a bit more bulletproof if it returned
a bool and filled in the resrouce_id as an output parameter.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
content/public/common/sandbox_init.h:45: // occurred.  If process_type isn't one
that needs sandboxing true is always
nit: *sandboxing true -> sandboxing, no action is taken and true

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
File content/public/common/sandbox_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
content/public/common/sandbox_type_mac.h:13: // SANDBOX_TYPE_AFTER_LAST_TYPE.
nit: does this fit on the previous line?

[jochen (gone - plz use gerrit), 2011-11-24 20:08:35]

http://codereview.chromium.org/8589001/diff/24003/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.h (right):

http://codereview.chromium.org/8589001/diff/24003/chrome/common/chrome_conten...
chrome/common/chrome_content_client.h:38: virtual int
GetSandboxProfileForSandboxType(int sandbox_type) const OVERRIDE;
On 2011/11/24 16:42:04, jeremy wrote:
> Can you add a comment on legal values for |sandbox_type|.

I'll add the comment to the interface. We usually don't have comments on the
implementations

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:24: // process type. Returns false, if the
current process type isn't sandboxed.
On 2011/11/24 16:42:04, jeremy wrote:
> How about:
> Fill in |sandbox_type| and |allowed_dir| based on the command line,  returns
> false if the current process type doesn't need to be sandboxed or if the
sandbox
> was disabled from the command line.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.h:62: // Sandbox is turned on. |sandbox_type| is the
type of sandbox to warm up.
On 2011/11/24 16:42:04, jeremy wrote:
> nit: sandbox with a small 's'.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.h:69: // |sandbox_type| - type of Sandbox to use.
On 2011/11/24 16:42:04, jeremy wrote:
> - see SandboxWarmup() above for legal values.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.mm:360:
GetSandboxProfileForSandboxType(sandbox_type);
On 2011/11/24 16:42:04, jeremy wrote:
> Can we make this function return true if it knows about sandbox_type and fill
in
> sandbox_profile_resource_id as a parameter?
> 
> I'm concerned about the case where an embedder returns some bogus value other
> than -1 when it can't load a profile. 

Done.

http://codereview.chromium.org/8589001/diff/24003/content/common/sandbox_mac....
content/common/sandbox_mac.mm:469: // being passed in.
On 2011/11/24 16:42:04, jeremy wrote:
> nit: rewrap comment.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:118: // Allows the embedder to define a
new |sandbox_type| by mapping it to a
On 2011/11/24 16:42:04, jeremy wrote:
> Can you point back to the enum of sandbox types in content ?

Done.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:119: // sandbox profile given by a
resource ID. Returns -1 if no sandbox profile
On 2011/11/24 16:42:04, jeremy wrote:
> *by mapping it to the resource id corresponding to the sandbox profile to use.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/conte...
content/public/common/content_client.h:122: virtual int
GetSandboxProfileForSandboxType(int sandbox_type) const = 0;
On 2011/11/24 16:42:04, jeremy wrote:
> Per previous comment I think this would be a bit more bulletproof if it
returned
> a bool and filled in the resrouce_id as an output parameter.

Done.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
content/public/common/sandbox_init.h:45: // occurred.  If process_type isn't one
that needs sandboxing true is always
On 2011/11/24 16:42:04, jeremy wrote:
> nit: *sandboxing true -> sandboxing, no action is taken and true

Done.

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
File content/public/common/sandbox_type_mac.h (right):

http://codereview.chromium.org/8589001/diff/24003/content/public/common/sandb...
content/public/common/sandbox_type_mac.h:13: // SANDBOX_TYPE_AFTER_LAST_TYPE.
On 2011/11/24 16:42:04, jeremy wrote:
> nit: does this fit on the previous line?

Done.

[jeremy, 2011-11-25 19:53:16]

LGTM with comments addressed.

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (left):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:2: // Use of this source code is governed by
a BSD-style license that can be
Did you remove these lines on purpose?

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac....
content/common/sandbox_mac.h:69: // |sandbox_type| - type of Sandbox to use. See
SandboxWarmup for legal
nit: SandboxWarmup()

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac....
content/common/sandbox_mac.mm:45: // policy for all process types known to
content.
1)*defining -> containing
2)policy -> profile

http://codereview.chromium.org/8589001/diff/25017/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/8589001/diff/25017/content/content_common.gypi...
content/content_common.gypi:276: 'common/process_watcher_posix.cc',
Is this part of this CL on purpose?

http://codereview.chromium.org/8589001/diff/25017/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/public/common/conte...
content/public/common/content_client.h:124: // the sandbox profile to use.
nit: and true is returned.

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
content/public/common/sandbox_init.h:34: // directory to allow access to. This
needs to be supported by the sandbox
*This -> Note: specifying a directory

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
content/public/common/sandbox_init.h:36: // |sandbox_type| are defined either by
the enum SandboxType, or by the
remove the word "the"

[jochen (gone - plz use gerrit), 2011-11-25 20:30:16]

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_init...
File content/common/sandbox_init_mac.cc (left):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_init...
content/common/sandbox_init_mac.cc:2: // Use of this source code is governed by
a BSD-style license that can be
On 2011/11/25 19:53:16, jeremy wrote:
> Did you remove these lines on purpose?

Oops, good catch!

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac.h
File content/common/sandbox_mac.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac....
content/common/sandbox_mac.h:69: // |sandbox_type| - type of Sandbox to use. See
SandboxWarmup for legal
On 2011/11/25 19:53:16, jeremy wrote:
> nit: SandboxWarmup()

Done.

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac.mm
File content/common/sandbox_mac.mm (right):

http://codereview.chromium.org/8589001/diff/25017/content/common/sandbox_mac....
content/common/sandbox_mac.mm:45: // policy for all process types known to
content.
On 2011/11/25 19:53:16, jeremy wrote:
> 1)*defining -> containing
> 2)policy -> profile

Done.

http://codereview.chromium.org/8589001/diff/25017/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/8589001/diff/25017/content/content_common.gypi...
content/content_common.gypi:276: 'common/process_watcher_posix.cc',
On 2011/11/25 19:53:16, jeremy wrote:
> Is this part of this CL on purpose?

no, that's a rebase/merge screw-up

Done

http://codereview.chromium.org/8589001/diff/25017/content/public/common/conte...
File content/public/common/content_client.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/public/common/conte...
content/public/common/content_client.h:124: // the sandbox profile to use.
On 2011/11/25 19:53:16, jeremy wrote:
> nit: and true is returned.

Done.

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
File content/public/common/sandbox_init.h (right):

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
content/public/common/sandbox_init.h:34: // directory to allow access to. This
needs to be supported by the sandbox
On 2011/11/25 19:53:16, jeremy wrote:
> *This -> Note: specifying a directory

Done.

http://codereview.chromium.org/8589001/diff/25017/content/public/common/sandb...
content/public/common/sandbox_init.h:36: // |sandbox_type| are defined either by
the enum SandboxType, or by the
On 2011/11/25 19:53:16, jeremy wrote:
> remove the word "the"

Done.

[brettw, 2011-11-25 20:54:32]

content/ppapi LGTM

[jbates, 2011-11-29 01:25:34]

Looks like this CL broke the GPU process on OSX. Any GL content now fails with:

FATAL:resource_bundle.cc(132)] Check failed: g_shared_instance_ != NULL.