Update client side navigator.connect API to use ServicePortCollection [2/3]

Update client side navigator.connect API to use ServicePortCollection [2/3]

This is part of a series of patches to update the client side API of what was
formerly known as navigator.connect to follow the new spec based on
navigator.services and a new ServicePort type.

This patch updates the content side code to use mojo for most of its IPC,
although still relying somewhat on MessagePort since the existing service side
code uses MessagePort concepts. A future patch series will also adjust the service
side API to use ServicePort and get rid of the dependency on MessagePort.

[1/3] https://codereview.chromium.org/1191393003 Blink side changes
[2/3] https://codereview.chromium.org/1192713004 This patch
[3/3] https://codereview.chromium.org/1198653004/
      Cleanup and removing old code paths on the blink side and in tests.

BUG=426458
Committed: https://crrev.com/d4ff9b576f5a9c1a0d3352db498fe222b69725cb
Cr-Commit-Position: refs/heads/master@{#336636}

[Marijn Kruisselbrink, 2015-06-22 17:41:36]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:43]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:41:52]

Patchset #1 (id:40001) has been deleted

[Marijn Kruisselbrink, 2015-06-22 17:48:21]

mek@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-06-24 05:20:06]

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
File content/browser/navigator_connect/service_port_service_impl.cc (right):

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:98: int32_t
port_id,
port_id should somewhere be verified or by implementation details be limited to
only work for ports that the renderer has a relationship with. At this point in
review I'm not clear on where that is. Probably add a comment in this method
explaining how security works for limiting access to ports.

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:105: // First
call QueueMessages for all transferred ports, since the ports haven't
First, ...

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:106: // send
their own IPC for that.
grammar fix, send->sent I think?

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:111: // Second
pass of the actual to MessagePortService now ServicePort instances
Second, pass ...

https://codereview.chromium.org/1192713004/diff/80001/content/common/service_...
File content/common/service_port_service.mojom (right):

