Added getField/setField to ObjectMirror. For InstanceMirrors, this get/sets

Added getField/setField to ObjectMirror. For InstanceMirrors, this get/sets 
fields; for InterfaceMirrors, static fields; for LibraryMirrors, top-level 
fields.
Committed: https://code.google.com/p/dart/source/detail?r=9832

[rmacnak, 2012-07-17 23:42:14]

getField/setField

[turnidge, 2012-07-17 23:59:33]

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:815: Dart_Handle result =
Can this fit on one line?

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:819: // provide reflective access to the error.
Update comment to not refer to invoke.

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:841: Dart_Handle result =
Can this fit on one line?

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors.cc...
runtime/lib/mirrors.cc:845: // provide reflective access to the error.
Update commentto not refer to invoke.

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors_im...
File runtime/lib/mirrors_impl.dart (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:132: completer.complete(
Can this fit on one line?

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:149: 'argument ($arg) was not a simple value');
Maybe make a helper function for arg checking to avoid duplicating this code.

https://chromiumcodereview.appspot.com/10704259/diff/1/runtime/lib/mirrors_im...
runtime/lib/mirrors_impl.dart:154: completer.complete(
Can this fit on one line?

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status#n...
tests/lib/lib.status:17: mirrors/*: Skip
Okay, so we're moving the tests up?

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
File tests/lib/mirrors/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:8: 
Add a TODO to move the existing mirrors tests out of runtime/tests....

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:18: 
No blank line here.

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:20: 
We will eventually have more tests in this file.  Let's break them out by what
they do.  How about move these tests into a function called:

testFieldAccess(mirrors)

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:27: Expect.equals(42,
libMirror.getField('topLevelField').value.reflectee );
It's a little weird for the test to rely on the future getting completed
synchronously.  I would rather that you don't grab ".value" directlly in the
test and instead check the value in a callback.

[rmacnak, 2012-07-18 00:48:55]

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status#n...
tests/lib/lib.status:17: mirrors/*: Skip
It took me a while to find the current location, so perhaps here is better for
when the dart2js folks will need to find it.

On 2012/07/17 23:59:33, turnidge wrote:
> Okay, so we're moving the tests up?

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
File tests/lib/mirrors/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:27: Expect.equals(42,
libMirror.getField('topLevelField').value.reflectee );
But then there should also be some check that the future was resolved, no?

On 2012/07/17 23:59:33, turnidge wrote:
> It's a little weird for the test to rely on the future getting completed
> synchronously.  I would rather that you don't grab ".value" directlly in the
> test and instead check the value in a callback.

[turnidge, 2012-07-18 05:07:35]

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/lib.status#n...
tests/lib/lib.status:17: mirrors/*: Skip
I'll buy that.

On 2012/07/18 00:48:55, rmacnak wrote:
> It took me a while to find the current location, so perhaps here is better for
> when the dart2js folks will need to find it.
> 
> On 2012/07/17 23:59:33, turnidge wrote:
> > Okay, so we're moving the tests up?
>

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
File tests/lib/mirrors/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:27: Expect.equals(42,
libMirror.getField('topLevelField').value.reflectee );
Yes.  I think they have some way of doing this in dart unit tests that I've seen
but I've forgotten what it is.  Some sort of ExpectCalled or something.  If you
don't find it, I'll help you hunt for it tomorrow.

On 2012/07/18 00:48:55, rmacnak wrote:
> But then there should also be some check that the future was resolved, no?
> 
> On 2012/07/17 23:59:33, turnidge wrote:
> > It's a little weird for the test to rely on the future getting completed
> > synchronously.  I would rather that you don't grab ".value" directlly in the
> > test and instead check the value in a callback.
>

[rmacnak, 2012-07-18 18:04:18]

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
File tests/lib/mirrors/mirrors_test.dart (right):

https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
tests/lib/mirrors/mirrors_test.dart:27: Expect.equals(42,
libMirror.getField('topLevelField').value.reflectee );
There is no such method on Expect, and I don't see anything in lib:unittest, so
I added Expect.isTrue(future.isComplete);

On 2012/07/18 05:07:35, turnidge wrote:
> Yes.  I think they have some way of doing this in dart unit tests that I've
seen
> but I've forgotten what it is.  Some sort of ExpectCalled or something.  If
you
> don't find it, I'll help you hunt for it tomorrow.
> 
> On 2012/07/18 00:48:55, rmacnak wrote:
> > But then there should also be some check that the future was resolved, no?
> > 
> > On 2012/07/17 23:59:33, turnidge wrote:
> > > It's a little weird for the test to rely on the future getting completed
> > > synchronously.  I would rather that you don't grab ".value" directlly in
the
> > > test and instead check the value in a callback.
> > 
>

[rmacnak, 2012-07-18 22:06:15]

Updated test to use the unittest library, including its support for testing
async calls.

On 2012/07/18 18:04:18, rmacnak wrote:
>
https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
> File tests/lib/mirrors/mirrors_test.dart (right):
> 
>
https://chromiumcodereview.appspot.com/10704259/diff/1/tests/lib/mirrors/mirr...
> tests/lib/mirrors/mirrors_test.dart:27: Expect.equals(42,
> libMirror.getField('topLevelField').value.reflectee );
> There is no such method on Expect, and I don't see anything in lib:unittest,
so
> I added Expect.isTrue(future.isComplete);
> 
> On 2012/07/18 05:07:35, turnidge wrote:
> > Yes.  I think they have some way of doing this in dart unit tests that I've
> seen
> > but I've forgotten what it is.  Some sort of ExpectCalled or something.  If
> you
> > don't find it, I'll help you hunt for it tomorrow.
> > 
> > On 2012/07/18 00:48:55, rmacnak wrote:
> > > But then there should also be some check that the future was resolved, no?
> > > 
> > > On 2012/07/17 23:59:33, turnidge wrote:
> > > > It's a little weird for the test to rely on the future getting completed
> > > > synchronously.  I would rather that you don't grab ".value" directlly in
> the
> > > > test and instead check the value in a callback.
> > > 
> >

[rmacnak, 2012-07-19 18:42:40]

Adding Ivan since Todd is out through next week.

[Ivan Posva, 2012-07-23 20:57:32]

LGTM -ip

https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors...
runtime/lib/mirrors.cc:808: 
Should be two lines vertical white space. Can you please fix the ones above as
well to get the file consistent again? Thanks!

https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors...
runtime/lib/mirrors.cc:828: 
ditto

[rmacnak, 2012-07-23 22:53:36]

Made mirrors.cc use two spaces between functions consistently.

On 2012/07/23 20:57:32, Ivan Posva wrote:
> LGTM -ip
> 
>
https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors...
> runtime/lib/mirrors.cc:808: 
> Should be two lines vertical white space. Can you please fix the ones above as
> well to get the file consistent again? Thanks!
> 
>
https://chromiumcodereview.appspot.com/10704259/diff/8008/runtime/lib/mirrors...
> runtime/lib/mirrors.cc:828: 
> ditto