- Refactor the isolate callback mechanism to also include creation of the

- Refactor the isolate callback mechanism to also include creation of the
  isolate, this enables creation of isolates using a snapshot
- remove source buffers of corelib, coreimpl-lib and builtin-lib from the binary
Committed: https://code.google.com/p/dart/source/detail?r=1958

[turnidge, 2011-11-23 18:05:12]

First round of comments.

Overall, I think that this change is a nice improvement.  Very nice.

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_natives.cc#newcode3
bin/builtin_natives.cc:3: // BSD-style license that can be found in the LICENSE
file.
If you would like to preserve the edit history of this file, you could branch it
from builtin_in.cc.  It would also make it easier to double-check that no
accidental changes are introduced.  Optional.

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc
File bin/builtin_nolib.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc#newcode27
bin/builtin_nolib.cc:27: ASSERT(!Dart_IsError(builtin_lib));
The above line is not needed, since you have the DART_CHECK_VALID below.

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc#newcode31
bin/builtin_nolib.cc:31: DART_CHECK_VALID(result);
You could get rid of the "result" variable here and do:

DART_CHECK_VALID(Dart_LibraryImportLibrary(...));

Your call.  Keep it as-is if you find it more readable.  Same comment applies in
builtin.cc.

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode198
bin/gen_snapshot.cc:198: " while generating snapshots");
Should we put an UNREACHABLE here?

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode234
bin/gen_snapshot.cc:234: // snapshot generation.
Instead of having this comment you could consider changing the name of the
callback from "CreateIsolateAndSetup" to something that documents this like
"DoNotCreateIsolate".

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode214
bin/main.cc:214: "Creation/Bootstrap of Isolate failed");
If Dart_CreateIsolate took a Dart_IsolateError parameter, then we could return
that here to provide a more detailed error message.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode225
bin/main.cc:225: "%s", Dart_GetError(library));
Should we use OS::SNPrint instead of snprintf?  Same comment applies in
bin/gen_snapshot.cc.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode242
bin/main.cc:242: }
Do we want to provide embedders with access to our Builtin:: stuff so that they
can override the Isolate creation callback but still use our builtin libraries?

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode235
include/dart_api.h:235: } Dart_IsolateError;
We are always using snprintf to fill this error.  We could provide a helper
function to help with this.  Something like:

  Dart_PrintIsolateError(Dart_IsolateError* error, format, ...)

It could use OS::VSNPrint.

If we had this, we might be able to hide the internal structure from the user if
we want to.  We could also considering allocating on demand when the error is
generated rather than pre-allocating, if that works out with the zones.

