- Add dart:json, dart:uri and dart:utf8 to the known and builtin libraries.

- Add dart:json, dart:uri and dart:utf8 to the known and builtin libraries.
- Unify and simplify the building of the creation of the source file.
Committed: https://code.google.com/p/dart/source/detail?r=4336

[turnidge, 2012-02-16 17:42:02]

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc
File bin/builtin.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode80
bin/builtin.cc:80: return Dart_Error("Unknown builtin library requested.");
This switch can go away if we get rid of ids.  Instead, we would just rely on
Source to return the Dart_Error when the library was unknown.

See my comment in main.cc

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h
File bin/builtin.h (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h#newcode30
bin/builtin.h:30: };
I suggest elsewhere to get rid of the id and just use the url directly.  I don't
think the id buys us much.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h#newcode37
bin/builtin.h:37: static void PrintString(FILE* out, Dart_Handle object);
We should make more of these functions private.  I believe Source() can be made
private.  Possibly SetupLibrary, ImportLibrary, and SetNativeResolver as well?

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc
File bin/builtin_in.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc#newco...
bin/builtin_in.cc:13: };
I like this change.  It gets rid of some boilerplate that needed to be repeated.

As an aside, I have been kicking around the idea of replacing all of the
generated files with one big generated file called file_map.cc that has a class
called FileMap that maps paths to strings.  That way we could have just one
build rule to generate all of the embedded files at once instead of having lots
of separate rules.  If we did it right, we could also stop doing the
auto-concatenation and just use #source to combine our library files.  We would
need to teach the builtin tag handler how to use the FileMap.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h
File bin/dartutils.h (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h#newcode74
bin/dartutils.h:74: static bool IsDartUtf8LibURL(const char* url_name);
These functions could become private helper functions in builtin.h possibly.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h#newcode95
bin/dartutils.h:95: static const char* kUtf8LibURL;
These constants could become private to builtin.h possibly.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode278
bin/main.cc:278: return Builtin::LoadLibrary(Builtin::kUtf8Library);
Do you notice that here we take a url and convert it to an id?  And then in
LoadLibrary we take that id and convert it back to a url?

I think that perhaps the ids are not useful.  Instead, I suggest:

- Make all apis in builtin.h take a url directly.
- Have LoadLibrary just return a Dart_Error() when we ask for an unknown lib.

That way we won't have to keep modifying main.cc every time a new lib is added.

[vsm, 2012-02-16 17:52:03]

DBC

It looks like these added to the command line embedder.  What do you think about
adding it to the core VM (alongside dart:core to my understanding)?

Otherwise, we'll need to keep dartium and the command line embedder in sync -
e.g., dartium has had json for a while, but no uri or utf8.

[siva, 2012-02-16 18:44:17]

LGTM

We could do another round of re-structuring once we figure out how we want to
handle some of these generic libraries that both the stand alone embedder and
dartium want to share.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/bin.gypi
File bin/bin.gypi (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/bin.gypi#newcode193
bin/bin.gypi:193: },
I am not an expert on gyp files but I am sure there must be a way to specify a
rule to operate on a list of input files on which this generation step needs to
happen (similar to how the compile rules work)

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc
File bin/builtin.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode57
bin/builtin.cc:57: DART_CHECK_VALID(Dart_SetNativeResolver(library,
NativeLookup));
Do all these libraries need this native hookup by default?

I don't think json, uri, utf8 have any native methods in their current form.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode80
bin/builtin.cc:80: return Dart_Error("Unknown builtin library requested.");
I would go for a table driven scheme for this, we have a table of uri, source,
native_fields etc. characteristics and implement the rest of the code
generically using the table. That way just an addition to the table should be
sufficient
(We could even generate the table from the build rules possibly).

On 2012/02/16 17:42:02, turnidge wrote:
> This switch can go away if we get rid of ids.  Instead, we would just rely on
> Source to return the Dart_Error when the library was unknown.
> 
> See my comment in main.cc

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode270
bin/main.cc:270: // Handle imports of dart:io.
The comment is invalid now, maybe
// handle imports of other built-in libraries present in the SDK.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode271
bin/main.cc:271: if (DartUtils::IsDartIOLibURL(url_string) && (tag ==
kImportTag)) {
I don't think we need this (tag == kImportTag) check.
We have already checked above for KCanonicalizeUrl so just an
ASSERT above that tag == kImportTag would be sufficient.

[turnidge, 2012-02-16 19:47:22]

LGTM

Talked to Ivan offline.  He told me that he is intending to have a follow-up
change with cleanups later.  None of my suggestions impacted correctness and
they mostly address problems which already exist in the code, so I am willing to
defer them to a later CL.

Todd

On 2012/02/16 17:42:02, turnidge wrote:
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc
> File bin/builtin.cc (right):
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode80
> bin/builtin.cc:80: return Dart_Error("Unknown builtin library requested.");
> This switch can go away if we get rid of ids.  Instead, we would just rely on
> Source to return the Dart_Error when the library was unknown.
> 
> See my comment in main.cc
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h
> File bin/builtin.h (right):
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h#newcode30
> bin/builtin.h:30: };
> I suggest elsewhere to get rid of the id and just use the url directly.  I
don't
> think the id buys us much.
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h#newcode37
> bin/builtin.h:37: static void PrintString(FILE* out, Dart_Handle object);
> We should make more of these functions private.  I believe Source() can be
made
> private.  Possibly SetupLibrary, ImportLibrary, and SetNativeResolver as well?
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc
> File bin/builtin_in.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc#newco...
> bin/builtin_in.cc:13: };
> I like this change.  It gets rid of some boilerplate that needed to be
repeated.
> 
> As an aside, I have been kicking around the idea of replacing all of the
> generated files with one big generated file called file_map.cc that has a
class
> called FileMap that maps paths to strings.  That way we could have just one
> build rule to generate all of the embedded files at once instead of having
lots
> of separate rules.  If we did it right, we could also stop doing the
> auto-concatenation and just use #source to combine our library files.  We
would
> need to teach the builtin tag handler how to use the FileMap.
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h
> File bin/dartutils.h (right):
> 
>
https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h#newcode74
> bin/dartutils.h:74: static bool IsDartUtf8LibURL(const char* url_name);
> These functions could become private helper functions in builtin.h possibly.
> 
>
https://chromiumcodereview.appspot.com/9417012/diff/1/bin/dartutils.h#newcode95
> bin/dartutils.h:95: static const char* kUtf8LibURL;
> These constants could become private to builtin.h possibly.
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc
> File bin/main.cc (right):
> 
> https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode278
> bin/main.cc:278: return Builtin::LoadLibrary(Builtin::kUtf8Library);
> Do you notice that here we take a url and convert it to an id?  And then in
> LoadLibrary we take that id and convert it back to a url?
> 
> I think that perhaps the ids are not useful.  Instead, I suggest:
> 
> - Make all apis in builtin.h take a url directly.
> - Have LoadLibrary just return a Dart_Error() when we ask for an unknown lib.
> 
> That way we won't have to keep modifying main.cc every time a new lib is
added.

