Introduce Sky packaged apps.

Introduce Sky packaged apps.

This CL introduces Sky packaged applications, which are very similar to Dart
packaged applications but run in sky_packaged_app_viewer rather than the Dart
content handler. sky_packaged_app_viewer does the following:

- Unzips the application into a temporary dir.
- Sets up a local HttpServer and HttpHandler to serve requests for files in
  this temporary dir.
- Loads the unpacked Sky application, treating "main.sky" as the entrypoint.

An example sky_packaged_application //sky/examples/hello_world. To see it in
action, run the following command after building:

./out/Debug/mojo_shell '--args-for=mojo:kiosk_wm mojo:sky_hello_world' mojo:kiosk_wm

or:

./mojo/tools/android_mojo_shell '--args-for=mojo:kiosk_wm mojo:sky_hello_world' mojo:kiosk_wm

[blundell, 2015-04-14 14:22:02]

blundell@chromium.org changed reviewers:
+ eseidel@chromium.org, tonyg@chromium.org

[blundell, 2015-04-14 14:22:03]

Hi Eric/Tony,

There are a couple cleanup TODOs for me in the code but I wanted to get your
thoughts on the approach first.

Thanks,

Colin

[tonyg, 2015-04-14 14:36:57]

The approach looks good to me.

https://codereview.chromium.org/1085853002/diff/20001/mojo/public/dart/rules.gni
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1085853002/diff/20001/mojo/public/dart/rules....
mojo/public/dart/rules.gni:193: line = "#!mojo mojo:sky_viewer"
Is strict=true applicable? Should it be?

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
File sky/examples/hello_world/BUILD.gn (right):

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
sky/examples/hello_world/BUILD.gn:5:
dart_packaged_application("sky_hello_world") {
This is kind of a bikeshed, but I'm wondering whether this'd make more sense as
sky_packaged_application (which is basically an alias for uses_pub and
uses_sky).

https://codereview.chromium.org/1085853002/diff/20001/sky/viewer/content_hand...
File sky/viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/1085853002/diff/20001/sky/viewer/content_hand...
sky/viewer/content_handler_impl.cc:41: if (url.get().rfind(".mojo") ==
url.size() - 5) {
Maybe just a TODO in this patch, but it seems like eventually we'll want to do
this in a more robust way than extension.

https://codereview.chromium.org/1085853002/diff/20001/sky/viewer/content_hand...
sky/viewer/content_handler_impl.cc:82: base::FilePath entry_path =
package_root.Append("main.sky");
Is it a good idea to enforce the "main.sky" name? Perhaps it is, but I'm just
wondering whether this was done purposefully or for convenience.

https://codereview.chromium.org/1085853002/diff/20001/sky/viewer/content_hand...
sky/viewer/content_handler_impl.cc:111: const std::string input_data =
CopyToString(response->body.Pass());
This seems worthy of a comment about a background thread like you have for
ReadFileToString(). If I'm understanding, the currently implementation would
lock the UI for the duration of the download, right?

https://codereview.chromium.org/1085853002/diff/20001/sky/viewer/content_hand...
sky/viewer/content_handler_impl.cc:112:
CHECK(reader.OpenFromString(input_data));
Do we really want to crash upon corrupted .mojo files? Seems like we could fail
more gracefully.

[hansmuller1, 2015-04-14 15:02:39]

hansmuller@google.com changed reviewers:
+ hansmuller@google.com

[hansmuller1, 2015-04-14 15:02:40]

What's the rule for checking in pubspec.lock files?

I assume that relative Dart import pathnames, like the imports in sky/framework
will carry on working as is?

It would be really helpful to leave [a pointer to] the instructions for setting
up a Mojo/Sky application in examples/README.md or sky/README.md.

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
File sky/examples/hello_world/BUILD.gn (right):

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
sky/examples/hello_world/BUILD.gn:5:
dart_packaged_application("sky_hello_world") {
On 2015/04/14 14:36:56, tonyg wrote:
> This is kind of a bikeshed, but I'm wondering whether this'd make more sense
as
> sky_packaged_application (which is basically an alias for uses_pub and
> uses_sky).

I agree!

[eseidel, 2015-04-14 17:06:31]

Why are we handling any of this inside sky?  Can we just have a packaged app
content handler which knows how to unpackage things and then serve them via
local HTTP to whatever the next content handler is?

Just seems like a lot of special casing for each content handler we want to
teach about this "packaging" format for mojo.

https://codereview.chromium.org/1085853002/diff/20001/sky/engine/core/script/...
File sky/engine/core/script/dart_loader.cc (right):

https://codereview.chromium.org/1085853002/diff/20001/sky/engine/core/script/...
sky/engine/core/script/dart_loader.cc:36: bool is_packaged_app =
baseUri.string().startsWith("file://");
Why aren't packaged apps just served over http locally?  That seems like it
would require the least amount of code modification?

https://codereview.chromium.org/1085853002/diff/20001/sky/engine/core/script/...
sky/engine/core/script/dart_loader.cc:79: if (response->status_code == 0) {
Here again, instead of teaching Sky to understand a non-networked world, why not
just make packaged apps behave like a network?

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
File sky/examples/hello_world/pubspec.lock (right):

https://codereview.chromium.org/1085853002/diff/20001/sky/examples/hello_worl...
sky/examples/hello_world/pubspec.lock:1: # Generated by pub
Not clear if these should be committed or not:
https://www.dartlang.org/tools/private-files.html#pubspeclock

[blundell, 2015-04-15 16:13:03]

blundell@chromium.org changed reviewers:
+ qsr@chromium.org

[blundell, 2015-04-15 16:13:04]

Thanks for the reviews, everyone. Tony/Hans, I'll address your comments once the
dust settles on the basic approach.

Eric/Benjamin, I'd like your feedback on the new approach (which follows Eric's
suggestion). There's some polishing I still have to do, but I wanted to get
early feedback on any major problems you see with this approach.

Thanks,

Colin

[blundell, 2015-04-15 16:14:04]

(note that while the idea would be to generalize this content handler
eventually, I would worry about that in a later pass where we move some other
type of application to use this).

[eseidel, 2015-04-15 19:45:04]

This looks like it would work well to me.  lgtm

https://codereview.chromium.org/1085853002/diff/40001/services/sky_packaged_a...
File services/sky_packaged_app_viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/1085853002/diff/40001/services/sky_packaged_a...
services/sky_packaged_app_viewer/content_handler_impl.cc:129:
"http://localhost:" + std::to_string(port_) + "/main.sky",
We're eventually going to remove the main.sky and move to a main.dart instead. 
But we'll easily update this when we do.

https://codereview.chromium.org/1085853002/diff/40001/services/sky_packaged_a...
services/sky_packaged_app_viewer/content_handler_impl.cc:140:
ExtractApplication(response.Pass());
Eventually one could just serve straight out of the zip file I figure.  But for
now this looks like it would work great.

[qsr, 2015-04-15 21:56:29]

https://codereview.chromium.org/1085853002/diff/40001/services/sky_packaged_a...
File services/sky_packaged_app_viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/1085853002/diff/40001/services/sky_packaged_a...
services/sky_packaged_app_viewer/content_handler_impl.cc:130: services.Pass(),
exposed_services.Pass());
I think we should not do this. Application identity is their URL. having an
application that change identity each time it is called seems wrong to me. For
example, each connection from any application will come from
http://localhost:2345/main.sky. How can any service get any useful information
about which application is connecting?

[blundell, 2015-04-16 13:06:01]

Patchset #4 (id:60001) has been deleted

[blundell, 2015-04-16 13:30:39]

Patchset #4 (id:80001) has been deleted

[blundell, 2015-04-16 13:55:19]

Patchset #4 (id:100001) has been deleted

[blundell, 2015-04-16 14:07:16]

Thanks for the reviews! After Benjamin's comment, the latest patch now loads
main.sky directly in the sky_packaged_app_viewer content handler rather than
connecting to it through the shell. This required exposing some plumbing from
sky_viewer for reuse (https://codereview.chromium.org/1083273003/). This patch
also handles connection requests that come in while the app is being
extracted/set up. eseidel, qsr: PTAL.

[qsr, 2015-04-16 16:27:33]

https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:192: if (defined(invoker.uses_sky) &&
invoker.uses_sky) {
Shouldn't this template for this case be somewhere in sky and named
sky_packaged_app...

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
File services/sky_packaged_app_viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
services/sky_packaged_app_viewer/content_handler_impl.cc:130: std::stringstream
ss;
Are we using stringstream anywhere in chromium? Usually, we use
base::StringPrintf

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
services/sky_packaged_app_viewer/content_handler_impl.cc:136:
local_entrypoint_url_);
I'm probably missing something, but why do we need to start a local webserver,
can't you pass a file:/// URL here?

[blundell, 2015-04-16 18:07:56]

Thanks!

https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
File mojo/public/dart/rules.gni (right):

https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
mojo/public/dart/rules.gni:192: if (defined(invoker.uses_sky) &&
invoker.uses_sky) {
On 2015/04/16 16:27:33, qsr wrote:
> Shouldn't this template for this case be somewhere in sky and named
> sky_packaged_app...

Added sky_packaged_app, which wraps dart_packaged_app and will be the
client-facing way to create sky packaged apps.

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
File services/sky_packaged_app_viewer/content_handler_impl.cc (right):

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
services/sky_packaged_app_viewer/content_handler_impl.cc:130: std::stringstream
ss;
On 2015/04/16 16:27:33, qsr wrote:
> Are we using stringstream anywhere in chromium? Usually, we use
> base::StringPrintf

Done.

https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
services/sky_packaged_app_viewer/content_handler_impl.cc:136:
local_entrypoint_url_);
On 2015/04/16 16:27:33, qsr wrote:
> I'm probably missing something, but why do we need to start a local webserver,
> can't you pass a file:/// URL here?

Doing it this way enables avoiding any changes within Sky itself; see PS2 and
Eric's comments on it.

[qsr, 2015-04-16 20:33:26]

On 2015/04/16 18:07:56, blundell wrote:
> Thanks!
> 
>
https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
> File mojo/public/dart/rules.gni (right):
> 
>
https://codereview.chromium.org/1085853002/diff/120001/mojo/public/dart/rules...
> mojo/public/dart/rules.gni:192: if (defined(invoker.uses_sky) &&
> invoker.uses_sky) {
> On 2015/04/16 16:27:33, qsr wrote:
> > Shouldn't this template for this case be somewhere in sky and named
> > sky_packaged_app...
> 
> Added sky_packaged_app, which wraps dart_packaged_app and will be the
> client-facing way to create sky packaged apps.
> 
>
https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
> File services/sky_packaged_app_viewer/content_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
> services/sky_packaged_app_viewer/content_handler_impl.cc:130:
std::stringstream
> ss;
> On 2015/04/16 16:27:33, qsr wrote:
> > Are we using stringstream anywhere in chromium? Usually, we use
> > base::StringPrintf
> 
> Done.
> 
>
https://codereview.chromium.org/1085853002/diff/120001/services/sky_packaged_...
> services/sky_packaged_app_viewer/content_handler_impl.cc:136:
> local_entrypoint_url_);
> On 2015/04/16 16:27:33, qsr wrote:
> > I'm probably missing something, but why do we need to start a local
webserver,
> > can't you pass a file:/// URL here?
> 
> Doing it this way enables avoiding any changes within Sky itself; see PS2 and
> Eric's comments on it.

I'm not sure I'm buying this. We are trading 2 lines of code to let sky use file
URLs against 100s to setup this web server, and also we now have copy back and
forth for no benefit.

Also, on the 2 changes you needed for file, the first one (adding packages) seem
not necessary, as you should be able to put the right directory structure so
that packages is a top directory in your zip file.

The second one about network fetch doesn't seem that much of an issue to me...

[eseidel, 2015-04-16 21:39:59]

We can teach sky to handle file urls.  But it seems a bit silly given that mojo
is a network-focused OS.  Seems better to assume network loads always?

[qsr, 2015-04-16 21:51:02]

On 2015/04/16 21:39:59, eseidel wrote:
> We can teach sky to handle file urls.  But it seems a bit silly given that
mojo
> is a network-focused OS.  Seems better to assume network loads always?

I don't think so. mojo is not only network focus. It considered apps are at
given URL. file URL is perfectly fine. I can easily imagine that some device
running mojo will have a filesystem.

[eseidel, 2015-04-16 21:56:50]

eseidel@chromium.org changed reviewers:
+ tonyg@chromium.orgfi
- tonyg@chromium.org

[eseidel, 2015-04-16 21:56:51]

File urls have no origin and no headers and were an endless (security) headache
for Chrome.  Then again, I guess if we're zipping apps before we send them down
we have neither of those either.

I don't feel super strongly.  We certainly can teach Sky about file: urls.  It
just seems silly to have two ways to do the same thing.  For the same reasons
why the mojo: scheme is harmful to mojo in that it creates two separate loader
paths for mojo, only one of which actually works well (the mojo: scheme) and the
other (network) of which (historically) has had lots of bugs.  We're doing the
same here.  We're creating two paths.  One of which will be tested and one of
which will have bugs.

[blundell, 2015-04-17 13:12:49]

On 2015/04/16 21:56:51, eseidel wrote:
> File urls have no origin and no headers and were an endless (security)
headache
> for Chrome.  Then again, I guess if we're zipping apps before we send them
down
> we have neither of those either.
> 
> I don't feel super strongly.  We certainly can teach Sky about file: urls.  It
> just seems silly to have two ways to do the same thing.  For the same reasons
> why the mojo: scheme is harmful to mojo in that it creates two separate loader
> paths for mojo, only one of which actually works well (the mojo: scheme) and
the
> other (network) of which (historically) has had lots of bugs.  We're doing the
> same here.  We're creating two paths.  One of which will be tested and one of
> which will have bugs.

Thanks for the discussion.

In creating sky_packaged_apps at all, we're creating two paths, whether the
sky_packaged_app path lives entirely outside of //sky, entirely inside //sky, or
a mixture. I agree that that introduces the potential for rot along the
less-tested path. However, I don't see a better quick solution to allow
mojom.dart files to be generated locally and referenced from sky apps. I could
mitigate this risk by creating some kind of tests for packaged apps, of course.

With regards to putting code inside //sky, getting rid of the need to have
dart_loader.cc handle the |packages:| scheme differently for packaged apps vs.
non-packaged apps is slightly more complex than just manipulating the build
system so that packaged apps end up with a top-level packages/ directory. As of
now, Sky resolves the |packages:| prefix to /packages, which is top-level wrt
the host (so you end up with file:///packages/<foo.dart> for a file URL).
However, for packaged apps, it needs to be resolved relative to the base URL of
the Sky application (e.g., file:///tmp/.Chromium.x25aa/packages/<foo.dart>). To
avoid needing a forked codepath in dart_loader.cc, unpackaged Sky apps would
need to change to also resolve |package:| relative to the base URL of the
application. This change might be a Good Thing in any case as it matches the
Dart spec: https://www.dartlang.org/tools/pub/faq.html (see 'What are all the
"packages" directories for?').

I confess that I didn't deeply investigate why the url load of file urls was
returning an http status of 0.

Note that Adam has also expressed reservations with the idea of having a second
content handler that runs the Sky engine directly:
https://codereview.chromium.org/1083273003/.

[eseidel, 2015-04-20 17:13:13]

The Dart shell respects a PACKAGE_ROOT env variable.  Similarly I expected we
would eventually teach Sky to have the path to /packages be controllable.  For
now it's just hard-coded.

Presumably packages should be resolved relative to the root_library in Dart,
which in our case is the main.sky file.