If we don't hide the internals, it would be good to comment about how/where the
buffer is allocated.  The buffer is allocated before the callback is called. 
Length is an incoming parameter indicating the buffer length, etc.  (Otherwise,
they might think that they should allocate the buffer themselves.)

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode238
include/dart_api.h:238: * An isolate create and initialization callback
function.
s/create/creation/

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode251
include/dart_api.h:251: *   isolate is deleted).
Instead of saying "deleted" maybe frame this in terms of Dart_ShutdownIsolate.

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode253
include/dart_api.h:253: typedef bool
(*Dart_IsolateCreateAndInitCallback)(Dart_IsolateError error);
Dart_IsolateCreateAndInitCallback is a bit long.  Do you think we could get away
with calling this Dart_IsolateCreateCallback?

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode309
include/dart_api.h:309: DART_EXPORT Dart_Isolate Dart_CreateIsolate(const
Dart_Snapshot* snapshot);
Consider taking a Dart_IsolateError parameter.  I guess we would need to do
allocation of the error string differently in that case.

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc
File vm/bootstrap_natives.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc#newc...
vm/bootstrap_natives.cc:29: int argument_count) {
For parallelism with the Builtin class, you could consider making this
Bootstrap::NativeLookup.

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc#newc...
vm/bootstrap_natives.cc:55: ASSERT(!library.IsNull());
Change these ASSERTs to DART_CHECK_VALID to get a better error message?

http://codereview.chromium.org/8673002/diff/5039/vm/dart.h
File vm/dart.h (left):

http://codereview.chromium.org/8673002/diff/5039/vm/dart.h#oldcode23
vm/dart.h:23: static void InitializeIsolate(const Dart_Snapshot* snapshot, void*
data);
Glad to see the "data" parameter go away.

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_linux.cc
File vm/isolate_linux.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_linux.cc#newcode33
vm/isolate_linux.cc:33: static bool VMIsolateInitCallback(Dart_IsolateError
error) {
Maybe rename to "VMIsolateCreateCallback".

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_macos.cc
File vm/isolate_macos.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_macos.cc#newcode33
vm/isolate_macos.cc:33: static bool VMIsolateInitCallback(Dart_IsolateError
error) {
Init => Create

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_win.cc
File vm/isolate_win.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_win.cc#newcode24
vm/isolate_win.cc:24: static bool VMIsolateInitCallback(Dart_IsolateError error)
{
Init => Create

[siva, 2011-11-23 18:24:40]

reintroduce callback_data as dartium was apparently using it.

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc
File bin/builtin_nolib.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc#newcode27
bin/builtin_nolib.cc:27: ASSERT(!Dart_IsError(builtin_lib));
Removed.

On 2011/11/23 18:05:12, turnidge wrote:
> The above line is not needed, since you have the DART_CHECK_VALID below.

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_nolib.cc#newcode31
bin/builtin_nolib.cc:31: DART_CHECK_VALID(result);
Removed result.

On 2011/11/23 18:05:12, turnidge wrote:
> You could get rid of the "result" variable here and do:
> 
> DART_CHECK_VALID(Dart_LibraryImportLibrary(...));
> 
> Your call.  Keep it as-is if you find it more readable.  Same comment applies
in
> builtin.cc.

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode198
bin/gen_snapshot.cc:198: " while generating snapshots");
On 2011/11/23 18:05:12, turnidge wrote:
> Should we put an UNREACHABLE here?

Done.

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode234
bin/gen_snapshot.cc:234: // snapshot generation.
I renamed the callback to "DoNotCreateIsolate" but left the comment.
I could pass a NULL and check for that during invocation, do you think that
would be more readable?

On 2011/11/23 18:05:12, turnidge wrote:
> Instead of having this comment you could consider changing the name of the
> callback from "CreateIsolateAndSetup" to something that documents this like
> "DoNotCreateIsolate".

[turnidge, 2011-11-23 19:03:33]

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode234
bin/gen_snapshot.cc:234: // snapshot generation.
On 2011/11/23 18:24:40, asiva wrote:
> I renamed the callback to "DoNotCreateIsolate" but left the comment.
> I could pass a NULL and check for that during invocation, do you think that
> would be more readable?
> 
> On 2011/11/23 18:05:12, turnidge wrote:
> > Instead of having this comment you could consider changing the name of the
> > callback from "CreateIsolateAndSetup" to something that documents this like
> > "DoNotCreateIsolate".
> 

About the NULL, I don't really have a strong opinion.  It seems like it only
comes up in this one case.

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc
File bin/builtin.cc (right):

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc#newcode18
bin/builtin.cc:18: DART_CHECK_VALID(result);
Remove result.

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc#newcode24
bin/builtin.cc:24: DART_CHECK_VALID(result);
Remove result.

[siva, 2011-11-23 22:37:27]

addressed review comments.

PTAL

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/builtin_natives.cc#newcode3
bin/builtin_natives.cc:3: // BSD-style license that can be found in the LICENSE
file.
On 2011/11/23 18:05:12, turnidge wrote:
> If you would like to preserve the edit history of this file, you could branch
it
> from builtin_in.cc.  It would also make it easier to double-check that no
> accidental changes are introduced.  Optional.

Done.

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/gen_snapshot.cc#newcode234
bin/gen_snapshot.cc:234: // snapshot generation.
I have added a check in Dart_Initialize to avoid NULL being passed in as the
callback argument.

On 2011/11/23 19:03:33, turnidge wrote:
> On 2011/11/23 18:24:40, asiva wrote:
> > I renamed the callback to "DoNotCreateIsolate" but left the comment.
> > I could pass a NULL and check for that during invocation, do you think that
> > would be more readable?
> > 
> > On 2011/11/23 18:05:12, turnidge wrote:
> > > Instead of having this comment you could consider changing the name of the
> > > callback from "CreateIsolateAndSetup" to something that documents this
like
> > > "DoNotCreateIsolate".
> > 
> 
> About the NULL, I don't really have a strong opinion.  It seems like it only
> comes up in this one case.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode214
bin/main.cc:214: "Creation/Bootstrap of Isolate failed");
On 2011/11/23 18:05:12, turnidge wrote:
> If Dart_CreateIsolate took a Dart_IsolateError parameter, then we could return
> that here to provide a more detailed error message.

Done.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode225
bin/main.cc:225: "%s", Dart_GetError(library));
OS class is not avaialble here as it is the embedder code and I noticed that the
windows port has also been using snprintf. As discussed offline I am going to
leave it as snprintf. If needed we can do another sweep over all snprintf calls
and replace them with a Dart_ api call.

On 2011/11/23 18:05:12, turnidge wrote:
> Should we use OS::SNPrint instead of snprintf?  Same comment applies in
> bin/gen_snapshot.cc.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode242
bin/main.cc:242: }
The Builtin stuff is in our dart embedder, this comes back to having a dartutils
library that other embedders could link against.