[Ivan Posva, 2012-02-16 19:58:50]

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/bin.gypi
File bin/bin.gypi (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/bin.gypi#newcode193
bin/bin.gypi:193: },
On 2012/02/16 18:44:17, asiva wrote:
> I am not an expert on gyp files but I am sure there must be a way to specify a
> rule to operate on a list of input files on which this generation step needs
to
> happen (similar to how the compile rules work) 

It is likely the case that the duplication of the actions could be avoided, but
as we are planning to restructure this whole area of building libraries, I think
we should punt this to a different CL.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc
File bin/builtin.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode57
bin/builtin.cc:57: DART_CHECK_VALID(Dart_SetNativeResolver(library,
NativeLookup));
On 2012/02/16 18:44:17, asiva wrote:
> Do all these libraries need this native hookup by default?
> 
> I don't think json, uri, utf8 have any native methods in their current form.

Good point, I will only add the native resolve to builtin and io.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.cc#newcode80
bin/builtin.cc:80: return Dart_Error("Unknown builtin library requested.");
I agree with you both that the way we are collecting the sources for these
libraries and putting them into both bootstrap or builtin should be improved. I
do think I have a solution for simplifying all of this, but I need to sit down
and study the capabilities of gyp a bit more in order to avoid the "one big
source file" problem.

On 2012/02/16 18:44:17, asiva wrote:
> I would go for a table driven scheme for this, we have a table of uri, source,
> native_fields etc. characteristics and implement the rest of the code
> generically using the table. That way just an addition to the table should be
> sufficient
> (We could even generate the table from the build rules possibly).
> 
> On 2012/02/16 17:42:02, turnidge wrote:
> > This switch can go away if we get rid of ids.  Instead, we would just rely
on
> > Source to return the Dart_Error when the library was unknown.
> > 
> > See my comment in main.cc
>

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h
File bin/builtin.h (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin.h#newcode30
bin/builtin.h:30: };
On 2012/02/16 17:42:02, turnidge wrote:
> I suggest elsewhere to get rid of the id and just use the url directly.  I
don't
> think the id buys us much.

Agreed, different CL.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc
File bin/builtin_in.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/builtin_in.cc#newco...
bin/builtin_in.cc:13: };
On 2012/02/16 17:42:02, turnidge wrote:
> I like this change.  It gets rid of some boilerplate that needed to be
repeated.
> 
> As an aside, I have been kicking around the idea of replacing all of the
> generated files with one big generated file called file_map.cc that has a
class
> called FileMap that maps paths to strings.  That way we could have just one
> build rule to generate all of the embedded files at once instead of having
lots
> of separate rules.  If we did it right, we could also stop doing the
> auto-concatenation and just use #source to combine our library files.  We
would
> need to teach the builtin tag handler how to use the FileMap.

See previous comment about fixing the one big source file problem in a different
way.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc
File bin/main.cc (right):

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode270
bin/main.cc:270: // Handle imports of dart:io.
On 2012/02/16 18:44:17, asiva wrote:
> The comment is invalid now, maybe
> // handle imports of other built-in libraries present in the SDK.

Done.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode271
bin/main.cc:271: if (DartUtils::IsDartIOLibURL(url_string) && (tag ==
kImportTag)) {
On 2012/02/16 18:44:17, asiva wrote:
> I don't think we need this (tag == kImportTag) check.
> We have already checked above for KCanonicalizeUrl so just an
> ASSERT above that tag == kImportTag would be sufficient.

Done.

https://chromiumcodereview.appspot.com/9417012/diff/1/bin/main.cc#newcode278
bin/main.cc:278: return Builtin::LoadLibrary(Builtin::kUtf8Library);
On 2012/02/16 17:42:02, turnidge wrote:
> Do you notice that here we take a url and convert it to an id?  And then in
> LoadLibrary we take that id and convert it back to a url?
> 
> I think that perhaps the ids are not useful.  Instead, I suggest:
> 
> - Make all apis in builtin.h take a url directly.
> - Have LoadLibrary just return a Dart_Error() when we ask for an unknown lib.
> 
> That way we won't have to keep modifying main.cc every time a new lib is
added.

Yes, I did notice this back and forth. But I decided to keep the current
structure and add the libraries we need at the moment. A big restructure of this
is needed anyway, but was outside of the scope of this change.