Added anti-singleton services (new process/thread per connection).

Added anti-singleton services (new process/thread per connection).

TEST=`./out/Debug/mojo_shell --enable-multiprocess ./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/78eb28d26d57dc51e7ff15c8c2cbeea19980297b

[Sean Klein, 2015-11-13 01:16:30]

Patchset #2 (id:20001) has been deleted

[Sean Klein, 2015-11-13 01:47:45]

Patchset #2 (id:40001) has been deleted

[Sean Klein, 2015-11-13 01:48:05]

Description was changed from

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Clean logging statements
- Clean crappy code
- Add unit test
- Look into moving options somewhere else?

BUG=#396
==========

to

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Clean crappy code
- Fix broken tests
- Add unit test
- Look into moving options somewhere else?

BUG=#396
==========

[Sean Klein, 2015-11-13 02:47:54]

Patchset #2 (id:60001) has been deleted

[Sean Klein, 2015-11-13 02:48:58]

Description was changed from

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Clean crappy code
- Fix broken tests
- Add unit test
- Look into moving options somewhere else?

BUG=#396
==========

to

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Add unit test
- Look into moving options somewhere else?

BUG=#396
==========

[Sean Klein, 2015-11-13 02:59:14]

Description was changed from

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Add unit test
- Look into moving options somewhere else?

BUG=#396
==========

to

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Add unit test
- Look into moving options somewhere else?

TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
==========

[Sean Klein, 2015-11-13 03:26:54]

Patchset #2 (id:80001) has been deleted

[Sean Klein, 2015-11-13 03:31:03]

Patchset #3 (id:120001) has been deleted

[Sean Klein, 2015-11-13 03:32:13]

Description was changed from

==========
Added anti-singleton services (new process/thread per connection).

Super hacky right now but technically working.

TODO:
- Add unit test
- Look into moving options somewhere else?

TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
==========

to

==========
Added anti-singleton services (new process/thread per connection).

I would like to add a unit test, but first I need a better mechanism
to add options.


TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
==========

[Sean Klein, 2015-11-13 03:40:41]

Patchset #3 (id:140001) has been deleted

[Sean Klein, 2015-11-13 03:55:04]

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

[Sean Klein, 2015-11-13 03:55:04]

PTAL

I'm currently selecting the "unique process" eligible applications in
InitNativeOptions, from context.cc.

As you said (on line 193), "This is a total hack". Suggestions for a better
location for this configuration?

[viettrungluu, 2015-11-13 18:41:27]

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:7: #include
"shell/application_manager/application_manager.h"
This include should go *first* (see the style guide).

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:49: std::string
create_id_string(uint64_t uid) {
Nit: Functions should be NamedLikeThis.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:52: CHECK_EQ(20,
snprintf(buffer, sizeof(buffer), "%020" PRIu64, uid));
Probably you should just use one of the functions in
base/strings/string_number_conversions.h. I wouldn't worry too much about it
having a fixed length, but if you really want you could use HexEncode().

(Note that using snprintf() might make you sad, since its output is locale
dependent!)

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:79: Identity get_identity() {
return id_; }
nit: Getters of this type usually omit the word "get", so this should be
|identity()|. But since getters of this type are usually named in fairly direct
correspondence to the member variable, you should probably either rename |id_|
to |identity_| or this to |id()|.

Also, this method should be const, and return a |const Identity&|. (Ditto for
|content_handler_url()|, I guess.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:83: Identity id_;
nit: Maybe this should be const.

(And I guess the above should be |ApplicationManager* const manager_;|.)

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:242:
DCHECK(!this->GetNativeApplicationOptionsForURL(application_url)
No need for "this->" (here and elsewhere).

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:269: static uint64_t uid = 1;
nit: Calling it "uid" is a little strange.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:273: if (force_unique_process)
{
Style nits:
- In this file/directory, we omit braces for single-line blocks.
- It's fairly conventional to not have an |else| if the |if| block ends with a
|return|.
- But in this case, I'd just do |return force_unique_process ? Identity(...) :
Identity(...);|.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
File shell/application_manager/native_application_options.h (right):

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/native_application_options.h:15: bool
force_unique_process = false;
Terminology bikeshed: I think "unique" is rather confusing in this context --
one may well think that it means that there's a unique process for the given
application/URL (i.e., means "force singleton").

Maybe it should be "process_per_connection"? (Or "new_process_per_connection"?)

[viettrungluu, 2015-11-13 18:44:33]

On 2015/11/13 03:55:04, smklein1 wrote:
> PTAL
> 
> I'm currently selecting the "unique process" eligible applications in
> InitNativeOptions, from context.cc.
> 
> As you said (on line 193), "This is a total hack". Suggestions for a better
> location for this configuration?

I don't have a good short-term solution, and the long-term solution hasn't been
designed yet.

It's not clear who/what gets to decide various properties of each application.
E.g., some may be decided "by" the application itself (e.g., via flags in the
binary or a manifest file alongside the binary) and others may have to be
configured by the shell (using some sort of config file? baked-in in some way?).

So I wouldn't worry about that too much right now.

For testing purposes, you might consider adding a command-line flag to allow
this to be set for a particular app (e.g., your test app).

[Sean Klein, 2015-11-16 02:24:22]

Where do you think the test for this CL should exist?
application_manager_unittest.cc? Or somewhere else in shell/?

[Sean Klein, 2015-11-16 02:24:27]

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:7: #include
"shell/application_manager/application_manager.h"
On 2015/11/13 18:41:26, viettrungluu wrote:
> This include should go *first* (see the style guide).

Done (well, removed, actually, since no longer used).

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:49: std::string
create_id_string(uint64_t uid) {
On 2015/11/13 18:41:26, viettrungluu wrote:
> Nit: Functions should be NamedLikeThis.

Done (well, also removed)

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:52: CHECK_EQ(20,
snprintf(buffer, sizeof(buffer), "%020" PRIu64, uid));
On 2015/11/13 18:41:26, viettrungluu wrote:
> Probably you should just use one of the functions in
> base/strings/string_number_conversions.h. I wouldn't worry too much about it
> having a fixed length, but if you really want you could use HexEncode().
> 
> (Note that using snprintf() might make you sad, since its output is locale
> dependent!)

Updating to "Uint64ToString", which eliminates the need for the "CreateIdString"
function. Good call on using base.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:79: Identity get_identity() {
return id_; }
On 2015/11/13 18:41:26, viettrungluu wrote:
> nit: Getters of this type usually omit the word "get", so this should be
> |identity()|. But since getters of this type are usually named in fairly
direct
> correspondence to the member variable, you should probably either rename |id_|
> to |identity_| or this to |id()|.
> 
> Also, this method should be const, and return a |const Identity&|. (Ditto for
> |content_handler_url()|, I guess.

Done (renamed to identity, added const, returning references).

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:83: Identity id_;
On 2015/11/13 18:41:26, viettrungluu wrote:
> nit: Maybe this should be const.
> 
> (And I guess the above should be |ApplicationManager* const manager_;|.)

Done.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:242:
DCHECK(!this->GetNativeApplicationOptionsForURL(application_url)
On 2015/11/13 18:41:26, viettrungluu wrote:
> No need for "this->" (here and elsewhere).

Done.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:269: static uint64_t uid = 1;
On 2015/11/13 18:41:26, viettrungluu wrote:
> nit: Calling it "uid" is a little strange.

Renaming to "unique_id_number", since it is intended to be a unique
identification number.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/application_manager.cc:273: if (force_unique_process)
{
On 2015/11/13 18:41:26, viettrungluu wrote:
> Style nits:
> - In this file/directory, we omit braces for single-line blocks.
> - It's fairly conventional to not have an |else| if the |if| block ends with a
> |return|.
> - But in this case, I'd just do |return force_unique_process ? Identity(...) :
> Identity(...);|.

Done.

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
File shell/application_manager/native_application_options.h (right):

https://codereview.chromium.org/1444433002/diff/160001/shell/application_mana...
shell/application_manager/native_application_options.h:15: bool
force_unique_process = false;
On 2015/11/13 18:41:27, viettrungluu wrote:
> Terminology bikeshed: I think "unique" is rather confusing in this context --
> one may well think that it means that there's a unique process for the given
> application/URL (i.e., means "force singleton").
> 
> Maybe it should be "process_per_connection"? (Or
"new_process_per_connection"?)

Updating to "new_process_per_connection". I think think that unambiguously
describes the "anti-singleton" effect.

[viettrungluu, 2015-11-16 17:13:32]

lgtm w/nit

https://codereview.chromium.org/1444433002/diff/180001/shell/application_mana...
File shell/application_manager/application_manager.h (right):

https://codereview.chromium.org/1444433002/diff/180001/shell/application_mana...
shell/application_manager/application_manager.h:178: Identity
MakeApplicationIdentity(const GURL& resolved_url);
nit: Existing bad precedent notwithstanding, please add a short comment
describing what this method does.

[Sean Klein, 2015-11-16 17:26:32]

https://codereview.chromium.org/1444433002/diff/180001/shell/application_mana...
File shell/application_manager/application_manager.h (right):

https://codereview.chromium.org/1444433002/diff/180001/shell/application_mana...
shell/application_manager/application_manager.h:178: Identity
MakeApplicationIdentity(const GURL& resolved_url);
On 2015/11/16 17:13:32, viettrungluu wrote:
> nit: Existing bad precedent notwithstanding, please add a short comment
> describing what this method does.

Done.

[Sean Klein, 2015-11-17 02:07:38]

Added a multiprocess apptest.

[Sean Klein, 2015-11-17 02:07:50]

Description was changed from

==========
Added anti-singleton services (new process/thread per connection).

I would like to add a unit test, but first I need a better mechanism
to add options.


TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
==========

to

==========
Added anti-singleton services (new process/thread per connection).

TEST=`./out/Debug/mojo_shell --enable-multiprocess
./out/Debug/spinning_cube.pexe.mojo ./out/Debug/echo_client.pexe.mojo`
BUG=#396
==========

[Sean Klein, 2015-11-17 18:15:17]

Committed patchset #6 (id:220001) manually as
78eb28d26d57dc51e7ff15c8c2cbeea19980297b (presubmit successful).