Add support for snapshot loading to Sky

Add support for snapshot loading to Sky

This CL adds the ability to load Dart snapshot files created by sky_packager in
Sky. Using a snapshot lets us transmit all the code for an app in a single blob
and should improve startup time. Later CLs will make this codepath easier to
use and evaluate performance.

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/0c171c480b0dc35add78dac60832e8567f9d7435

[abarth-chromium, 2015-06-23 06:02:01]

(Don't worry about the red boxes---this CL depends on some earlier ones that
haven't landed.)

[eseidel, 2015-06-23 06:21:39]

lgtm

Why the sky internals removal?

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_controller.cc:314: // Ensure the isolate has a root
library.
I don't think this is necessary anymore  This was for when we had .sky files. So
I guess we still need it, but not in the SkyView case.

I think if we remove it we have to use a different loading function.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_snapshot_loader.cc (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.cc:34: if (!response) {
I regret this design choice.  We need to untangle this after deleting WebView.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.cc:42: if (response->status_code !=
200) {
Are you sure you only want 200?  You ahd me change to >= 400 in other code...

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_snapshot_loader.h (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.h:44: // TODO(abarth): Should we be
using SharedBuffer to buffer the data?
SharedBuffer is about making the data easy to pass around iirc.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.h:44: // TODO(abarth): Should we be
using SharedBuffer to buffer the data?
SharedBuffer is about making the data easy to pass around iirc.

[abarth-chromium, 2015-06-23 14:50:43]

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_controller.cc:314: // Ensure the isolate has a root
library.
On 2015/06/23 at 06:21:39, eseidel wrote:
> I don't think this is necessary anymore  This was for when we had .sky files.
So I guess we still need it, but not in the SkyView case.
> 
> I think if we remove it we have to use a different loading function.

Yeah, I was close to removing it entirely.  I think it's only needed for one of
the oddball isolates.  I can try again to remove it entirely, but for now I'll
add a comment explaining.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_snapshot_loader.cc (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.cc:34: if (!response) {
On 2015/06/23 at 06:21:39, eseidel wrote:
> I regret this design choice.  We need to untangle this after deleting WebView.

Yeah, we should be able to untangle it then.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.cc:42: if (response->status_code !=
200) {
On 2015/06/23 at 06:21:39, eseidel wrote:
> Are you sure you only want 200?  You ahd me change to >= 400 in other code...

I just cargo-culted this from the DartLoader.  I can refactor them to use a
common function so it's obvious that they should be changed together.

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_snapshot_loader.h (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_snapshot_loader.h:44: // TODO(abarth): Should we be
using SharedBuffer to buffer the data?
On 2015/06/23 at 06:21:39, eseidel wrote:
> SharedBuffer is about making the data easy to pass around iirc.

It also has a scatter/gather way of storing data so that it doesn't need to
reallocate a large buffer during the load.  Instead, it creates a bunch of
smaller buffers and then only merges them into a single buffer at the end.  It's
unclear to me whether that's important for this code.

[abarth-chromium, 2015-06-23 15:15:30]

On 2015/06/23 at 14:50:43, abarth wrote:
>
https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
> File sky/engine/core/script/dart_controller.cc (right):
> 
>
https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
> sky/engine/core/script/dart_controller.cc:314: // Ensure the isolate has a
root library.
> On 2015/06/23 at 06:21:39, eseidel wrote:
> > I don't think this is necessary anymore  This was for when we had .sky
files. So I guess we still need it, but not in the SkyView case.
> > 
> > I think if we remove it we have to use a different loading function.
> 
> Yeah, I was close to removing it entirely.  I think it's only needed for one
of the oddball isolates.  I can try again to remove it entirely, but for now
I'll add a comment explaining.

Just checked---still crashes on startup if removed.

[eseidel, 2015-06-23 16:02:03]

lgtm

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
File sky/engine/core/script/dart_controller.cc (right):

https://codereview.chromium.org/1197133004/diff/1/sky/engine/core/script/dart...
sky/engine/core/script/dart_controller.cc:314: // Ensure the isolate has a root
library.
On 2015/06/23 at 14:50:42, abarth wrote:
> On 2015/06/23 at 06:21:39, eseidel wrote:
> > I don't think this is necessary anymore  This was for when we had .sky
files. So I guess we still need it, but not in the SkyView case.
> > 
> > I think if we remove it we have to use a different loading function.
> 
> Yeah, I was close to removing it entirely.  I think it's only needed for one
of the oddball isolates.  I can try again to remove it entirely, but for now
I'll add a comment explaining.

My recollection was it was needed because of the way we did library creation (to
allow mutiple libraries in a single .sky file).

[abarth-chromium, 2015-06-23 16:13:02]

On 2015/06/23 at 16:02:03, eseidel wrote:
> My recollection was it was needed because of the way we did library creation
(to allow multiple libraries in a single .sky file).

I see.  Yeah, we could load the first .dart file as a "script" and then we
wouldn't need it.  Instead, we load it as a "library", which means the DartVM
doesn't think we have a root library.

[abarth-chromium, 2015-06-24 01:40:07]

Committed patchset #3 (id:40001) manually as
0c171c480b0dc35add78dac60832e8567f9d7435 (presubmit successful).