Add patch file support to Dart embedder

Add patch file support to Dart embedder

BUG=
R=zra@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/6104465d68cc43f0244b1319bfdc9bcdec058f8b

[Cutch, 2015-02-23 19:47:09]

johnmccutchan@google.com changed reviewers:
+ zra@google.com

[zra, 2015-02-23 23:21:03]

https://codereview.chromium.org/950063002/diff/1/mojo/dart/embedder/BUILD.gn
File mojo/dart/embedder/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/1/mojo/dart/embedder/BUILD.gn#...
mojo/dart/embedder/BUILD.gn:26:
"//mojo/dart/embedder:generate_dart_mojo_core_patch_cc",
I believe this can just be

":generate_dart_mojo_core_patch_cc"

and be moved to the top of the list.

https://codereview.chromium.org/950063002/diff/20001/mojo/dart/embedder/test/...
File mojo/dart/embedder/test/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/20001/mojo/dart/embedder/test/...
mojo/dart/embedder/test/BUILD.gn:16:
"//mojo/dart/embedder/core/buffer_patch.dart",
rm

[Cutch, 2015-02-24 15:36:48]

https://codereview.chromium.org/950063002/diff/1/mojo/dart/embedder/BUILD.gn
File mojo/dart/embedder/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/1/mojo/dart/embedder/BUILD.gn#...
mojo/dart/embedder/BUILD.gn:26:
"//mojo/dart/embedder:generate_dart_mojo_core_patch_cc",
On 2015/02/23 23:21:02, zra wrote:
> I believe this can just be
> 
> ":generate_dart_mojo_core_patch_cc"
> 
> and be moved to the top of the list.

Done.

https://codereview.chromium.org/950063002/diff/20001/mojo/dart/embedder/test/...
File mojo/dart/embedder/test/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/20001/mojo/dart/embedder/test/...
mojo/dart/embedder/test/BUILD.gn:16:
"//mojo/dart/embedder/core/buffer_patch.dart",
On 2015/02/23 23:21:02, zra wrote:
> rm

Done.

[zra, 2015-02-24 15:56:35]

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/BUILD.gn
File mojo/dart/embedder/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/BUILD...
mojo/dart/embedder/BUILD.gn:46: "//dart/runtime/tools/create_resources.py",
alphabetical order, so this goes to the top.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
File mojo/dart/embedder/builtin.cc (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:7: #include "base/files/file_enumerator.h"
These aren't needed anymore, I think.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:9: #include "base/files/file_util.h"
ditto.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:82: return
Dart_NewStringFromCString(resource_string);
I think the source files can be UTF8, in which case we'd need NewStringFromUTF8.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/test/...
File mojo/dart/embedder/test/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/test/...
mojo/dart/embedder/test/BUILD.gn:109: ":async_helper",
alphabetical order.

[Cutch, 2015-02-24 16:02:04]

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/BUILD.gn
File mojo/dart/embedder/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/BUILD...
mojo/dart/embedder/BUILD.gn:46: "//dart/runtime/tools/create_resources.py",
On 2015/02/24 15:56:35, zra wrote:
> alphabetical order, so this goes to the top.

Done.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
File mojo/dart/embedder/builtin.cc (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:7: #include "base/files/file_enumerator.h"
On 2015/02/24 15:56:35, zra wrote:
> These aren't needed anymore, I think.

Done.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:9: #include "base/files/file_util.h"
On 2015/02/24 15:56:35, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/built...
mojo/dart/embedder/builtin.cc:82: return
Dart_NewStringFromCString(resource_string);
On 2015/02/24 15:56:35, zra wrote:
> I think the source files can be UTF8, in which case we'd need
NewStringFromUTF8.

Done.

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/test/...
File mojo/dart/embedder/test/BUILD.gn (right):

https://codereview.chromium.org/950063002/diff/80001/mojo/dart/embedder/test/...
mojo/dart/embedder/test/BUILD.gn:109: ":async_helper",
On 2015/02/24 15:56:35, zra wrote:
> alphabetical order.

Done.

[zra, 2015-02-24 16:31:32]

lgtm

[zra, 2015-02-24 17:25:13]

+erg

This is the first of two changes needed to hide |native| from dartanalyzer. The
second part will fill in the _patch.dart files.

[Cutch, 2015-02-24 17:28:02]

Committed patchset #7 (id:120001) manually as
6104465d68cc43f0244b1319bfdc9bcdec058f8b (presubmit successful).

[rafaelw, 2015-02-24 19:32:43]

rafaelw@chromium.org changed reviewers:
+ eseidel@chromium.org, rafaelw@chromium.org

[rafaelw, 2015-02-24 19:32:44]

lgtm.

+eseidel. It'd be nice to do something like this for the generated dart bindings
code that the analyzer reads will also not be confused by the native keyword.

[eseidel, 2015-02-24 20:22:23]

It's not clear to me what this does?

[zra, 2015-02-24 20:36:23]

On 2015/02/24 20:22:23, eseidel wrote:
> It's not clear to me what this does?

Background: To bring things up quickly, we ignored that the "native" keyword,
while supported by the Dart VM, is not officially part of the Dart language.
Therefore the Dart analyzer complains about it.

The correct way to call out to C code is to instead mark the Dart functions
"external" without the native keyword. Then the embedder side-loads a "patch"
file containing the native keyword that wires us up to the C call. The analyzer
then only sees a function marked "external", which is legal Dart code, while the
embedder continues to call out to C correctly.

This change: https://codereview.chromium.org/956653003/ completes this refactor,
and does the similar thing for the Sky embedder. If code is generated for Sky
that contains the native keyword, it will need to be refactored into a Dart file
with "external" declarations, and a patch file with "native" declarations in
order to make the Dart analyzer happy.

Sorry this is a bit of a mess. I can rope some other folks into this discussion
if more details are needed.

[abarth-chromium, 2015-02-24 21:06:33]

Does that mean we pay a cost at startup time to patch the library instead of
having the snapshotted library in the binary all ready to go?

[zra, 2015-02-24 21:14:56]

On 2015/02/24 21:06:33, abarth wrote:
> Does that mean we pay a cost at startup time to patch the library instead of
> having the snapshotted library in the binary all ready to go?

Yes, though the _patch.dart files are compiled in as byte arrays, and should
generally be quite small, containing only the "native" declarations.