Remove ShellConnection::WaitForInitialize

Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than a ConnectorPtr being passed in with the call.
This allows clients to create their own Connector
pipe and lazily connect it to the shell thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first
   Connector pipe error: instead an Instance is
   kept alive by the shell as long as either the
   ShellClient pipe is connected OR any Connector
   pipes are connected.

 - ShellClientFactory endpoints are now strongly
   typed instead of being raw message pipe
   handles.

 - Some uses of MessagePumpMojo near other changes
   in this CL have been opportunistically removed.

BUG=591742
Committed: https://crrev.com/35f7270e84da5abb0f7895304a53d632efbbfe71
Cr-Commit-Position: refs/heads/master@{#380909}

[Ken Rockot(use gerrit already), 2016-03-12 18:11:44]

Description was changed from

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than passing a Connector to the client.

This allows clients to create their own Connector
pipe and lazily connect it to the shell, thus
avoiding any practical need to wait for an
Initialize message.

Additionally, client instances no longer die on
the first Connector pipe error: instead an Instance
is kept alive by the shell as long as either the
ShellClient pipe is connected OR any Connector
pipes are connected.

BUG=None
==========

to

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than passing a Connector to the client.

This allows clients to create their own Connector
pipe and lazily connect it to the shell, thus
avoiding any practical need to wait for an
Initialize message.

Additionally, client instances no longer die on
the first Connector pipe error: instead an Instance
is kept alive by the shell as long as either the
ShellClient pipe is connected OR any Connector
pipes are connected.

BUG=591742
==========

[Ken Rockot(use gerrit already), 2016-03-12 20:04:07]

Description was changed from

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than passing a Connector to the client.

This allows clients to create their own Connector
pipe and lazily connect it to the shell, thus
avoiding any practical need to wait for an
Initialize message.

Additionally, client instances no longer die on
the first Connector pipe error: instead an Instance
is kept alive by the shell as long as either the
ShellClient pipe is connected OR any Connector
pipes are connected.

BUG=591742
==========

to

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than passing a Connector to the client.

This allows clients to create their own Connector
pipe and lazily connect it to the shell, thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first Connector
   pipe error: instead an Instance is kept alive by the
   shell as long as either the ShellClient pipe is
   connected OR any Connector pipes are connected.

 - ShellClientFactory endpoints are now strongly typed
   instead of being raw message pipe handles.

 - Some uses of MessagePumpMojo near other changes in
   this CL have been opportunistically removed.

BUG=591742
==========

[Ken Rockot(use gerrit already), 2016-03-12 20:05:33]

Description was changed from

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than passing a Connector to the client.

This allows clients to create their own Connector
pipe and lazily connect it to the shell, thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first Connector
   pipe error: instead an Instance is kept alive by the
   shell as long as either the ShellClient pipe is
   connected OR any Connector pipes are connected.

 - ShellClientFactory endpoints are now strongly typed
   instead of being raw message pipe handles.

 - Some uses of MessagePumpMojo near other changes in
   this CL have been opportunistically removed.

BUG=591742
==========

to

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than a ConnectorPtr being passed in with the call.
This allows clients to create their own Connector
pipe and lazily connect it to the shell thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first
   Connector pipe error: instead an Instance is
   kept alive by the shell as long as either the
   ShellClient pipe is connected OR any Connector
   pipes are connected.

 - ShellClientFactory endpoints are now strongly
   typed instead of being raw message pipe
   handles.

 - Some uses of MessagePumpMojo near other changes
   in this CL have been opportunistically removed.

BUG=591742
==========

[Ken Rockot(use gerrit already), 2016-03-12 20:10:17]

rockot@chromium.org changed reviewers:
+ ben@chromium.org

[Ken Rockot(use gerrit already), 2016-03-12 20:10:18]

This is really all we need to do to prevent clients from blocking. I think if we
want to pass Identity (and maybe instance ID) to MojoMain we can make that
change separately.

[Ben Goodger (Google), 2016-03-13 00:58:33]

I need to study this some more... will write more later

One observation I have... do we need to pass SCF still, or could it just be a
SC&?

https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
content/child/child_thread_impl.cc:419: switches::kMojoPrimordialPipeToken);
since you're passing this on the command line too can you consolidate this with
the flag the shell uses for the processes it launches & then just get rid of the
parameter to Connect()?

