Add Initialize() method to Application with ability to send args using

Add Initialize() method to Application with ability to send args using
new api on ApplicationManager.

BUG=

Committed: https://crrev.com/2245c392b9a37199c868213d32e73865756416d3
Cr-Commit-Position: refs/heads/master@{#295816}

[DaveMoore, 2014-09-12 22:00:44]

Add assert to ensure Initialize() gets calls exactly once and before any
connections

[DaveMoore, 2014-09-12 22:02:49]

davemoore@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[darin (slow to review), 2014-09-15 03:58:54]

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

[darin (slow to review), 2014-09-15 03:58:55]

What's the intended use case? Passing arguments to the app specified on the
command line to mojo_shell? Any other use cases?

https://codereview.chromium.org/568173003/diff/20001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/568173003/diff/20001/mojo/application_manager...
mojo/application_manager/application_manager.cc:224: std::vector<std::string>
args;
nit: allocate the Array<String> here to avoid copying args twice?

[DaveMoore, 2014-09-15 04:46:16]

Review nit

[DaveMoore, 2014-09-15 04:47:58]

This will be used with args specified to mojo_shell, but also in some test cases
where we want to configure a service to behave in specific ways, even though
that service is created by another service being tested.

https://codereview.chromium.org/568173003/diff/20001/mojo/application_manager...
File mojo/application_manager/application_manager.cc (right):

https://codereview.chromium.org/568173003/diff/20001/mojo/application_manager...
mojo/application_manager/application_manager.cc:224: std::vector<std::string>
args;
On 2014/09/15 03:58:55, darin wrote:
> nit: allocate the Array<String> here to avoid copying args twice?

Done.

[darin (slow to review), 2014-09-15 04:54:36]

On Sun, Sep 14, 2014 at 9:47 PM, <davemoore@chromium.org> wrote:

> This will be used with args specified to mojo_shell, but also in some test
> cases
> where we want to configure a service to behave in specific ways, even
> though
> that service is created by another service being tested.


Interesting. What example do you have in mind? Is the idea that there will
be a manifest somewhere (or command line flags) indicating arguments for
application URLs?

-Darin



>
>
>
> https://codereview.chromium.org/568173003/diff/20001/mojo/
> application_manager/application_manager.cc
> File mojo/application_manager/application_manager.cc (right):
>
> https://codereview.chromium.org/568173003/diff/20001/mojo/
> application_manager/application_manager.cc#newcode224
> mojo/application_manager/application_manager.cc:224:
> std::vector<std::string> args;
> On 2014/09/15 03:58:55, darin wrote:
>
>> nit: allocate the Array<String> here to avoid copying args twice?
>>
>
> Done.
>
> https://codereview.chromium.org/568173003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-09-15 17:26:19]

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_impl.h (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_impl.h:63: const Array<String>& args() {
return args_; }
Needs comment.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_impl.cc (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:58: assert(!initialized_);
You can actually use mojo/public/cpp/environment/logging.h here, and thus
MOJO_DCHECK.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:66: assert(initialized_);
"

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:82: initialized_ = false;
I don't think you need this. Maybe a MOJO_DCHECK(!initialized); instead.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/interfaces/a...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/interfaces/a...
mojo/public/interfaces/application/application.mojom:14: Initialize(string[]
args);
I'd make this optional/nullable.

[DaveMoore, 2014-09-15 22:33:41]

Address review comments

[DaveMoore, 2014-09-15 22:35:08]

One example of a case where I would like to set some arguments in tests is the
NativeViewportService. It needs to initialize with a different method
(InitializeOneOffForTests()). But it's instantiated from the ViewManager, not
directly from the test.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/application_impl.h (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/application_impl.h:63: const Array<String>& args() {
return args_; }
On 2014/09/15 17:26:18, viettrungluu wrote:
> Needs comment.

Done.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_impl.cc (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:58: assert(!initialized_);
On 2014/09/15 17:26:19, viettrungluu wrote:
> You can actually use mojo/public/cpp/environment/logging.h here, and thus
> MOJO_DCHECK.

Done.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:66: assert(initialized_);
On 2014/09/15 17:26:19, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl.cc:82: initialized_ = false;
Removed. At one point this was where I initialized it.
On 2014/09/15 17:26:19, viettrungluu wrote:
> I don't think you need this. Maybe a MOJO_DCHECK(!initialized); instead.

https://codereview.chromium.org/568173003/diff/40001/mojo/public/interfaces/a...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/568173003/diff/40001/mojo/public/interfaces/a...
mojo/public/interfaces/application/application.mojom:14: Initialize(string[]
args);
On 2014/09/15 17:26:19, viettrungluu wrote:
> I'd make this optional/nullable.

Done.

[viettrungluu, 2014-09-15 22:43:29]

LGTM (but maybe wait for Darin to weigh in also), w/suggestion.

Also, we should probably have a shell-level test that verifies that Initialize
is actually the first message on the message pipe (mainly to prevent any
regressions). You don't need to that now, but could you file on a bug for that?

https://codereview.chromium.org/568173003/diff/60001/mojo/application_manager...
File mojo/application_manager/application_manager_unittest.cc (right):

https://codereview.chromium.org/568173003/diff/60001/mojo/application_manager...
mojo/application_manager/application_manager_unittest.cc:472: // Confirm that no
arguments are sent to an application by default.
I wonder if you should test that "no arguments" means "null args" (which is
distinct from "empty array of args").

[DaveMoore, 2014-09-15 23:44:23]

On 2014/09/15 22:43:29, viettrungluu wrote:
> LGTM (but maybe wait for Darin to weigh in also), w/suggestion.
> 
> Also, we should probably have a shell-level test that verifies that Initialize
> is actually the first message on the message pipe (mainly to prevent any
> regressions). You don't need to that now, but could you file on a bug for
that?
> 
>
https://codereview.chromium.org/568173003/diff/60001/mojo/application_manager...
> File mojo/application_manager/application_manager_unittest.cc (right):
> 
>
https://codereview.chromium.org/568173003/diff/60001/mojo/application_manager...
> mojo/application_manager/application_manager_unittest.cc:472: // Confirm that
no
> arguments are sent to an application by default.
> I wonder if you should test that "no arguments" means "null args" (which is
> distinct from "empty array of args").

I don't want the clients of ApplicationImpl to care. 0 args == null args.

[viettrungluu, 2014-09-16 00:32:58]

This is a test of the shell code, not of the client library.
On Sep 15, 2014 4:44 PM, <davemoore@chromium.org> wrote:

> On 2014/09/15 22:43:29, viettrungluu wrote:
>
>> LGTM (but maybe wait for Darin to weigh in also), w/suggestion.
>>
>
>  Also, we should probably have a shell-level test that verifies that
>> Initialize
>> is actually the first message on the message pipe (mainly to prevent any
>> regressions). You don't need to that now, but could you file on a bug for
>>
> that?
>
>
> https://codereview.chromium.org/568173003/diff/60001/mojo/
> application_manager/application_manager_unittest.cc
>
>> File mojo/application_manager/application_manager_unittest.cc (right):
>>
>
>
> https://codereview.chromium.org/568173003/diff/60001/mojo/
> application_manager/application_manager_unittest.cc#newcode472
>
>> mojo/application_manager/application_manager_unittest.cc:472: // Confirm
>> that
>>
> no
>
>> arguments are sent to an application by default.
>> I wonder if you should test that "no arguments" means "null args" (which
>> is
>> distinct from "empty array of args").
>>
>
> I don't want the clients of ApplicationImpl to care. 0 args == null args.
>
> https://codereview.chromium.org/568173003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-09-18 18:10:01]

I don't think you need to null check an Array before calling .Pass() on it.

Otherwise LGTM

[DaveMoore, 2014-09-18 19:28:16]

Review nit

[DaveMoore, 2014-09-18 19:28:27]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-09-18 19:29:45]

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

[commit-bot: I haz the power, 2014-09-18 20:46:24]

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

[commit-bot: I haz the power, 2014-09-18 20:46:25]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Aaron Boodman, 2014-09-19 00:37:27]

It just occurred to me that for the command line case, it might be cool to
just use URLs:

./mojo_shell mojo://mojo_demo_launcher?foo=bar&hot=dog

We can pass the requestor_url to the application as an additional param to
AcceptConnection().

On Thu, Sep 18, 2014 at 1:46 PM, <commit-bot@chromium.org> wrote:

> Try jobs failed on following builders:
>   win_chromium_rel_swarming on tryserver.chromium.win
> (http://build.chromium.org/p/tryserver.chromium.win/
> builders/win_chromium_rel_swarming/builds/13978)
>
> https://codereview.chromium.org/568173003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-09-19 01:05:48]

Nevermind, Dave just told me in person this doesn't work for his use case.

I still think something like this is a cool idea, but I need to think about
it some more to decide whether I want to propose this particular API or
something different.

- a

On Thu, Sep 18, 2014 at 5:36 PM, Aaron Boodman <aa@chromium.org> wrote:

> It just occurred to me that for the command line case, it might be cool to
> just use URLs:
>
> ./mojo_shell mojo://mojo_demo_launcher?foo=bar&hot=dog
>
> We can pass the requestor_url to the application as an additional param to
> AcceptConnection().
>
> On Thu, Sep 18, 2014 at 1:46 PM, <commit-bot@chromium.org> wrote:
>
>> Try jobs failed on following builders:
>>   win_chromium_rel_swarming on tryserver.chromium.win
>> (http://build.chromium.org/p/tryserver.chromium.win/
>> builders/win_chromium_rel_swarming/builds/13978)
>>
>> https://codereview.chromium.org/568173003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-09-19 05:43:40]

Yeah, it is a very interesting idea. We have talked about the idea of an
app at URL X claiming to handle a subpath of that URL. So it could make
sense for mojo://mojo_demo_launcher?foo=bar&hot=dog to somehow indicate
that its base URL is mojo://mojo_demo_launcher, which would allow for the
same app instance to handle multiple variations of parameters. (In this
case it could be as simple as saying URL minus query parameters indicates
the base URL. For HTTP URLs, there could be a response header.)

On Thu, Sep 18, 2014 at 6:05 PM, Aaron Boodman <aa@chromium.org> wrote:

> Nevermind, Dave just told me in person this doesn't work for his use case.
>
> I still think something like this is a cool idea, but I need to think
> about it some more to decide whether I want to propose this particular API
> or something different.
>
> - a
>
> On Thu, Sep 18, 2014 at 5:36 PM, Aaron Boodman <aa@chromium.org> wrote:
>
>> It just occurred to me that for the command line case, it might be cool
>> to just use URLs:
>>
>> ./mojo_shell mojo://mojo_demo_launcher?foo=bar&hot=dog
>>
>> We can pass the requestor_url to the application as an additional param
>> to AcceptConnection().
>>
>> On Thu, Sep 18, 2014 at 1:46 PM, <commit-bot@chromium.org> wrote:
>>
>>> Try jobs failed on following builders:
>>>   win_chromium_rel_swarming on tryserver.chromium.win
>>> (http://build.chromium.org/p/tryserver.chromium.win/
>>> builders/win_chromium_rel_swarming/builds/13978)
>>>
>>> https://codereview.chromium.org/568173003/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaveMoore, 2014-09-19 21:52:31]

Rebase

[DaveMoore, 2014-09-19 21:53:01]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-09-19 21:54:54]

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

[commit-bot: I haz the power, 2014-09-19 21:54:55]

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

[commit-bot: I haz the power, 2014-09-19 23:08:23]

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

[commit-bot: I haz the power, 2014-09-19 23:08:25]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[DaveMoore, 2014-09-19 23:16:25]

The CQ bit was checked by davemoore@chromium.org

[commit-bot: I haz the power, 2014-09-19 23:18:46]

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

[commit-bot: I haz the power, 2014-09-19 23:55:40]

Committed patchset #6 (id:100001) as 1c4ad409f8796c9113288c35a5740fe4702a2f33

[commit-bot: I haz the power, 2014-09-19 23:56:27]

Patchset 6 (id:??) landed as
https://crrev.com/2245c392b9a37199c868213d32e73865756416d3
Cr-Commit-Position: refs/heads/master@{#295816}