Standalone Mojo Javascript application

Standalone Mojo Javascript application

Refactored the JSApp et al classes to enable launching apps with the content launcher (as before) as well as launching a single standalone JS app.

The content handler class has been moved to its own file. The part of the JSApp Start() method that loads the initial script is now an abstract method called Load(). There are content handler and standalone versions of JSApp::Load().

The code that's specific to the standalone case is now in standalone_main.cc. The code that's specific to the content handler case is in content_handler_main.cc and content_handler_impl.{h,cc}.

Currently the standalone app's filename is wired into standalone_main.cc.  Once crbug.com/418797 has been fixed the filename can be passed through mojo_shell.

BUG=417530
Committed: https://crrev.com/06c28d7358731ae3a7788656a7c7468b813a4b3e
Cr-Commit-Position: refs/heads/master@{#297675}

[hansmuller, 2014-09-29 23:14:58]

Patchset #1 (id:1) has been deleted

[hansmuller, 2014-09-29 23:38:49]

hansmuller@chromium.org changed reviewers:
+ aa@chromium.org

[hansmuller, 2014-09-29 23:41:03]

This is a refactoring of the JS content handler
(https://codereview.chromium.org/467263006) to add support for the standalone
case. I'd appreciate it if you'd take a look.

[Aaron Boodman, 2014-10-01 04:25:44]

LGTM w/ nits

Just fix these and land it, you don't need another review from me.

At some point we need to start adding test code for this stuff. Please add it to
the TODO list.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
File mojo/apps/js/application_delegate_impl.h (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:8: #include "base/files/file_util.h"
I don't think this is used by this file.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:10: #include
"mojo/apps/js/content_handler_impl.h"
I don't think this is used.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:45: // ApplicationDelegate method
You can just say:

// ApplicationDelegate:

so that you don't have to keep switching singular/plural

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
File mojo/apps/js/content_handler_impl.h (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
mojo/apps/js/content_handler_impl.h:16: 
Delete empty line

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
mojo/apps/js/content_handler_impl.h:20: virtual ~ContentHandlerImpl();
Does this need to be public?

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/js_app.cc
File mojo/apps/js/js_app.cc (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/js_app.cc#n...
mojo/apps/js/js_app.cc:18: thread_("Mojo JS "),
Remove trailing space.

[hansmuller, 2014-10-01 15:40:12]

Thanks for the feedback!

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
File mojo/apps/js/application_delegate_impl.h (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:8: #include "base/files/file_util.h"
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> I don't think this is used by this file.

Done.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:10: #include
"mojo/apps/js/content_handler_impl.h"
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> I don't think this is used.

Done.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/application...
mojo/apps/js/application_delegate_impl.h:45: // ApplicationDelegate method
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> You can just say:
> 
> // ApplicationDelegate:
> 
> so that you don't have to keep switching singular/plural

Done.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
File mojo/apps/js/content_handler_impl.h (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
mojo/apps/js/content_handler_impl.h:16: 
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> Delete empty line

Done.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/content_han...
mojo/apps/js/content_handler_impl.h:20: virtual ~ContentHandlerImpl();
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> Does this need to be public?
I've made it private for now. When we manage the content handler app's
liftetime, e.g. by quitting when the last JSApp is quit, then I'll probably have
to revisit this.

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/js_app.cc
File mojo/apps/js/js_app.cc (right):

https://codereview.chromium.org/608333002/diff/20001/mojo/apps/js/js_app.cc#n...
mojo/apps/js/js_app.cc:18: thread_("Mojo JS "),
On 2014/10/01 04:25:44, Aaron Boodman wrote:
> Remove trailing space.

Done.

[hansmuller, 2014-10-01 15:40:25]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-10-01 15:41:31]

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

[commit-bot: I haz the power, 2014-10-01 15:53:07]

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

[commit-bot: I haz the power, 2014-10-01 15:53:08]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hansmuller, 2014-10-01 16:06:15]

The CQ bit was unchecked by hansmuller@chromium.org

[hansmuller, 2014-10-01 16:55:48]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-10-01 16:56:46]

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

[Aaron Boodman, 2014-10-01 17:45:52]

https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
File mojo/apps/js/standalone_main.cc (right):

https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
mojo/apps/js/standalone_main.cc:39: base::FilePath
path(base::FilePath::FromUTF8Unsafe(
I forgot to put this in the review, but the hardcoded path here is really weird,
and makes it impossible for anyone to use this other than you.

I realize you need command line support to make this work right. Are you working
on that, or is someone else?

I think it would be better for now to use base::PathService to find main.js in
the source tree. At least it would work for all developers then.

[commit-bot: I haz the power, 2014-10-01 18:01:35]

Committed patchset #3 (id:60001) as 64fc709823fd748574514771747563cd48ce808e

[commit-bot: I haz the power, 2014-10-01 18:02:28]

Patchset 3 (id:??) landed as
https://crrev.com/06c28d7358731ae3a7788656a7c7468b813a4b3e
Cr-Commit-Position: refs/heads/master@{#297675}

[hansmuller, 2014-10-01 20:09:17]

On 2014/10/01 17:45:52, Aaron Boodman wrote:
>
https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
> File mojo/apps/js/standalone_main.cc (right):
> 
>
https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
> mojo/apps/js/standalone_main.cc:39: base::FilePath
> path(base::FilePath::FromUTF8Unsafe(
> I forgot to put this in the review, but the hardcoded path here is really
weird,
> and makes it impossible for anyone to use this other than you.
> 
> I realize you need command line support to make this work right. Are you
working
> on that, or is someone else?
> 
> I think it would be better for now to use base::PathService to find main.js in
> the source tree. At least it would work for all developers then.

Sorry about that. 

I'm waiting for a review of https://codereview.chromium.org/617943002/, which
adds support for mojo_shell command line arguments.

In what I hope is the very short interim, you'd have to hack standalone_main.cc.

- Hans

[hansmuller, 2014-10-03 00:39:13]

On 2014/10/01 20:09:17, hansmuller wrote:
> On 2014/10/01 17:45:52, Aaron Boodman wrote:
> >
>
https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
> > File mojo/apps/js/standalone_main.cc (right):
> > 
> >
>
https://codereview.chromium.org/608333002/diff/60001/mojo/apps/js/standalone_...
> > mojo/apps/js/standalone_main.cc:39: base::FilePath
> > path(base::FilePath::FromUTF8Unsafe(
> > I forgot to put this in the review, but the hardcoded path here is really
> weird,
> > and makes it impossible for anyone to use this other than you.
> > 
> > I realize you need command line support to make this work right. Are you
> working
> > on that, or is someone else?
> > 
> > I think it would be better for now to use base::PathService to find main.js
in
> > the source tree. At least it would work for all developers then.
> 
> Sorry about that. 
> 
> I'm waiting for a review of https://codereview.chromium.org/617943002/, which
> adds support for mojo_shell command line arguments.
> 
> In what I hope is the very short interim, you'd have to hack
standalone_main.cc.
> 
> - Hans

The fix is in: https://codereview.chromium.org/627563002/