https://codereview.chromium.org/1793793002/diff/20001/mojo/shell/public/inter...
File mojo/shell/public/interfaces/shell_client.mojom (right):

https://codereview.chromium.org/1793793002/diff/20001/mojo/shell/public/inter...
mojo/shell/public/interfaces/shell_client.mojom:36: //    initialized client to
connect to others.
Why optional?

[Ken Rockot(use gerrit already), 2016-03-13 01:35:38]

On 2016/03/13 at 00:58:33, ben wrote:
> I need to study this some more... will write more later
> 
> One observation I have... do we need to pass SCF still, or could it just be a
SC&?

Where?

> 
>
https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
> File content/child/child_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
> content/child/child_thread_impl.cc:419: switches::kMojoPrimordialPipeToken);
> since you're passing this on the command line too can you consolidate this
with the flag the shell uses for the processes it launches & then just get rid
of the parameter to Connect()?

Sure, I'll do that

> 
>
https://codereview.chromium.org/1793793002/diff/20001/mojo/shell/public/inter...
> File mojo/shell/public/interfaces/shell_client.mojom (right):
> 
>
https://codereview.chromium.org/1793793002/diff/20001/mojo/shell/public/inter...
> mojo/shell/public/interfaces/shell_client.mojom:36: //    initialized client
to connect to others.
> Why optional?

Some clients don't care about having a Connector, they just want to expose
services. Because a ShellClient pipe can also now keep an instance alive on its
own, a Connector is no longer strictly necessary; so it might as well be
optional.

Of course with ShellConnection being the only user now it doesn't really matter,
but I think the interfaces should only be as constrained as they need to be.

[Ben Goodger (Google), 2016-03-13 04:16:24]

Re the SC&... what I meant was perhaps the primordial pipe could *be* the
ShellClient pipe.

Right now it's the ShellClientFactory, which was due to the vestigial
infrastructure... but in practice we never call CreateShellClient more than
once, so I'm wondering if the server handle of the ShellClient pipe couldn't be
sent thru instead. That seems like it'd streamline/eliminate the need for
RunnerConnection almost.

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
content/common/mojo/mojo_shell_connection_impl.cc:52:
connection->shell_connection_->BindToRequest(std::move(request));
How is this different than just passing request thru the ctor?

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
File content/common/mojo/mojo_shell_connection_impl.h (right):

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
content/common/mojo/mojo_shell_connection_impl.h:48: void
Connect(mojo::shell::mojom::ShellClientFactoryRequest request);
I think this name is confusing. Generally we use the "connect" verb to imply
establishing a connection to a remote service, which is not an idempotent
action.

https://codereview.chromium.org/1793793002/diff/40001/mojo/shell/public/cpp/s...
File mojo/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1793793002/diff/40001/mojo/shell/public/cpp/s...
mojo/shell/public/cpp/shell_connection.h:66: // TODO(rockot): Remove this once
we get rid of app tests.
I sent a note to chromium-mojo deprecating apptests. I'll take a look at
converting a few of the suites next week.

[Ken Rockot(use gerrit already), 2016-03-13 16:14:25]

I agree that removing SCF is a good idea. I started making that change and it
unsurprisingly deletes lots of code, but it turns into more of a yak than I'd
like to shave here. How about I do that in a follow-up patch?

https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/1793793002/diff/20001/content/child/child_thr...
content/child/child_thread_impl.cc:419: switches::kMojoPrimordialPipeToken);
On 2016/03/13 at 00:58:33, Ben Goodger (Google) wrote:
> since you're passing this on the command line too can you consolidate this
with the flag the shell uses for the processes it launches & then just get rid
of the parameter to Connect()?

