Group ConnectToApplication-related info into a params struct.

Group ConnectToApplication-related info into a params struct.

It makes the code easier to read and less error-prone.

Besides, this CL also:
- stops using raw ApplicationInstance pointer to refer to originator, because it becomes invalid after the originator is gone.
- removes requestor_url because orignator_identity.url serves the same purpose.

BUG=None

Committed: https://crrev.com/be2a3831785d9ad205e542ca6e9ccb52f734307f
Cr-Commit-Position: refs/heads/master@{#347213}

[yzshen1, 2015-09-02 18:26:33]

yzshen@chromium.org changed reviewers:
+ ben@chromium.org, sky@chromium.org

[yzshen1, 2015-09-02 18:28:05]

Hi, Ben and Scott.

Would you please take a look? Thanks!

[yzshen1, 2015-09-02 21:27:04]

On 2015/09/02 18:28:05, yzshen1 wrote:
> Hi, Ben and Scott.
> 
> Would you please take a look? Thanks!

FYI, I have rebased and resolved the conflicts.
Please take a look.

[yzshen1, 2015-09-02 21:30:55]

https://codereview.chromium.org/1307273004/diff/40001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1307273004/diff/40001/mojo/shell/application_...
mojo/shell/application_instance.cc:114:
params->connect_callback().Run(requesting_content_handler_id_);
Scott: I have moved this line from ConnectToClient() to here, so that it can
handle the case when we resend this request to the application manager in
OnConnectionError(). What do you think? Thanks!

[Ben Goodger (Google), 2015-09-02 22:20:33]

lgtm

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:153: // second one directly.
sgtm

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/identity.h
File mojo/shell/identity.h (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/identity.h#n...
mojo/shell/identity.h:16: * of an application to another. A typical use case of
qualifier is to control
nit: C++ comments since you're changing the comment so much

[sky, 2015-09-02 22:38:26]

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:58:
STLDeleteElements(&queued_client_requests_);
Do you need to also run the callbacks in queued_client_requests_?

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:173:
STLDeleteElements(&queued_client_requests_);
Shouldn't you remove this line now since the call on 171 takes ownership of each
item in the vector?

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_manager.h:183: const GURL& resolved_url,
Document what resolved_url is. It's not obvious why there is both a url in
params and resolved_url. In fact, why don't you set resolved_url in params?

[sky, 2015-09-02 22:48:34]

https://codereview.chromium.org/1307273004/diff/40001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1307273004/diff/40001/mojo/shell/application_...
mojo/shell/application_instance.cc:114:
params->connect_callback().Run(requesting_content_handler_id_);
On 2015/09/02 21:30:55, yzshen1 wrote:
> Scott: I have moved this line from ConnectToClient() to here, so that it can
> handle the case when we resend this request to the application manager in
> OnConnectionError(). What do you think? Thanks!

I think you are right!

[yzshen1, 2015-09-03 00:06:39]

Thanks, Ben and Scott!

Please take another look.

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:58:
STLDeleteElements(&queued_client_requests_);
On 2015/09/02 22:38:26, sky wrote:
> Do you need to also run the callbacks in queued_client_requests_?

Good catch. :)

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:173:
STLDeleteElements(&queued_client_requests_);
On 2015/09/02 22:38:26, sky wrote:
> Shouldn't you remove this line now since the call on 171 takes ownership of
each
> item in the vector?

Wow... good catch++.

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/application_...
mojo/shell/application_manager.h:183: const GURL& resolved_url,
On 2015/09/02 22:38:26, sky wrote:
> Document what resolved_url is. It's not obvious why there is both a url in
> params and resolved_url.
I have documented the meaning of resolved_url.

> In fact, why don't you set resolved_url in params?
Do you mean introducing a new field in Params? It seems unnecessary because it
is not used anywhere else.

If I set params->app_url_ to this value, it changes the logic if
params->app_url_ is "mojo:" which we want to preserve.

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/identity.h
File mojo/shell/identity.h (right):

https://codereview.chromium.org/1307273004/diff/60001/mojo/shell/identity.h#n...
mojo/shell/identity.h:16: * of an application to another. A typical use case of
qualifier is to control
On 2015/09/02 22:20:33, Ben Goodger (Google) wrote:
> nit: C++ comments since you're changing the comment so much

Done.

[sky, 2015-09-03 13:21:23]

LGTM

[yzshen1, 2015-09-03 19:19:21]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2015-09-03 19:19:21]

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

[commit-bot: I haz the power, 2015-09-03 19:19:42]

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

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

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-03 19:26:52]

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