Wrap deprecated sandbox functions in C++ class.

Wrap deprecated sandbox functions in C++ class.

OS X deprecated sandbox functions and did not replace them with
anything suitable for Chromium. This wraps them in a C++ class,
so that the deprecation supression directives are not littered
through the source code.

BUG=547071
Committed: https://crrev.com/dc059fee81609043169f2a21593ef209a78bdecf
Cr-Commit-Position: refs/heads/master@{#386711}

[Greg K, 2016-04-05 14:10:05]

kerrnel@chromium.org changed reviewers:
+ rsesek@chromium.org, thakis@chromium.org

[Greg K, 2016-04-05 14:10:05]

It looks like the one failing trybot is broken, so please review, thanks!

[Robert Sesek, 2016-04-05 14:13:25]

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
content/common/sandbox_mac.mm:13: #include <sandbox.h>
Remove now?

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
content/common/sandbox_mac.mm:51: int sandbox_init_with_parameters(const char*
profile,
Might as well wrap this one, too.

https://codereview.chromium.org/1855303003/diff/60001/ipc/DEPS
File ipc/DEPS (right):

https://codereview.chromium.org/1855303003/diff/60001/ipc/DEPS#newcode5
ipc/DEPS:5: "+sandbox",
Can limit this to sandbox/mac/seatbelt.h.

https://codereview.chromium.org/1855303003/diff/60001/ipc/ipc_send_fds_test.cc
File ipc/ipc_send_fds_test.cc (right):

https://codereview.chromium.org/1855303003/diff/60001/ipc/ipc_send_fds_test.c...
ipc/ipc_send_fds_test.cc:10: #include <sandbox.h>
Remove.

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h
File sandbox/mac/seatbelt.h (right):

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:19: static int sandbox_init(const char* profile, uint64_t
flags, char** errorbuf);
These should be named in CamelCase style, but since they're now wrapped in a
namespace and class, they can probably just be

Seatbelt::Init
Seatbelt::InitWithParams (if you wrap this one too)
Seatbelt::FreeError

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:23: Seatbelt();
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:25: }
Add comment: "  // sandbox"

and a blank line before and after.

[Greg K, 2016-04-05 18:24:44]

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
File content/common/sandbox_mac.mm (right):

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
content/common/sandbox_mac.mm:13: #include <sandbox.h>
On 2016/04/05 14:13:25, Robert Sesek wrote:
> Remove now?

Done.

https://codereview.chromium.org/1855303003/diff/60001/content/common/sandbox_...
content/common/sandbox_mac.mm:51: int sandbox_init_with_parameters(const char*
profile,
On 2016/04/05 14:13:25, Robert Sesek wrote:
> Might as well wrap this one, too.

Done.

https://codereview.chromium.org/1855303003/diff/60001/ipc/DEPS
File ipc/DEPS (right):

https://codereview.chromium.org/1855303003/diff/60001/ipc/DEPS#newcode5
ipc/DEPS:5: "+sandbox",
On 2016/04/05 14:13:25, Robert Sesek wrote:
> Can limit this to sandbox/mac/seatbelt.h.

Done.

https://codereview.chromium.org/1855303003/diff/60001/ipc/ipc_send_fds_test.cc
File ipc/ipc_send_fds_test.cc (right):

https://codereview.chromium.org/1855303003/diff/60001/ipc/ipc_send_fds_test.c...
ipc/ipc_send_fds_test.cc:10: #include <sandbox.h>
On 2016/04/05 14:13:25, Robert Sesek wrote:
> Remove.

It's still needed for the profile constants.

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h
File sandbox/mac/seatbelt.h (right):

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:19: static int sandbox_init(const char* profile, uint64_t
flags, char** errorbuf);
On 2016/04/05 14:13:25, Robert Sesek wrote:
> These should be named in CamelCase style, but since they're now wrapped in a
> namespace and class, they can probably just be
> 
> Seatbelt::Init
> Seatbelt::InitWithParams (if you wrap this one too)
> Seatbelt::FreeError

Done.

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:23: Seatbelt();
On 2016/04/05 14:13:25, Robert Sesek wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1855303003/diff/60001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:25: }
On 2016/04/05 14:13:25, Robert Sesek wrote:
> Add comment: "  // sandbox"
> 
> and a blank line before and after.

Done.

[Robert Sesek, 2016-04-05 18:26:05]

LGTM

[Greg K, 2016-04-05 18:27:51]

kerrnel@chromium.org changed reviewers:
+ avi@chromium.org, tsepez@chromium.org

[Greg K, 2016-04-05 18:27:51]

avi@chromium.org: Please review changes in 
content/common/BUILD.gn

tsepez@chromium.org: Please review changes in:
ipc/

Thanks,

Greg

[Greg K, 2016-04-05 18:28:30]

kerrnel@chromium.org changed reviewers:
+ jln@chromium.org

[Greg K, 2016-04-05 18:28:31]

jln@chromium.org: Please review changes in 
sandbox/BUILD.gn

Thanks,

Greg

[Nico, 2016-04-05 18:37:18]

thanks!

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/BUILD.gn
File sandbox/mac/BUILD.gn (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/BUILD.gn#ne...
sandbox/mac/BUILD.gn:39: "seatbelt.h",
(consider adding seatbelt_exports.h here too)

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/sandbox_mac...
File sandbox/mac/sandbox_mac.gypi (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/sandbox_mac...
sandbox/mac/sandbox_mac.gypi:12: 'seatbelt.h',
(likewise)

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc
File sandbox/mac/seatbelt.cc (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc...
sandbox/mac/seatbelt.cc:20: #pragma clang diagnostic push
This still needs a comment why ignoring the diagnostic is ok :-)

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc...
sandbox/mac/seatbelt.cc:38: return ::sandbox_free_error(errorbuf);
likewise

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h
File sandbox/mac/seatbelt.h (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:11: #include "sandbox/sandbox_export.h"
Oh, this probably won't work, you probably want a separate _export.h file for
your new component. Else, if you include a sandbox header in this file some time
down the line, the SANDBOX_EXPORTS in there will mean that the new seatbelt
component will also export these sandbox files.

[Tom Sepez, 2016-04-05 18:39:11]

IPC LGTM

[jln (very slow on Chromium), 2016-04-05 18:40:55]

sandbox/BUILD.gn owner lgtm rubberstamp

[Greg K, 2016-04-05 18:44:54]

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc
File sandbox/mac/seatbelt.cc (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc...
sandbox/mac/seatbelt.cc:20: #pragma clang diagnostic push
On 2016/04/05 18:37:18, Nico (hiding until Fri) wrote:
> This still needs a comment why ignoring the diagnostic is ok :-)

Done.

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.cc...
sandbox/mac/seatbelt.cc:38: return ::sandbox_free_error(errorbuf);
On 2016/04/05 18:37:18, Nico (hiding until Fri) wrote:
> likewise

Done.

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h
File sandbox/mac/seatbelt.h (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:11: #include "sandbox/sandbox_export.h"
On 2016/04/05 18:37:18, Nico (hiding until Fri) wrote:
> Oh, this probably won't work, you probably want a separate _export.h file for
> your new component. Else, if you include a sandbox header in this file some
time
> down the line, the SANDBOX_EXPORTS in there will mean that the new seatbelt
> component will also export these sandbox files.

sandbox/linux uses sandbox_export across all of its different targets, so is
there a reason it's not a problem there but a problem on OS X?

Thanks,

Greg

[Nico, 2016-04-05 18:46:36]

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h
File sandbox/mac/seatbelt.h (right):

https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h#...
sandbox/mac/seatbelt.h:11: #include "sandbox/sandbox_export.h"
On 2016/04/05 18:44:54, Greg Kerr wrote:
> On 2016/04/05 18:37:18, Nico (hiding until Fri) wrote:
> > Oh, this probably won't work, you probably want a separate _export.h file
for
> > your new component. Else, if you include a sandbox header in this file some
> time
> > down the line, the SANDBOX_EXPORTS in there will mean that the new seatbelt
> > component will also export these sandbox files.
> 
> sandbox/linux uses sandbox_export across all of its different targets, so is
> there a reason it's not a problem there but a problem on OS X?

It's a problem there too.

(source_sets can share exports as long as they're all linked into the same
component() in the end. Several components() can't use the same _EXPORT macro.
sandbox/linux has at least two components() that use SANDBOX_EXPORT; that's
incorrect as well.)

[Greg K, 2016-04-06 15:43:58]

On 2016/04/05 18:46:36, Nico (hiding until Fri) wrote:
> https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h
> File sandbox/mac/seatbelt.h (right):
> 
>
https://codereview.chromium.org/1855303003/diff/80001/sandbox/mac/seatbelt.h#...
> sandbox/mac/seatbelt.h:11: #include "sandbox/sandbox_export.h"
> On 2016/04/05 18:44:54, Greg Kerr wrote:
> > On 2016/04/05 18:37:18, Nico (hiding until Fri) wrote:
> > > Oh, this probably won't work, you probably want a separate _export.h file
> for
> > > your new component. Else, if you include a sandbox header in this file
some
> > time
> > > down the line, the SANDBOX_EXPORTS in there will mean that the new
seatbelt
> > > component will also export these sandbox files.
> > 
> > sandbox/linux uses sandbox_export across all of its different targets, so is
> > there a reason it's not a problem there but a problem on OS X?
> 
> It's a problem there too.
> 
> (source_sets can share exports as long as they're all linked into the same
> component() in the end. Several components() can't use the same _EXPORT macro.
> sandbox/linux has at least two components() that use SANDBOX_EXPORT; that's
> incorrect as well.)

Ok, I created a new export macro, please take a look. Thanks.

[Avi (use Gerrit), 2016-04-06 16:46:35]

lgtm

[Greg K, 2016-04-11 20:11:07]

On 2016/04/06 16:46:35, Avi wrote:
> lgtm

Nico,

Please take a look.

Thanks,

Greg

[Nico, 2016-04-11 20:29:04]

lgtm

[Greg K, 2016-04-11 20:31:23]

The CQ bit was checked by kerrnel@chromium.org

[Greg K, 2016-04-11 20:31:23]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
tsepez@chromium.org, jln@chromium.org
Link to the patchset: https://codereview.chromium.org/1855303003/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-04-11 20:32:26]

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

[commit-bot: I haz the power, 2016-04-11 23:57:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-11 23:57:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Greg K, 2016-04-12 16:14:56]

The CQ bit was checked by kerrnel@chromium.org

[commit-bot: I haz the power, 2016-04-12 16:15:12]

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

[commit-bot: I haz the power, 2016-04-12 16:39:16]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-12 16:40:26]

Description was changed from

==========
Wrap deprecated sandbox functions in C++ class.

OS X deprecated sandbox functions and did not replace them with
anything suitable for Chromium. This wraps them in a C++ class,
so that the deprecation supression directives are not littered
through the source code.

BUG=547071
==========

to

==========
Wrap deprecated sandbox functions in C++ class.

OS X deprecated sandbox functions and did not replace them with
anything suitable for Chromium. This wraps them in a C++ class,
so that the deprecation supression directives are not littered
through the source code.

BUG=547071

Committed: https://crrev.com/dc059fee81609043169f2a21593ef209a78bdecf
Cr-Commit-Position: refs/heads/master@{#386711}
==========

[commit-bot: I haz the power, 2016-04-12 16:40:27]

Patchset 8 (id:??) landed as
https://crrev.com/dc059fee81609043169f2a21593ef209a78bdecf
Cr-Commit-Position: refs/heads/master@{#386711}