Move non-content specific parts of content::MachBroker into base::MachPortBroker.

Move non-content specific parts of content::MachBroker into base::MachPortBroker.

This change only moves mach port functions into a new class,
base::MachPortBroker. There is no new functionality, or change to
existing behaviour.

BUG=582468
Committed: https://crrev.com/11ca9a5d48580d5eb98fc4d915ce308d3faf3314
Cr-Commit-Position: refs/heads/master@{#379178}

[Anand Mistry (off Chromium), 2016-03-02 06:31:12]

amistry@chromium.org changed reviewers:
+ erikchen@chromium.org, mark@chromium.org

[erikchen, 2016-03-02 06:56:11]

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
File base/mac/mach_port_broker.h (right):

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
base/mac/mach_port_broker.h:23: class MachBrokerTest;
A test in content/ needs access to private members of this class? Maybe those
members should be protected & virtual instead? 

It looks like a single test in content accesses a private member of a private
member. Surely there must be a better way of structuring the test so that this
isn't needed?

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
base/mac/mach_port_broker_unittest.cc:53: FinalizePid(1u, 100u);
Sorry to impose, but could you add a multiprocess unit test which tests the
functionality of ChildSendTaskPortToParent(), which seems like the real business
logic in the mach port broker?

I won't make this a requirement, since this CL is just a refactor, and if you
end up writing mojo-related tests that end up testing the functionality, then
I'm less worried.

[Anand Mistry (off Chromium), 2016-03-03 01:57:40]

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
File base/mac/mach_port_broker.h (right):

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
base/mac/mach_port_broker.h:23: class MachBrokerTest;
On 2016/03/02 06:56:11, erikchen wrote:
> A test in content/ needs access to private members of this class? Maybe those
> members should be protected & virtual instead? 
> 
> It looks like a single test in content accesses a private member of a private
> member. Surely there must be a better way of structuring the test so that this
> isn't needed?

I kinda worked around this. I figured the existing content tests weren't as
useful and largely duplicative of the base::MachPortBroker tests. So I removed
them and combined the tested functionality into a single multi-process test. I
hope that works for you.

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/1755973002/diff/60001/base/mac/mach_port_brok...
base/mac/mach_port_broker_unittest.cc:53: FinalizePid(1u, 100u);
On 2016/03/02 06:56:11, erikchen wrote:
> Sorry to impose, but could you add a multiprocess unit test which tests the
> functionality of ChildSendTaskPortToParent(), which seems like the real
business
> logic in the mach port broker?
> 
> I won't make this a requirement, since this CL is just a refactor, and if you
> end up writing mojo-related tests that end up testing the functionality, then
> I'm less worried.

Done.

[erikchen, 2016-03-03 02:26:23]

thanks! lgtm

https://codereview.chromium.org/1755973002/diff/80001/base/mac/mach_port_brok...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/1755973002/diff/80001/base/mac/mach_port_brok...
base/mac/mach_port_broker_unittest.cc:17: const char kBootstrapPortName[] =
"thisisatest";
is it possible for tests from a single suite to be sharded? If so, using a
static bootstrap port name will cause race conditions between the two tests.

[Anand Mistry (off Chromium), 2016-03-03 02:48:55]

https://codereview.chromium.org/1755973002/diff/80001/base/mac/mach_port_brok...
File base/mac/mach_port_broker_unittest.cc (right):

https://codereview.chromium.org/1755973002/diff/80001/base/mac/mach_port_brok...
base/mac/mach_port_broker_unittest.cc:17: const char kBootstrapPortName[] =
"thisisatest";
On 2016/03/03 02:26:22, erikchen wrote:
> is it possible for tests from a single suite to be sharded? If so, using a
> static bootstrap port name will cause race conditions between the two tests.

Sharded test suites run in different processes, and the actual port name is
constructed from this and the pid and the bundle ID. So you won't get a name
collision. The only potential problem is when the two tests run consecutively in
the same process. But that seems to work fine.

[Anand Mistry (off Chromium), 2016-03-03 22:12:18]

amistry@chromium.org changed reviewers:
+ nasko@chromium.org

[Anand Mistry (off Chromium), 2016-03-03 22:12:19]

nasko@chromium.org: For content owners.

[nasko, 2016-03-03 22:14:13]

Rubberstamp LGTM based on erikchen@'s review.

[Mark Mentovai, 2016-03-03 23:01:59]

LGTM in base

https://codereview.chromium.org/1755973002/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1755973002/diff/140001/base/base.gyp#newcode205
base/base.gyp:205: '$(SDKROOT)/usr/lib/libbsm.dylib',
Do you have to pull this out of some other .gyp file now, or is it still used by
the target that this is being migrated from?

https://codereview.chromium.org/1755973002/diff/140001/content/browser/mach_b...
File content/browser/mach_broker_mac.mm (left):

https://codereview.chromium.org/1755973002/diff/140001/content/browser/mach_b...
content/browser/mach_broker_mac.mm:217:
audit_token_to_au32(msg.trailer.msgh_audit,
since this was the reason for libbsm, and now it’s gone from content

[Anand Mistry (off Chromium), 2016-03-04 00:00:50]

https://codereview.chromium.org/1755973002/diff/140001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1755973002/diff/140001/base/base.gyp#newcode205
base/base.gyp:205: '$(SDKROOT)/usr/lib/libbsm.dylib',
On 2016/03/03 23:01:59, Mark Mentovai wrote:
> Do you have to pull this out of some other .gyp file now, or is it still used
by
> the target that this is being migrated from?

I've removed this from both content gyp and gn rules. From what I can tell (by
looking for libbsm.h includes), there are no other users in //content.

[Anand Mistry (off Chromium), 2016-03-04 00:01:07]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-03-04 00:01:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org, mark@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1755973002/#ps160001
(title: "Remove libbsm from content build rules.")

[Mark Mentovai, 2016-03-04 00:01:44]

LGTM

[commit-bot: I haz the power, 2016-03-04 00:01:49]

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

[commit-bot: I haz the power, 2016-03-04 02:04:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-04 02:07:07]

Description was changed from

==========
Move non-content specific parts of content::MachBroker into
base::MachPortBroker.

This change only moves mach port functions into a new class,
base::MachPortBroker. There is no new functionality, or change to
existing behaviour.

BUG=582468
==========

to

==========
Move non-content specific parts of content::MachBroker into
base::MachPortBroker.

This change only moves mach port functions into a new class,
base::MachPortBroker. There is no new functionality, or change to
existing behaviour.

BUG=582468

Committed: https://crrev.com/11ca9a5d48580d5eb98fc4d915ce308d3faf3314
Cr-Commit-Position: refs/heads/master@{#379178}
==========

[commit-bot: I haz the power, 2016-03-04 02:07:09]

Patchset 9 (id:??) landed as
https://crrev.com/11ca9a5d48580d5eb98fc4d915ce308d3faf3314
Cr-Commit-Position: refs/heads/master@{#379178}