https://codereview.chromium.org/1192713004/diff/80001/content/common/service_...
content/common/service_port_service.mojom:7: enum ServicePortError {
SUCCESS is a strange value for an error.

How about something like ServicePortResult, ServicePortStatus,
ServicePortConnectResult, ServicePortConnectStatus?

[mahat, 2015-06-24 05:26:36]

mahatmawlit@gmail.com changed reviewers:
+ mahatmawlit@gmail.com

[mahat, 2015-06-24 05:26:37]

i need help

[Marijn Kruisselbrink, 2015-06-24 17:33:54]

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
File content/browser/navigator_connect/service_port_service_impl.cc (right):

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:98: int32_t
port_id,
On 2015/06/24 at 05:20:06, scheib wrote:
> port_id should somewhere be verified or by implementation details be limited
to only work for ports that the renderer has a relationship with. At this point
in review I'm not clear on where that is. Probably add a comment in this method
explaining how security works for limiting access to ports.

This is kind of similar to the existing problems with message ports
(http://crbug.com/490222) where there currently are no checks for any of this,
and it's trivially easy for any renderer to transfer any message port to itself
to claim full ownership of it. It's a bit less severe here since this code isn't
going to support transferring of ports, which also should make it easier to add
these checks. It's just a bit tricky to do while ServicePort still is built on
top of MessagePort, but I've added a TODO to make sure this gets addressed.

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:105: // First
call QueueMessages for all transferred ports, since the ports haven't
On 2015/06/24 at 05:20:06, scheib wrote:
> First, ...

Done

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:106: // send
their own IPC for that.
On 2015/06/24 at 05:20:06, scheib wrote:
> grammar fix, send->sent I think?

Done

https://codereview.chromium.org/1192713004/diff/80001/content/browser/navigat...
content/browser/navigator_connect/service_port_service_impl.cc:111: // Second
pass of the actual to MessagePortService now ServicePort instances
On 2015/06/24 at 05:20:06, scheib wrote:
> Second, pass ...

Done

https://codereview.chromium.org/1192713004/diff/80001/content/common/service_...
File content/common/service_port_service.mojom (right):

https://codereview.chromium.org/1192713004/diff/80001/content/common/service_...
content/common/service_port_service.mojom:7: enum ServicePortError {
On 2015/06/24 at 05:20:06, scheib wrote:
> SUCCESS is a strange value for an error.
> 
> How about something like ServicePortResult, ServicePortStatus,
ServicePortConnectResult, ServicePortConnectStatus?

Agreed, changed it to ServicePortConnectResult

[scheib, 2015-06-24 23:06:10]

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

[scheib, 2015-06-24 23:06:11]

LGTM

+tsepez, new .mojom file. IIRC chromium mojo design docs called for OWNERS
per-file for .mojom files? Unsure what the resolution was.

[Marijn Kruisselbrink, 2015-06-25 06:19:22]

The CQ bit was checked by mek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-25 09:01:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192713004/100001

[commit-bot: I haz the power, 2015-06-25 09:25:19]

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

[commit-bot: I haz the power, 2015-06-25 09:25:20]

Dry run: Exceeded global retry quota

[Marijn Kruisselbrink, 2015-06-25 16:56:35]

mek@chromium.org changed reviewers:
+ avi@chromium.org
- mahatmawlit@gmail.com

[Marijn Kruisselbrink, 2015-06-25 16:56:36]

tsepez: could you take a look for IPC/mojo OWNERS (you're the only OWNER for
mojom files it seems)

+avi: for content/ OWNERS review

[Avi (use Gerrit), 2015-06-25 18:21:42]

lgtm

stampity stamp

[Tom Sepez, 2015-06-29 18:31:19]

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
File content/common/service_port_service.mojom (right):

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
content/common/service_port_service.mojom:23: PostMessage(int32 port_id, string
message, array<MojoTransferredMessagePort> ports);
nit: 80 cols.

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
content/common/service_port_service.mojom:28: PostMessage(int32 port_id, string
message, array<MojoTransferredMessagePort> ports, array<int32> new_routing_ids);
nit: here too.

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
content/common/service_port_service.mojom:29: };
do the elements of the two arrays correspond to each other?  If so, it's usually
better to send a single array of structs rather than two parallel arrays, so you
don't have to worry about size mismatches between the two.  Otherwise OK.

https://codereview.chromium.org/1192713004/diff/160001/content/content_common...
File content/content_common.gypi (left):

https://codereview.chromium.org/1192713004/diff/160001/content/content_common...
content/content_common.gypi:452: 'common/navigator_connect_messages.h',
If we're killing this file, there is an enum constant that can be removed as
well in ipc/ipc_message_start.h

[Marijn Kruisselbrink, 2015-06-29 19:33:39]

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
File content/common/service_port_service.mojom (right):

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
content/common/service_port_service.mojom:23: PostMessage(int32 port_id, string
message, array<MojoTransferredMessagePort> ports);
On 2015/06/29 at 18:31:19, Tom Sepez (present) wrote:
> nit: 80 cols.

Done.

If that's the required style, somebody should probably add a "mojom" section to
https://www.chromium.org/developers/coding-style
Currently it's pretty unclear what style guide would apply for mojom files. IDL
seems like a logical choice (and doesn't have an 80 col limit currently),
although mojom is of course different from webidl (but even more different than
C++, so it isn't obvious that the c++ style guide would rule here).

https://codereview.chromium.org/1192713004/diff/160001/content/common/service...
content/common/service_port_service.mojom:29: };
On 2015/06/29 at 18:31:19, Tom Sepez (present) wrote:
> do the elements of the two arrays correspond to each other?  If so, it's
usually better to send a single array of structs rather than two parallel
arrays, so you don't have to worry about size mismatches between the two. 
Otherwise OK.

Yes, the elements correspond to each other. I thought about adding the
new_routing_id to the TransferredMessagePort struct when I introduced that
struct a while ago, but at the time decided against it, since the field only is
meaningful in messages send from the browser to the renderer. Messages that go
the other way never would never need to include it (an alternative would of
course be to have arrays of different structs for messages in either direction,
but that would require more annoying conversion code to pass on a message from
one renderer to the next). 
Maybe that decision was wrong and it would be cleaner to have all attributes in
the same struct and just not use the new_routing_id field for messages to the
browser, but changing that should probably change it all over the place (there
are various IPCs that currently all pass a vector<TransferredMessagePort> and a
vector<int>, and all code on both sides is currently setup to deal with that),
and not just change it here (requiring code on both sides to combine/split the
arrays).

So if you think I should change this to merge new_routing_id into
TransferredMessagePort, I'll gladly do so as a follow-up CL, but I'd prefer to
do that separately.

https://codereview.chromium.org/1192713004/diff/160001/content/content_common...
File content/content_common.gypi (left):

https://codereview.chromium.org/1192713004/diff/160001/content/content_common...
content/content_common.gypi:452: 'common/navigator_connect_messages.h',
On 2015/06/29 at 18:31:19, Tom Sepez (present) wrote:
> If we're killing this file, there is an enum constant that can be removed as
well in ipc/ipc_message_start.h

Done

[Tom Sepez, 2015-06-29 19:37:35]

> Maybe that decision was wrong and it would be cleaner to have all attributes
in
> the same struct and just not use the new_routing_id field for messages to the
> browser, but changing that should probably change it all over the place (there
> are various IPCs that currently all pass a vector<TransferredMessagePort> and
a
> vector<int>, and all code on both sides is currently setup to deal with that),
> and not just change it here (requiring code on both sides to combine/split the
> arrays).
Since its a browser -> renderer message, it's OK to leave it.  If it were the
other way around, we'd like to fix it.

LGTM

[Marijn Kruisselbrink, 2015-06-29 20:28:49]

The CQ bit was checked by mek@chromium.org

[Marijn Kruisselbrink, 2015-06-29 20:28:50]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1192713004/#ps180001
(title: "address comments")

[commit-bot: I haz the power, 2015-06-29 20:29:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192713004/180001

[commit-bot: I haz the power, 2015-06-29 21:02:32]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-06-29 21:03:33]

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