Rename mojo::TokenSerializer to mojo::Broker.

Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This change just renames it (hopefully using established names) and compiles it on POSIX.

BUG=556259
Committed: https://crrev.com/3c8c50cd432aaacefa681e707e3f60ccfa10855f
Cr-Commit-Position: refs/heads/master@{#361709}

[jam, 2015-11-24 21:36:03]

Description was changed from

==========
git # Enter a description of the change.
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259
==========

to

==========
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259
==========

[jam, 2015-11-24 21:36:04]

jam@chromium.org changed reviewers:
+ yzshen@chromium.org

[jam, 2015-11-24 22:00:27]

Patchset #1 (id:1) has been deleted

[jam, 2015-11-24 22:00:34]

Patchset #1 (id:20001) has been deleted

[jam, 2015-11-25 00:46:06]

Looks like we will need this after all on other platforms (for multiplexing).

[yzshen1, 2015-11-25 16:47:09]

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker.h:20: // ParentBroker) to convert handles to tokens
and vice versa.
nit: the name ParentBroker is not the actual name of the parent side class.

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.h:23: #if defined(OS_WIN)
I think it will be cleaner if we have ChildBrokerHost{Win,Posix}. WDYT?

https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
File mojo/runner/host/child_process.cc (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
mojo/runner/host/child_process.cc:334: #if defined(OS_WIN)
Does it make sense to have helper methods (in either class) to convert between
edk::ScopedPlatformHandle and mojo::embedder::ScopedPlatformHandle? That way we
don't have to do this in multiple places.

Besides, is it possible to change |handle| and |fd| to the same name, say
|value|? I don't see the benefit of having different names on different
platforms. :/

[jam, 2015-11-25 16:56:09]

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.h:23: #if defined(OS_WIN)
On 2015/11/25 16:47:09, yzshen1 wrote:
> I think it will be cleaner if we have ChildBrokerHost{Win,Posix}. WDYT?

I was waiting for the change after this, to implement it on POSIX, and then to
see how much code sharing there is and whether it's better to have two classes
instead of one (or have some inner platform specific class)

https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
File mojo/runner/host/child_process.cc (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
mojo/runner/host/child_process.cc:334: #if defined(OS_WIN)
On 2015/11/25 16:47:09, yzshen1 wrote:
> Does it make sense to have helper methods (in either class) to convert between
> edk::ScopedPlatformHandle and mojo::embedder::ScopedPlatformHandle? That way
we
> don't have to do this in multiple places.

I thought of that; but the goal should be to delete the old edk soon and so
that's why i was ok with this extra code.

> 
> Besides, is it possible to change |handle| and |fd| to the same name, say
> |value|? I don't see the benefit of having different names on different
> platforms. :/

I agree! Although I think it should be done outside this change.

[yzshen1, 2015-11-25 17:28:40]

On 2015/11/25 16:56:09, jam wrote:
>
https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
> File mojo/edk/system/child_broker_host.h (right):
> 
>
https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
> mojo/edk/system/child_broker_host.h:23: #if defined(OS_WIN)
> On 2015/11/25 16:47:09, yzshen1 wrote:
> > I think it will be cleaner if we have ChildBrokerHost{Win,Posix}. WDYT?
> 
> I was waiting for the change after this, to implement it on POSIX, and then to
> see how much code sharing there is and whether it's better to have two classes
> instead of one (or have some inner platform specific class)
> 
>
https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
> File mojo/runner/host/child_process.cc (right):
> 
>
https://codereview.chromium.org/1465183005/diff/160001/mojo/runner/host/child...
> mojo/runner/host/child_process.cc:334: #if defined(OS_WIN)
> On 2015/11/25 16:47:09, yzshen1 wrote:
> > Does it make sense to have helper methods (in either class) to convert
between
> > edk::ScopedPlatformHandle and mojo::embedder::ScopedPlatformHandle? That way
> we
> > don't have to do this in multiple places.
> 
> I thought of that; but the goal should be to delete the old edk soon and so
> that's why i was ok with this extra code.
> 
> > 
> > Besides, is it possible to change |handle| and |fd| to the same name, say
> > |value|? I don't see the benefit of having different names on different
> > platforms. :/
> 
> I agree! Although I think it should be done outside this change.

LGTM

[jam, 2015-11-25 17:39:27]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-11-25 17:39:58]

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

[commit-bot: I haz the power, 2015-11-25 17:51:54]

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

[commit-bot: I haz the power, 2015-11-25 17:51:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jam, 2015-11-25 17:55:11]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org

[jam, 2015-11-25 17:55:12]

+tsepez for child_process_messages.h

Tom: the chrome IPC isn't sent on POSIX yet. The underlying mojo broker IPCs are
also always going to be Windows only. When I add new ones, I'll ask you or
another person from security to take a look.

[Tom Sepez, 2015-11-25 18:25:40]

lgtm

[jam, 2015-11-25 18:37:27]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-11-25 18:38:39]

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

[commit-bot: I haz the power, 2015-11-25 18:48:46]

Description was changed from

==========
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259
==========

to

==========
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259
==========

[commit-bot: I haz the power, 2015-11-25 18:48:46]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-11-25 18:50:32]