Done

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
content/common/mojo/mojo_shell_connection_impl.cc:52:
connection->shell_connection_->BindToRequest(std::move(request));
On 2016/03/13 at 04:16:24, Ben Goodger (Google) wrote:
> How is this different than just passing request thru the ctor?

Oops, it's not. Fixed

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
File content/common/mojo/mojo_shell_connection_impl.h (right):

https://codereview.chromium.org/1793793002/diff/40001/content/common/mojo/moj...
content/common/mojo/mojo_shell_connection_impl.h:48: void
Connect(mojo::shell::mojom::ShellClientFactoryRequest request);
On 2016/03/13 at 04:16:24, Ben Goodger (Google) wrote:
> I think this name is confusing. Generally we use the "connect" verb to imply
establishing a connection to a remote service, which is not an idempotent
action.

I've changed it to BindToRequestFromCommandLine for now. I think we can get rid
of this soon and roll the behavior into Create(), it would just require changing
more than I'd like to change for this patch.

https://codereview.chromium.org/1793793002/diff/40001/mojo/shell/public/cpp/s...
File mojo/shell/public/cpp/shell_connection.h (right):

https://codereview.chromium.org/1793793002/diff/40001/mojo/shell/public/cpp/s...
mojo/shell/public/cpp/shell_connection.h:66: // TODO(rockot): Remove this once
we get rid of app tests.
On 2016/03/13 at 04:16:24, Ben Goodger (Google) wrote:
> I sent a note to chromium-mojo deprecating apptests. I'll take a look at
converting a few of the suites next week.

Cool, I was going to try to do some conversions too

[Ken Rockot(use gerrit already), 2016-03-13 16:19:10]

https://codereview.chromium.org/1793793002/diff/60001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1793793002/diff/60001/content/browser/browser...
content/browser/browser_main_loop.cc:917:
mojo::edk::SetParentPipeHandleFromCommandLine();
Also note: I added this API for convenience since sharing a common init path
through RunnerConnection meant ripping this magic out from it. We don't always
use the command line handle for content children.

[Ben Goodger (Google), 2016-03-13 22:17:12]

ok.

lgtm

[Ken Rockot(use gerrit already), 2016-03-13 23:17:53]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-03-13 23:18:07]

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

[commit-bot: I haz the power, 2016-03-13 23:42:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-13 23:43:53]

Description was changed from

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than a ConnectorPtr being passed in with the call.
This allows clients to create their own Connector
pipe and lazily connect it to the shell thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first
   Connector pipe error: instead an Instance is
   kept alive by the shell as long as either the
   ShellClient pipe is connected OR any Connector
   pipes are connected.

 - ShellClientFactory endpoints are now strongly
   typed instead of being raw message pipe
   handles.

 - Some uses of MessagePumpMojo near other changes
   in this CL have been opportunistically removed.

BUG=591742
==========

to

==========
Remove ShellConnection::WaitForInitialize

This changes ShellClient::Initialize to expect a
response with an optional Connector request rather
than a ConnectorPtr being passed in with the call.
This allows clients to create their own Connector
pipe and lazily connect it to the shell thus
avoiding any practical need to wait for an
Initialize message.

Additionally:

 - Client instances no longer die on the first
   Connector pipe error: instead an Instance is
   kept alive by the shell as long as either the
   ShellClient pipe is connected OR any Connector
   pipes are connected.

 - ShellClientFactory endpoints are now strongly
   typed instead of being raw message pipe
   handles.

 - Some uses of MessagePumpMojo near other changes
   in this CL have been opportunistically removed.

BUG=591742

Committed: https://crrev.com/35f7270e84da5abb0f7895304a53d632efbbfe71
Cr-Commit-Position: refs/heads/master@{#380909}
==========

[commit-bot: I haz the power, 2016-03-13 23:43:54]

Patchset 5 (id:??) landed as
https://crrev.com/35f7270e84da5abb0f7895304a53d632efbbfe71
Cr-Commit-Position: refs/heads/master@{#380909}