On 2011/11/23 18:05:12, turnidge wrote:
> Do we want to provide embedders with access to our Builtin:: stuff so that
they
> can override the Isolate creation callback but still use our builtin
libraries?

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode235
include/dart_api.h:235: } Dart_IsolateError;
As discussed offline we will resolve the snprintf stuff in another CL if needed.
I have added documentation about the buffer.

On 2011/11/23 18:05:12, turnidge wrote:
> We are always using snprintf to fill this error.  We could provide a helper
> function to help with this.  Something like:
> 
>   Dart_PrintIsolateError(Dart_IsolateError* error, format, ...)
> 
> It could use OS::VSNPrint.
> 
> If we had this, we might be able to hide the internal structure from the user
if
> we want to.  We could also considering allocating on demand when the error is
> generated rather than pre-allocating, if that works out with the zones.
> 
> If we don't hide the internals, it would be good to comment about how/where
the
> buffer is allocated.  The buffer is allocated before the callback is called. 
> Length is an incoming parameter indicating the buffer length, etc. 
(Otherwise,
> they might think that they should allocate the buffer themselves.)

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode238
include/dart_api.h:238: * An isolate create and initialization callback
function.
On 2011/11/23 18:05:12, turnidge wrote:
> s/create/creation/

Done.

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode251
include/dart_api.h:251: *   isolate is deleted).
On 2011/11/23 18:05:12, turnidge wrote:
> Instead of saying "deleted" maybe frame this in terms of Dart_ShutdownIsolate.

Done.

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode253
include/dart_api.h:253: typedef bool
(*Dart_IsolateCreateAndInitCallback)(Dart_IsolateError error);
On 2011/11/23 18:05:12, turnidge wrote:
> Dart_IsolateCreateAndInitCallback is a bit long.  Do you think we could get
away
> with calling this Dart_IsolateCreateCallback?

Done.

http://codereview.chromium.org/8673002/diff/5039/include/dart_api.h#newcode309
include/dart_api.h:309: DART_EXPORT Dart_Isolate Dart_CreateIsolate(const
Dart_Snapshot* snapshot);
On 2011/11/23 18:05:12, turnidge wrote:
> Consider taking a Dart_IsolateError parameter.  I guess we would need to do
> allocation of the error string differently in that case.