Description was changed from

==========
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259
==========

to

==========
Rename mojo::TokenSerializer to mojo::Broker.

This will be used for more than just serializing tokens going forward. This
change just renames it (hopefully using established names) and compiles it on
POSIX.

BUG=556259

Committed: https://crrev.com/3c8c50cd432aaacefa681e707e3f60ccfa10855f
Cr-Commit-Position: refs/heads/master@{#361709}
==========

[commit-bot: I haz the power, 2015-11-25 18:50:33]

Patchset 7 (id:??) landed as
https://crrev.com/3c8c50cd432aaacefa681e707e3f60ccfa10855f
Cr-Commit-Position: refs/heads/master@{#361709}

[hans, 2015-11-25 20:35:41]

hans@chromium.org changed reviewers:
+ hans@chromium.org

[hans, 2015-11-25 20:35:41]

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.h:37: virtual ~ChildBrokerHost();
The Win-Clang style plugin complains about this one. It needs to be marked
override when there's a base class, i.e. when OS_WIN is defined.

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4485/step...

[jam, 2015-11-25 21:28:57]

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.h:37: virtual ~ChildBrokerHost();
On 2015/11/25 20:35:41, hans wrote:
> The Win-Clang style plugin complains about this one. It needs to be marked
> override when there's a base class, i.e. when OS_WIN is defined.
> 
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4485/step...

I'll fix it. Is there a trybot I can use to verify?

[hans, 2015-11-25 21:32:17]

On 2015/11/25 21:28:57, jam wrote:
>
https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
> mojo/edk/system/child_broker_host.h:37: virtual ~ChildBrokerHost();
> On 2015/11/25 20:35:41, hans wrote:
> > The Win-Clang style plugin complains about this one. It needs to be marked
> > override when there's a base class, i.e. when OS_WIN is defined.
> > 
> >
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4485/step...
> 
> I'll fix it. Is there a trybot I can use to verify?

I just uploaded a fix: https://codereview.chromium.org/1476143002

There is a win_clang_rel and win_clang_dbg trybot.

[Nico, 2015-11-25 22:03:33]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-11-25 22:03:33]

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
File mojo/edk/system/child_broker_host.h (right):

https://codereview.chromium.org/1465183005/diff/160001/mojo/edk/system/child_...
mojo/edk/system/child_broker_host.h:37: virtual ~ChildBrokerHost();
On 2015/11/25 21:28:57, jam wrote:
> On 2015/11/25 20:35:41, hans wrote:
> > The Win-Clang style plugin complains about this one. It needs to be marked
> > override when there's a base class, i.e. when OS_WIN is defined.
> > 
> >
>
http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4485/step...
> 
> I'll fix it. Is there a trybot I can use to verify?

win_clang_dbg, but it's opt-in and backed by a measly 3 slaves.

(I had fixed this before you renamed this, as you can see it's correct on the
LHS -- how did this get reverted?)