Add support for native ports in the vm.

Add support for native ports in the vm.

Dart_NewNativePort creates a port associated with a C handler
function.  When messages come in on this port, they are forwarded to
the C function for processing.

To support this, refactored PortMap so that it operates on a new
MessageHandler type instead of directly on Isolates.

For now, native ports have a dedicated single thread.  Eventually we
will back native ports (and possibly Isolates as well) by a shared
thread pool.
Committed: https://code.google.com/p/dart/source/detail?r=3804

[Søren Gjesse, 2012-01-27 13:33:14]

lgtm

I have mostly looked at the API part of this, so the rest should be reviewed by
somebody else.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:540: * \param name The name of this port in debugging
messags.
messags -> messages

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:757: Isolate* saved = Isolate::Current();
How about a stack allocated isolate switcher?

{
  SwitchIsolate switch(NULL);

  ...
}

The name could probably be better.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:68: ASSERT(priority < Message::kOOBPriority);
With this ASSERT the condition of the if statement is always false.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.cc
File runtime/vm/message.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.c...
runtime/vm/message.cc:161: monitor_.Enter();
In runtime/bin/thread.h we have a MonitorLocker which is not a stack resource.
We could move that to runtime/include/thread.h to share it. Of cause the
runtime/vm/thread.h MonitorLocker would have to be renamed then. Maybe its not
worth it.

[siva, 2012-01-28 00:21:05]

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:525: * A native message handling funciton.
function.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:528: * Dart_NewNativePort.
Should we say:

/**
* A native message handler.
*
* This handler is associated with a ....
...
**/

