Implement automatic loading of dart:core_native_fields library

Implement automatic loading of dart:core_native_fields library
with predefined classes having native fields.
Classes that need native fields in them can import this library and
extend these predefined classes.
Committed: https://code.google.com/p/dart/source/detail?r=1629

[Anton Muhin, 2011-11-13 16:19:32]

Siva, I couldn't give the new API a try as it doesn't apply cleanly to ToT,
mainly due to recent changes in embedder's API.

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc
File bin/builtin_in.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode99
bin/builtin_in.cc:99: ASSERT(Dart_IsValid(core_lib));
you probably need rebase to new API.

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode116
bin/builtin_in.cc:116: Dart_Handle builtin_lib = Dart_LoadScript(url, source,
handler);
why Dart_LoadScript here?  why not Dart_LoadLibrary?

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode149
bin/builtin_in.cc:149: if (Dart_IsValid(builtin_lib)) {
isn't it an error if one invokes Builtin_SetNativeResolver before builtin lib
gets loaded?

http://codereview.chromium.org/8537023/diff/3008/bin/dartutils.cc
File bin/dartutils.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/dartutils.cc#newcode7
bin/dartutils.cc:7: const char* DartUtils::kBuiltinLibURL = "dart:builtin-lib";
up to you, but IMHO we have too many styles of building long names: no
separators (coreimpl), dash-separated (builtin-lib) and underscore-separated
(core_native_fields).

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart#newcode7
bin/eventhandler.dart:7: class EventHandlerNativeWrapper extends
NativeFieldWrapperClass1 {
do you need this helper class, maybe simply class EventHandler extends
NativeFieldWrapperClass1 ?

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode109
bin/gen_snapshot.cc:109: // load up the script before a snapshot is created.
nit: [L]oad the script

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode116
bin/gen_snapshot.cc:116: library = Builtin_LoadLibrary(LibraryTagHandler);
cannot you migrate to the common infrastructure for DartVM bin snapshoting now? 
might be a good experiment in dogfooding.

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode124
bin/gen_snapshot.cc:124: if (!Dart_IsLibrary(library)) {
should it be an assert?  esp. with new API.

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc
File bin/main.cc (left):

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc#oldcode183
bin/main.cc:183: Builtin_SetNativeResolver();
why it is removed?

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc#newcode250
bin/main.cc:250: Builtin_SetNativeResolver();
why not set builtin native resolver unconditionally?  It'll be needed anyway.

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.cc
File bin/process_script.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.cc#newcod...
bin/process_script.cc:105: bool is_native_flds_library = !strcmp(url_chars,
I am somewhat concerned that logic for dart:core_native_fields resolution belong
to library handler callback.  Now every embedder must remember to implement it
(because technically any loaded lib may do #import(dart:core_native_fields).

May we somehow hide this from embedders?  One option would be to implement this
logic in parser.cc (CallLibraryTagHandler).

Of course, processing of dart:core_native_fields adds some flexibility, but I am
not sure if complexity it adds worths the flexibility we gain.

WDYT?

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.h
File bin/process_script.h (right):

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.h#newcode62
bin/process_script.h:62: extern Dart_Handle LoadSnapshotCreationScript(const
char* script_name,
why change to this LoadSnapshotCreationScript?  It's apparently still used from
the single place.

http://codereview.chromium.org/8537023/diff/3008/vm/dart_api_impl_test.cc
File vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8537023/diff/3008/vm/dart_api_impl_test.cc#new...
vm/dart_api_impl_test.cc:1073: HandleScope scope;
may you defer VM Zone and HandleScope creation till later not to mask errors in
API function implementation (when those forget to setup VM's Zone and
HandleScope)?

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc
File vm/object.cc (right):

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc#newcode4099
vm/object.cc:4099: String::NewSymbol("dart:core_native_fields"));
somehow reuse kCoreNativeFieldsLibURL?

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc#newcode4110
vm/object.cc:4110: OS::SNPrint(chars, kBufferSize, "%d", fld_cnt);
up to you, but for me something like below would be more natural:

const char* const nativeClassPrefix = "NativeFieldWrapperClass";
const int class_name_len = strlen(nativeClassPrefix) + 1 + 1;
char class_name[class_name_len] = {0};

for (...) {
  OS::SNPrint(class_name, strlen(class_name) /* or class_name_len */, "%s%d",
....);
}

[siva, 2011-11-15 02:16:52]

Synched up to tot and addressed review comments.

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc
File bin/builtin_in.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode99
bin/builtin_in.cc:99: ASSERT(Dart_IsValid(core_lib));
On 2011/11/13 16:19:32, antonmuhin wrote:
> you probably need rebase to new API.

Done.

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode116
bin/builtin_in.cc:116: Dart_Handle builtin_lib = Dart_LoadScript(url, source,
handler);
Yes this is Dart_LoadLibrary.

On 2011/11/13 16:19:32, antonmuhin wrote:
> why Dart_LoadScript here?  why not Dart_LoadLibrary?

http://codereview.chromium.org/8537023/diff/3008/bin/builtin_in.cc#newcode149
bin/builtin_in.cc:149: if (Dart_IsValid(builtin_lib)) {
True, converted to an assert.

On 2011/11/13 16:19:32, antonmuhin wrote:
> isn't it an error if one invokes Builtin_SetNativeResolver before builtin lib
> gets loaded?

http://codereview.chromium.org/8537023/diff/3008/bin/dartutils.cc
File bin/dartutils.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/dartutils.cc#newcode7
bin/dartutils.cc:7: const char* DartUtils::kBuiltinLibURL = "dart:builtin-lib";
I have standardized to use dash separated names.
Renaming coreimpl to core-impl can be done in another CL if needed.

On 2011/11/13 16:19:32, antonmuhin wrote:
> up to you, but IMHO we have too many styles of building long names: no
> separators (coreimpl), dash-separated (builtin-lib) and underscore-separated
> (core_native_fields).

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart#newcode7
bin/eventhandler.dart:7: class EventHandlerNativeWrapper extends
NativeFieldWrapperClass1 {
I added it specifically to address the concern you had about names not being
readable when actually extending the class. This would serve as an example of
creating empty classes with more readable names and then using them in the dart
code.

On 2011/11/13 16:19:32, antonmuhin wrote:
> do you need this helper class, maybe simply class EventHandler extends
> NativeFieldWrapperClass1 ?

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode109
bin/gen_snapshot.cc:109: // load up the script before a snapshot is created.
On 2011/11/13 16:19:32, antonmuhin wrote:
> nit: [L]oad the script

Done.

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode116
bin/gen_snapshot.cc:116: library = Builtin_LoadLibrary(LibraryTagHandler);
I would have if we had a 'builtin.dart' file to use but we concat all dart files
into a Builtin_source_ C array and hence the URL mapping scheme is not valid
here.
Maybe in another CL after the URL mapping CL has landed I can remove the step
which auto generates Builtin_source_ and instead auto generate a
'builtin_gen.dart' file which contains #source... of all the other files.

On 2011/11/13 16:19:32, antonmuhin wrote:
> cannot you migrate to the common infrastructure for DartVM bin snapshoting
now? 
> might be a good experiment in dogfooding.

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode124
bin/gen_snapshot.cc:124: if (!Dart_IsLibrary(library)) {
No we want to fail if we don't find a library object here.

On 2011/11/13 16:19:32, antonmuhin wrote:
> should it be an assert?  esp. with new API.

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc
File bin/main.cc (left):

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc#oldcode183
bin/main.cc:183: Builtin_SetNativeResolver();
It is not removed, it has been moved into Builtin_ImportLibrary in order to
avoid the multiple lookups.


On 2011/11/13 16:19:32, antonmuhin wrote:
> why it is removed?

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/main.cc#newcode250
bin/main.cc:250: Builtin_SetNativeResolver();
We don't have to do this for the regular binary as it has already been done in
Builtin_importLibrary....

On 2011/11/13 16:19:32, antonmuhin wrote:
> why not set builtin native resolver unconditionally?  It'll be needed anyway.

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.cc
File bin/process_script.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.cc#newcod...
bin/process_script.cc:105: bool is_native_flds_library = !strcmp(url_chars,
Moved this check to just before invoking the library tag handler so that every
embedder does not need to add this check.

On 2011/11/13 16:19:32, antonmuhin wrote:
> I am somewhat concerned that logic for dart:core_native_fields resolution
belong
> to library handler callback.  Now every embedder must remember to implement it
> (because technically any loaded lib may do #import(dart:core_native_fields).
> 
> May we somehow hide this from embedders?  One option would be to implement
this
> logic in parser.cc (CallLibraryTagHandler).
> 
> Of course, processing of dart:core_native_fields adds some flexibility, but I
am
> not sure if complexity it adds worths the flexibility we gain.
> 
> WDYT?

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.h
File bin/process_script.h (right):

http://codereview.chromium.org/8537023/diff/3008/bin/process_script.h#newcode62
bin/process_script.h:62: extern Dart_Handle LoadSnapshotCreationScript(const
char* script_name,
Changed to original version.

On 2011/11/13 16:19:32, antonmuhin wrote:
> why change to this LoadSnapshotCreationScript?  It's apparently still used
from
> the single place.

http://codereview.chromium.org/8537023/diff/3008/vm/dart_api_impl_test.cc
File vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8537023/diff/3008/vm/dart_api_impl_test.cc#new...
vm/dart_api_impl_test.cc:1073: HandleScope scope;
On 2011/11/13 16:19:32, antonmuhin wrote:
> may you defer VM Zone and HandleScope creation till later not to mask errors
in
> API function implementation (when those forget to setup VM's Zone and
> HandleScope)?

Done.

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc
File vm/object.cc (right):

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc#newcode4099
vm/object.cc:4099: String::NewSymbol("dart:core_native_fields"));
Agree, would be nice, we need to expose thse through the API for that to work.
Probably another CL.

On 2011/11/13 16:19:32, antonmuhin wrote:
> somehow reuse kCoreNativeFieldsLibURL?

http://codereview.chromium.org/8537023/diff/3008/vm/object.cc#newcode4110
vm/object.cc:4110: OS::SNPrint(chars, kBufferSize, "%d", fld_cnt);
On 2011/11/13 16:19:32, antonmuhin wrote:
> up to you, but for me something like below would be more natural:
> 
> const char* const nativeClassPrefix = "NativeFieldWrapperClass";
> const int class_name_len = strlen(nativeClassPrefix) + 1 + 1;
> char class_name[class_name_len] = {0};
> 
> for (...) {
>   OS::SNPrint(class_name, strlen(class_name) /* or class_name_len */, "%s%d",
> ....);
> }

Done.

[Anton Muhin, 2011-11-15 12:22:22]

Great!

LGTM w/ comments addressed.

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart#newcode7
bin/eventhandler.dart:7: class EventHandlerNativeWrapper extends
NativeFieldWrapperClass1 {
Up to you, it's a minor thing.  My concern was for somewhat different situation:
we have a DOMWrapperBase which used in zillion of classes, so it's natural to
'rename' NativeFieldWrapper thing.  In this case, benefit might be minor.

Again, cosmetic, up to your judgement.

On 2011/11/15 02:16:52, asiva wrote:
> I added it specifically to address the concern you had about names not being
> readable when actually extending the class. This would serve as an example of
> creating empty classes with more readable names and then using them in the
dart
> code.
> 
> On 2011/11/13 16:19:32, antonmuhin wrote:
> > do you need this helper class, maybe simply class EventHandler extends
> > NativeFieldWrapperClass1 ?
>

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode116
bin/gen_snapshot.cc:116: library = Builtin_LoadLibrary(LibraryTagHandler);
sure

On 2011/11/15 02:16:52, asiva wrote:
> I would have if we had a 'builtin.dart' file to use but we concat all dart
files
> into a Builtin_source_ C array and hence the URL mapping scheme is not valid
> here.
> Maybe in another CL after the URL mapping CL has landed I can remove the step
> which auto generates Builtin_source_ and instead auto generate a
> 'builtin_gen.dart' file which contains #source... of all the other files.
> 
> On 2011/11/13 16:19:32, antonmuhin wrote:
> > cannot you migrate to the common infrastructure for DartVM bin snapshoting
> now? 
> > might be a good experiment in dogfooding.
>

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode124
bin/gen_snapshot.cc:124: if (!Dart_IsLibrary(library)) {
Is it a possible case when Builtin_LoadLibrary successfully finished (as checked
by Dart_IsValid) and still returned something different from a lib?

On 2011/11/15 02:16:52, asiva wrote:
> No we want to fail if we don't find a library object here.
> 
> On 2011/11/13 16:19:32, antonmuhin wrote:
> > should it be an assert?  esp. with new API.
>

http://codereview.chromium.org/8537023/diff/10001/bin/builtin_in.cc
File bin/builtin_in.cc (right):

http://codereview.chromium.org/8537023/diff/10001/bin/builtin_in.cc#newcode131
bin/builtin_in.cc:131: if (Dart_IsError(builtin_lib)) {
this then clause looks pretty much like a copy of Builtin_LoadLibrary.  Maybe:

void Builtin_ImportLibrary(Dart_Handle library) {
  Dart_Handle builtin_lib = Builtin_LoadLibrary();
  ASSERT(!Dart_IsError(builtin_lib));
  Dart_Handle result = Dart_LibraryImportLibrary(library, builtin_lib);
  ASSERT(!Dart_IsError(result)));
}

If this helper needed at all.

http://codereview.chromium.org/8537023/diff/10001/vm/class_finalizer.cc
File vm/class_finalizer.cc (right):

http://codereview.chromium.org/8537023/diff/10001/vm/class_finalizer.cc#newco...
vm/class_finalizer.cc:921: ReportError("class '%s' is invalid, it is extending
an invalid class.\n",
very nice check, but message looks slightly misleading.  Maybe something along
'class %s with native fields, but no native resolver for lib %s'?

[siva, 2011-11-15 19:42:49]

Ok Ivan you can take a look now.

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/3008/bin/eventhandler.dart#newcode7
bin/eventhandler.dart:7: class EventHandlerNativeWrapper extends
NativeFieldWrapperClass1 {
I will leave it as such with a comment stating that the empty class is there
just to make the derived class definition more readable.

On 2011/11/15 12:22:23, antonmuhin wrote:
> Up to you, it's a minor thing.  My concern was for somewhat different
situation:
> we have a DOMWrapperBase which used in zillion of classes, so it's natural to
> 'rename' NativeFieldWrapper thing.  In this case, benefit might be minor.
> 
> Again, cosmetic, up to your judgement.
> 
> On 2011/11/15 02:16:52, asiva wrote:
> > I added it specifically to address the concern you had about names not being
> > readable when actually extending the class. This would serve as an example
of
> > creating empty classes with more readable names and then using them in the
> dart
> > code.
> > 
> > On 2011/11/13 16:19:32, antonmuhin wrote:
> > > do you need this helper class, maybe simply class EventHandler extends
> > > NativeFieldWrapperClass1 ?
> > 
>

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8537023/diff/3008/bin/gen_snapshot.cc#newcode124
bin/gen_snapshot.cc:124: if (!Dart_IsLibrary(library)) {
I see what you are saying.
Changed to an ASSERT.

On 2011/11/15 12:22:23, antonmuhin wrote:
> Is it a possible case when Builtin_LoadLibrary successfully finished (as
checked
> by Dart_IsValid) and still returned something different from a lib?
> 
> On 2011/11/15 02:16:52, asiva wrote:
> > No we want to fail if we don't find a library object here.
> > 
> > On 2011/11/13 16:19:32, antonmuhin wrote:
> > > should it be an assert?  esp. with new API.
> > 
>

http://codereview.chromium.org/8537023/diff/10001/bin/builtin_in.cc
File bin/builtin_in.cc (right):

http://codereview.chromium.org/8537023/diff/10001/bin/builtin_in.cc#newcode131
bin/builtin_in.cc:131: if (Dart_IsError(builtin_lib)) {
Yes, the helper is needed as it is called from the library tag handler to import
the builtin library into all libraries in standalone dart.

I modified it to use Builtin_LoadLibrary when it does not find the library.

On 2011/11/15 12:22:23, antonmuhin wrote:
> this then clause looks pretty much like a copy of Builtin_LoadLibrary.  Maybe:
> 
> void Builtin_ImportLibrary(Dart_Handle library) {
>   Dart_Handle builtin_lib = Builtin_LoadLibrary();
>   ASSERT(!Dart_IsError(builtin_lib));
>   Dart_Handle result = Dart_LibraryImportLibrary(library, builtin_lib);
>   ASSERT(!Dart_IsError(result)));
> }
> 
> If this helper needed at all.

http://codereview.chromium.org/8537023/diff/10001/vm/class_finalizer.cc
File vm/class_finalizer.cc (right):

http://codereview.chromium.org/8537023/diff/10001/vm/class_finalizer.cc#newco...
vm/class_finalizer.cc:921: ReportError("class '%s' is invalid, it is extending
an invalid class.\n",
On 2011/11/15 12:22:23, antonmuhin wrote:
> very nice check, but message looks slightly misleading.  Maybe something along
> 'class %s with native fields, but no native resolver for lib %s'?

Done.

[Ivan Posva, 2011-11-16 19:19:53]

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc
File bin/dartutils.cc (right):

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode7
bin/dartutils.cc:7: const char* DartUtils::kBuiltinLibURL = "dart:builtin-lib";
Please drop the -lib as well.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode10
bin/dartutils.cc:10: const char* DartUtils::kCoreNativeFieldsLibURL =
"dart:core-native-fields";
How about "dart:nativewrappers"?

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode11
bin/dartutils.cc:11: const char* DartUtils::kBuiltinLibSpec = "library { }";
Do we still need the kBuiltinLibSpec?

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode13
bin/dartutils.cc:13: 
Two lines?

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode21
bin/dartutils.cc:21: 
Two lines.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode36
bin/dartutils.cc:36: 
ditto.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode45
bin/dartutils.cc:45: 
ditto.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode54
bin/dartutils.cc:54: 
ditto.

http://codereview.chromium.org/8537023/diff/19001/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/19001/bin/eventhandler.dart#newcode8
bin/eventhandler.dart:8: // class EventHandler more readable.
Not sure if I agree with the statement that this extra indirection makes it more
readable.

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc
File vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:991: void NativeFieldLookup(Dart_NativeArguments args)
{
?

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:998: return
reinterpret_cast<Dart_NativeFunction>(&native_field_lookup);
This points to itself?

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:1189: EXPECT(Dart_IsError(result));
Please specify at least what kind of error you expect?

http://codereview.chromium.org/8537023/diff/19001/vm/parser.cc
File vm/parser.cc (right):

http://codereview.chromium.org/8537023/diff/19001/vm/parser.cc#newcode3102
vm/parser.cc:3102: bool is_native_flds_library_url =
url.Equals("dart:core-native-fields");
I do not see any reason why this library needs to be handled specially.

[siva, 2011-11-17 00:04:27]

Addressed comments.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc
File bin/dartutils.cc (right):

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode7
bin/dartutils.cc:7: const char* DartUtils::kBuiltinLibURL = "dart:builtin-lib";
On 2011/11/16 19:19:53, Ivan Posva wrote:
> Please drop the -lib as well.

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode10
bin/dartutils.cc:10: const char* DartUtils::kCoreNativeFieldsLibURL =
"dart:core-native-fields";
On 2011/11/16 19:19:53, Ivan Posva wrote:
> How about "dart:nativewrappers"?

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode11
bin/dartutils.cc:11: const char* DartUtils::kBuiltinLibSpec = "library { }";
Removed.

On 2011/11/16 19:19:53, Ivan Posva wrote:
> Do we still need the kBuiltinLibSpec?

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode13
bin/dartutils.cc:13: 
On 2011/11/16 19:19:53, Ivan Posva wrote:
> Two lines?

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode21
bin/dartutils.cc:21: 
On 2011/11/16 19:19:53, Ivan Posva wrote:
> Two lines.

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode36
bin/dartutils.cc:36: 
On 2011/11/16 19:19:53, Ivan Posva wrote:
> ditto.

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode45
bin/dartutils.cc:45: 
On 2011/11/16 19:19:53, Ivan Posva wrote:
> ditto.

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/dartutils.cc#newcode54
bin/dartutils.cc:54: 
On 2011/11/16 19:19:53, Ivan Posva wrote:
> ditto.

Done.

http://codereview.chromium.org/8537023/diff/19001/bin/eventhandler.dart
File bin/eventhandler.dart (right):

http://codereview.chromium.org/8537023/diff/19001/bin/eventhandler.dart#newcode8
bin/eventhandler.dart:8: // class EventHandler more readable.
I will just get rid of this empty class.

On 2011/11/16 19:19:53, Ivan Posva wrote:
> Not sure if I agree with the statement that this extra indirection makes it
more
> readable.

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc
File vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:991: void NativeFieldLookup(Dart_NativeArguments args)
{
See below.

On 2011/11/16 19:19:53, Ivan Posva wrote:
> ?

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:998: return
reinterpret_cast<Dart_NativeFunction>(&native_field_lookup);
Changed native_field_lookup to &NativeFieldLookup

On 2011/11/16 19:19:53, Ivan Posva wrote:
> This points to itself?

http://codereview.chromium.org/8537023/diff/19001/vm/dart_api_impl_test.cc#ne...
vm/dart_api_impl_test.cc:1189: EXPECT(Dart_IsError(result));
On 2011/11/16 19:19:53, Ivan Posva wrote:
> Please specify at least what kind of error you expect?

Done.

http://codereview.chromium.org/8537023/diff/19001/vm/parser.cc
File vm/parser.cc (right):

http://codereview.chromium.org/8537023/diff/19001/vm/parser.cc#newcode3102
vm/parser.cc:3102: bool is_native_flds_library_url =
url.Equals("dart:core-native-fields");
Removed.

On 2011/11/16 19:19:53, Ivan Posva wrote:
> I do not see any reason why this library needs to be handled specially.

[Ivan Posva, 2011-11-17 19:35:06]

LGTMwC -ip

http://codereview.chromium.org/8537023/diff/28003/bin/builtin.dart
File bin/builtin.dart (right):

http://codereview.chromium.org/8537023/diff/28003/bin/builtin.dart#newcode6
bin/builtin.dart:6: #import("dart:nativewrappers");
As discussed you can add #import("dart:coreimpl"); here as well and remove the
manual injection from the .cc file.

http://codereview.chromium.org/8537023/diff/28003/vm/unit_test.cc
File vm/unit_test.cc (right):

http://codereview.chromium.org/8537023/diff/28003/vm/unit_test.cc#newcode71
vm/unit_test.cc:71: return Dart_NewString(url_chars);
Why don't you just return the incoming url?

[siva, 2011-11-17 20:00:17]

http://codereview.chromium.org/8537023/diff/28003/bin/builtin.dart
File bin/builtin.dart (right):

http://codereview.chromium.org/8537023/diff/28003/bin/builtin.dart#newcode6
bin/builtin.dart:6: #import("dart:nativewrappers");
On 2011/11/17 19:35:06, Ivan Posva wrote:
> As discussed you can add #import("dart:coreimpl"); here as well and remove the
> manual injection from the .cc file.

Done.

http://codereview.chromium.org/8537023/diff/28003/vm/unit_test.cc
File vm/unit_test.cc (right):

http://codereview.chromium.org/8537023/diff/28003/vm/unit_test.cc#newcode71
vm/unit_test.cc:71: return Dart_NewString(url_chars);
On 2011/11/17 19:35:06, Ivan Posva wrote:
> Why don't you just return the incoming url?

Done.