Done.

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc
File vm/bootstrap_natives.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc#newc...
vm/bootstrap_natives.cc:29: int argument_count) {
I renamed the function to NativeLookup but did not include it in Bootstrap class
as it would mean having to include
bootstrap_natives.h and dart_api.h into bootstrap.h
Since this was an internal VM only function it did not seem necessary.

On 2011/11/23 18:05:12, turnidge wrote:
> For parallelism with the Builtin class, you could consider making this
> Bootstrap::NativeLookup.

http://codereview.chromium.org/8673002/diff/5039/vm/bootstrap_natives.cc#newc...
vm/bootstrap_natives.cc:55: ASSERT(!library.IsNull());
I am unable to change these to DART_CHECK_VALID as there are not Dart_Handle but
internal VM handles.

On 2011/11/23 18:05:12, turnidge wrote:
> Change these ASSERTs to DART_CHECK_VALID to get a better error message?

http://codereview.chromium.org/8673002/diff/5039/vm/dart.h
File vm/dart.h (left):

http://codereview.chromium.org/8673002/diff/5039/vm/dart.h#oldcode23
vm/dart.h:23: static void InitializeIsolate(const Dart_Snapshot* snapshot, void*
data);
Apparently Anton needs it. I was also happy to see it removed but he is
apparently relying on it for something.

On 2011/11/23 18:05:12, turnidge wrote:
> Glad to see the "data" parameter go away.

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_linux.cc
File vm/isolate_linux.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_linux.cc#newcode33
vm/isolate_linux.cc:33: static bool VMIsolateInitCallback(Dart_IsolateError
error) {
On 2011/11/23 18:05:12, turnidge wrote:
> Maybe rename to "VMIsolateCreateCallback".

Done.

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_macos.cc
File vm/isolate_macos.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_macos.cc#newcode33
vm/isolate_macos.cc:33: static bool VMIsolateInitCallback(Dart_IsolateError
error) {
On 2011/11/23 18:05:12, turnidge wrote:
> Init => Create

Done.

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_win.cc
File vm/isolate_win.cc (right):

http://codereview.chromium.org/8673002/diff/5039/vm/isolate_win.cc#newcode24
vm/isolate_win.cc:24: static bool VMIsolateInitCallback(Dart_IsolateError error)
{
On 2011/11/23 18:05:12, turnidge wrote:
> Init => Create

Done.

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc
File bin/builtin.cc (right):

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc#newcode18
bin/builtin.cc:18: DART_CHECK_VALID(result);
On 2011/11/23 19:03:33, turnidge wrote:
> Remove result.

Done.

http://codereview.chromium.org/8673002/diff/9001/bin/builtin.cc#newcode24
bin/builtin.cc:24: DART_CHECK_VALID(result);
On 2011/11/23 19:03:33, turnidge wrote:
> Remove result.

Done.

[turnidge, 2011-11-28 18:39:58]

LGTM w/ a few comments.

There's an extra diff in builtin_natives.cc that you should look at.

There's a use of Api::Error in dart_api_impl.cc that needs to be changed.

Other comments are more cosmetic.

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode225
bin/main.cc:225: "%s", Dart_GetError(library));
Ok.

On 2011/11/23 22:37:27, asiva wrote:
> OS class is not avaialble here as it is the embedder code and I noticed that
the
> windows port has also been using snprintf. As discussed offline I am going to
> leave it as snprintf. If needed we can do another sweep over all snprintf
calls
> and replace them with a Dart_ api call.
> 
> On 2011/11/23 18:05:12, turnidge wrote:
> > Should we use OS::SNPrint instead of snprintf?  Same comment applies in
> > bin/gen_snapshot.cc.
>

http://codereview.chromium.org/8673002/diff/5039/bin/main.cc#newcode242
bin/main.cc:242: }
Ok.

On 2011/11/23 22:37:27, asiva wrote:
> The Builtin stuff is in our dart embedder, this comes back to having a
dartutils
> library that other embedders could link against.
> 
> On 2011/11/23 18:05:12, turnidge wrote:
> > Do we want to provide embedders with access to our Builtin:: stuff so that
> they
> > can override the Isolate creation callback but still use our builtin
> libraries?
>

http://codereview.chromium.org/8673002/diff/8006/bin/builtin.cc
File bin/builtin.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/builtin.cc#newcode18
bin/builtin.cc:18: DART_CHECK_VALID(result);
Can get rid of "result" here if you want.

http://codereview.chromium.org/8673002/diff/8006/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/builtin_natives.cc#newco...
bin/builtin_natives.cc:35: V(File_Truncate, 2)                                  
                       \
Extra diff?  Need to sync?

http://codereview.chromium.org/8673002/diff/8006/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/main.cc#newcode317
bin/main.cc:317: if (Dart_IsError(result)) {
If you'd like, you can just say

if (Dart_IsError(Dart_CompileAll()) {

Up to you.

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h#newcode232
include/dart_api.h:232: * 'length' and it must also manage the lifetime of this
buffer.
Since the caller is usually the VM itself instead of the embedder, consider
changing the two "must"s to "will"s.

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h#newcode243
include/dart_api.h:243: * to be created. The callback should create an isolate
and load up the
Maybe we can say "load" instead of "load up".

http://codereview.chromium.org/8673002/diff/8006/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8673002/diff/8006/vm/dart_api_impl.cc#newcode470
vm/dart_api_impl.cc:470: return Api::Error("Isolate create callback parameter
cannot be NULL");
I don't think that Api::Error works here, because there is no current isolate
and this function returns a bool.  I think this will probably best be an assert
eventually.

http://codereview.chromium.org/8673002/diff/8006/vm/unit_test.h
File vm/unit_test.h (right):

http://codereview.chromium.org/8673002/diff/8006/vm/unit_test.h#newcode180
vm/unit_test.h:180: Dart_Isolate isolate = Dart_CreateIsolate(buffer, NULL,
err);
You might as well add an if that prints the isolate error if there is one. 
Somebody at some point will mess up isolate creation during their testing and
run into this.

[siva, 2011-11-29 00:54:25]

synced up to top of tree and addressed review comments.

http://codereview.chromium.org/8673002/diff/8006/bin/builtin.cc
File bin/builtin.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/builtin.cc#newcode18
bin/builtin.cc:18: DART_CHECK_VALID(result);
On 2011/11/28 18:39:58, turnidge wrote:
> Can get rid of "result" here if you want.

Done.

http://codereview.chromium.org/8673002/diff/8006/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/builtin_natives.cc#newco...
bin/builtin_natives.cc:35: V(File_Truncate, 2)                                  
                       \
Yes, I have synched up now.

On 2011/11/28 18:39:58, turnidge wrote:
> Extra diff?  Need to sync?

http://codereview.chromium.org/8673002/diff/8006/bin/main.cc
File bin/main.cc (right):

http://codereview.chromium.org/8673002/diff/8006/bin/main.cc#newcode317
bin/main.cc:317: if (Dart_IsError(result)) {
result is used in Dart_GetError(result) below.

On 2011/11/28 18:39:58, turnidge wrote:
> If you'd like, you can just say
> 
> if (Dart_IsError(Dart_CompileAll()) {
> 
> Up to you.

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h#newcode232
include/dart_api.h:232: * 'length' and it must also manage the lifetime of this
buffer.
We are passing Dart_ErrorBuffer in Dart_CreateIsolate also now and for this case
the caller is the embedder. Maybe I can word it as "the caller ensures that
'buffer'..." and
"'length' and also manages the lifetime of this buffer'

On 2011/11/28 18:39:58, turnidge wrote:
> Since the caller is usually the VM itself instead of the embedder, consider
> changing the two "must"s to "will"s.

http://codereview.chromium.org/8673002/diff/8006/include/dart_api.h#newcode243
include/dart_api.h:243: * to be created. The callback should create an isolate
and load up the
On 2011/11/28 18:39:58, turnidge wrote:
> Maybe we can say "load" instead of "load up".

Done.

http://codereview.chromium.org/8673002/diff/8006/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8673002/diff/8006/vm/dart_api_impl.cc#newcode470
vm/dart_api_impl.cc:470: return Api::Error("Isolate create callback parameter
cannot be NULL");
True, I will return false and add a TODO on how to return a meaningful message
here.

On 2011/11/28 18:39:58, turnidge wrote:
> I don't think that Api::Error works here, because there is no current isolate
> and this function returns a bool.  I think this will probably best be an
assert
> eventually.

http://codereview.chromium.org/8673002/diff/8006/vm/unit_test.h
File vm/unit_test.h (right):

http://codereview.chromium.org/8673002/diff/8006/vm/unit_test.h#newcode180
vm/unit_test.h:180: Dart_Isolate isolate = Dart_CreateIsolate(buffer, NULL,
err);
On 2011/11/28 18:39:58, turnidge wrote:
> You might as well add an if that prints the isolate error if there is one. 
> Somebody at some point will mess up isolate creation during their testing and
> run into this.

Done.

[Ivan Posva, 2011-11-30 00:58:21]

LGTM with comments.

-Ivan

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc#newcode24
bin/gen_snapshot.cc:24: // Global state which containts a pointer to the script
name for which
containts -> contains

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc#newcode233
bin/gen_snapshot.cc:233: // dart API is fixed to take a const char** in
Dart_Initialize).
Comment can be gone.

http://codereview.chromium.org/8673002/diff/21001/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/21001/include/dart_api.h#newcode237
include/dart_api.h:237: } Dart_ErrorBuffer;
I am not a big fan of the limited buffer size that is imposed by this design.
Can we just pass around C strings that have been malloced, where the consumer
has to free them (similarly to strdup)?

http://codereview.chromium.org/8673002/diff/21001/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8673002/diff/21001/vm/dart_api_impl.cc#newcode469
vm/dart_api_impl.cc:469: if (callback == NULL) {
Why is NULL not a legal value? Maybe the embedder is creating the VM with the
exact purpose of only creating isolates by hand.

[siva, 2011-11-30 18:56:31]

Addressed comments.

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc
File bin/gen_snapshot.cc (right):

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc#newcode24
bin/gen_snapshot.cc:24: // Global state which containts a pointer to the script
name for which
On 2011/11/30 00:58:21, Ivan Posva wrote:
> containts -> contains

Done.

http://codereview.chromium.org/8673002/diff/21001/bin/gen_snapshot.cc#newcode233
bin/gen_snapshot.cc:233: // dart API is fixed to take a const char** in
Dart_Initialize).
On 2011/11/30 00:58:21, Ivan Posva wrote:
> Comment can be gone.

Done.

http://codereview.chromium.org/8673002/diff/21001/include/dart_api.h
File include/dart_api.h (right):

http://codereview.chromium.org/8673002/diff/21001/include/dart_api.h#newcode237
include/dart_api.h:237: } Dart_ErrorBuffer;
Changed the type of Dart_ErrorBuffer to 'char*' and I pass a pointer to it
around so that the error string is malloced.

On 2011/11/30 00:58:21, Ivan Posva wrote:
> I am not a big fan of the limited buffer size that is imposed by this design.
> Can we just pass around C strings that have been malloced, where the consumer
> has to free them (similarly to strdup)?

http://codereview.chromium.org/8673002/diff/21001/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8673002/diff/21001/vm/dart_api_impl.cc#newcode469
vm/dart_api_impl.cc:469: if (callback == NULL) {
Removed the check for NULL and in isolate spawn I report an error if the
callback is NULL.

On 2011/11/30 00:58:21, Ivan Posva wrote:
> Why is NULL not a legal value? Maybe the embedder is creating the VM with the
> exact purpose of only creating isolates by hand.