Get rid of SandboxedProcessLauncherDelegate::PreSandbox.

Get rid of SandboxedProcessLauncherDelegate::PreSandbox.

The exposed_dir argument is only used in utility processes, so that
logic is moved to PreSpawnTarget in UtilityProcessHostImpl instead. The
disable_default_policy argument is replaced with a DisableDefaultPolicy
method instead.

BUG=549319
Committed: https://crrev.com/501093188a889c628ddf09e5e19de893bacbea86
Cr-Commit-Position: refs/heads/master@{#358978}

[rickyz (no longer on Chrome), 2015-11-04 06:40:36]

rickyz@chromium.org changed reviewers:
+ wfh@chromium.org

[rickyz (no longer on Chrome), 2015-11-04 06:40:36]

Here's a first small cleanup of the APIs for overriding policies. In a future
change, I plan to eliminate the any callbacks for modifying a policy, and just
have a single SandboxedProcessLauncherDelegate::CreatePolicy.

[Will Harris, 2015-11-04 23:46:41]

On 2015/11/04 06:40:36, rickyz wrote:
> Here's a first small cleanup of the APIs for overriding policies. In a future
> change, I plan to eliminate the any callbacks for modifying a policy, and just
> have a single SandboxedProcessLauncherDelegate::CreatePolicy.

This looks generally good, and simplifies things.

Can you fix the clang errors?

[rickyz (no longer on Chrome), 2015-11-05 01:18:10]

On 2015/11/04 at 23:46:41, wfh wrote:
> On 2015/11/04 06:40:36, rickyz wrote:
> > Here's a first small cleanup of the APIs for overriding policies. In a
future
> > change, I plan to eliminate the any callbacks for modifying a policy, and
just
> > have a single SandboxedProcessLauncherDelegate::CreatePolicy.
> 
> This looks generally good, and simplifies things.
> 
> Can you fix the clang errors?

Oops, looks like I missed one. The clang bots are now failing on an unrelated
error in pdfium (which will be fixed once
https://codereview.chromium.org/1432703002 is rolled).

[Will Harris, 2015-11-06 19:26:37]

This change lgtm for sandbox_win.cc

Please change CL title to SandboxedProcessLauncherDelegate::PreSandbox so it's
obvious what's going on. Also make clear this has no functional/behavior
changes.

You'll need to find some friendly content/ owners - perhaps nasko@ also
mseaborn@ for components/nacl

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2572: return false;
nit: no need for braces

[rickyz (no longer on Chrome), 2015-11-10 02:16:31]

rickyz@chromium.org changed reviewers:
+ jochen@chromium.org, mseaborn@chromium.org

[rickyz (no longer on Chrome), 2015-11-10 02:16:32]

jochen@ for content/ and chrome/
mseaborn@ for components/nacl/

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2572: return false;
On 2015/11/06 at 19:26:37, Will Harris wrote:
> nit: no need for braces

How strongly do you feel about leaving these braces out? I often find them
convenient when I want to quickly add a LOG(ERROR) to every single failure check
in a function (though maybe the real solution is just to windbg better :-P)

[Will Harris, 2015-11-10 17:45:38]

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2572: return false;
style guide says no braces... so they really shouldn't be here. I am working on
adding better error codes to sandbox failures, if that helps :)

[Mark Seaborn, 2015-11-10 18:08:47]

On 2015/11/10 02:16:32, rickyz wrote:
> mseaborn@ for components/nacl/

LGTM

[jochen (gone - plz use gerrit), 2015-11-10 19:52:11]

lgtm

[rickyz (no longer on Chrome), 2015-11-10 23:18:00]

The CQ bit was checked by rickyz@chromium.org

[rickyz (no longer on Chrome), 2015-11-10 23:18:01]

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1415483008/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2572: return false;
On 2015/11/10 at 17:45:38, Will Harris wrote:
> style guide says no braces... so they really shouldn't be here. I am working
on adding better error codes to sandbox failures, if that helps :)

The style guide actually allows both here:
https://google.github.io/styleguide/cppguide.html#Conditionals, but happy to get
rid of them if that's the preferred style for this code.

[rickyz (no longer on Chrome), 2015-11-10 23:18:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, jochen@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1415483008/#ps100001
(title: "Remove curly braces")

[commit-bot: I haz the power, 2015-11-10 23:18:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415483008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415483008/100001

[Will Harris, 2015-11-10 23:20:09]

The CQ bit was unchecked by wfh@chromium.org

[Will Harris, 2015-11-10 23:20:32]

can you update CL description as per comment if possible. thanks

[rickyz (no longer on Chrome), 2015-11-10 23:30:21]

Description was changed from

==========
Get rid of PreSandbox.

The exposed_dir argument is only used in utility processes, so that
logic is moved to PreSpawnTarget in UtilityProcessHostImpl instead. The
disable_default_policy argument is replaced with a DisableDefaultPolicy
method instead.

BUG=549319
==========

to

==========
Get rid of SandboxedProcessLauncherDelegate::PreSandbox.

The exposed_dir argument is only used in utility processes, so that
logic is moved to PreSpawnTarget in UtilityProcessHostImpl instead. The
disable_default_policy argument is replaced with a DisableDefaultPolicy
method instead.

BUG=549319
==========

[rickyz (no longer on Chrome), 2015-11-10 23:30:53]

Oops, missed that, sorry - hopefully that was fast enough to get picked up by
the CQ

[Will Harris, 2015-11-10 23:34:48]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2015-11-10 23:37:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415483008/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415483008/100001

[commit-bot: I haz the power, 2015-11-11 00:37:15]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-11 00:38:49]

Patchset 6 (id:??) landed as
https://crrev.com/501093188a889c628ddf09e5e19de893bacbea86
Cr-Commit-Position: refs/heads/master@{#358978}