instead?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:543: *                         native port
concurrently?
Would handle_concurrently be more readable than allow_concurrent?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:548: Dart_NativeMessageHandler func,
handler instead of func?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:557: * \param native_port_id The id of the native
port to close.
s/The id/Id/

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/lib/isolate....
runtime/lib/isolate.cc:347: intptr_t port_id =
PortMap::CreatePort(Isolate::Current()->message_handler());
you could use arguments->isolate() here.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:754: FATAL1("%s expects argument 'func' to be
non-null.", CURRENT_FUNC);
Why is this FATAL and not a
return Api::NewError(...);  // Expect argument to be non NULL

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:758: Isolate::SetCurrent(NULL);
Should we restrict creation of native ports only on threads which do not have an
isolate?

i.e change this to
ASSERT(Isolate::Current() == NULL);

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:772: Isolate::SetCurrent(NULL);
Ditto regarding current isolate.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl_test.cc:2768: Dart_NewNativePort("Port123",
NewNativePort_send123, true);
For completeness should we make this test case a UNIT_TEST_CASE create the
native port without any isolates and then create an isolate and do the
invokestatic business.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:133: delete message_handler_;
similar to mutex_ = NULL set message_handler_ = NULL ?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:362: ASSERT(message_notify_callback() == NULL);
ASSERT(message_handler() != NULL);

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.cc
File runtime/vm/message.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.c...
runtime/vm/message.cc:161: monitor_.Enter();
We should re-evaluate if MonitorLocker needs to be a stack resource maybe with
suitable asserts (isolate->long_jump_base() == NULL and something while throwing
exceptions to ensure we never throw exceptions from a block which has a
MutexLocker).

On 2012/01/27 13:33:14, Søren Gjesse wrote:
> In runtime/bin/thread.h we have a MonitorLocker which is not a stack resource.
> We could move that to runtime/include/thread.h to share it. Of cause the
> runtime/vm/thread.h MonitorLocker would have to be renamed then. Maybe its not
> worth it.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h
File runtime/vm/message.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h...
runtime/vm/message.h:61: 
Lost a blank line?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc
File runtime/vm/port.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:17: 
extra blank line?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:103: ASSERT(handler != NULL);
#ifdef DEBUG
     handler->CheckAccess();
#endif

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:120: EXPECT_STREQ(0, handler.notify_count);
EXPECT(0, handler.notify_count)? They are not strings, not sure how this worked.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:165: // the isolate as it does not run any dart code.
This comment is not true anymore right because we need the Isolate in order to
access the message handler and it needs to be initialized I suppose, otherwise
the handler is going to be NULL.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:166: Dart::CreateIsolate(NULL);
Isolate* isolate = Dart::CreateIsolate(NULL);

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:169: intptr_t local =
PortMap::CreatePort(Isolate::Current()->message_handler());
isolate->message_handler();

[turnidge, 2012-01-31 20:04:31]

Responded to the comments and made some changes...

I leave on vacation this Friday, so it would be great if I am striving to get
this in tomorrow, to leave some slack...

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:525: * A native message handling funciton.
On 2012/01/28 00:21:05, asiva wrote:
> function.

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:528: * Dart_NewNativePort.
On 2012/01/28 00:21:05, asiva wrote:
> Should we say:
> 
> /**
> * A native message handler.
> *
> * This handler is associated with a ....
> ...
> **/
> 
> instead?

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:540: * \param name The name of this port in debugging
messags.
On 2012/01/27 13:33:14, Søren Gjesse wrote:
> messags -> messages

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:543: *                         native port
concurrently?
On 2012/01/28 00:21:05, asiva wrote:
> Would handle_concurrently be more readable than allow_concurrent?

Changed.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:548: Dart_NativeMessageHandler func,
On 2012/01/28 00:21:05, asiva wrote:
> handler instead of func?

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:557: * \param native_port_id The id of the native
port to close.
On 2012/01/28 00:21:05, asiva wrote:
> s/The id/Id/

What's wrong with "The id"?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/lib/isolate.cc
File runtime/lib/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/lib/isolate....
runtime/lib/isolate.cc:347: intptr_t port_id =
PortMap::CreatePort(Isolate::Current()->message_handler());
On 2012/01/28 00:21:05, asiva wrote:
> you could use arguments->isolate() here.

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:754: FATAL1("%s expects argument 'func' to be
non-null.", CURRENT_FUNC);
On 2012/01/28 00:21:05, asiva wrote:
> Why is this FATAL and not a
> return Api::NewError(...);  // Expect argument to be non NULL

We can only do Api::NewError when we are in an isolate.  I don't want to force
callers of this function to be in an isolate.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:757: Isolate* saved = Isolate::Current();
I made an IsolateSaver class which handles the save/restore but not the switch.

On 2012/01/27 13:33:14, Søren Gjesse wrote:
> How about a stack allocated isolate switcher?
> 
> {
>   SwitchIsolate switch(NULL);
> 
>   ...
> }
> 
> The name could probably be better.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:758: Isolate::SetCurrent(NULL);
I'm not sure.  I could see us often creating the native thread on demand when it
is first needed in dart code.  It might be a pain to require them to always
Dart_ExitIsolate before they call this function.  Can we defer this decision?

On 2012/01/28 00:21:05, asiva wrote:
> Should we restrict creation of native ports only on threads which do not have
an
> isolate?
> 
> i.e change this to
> ASSERT(Isolate::Current() == NULL);

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:772: Isolate::SetCurrent(NULL);
On 2012/01/28 00:21:05, asiva wrote:
> Ditto regarding current isolate.

Maybe we can chat about it during the tea break.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl_test.cc:2768: Dart_NewNativePort("Port123",
NewNativePort_send123, true);
I don't understand why this helps.

On 2012/01/28 00:21:05, asiva wrote:
> For completeness should we make this test case a UNIT_TEST_CASE create the
> native port without any isolates and then create an isolate and do the
> invokestatic business.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:68: ASSERT(priority < Message::kOOBPriority);
Yes.  OOB messages are only partway done.  I've added a TODO to make this less
confusing.

On 2012/01/27 13:33:14, Søren Gjesse wrote:
> With this ASSERT the condition of the if statement is always false.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:133: delete message_handler_;
On 2012/01/28 00:21:05, asiva wrote:
> similar to mutex_ = NULL set message_handler_ = NULL ?

Done.  Maybe we should set all of these to NULL...

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/isolate.c...
runtime/vm/isolate.cc:362: ASSERT(message_notify_callback() == NULL);
On 2012/01/28 00:21:05, asiva wrote:
> ASSERT(message_handler() != NULL);

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.cc
File runtime/vm/message.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.c...
runtime/vm/message.cc:161: monitor_.Enter();
On 2012/01/27 13:33:14, Søren Gjesse wrote:
> In runtime/bin/thread.h we have a MonitorLocker which is not a stack resource.
> We could move that to runtime/include/thread.h to share it. Of cause the
> runtime/vm/thread.h MonitorLocker would have to be renamed then. Maybe its not
> worth it.

I have partially addressed this by making StackResources work when there is no
currrent isolate.  See below.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.c...
runtime/vm/message.cc:161: monitor_.Enter();
I have changed StackResource to function reasonably when there is no current
isolate.

On 2012/01/28 00:21:05, asiva wrote:
> We should re-evaluate if MonitorLocker needs to be a stack resource maybe with
> suitable asserts (isolate->long_jump_base() == NULL and something while
throwing
> exceptions to ensure we never throw exceptions from a block which has a
> MutexLocker).
> 
> On 2012/01/27 13:33:14, Søren Gjesse wrote:
> > In runtime/bin/thread.h we have a MonitorLocker which is not a stack
resource.
> > We could move that to runtime/include/thread.h to share it. Of cause the
> > runtime/vm/thread.h MonitorLocker would have to be renamed then. Maybe its
not
> > worth it.
>

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h
File runtime/vm/message.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h...
runtime/vm/message.h:61: 
On 2012/01/28 00:21:05, asiva wrote:
> Lost a blank line?

I thought there was a different convention in headers and .cc files.  Fixed.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc
File runtime/vm/port.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:17: 
On 2012/01/28 00:21:05, asiva wrote:
> extra blank line?

So when is it two and when is it one?  Functions get two, but variables do not? 
Or is this because I am following a DECLARE?  Just confused, that's all.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:103: ASSERT(handler != NULL);
On 2012/01/28 00:21:05, asiva wrote:
> #ifdef DEBUG
>      handler->CheckAccess();
> #endif

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test.cc
File runtime/vm/port_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:120: EXPECT_STREQ(0, handler.notify_count);
On 2012/01/28 00:21:05, asiva wrote:
> EXPECT(0, handler.notify_count)? They are not strings, not sure how this
worked.

Woops.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:165: // the isolate as it does not run any dart code.
On 2012/01/28 00:21:05, asiva wrote:
> This comment is not true anymore right because we need the Isolate in order to
> access the message handler and it needs to be initialized I suppose, otherwise
> the handler is going to be NULL.

Rewrote the comment a bit and made it a TODO.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:166: Dart::CreateIsolate(NULL);
On 2012/01/28 00:21:05, asiva wrote:
> Isolate* isolate = Dart::CreateIsolate(NULL);

Done.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port_test...
runtime/vm/port_test.cc:169: intptr_t local =
PortMap::CreatePort(Isolate::Current()->message_handler());
On 2012/01/28 00:21:05, asiva wrote:
> isolate->message_handler();

Done.

[siva, 2012-02-01 00:38:56]

LGTM

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:557: * \param native_port_id The id of the native
port to close.
"The id of the native port" sounds weird, I could be wrong.

On 2012/01/31 20:04:31, turnidge wrote:
> On 2012/01/28 00:21:05, asiva wrote:
> > s/The id/Id/
> 
> What's wrong with "The id"?

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:754: FATAL1("%s expects argument 'func' to be
non-null.", CURRENT_FUNC);
Agreed but it seems odd that a bad parameter results in a fatal error. Can we
have some kind of an invalid sentinel port that can be returned on all error
cases?

On 2012/01/31 20:04:31, turnidge wrote:
> On 2012/01/28 00:21:05, asiva wrote:
> > Why is this FATAL and not a
> > return Api::NewError(...);  // Expect argument to be non NULL
> 
> We can only do Api::NewError when we are in an isolate.  I don't want to force
> callers of this function to be in an isolate.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:758: Isolate::SetCurrent(NULL);
Sure we can revisit this once we have usage patterns. Maybe a TODO here to
revisit would suffice for now.

On 2012/01/31 20:04:31, turnidge wrote:
> I'm not sure.  I could see us often creating the native thread on demand when
it
> is first needed in dart code.  It might be a pain to require them to always
> Dart_ExitIsolate before they call this function.  Can we defer this decision?
> 
> On 2012/01/28 00:21:05, asiva wrote:
> > Should we restrict creation of native ports only on threads which do not
have
> an
> > isolate?
> > 
> > i.e change this to
> > ASSERT(Isolate::Current() == NULL);
>

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl_test.cc:2768: Dart_NewNativePort("Port123",
NewNativePort_send123, true);
This ties back to my comment regarding Dart_NewNativePort(...) not requiring an
Isolate to be present.

We want to test that Dart_NewNativePort works even when there is no current
Isolate and hence my suggestion to turn it into a UNIT_TEST

On 2012/01/31 20:04:31, turnidge wrote:
> I don't understand why this helps.
> 
> On 2012/01/28 00:21:05, asiva wrote:
> > For completeness should we make this test case a UNIT_TEST_CASE create the
> > native port without any isolates and then create an isolate and do the
> > invokestatic business.
>

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h
File runtime/vm/message.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/message.h...
runtime/vm/message.h:61: 
Sorry my mistake, didn't realize this was a header file.

On 2012/01/31 20:04:31, turnidge wrote:
> On 2012/01/28 00:21:05, asiva wrote:
> > Lost a blank line?
> 
> I thought there was a different convention in headers and .cc files.  Fixed.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc
File runtime/vm/port.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:17: 
I think functions get 2 and vars get 1.

On 2012/01/31 20:04:31, turnidge wrote:
> On 2012/01/28 00:21:05, asiva wrote:
> > extra blank line?
> 
> So when is it two and when is it one?  Functions get two, but variables do
not? 
> Or is this because I am following a DECLARE?  Just confused, that's all.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/include/dar...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/include/dar...
runtime/include/dart_api.h:550: // TODO(turnidge): Currently allow_concurrent is
ignored.
Currently handle_concurrently is ignored.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
File runtime/vm/allocation.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
runtime/vm/allocation.cc:16: // isolate.  If there is no is no current isolate,
we don't need to
"is no" twice?

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
runtime/vm/allocation.cc:34: }
Could we have a generic assert
ASSERT(Isolate::Current() == isolate());
This would work for both cases.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/dart_api...
runtime/vm/dart_api_impl.h:135: Isolate* saved_isolate_;
DISALLOW_COPY_AND_ASSIGN etc...

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/isolate....
runtime/vm/isolate.cc:69: ASSERT(priority < Message::kOOBPriority);
Instead of this assert which seems confusing to people just have an
UNIMPLEMENTED in the priority >= Message::kOOBPriority case.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/native_m...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/native_m...
runtime/vm/native_message_handler.cc:49: }
I suppose CMessageReader::ReadObject will be called here and the resulting
object will be passed in as data.
Maybe a TODO here will make that clear.

[turnidge, 2012-02-01 18:51:26]

I think I've addressed all of the comments.  Going to go ahead and commt the
change.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/include/dart...
runtime/include/dart_api.h:557: * \param native_port_id The id of the native
port to close.
It sounds okay to my ears.  My guess is that this is a dialectical difference
between American English and Indian English.  There are definitely variations in
how articles are used in the two.

On 2012/02/01 00:38:56, asiva wrote:
> "The id of the native port" sounds weird, I could be wrong.
> 
> On 2012/01/31 20:04:31, turnidge wrote:
> > On 2012/01/28 00:21:05, asiva wrote:
> > > s/The id/Id/
> > 
> > What's wrong with "The id"?
>

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:754: FATAL1("%s expects argument 'func' to be
non-null.", CURRENT_FUNC);
Ok.  Done.  Added a test for this too.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl_test.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/dart_api_...
runtime/vm/dart_api_impl_test.cc:2768: Dart_NewNativePort("Port123",
NewNativePort_send123, true);
On 2012/02/01 00:38:56, asiva wrote:
> This ties back to my comment regarding Dart_NewNativePort(...) not requiring
an
> Isolate to be present.
> 
> We want to test that Dart_NewNativePort works even when there is no current
> Isolate and hence my suggestion to turn it into a UNIT_TEST
> 
> On 2012/01/31 20:04:31, turnidge wrote:
> > I don't understand why this helps.
> > 
> > On 2012/01/28 00:21:05, asiva wrote:
> > > For completeness should we make this test case a UNIT_TEST_CASE create the
> > > native port without any isolates and then create an isolate and do the
> > > invokestatic business.
> > 
> 

I now test in both configurations.

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc
File runtime/vm/port.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/5007/runtime/vm/port.cc#n...
runtime/vm/port.cc:17: 
On 2012/02/01 00:38:56, asiva wrote:
> I think functions get 2 and vars get 1.
> 
> On 2012/01/31 20:04:31, turnidge wrote:
> > On 2012/01/28 00:21:05, asiva wrote:
> > > extra blank line?
> > 
> > So when is it two and when is it one?  Functions get two, but variables do
> not? 
> > Or is this because I am following a DECLARE?  Just confused, that's all.
> 

Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/include/dar...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/include/dar...
runtime/include/dart_api.h:550: // TODO(turnidge): Currently allow_concurrent is
ignored.
On 2012/02/01 00:38:56, asiva wrote:
> Currently handle_concurrently is ignored.

Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
File runtime/vm/allocation.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
runtime/vm/allocation.cc:16: // isolate.  If there is no is no current isolate,
we don't need to
On 2012/02/01 00:38:56, asiva wrote:
> "is no" twice?

Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/allocati...
runtime/vm/allocation.cc:34: }
On 2012/02/01 00:38:56, asiva wrote:
> Could we have a generic assert
> ASSERT(Isolate::Current() == isolate());
> This would work for both cases.

Good idea.  Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/dart_api...
File runtime/vm/dart_api_impl.h (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/dart_api...
runtime/vm/dart_api_impl.h:135: Isolate* saved_isolate_;
On 2012/02/01 00:38:56, asiva wrote:
> DISALLOW_COPY_AND_ASSIGN etc...

Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/isolate....
runtime/vm/isolate.cc:69: ASSERT(priority < Message::kOOBPriority);
On 2012/02/01 00:38:56, asiva wrote:
> Instead of this assert which seems confusing to people just have an
> UNIMPLEMENTED in the priority >= Message::kOOBPriority case.

Done.

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/native_m...
File runtime/vm/native_message_handler.cc (right):

https://chromiumcodereview.appspot.com/9169063/diff/15004/runtime/vm/native_m...
runtime/vm/native_message_handler.cc:49: }
On 2012/02/01 00:38:56, asiva wrote:
> I suppose CMessageReader::ReadObject will be called here and the resulting
> object will be passed in as data.
> Maybe a TODO here will make that clear.

Done.