Moved some MethodMirror properties from embedded API to native calls.

Moved some MethodMirror getters from  embedded API to lazy native calls.

The more complex ones (creating LazyXXXMirror) are still left and will be handled in a separate CL.

Tests are (still) excluded from dart2js, because of one regression test in the same file.

BUG=
R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25081

[ahe, 2013-07-16 20:36:53]

DBC

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
File tests/lib/mirrors/method_mirror_test.dart (right):

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
tests/lib/mirrors/method_mirror_test.dart:7: import
"../../../pkg/unittest/lib/unittest.dart";
It would be great if you could avoid using the unittest framework.

The unit test framework will likely use reflection at some point, so it is
problematic to rely on it for testing dart:mirrors and any platform library.

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
tests/lib/mirrors/method_mirror_test.dart:55: test("FunctionKinds", () {
As far as I can tell, the added tests reuse virtually of the existing test, so
it would be better to create a separate test file.

[rmacnak, 2013-07-16 20:44:53]

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1781: isStatic = 0,
VM code tends to prefix constants like these with k.  Eg. kIsStatic or
kPropertyStatic.

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1810: return NULL;
Instance::null()

[Michael Lippautz (Google), 2013-07-16 21:14:00]

Tests are now separated into two files with proper names. Both are excluded from
dart2js, because of missing features.

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1781: isStatic = 0,
On 2013/07/16 20:44:53, Ryan Macnak wrote:
> VM code tends to prefix constants like these with k.  Eg. kIsStatic or
> kPropertyStatic.

Done.

https://codereview.chromium.org/19299003/diff/5001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1810: return NULL;
On 2013/07/16 20:44:53, Ryan Macnak wrote:
> Instance::null()

Done.

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
File tests/lib/mirrors/method_mirror_test.dart (right):

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
tests/lib/mirrors/method_mirror_test.dart:7: import
"../../../pkg/unittest/lib/unittest.dart";
On 2013/07/16 20:36:53, ahe wrote:
> It would be great if you could avoid using the unittest framework.
> 
> The unit test framework will likely use reflection at some point, so it is
> problematic to rely on it for testing dart:mirrors and any platform library.

Using stringify.dart or expect/expect.dart now. Done.

https://codereview.chromium.org/19299003/diff/5001/tests/lib/mirrors/method_m...
tests/lib/mirrors/method_mirror_test.dart:55: test("FunctionKinds", () {
On 2013/07/16 20:36:53, ahe wrote:
> As far as I can tell, the added tests reuse virtually of the existing test, so
> it would be better to create a separate test file.

Done.

[siva, 2013-07-16 21:31:55]

As discussed offline we can simplify this CL without the native method.

[Michael Lippautz (Google), 2013-07-16 22:13:56]

On 2013/07/16 21:31:55, siva wrote:
> As discussed offline we can simplify this CL without the native method.

Embedded API calls are now converted to native calls. The data is still passed
to the constructor, as it is easily available.

[Ivan Posva, 2013-07-16 22:21:48]

https://codereview.chromium.org/19299003/diff/15001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_name_test.dart (right):

https://codereview.chromium.org/19299003/diff/15001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_name_test.dart:15:
Expect.equals(stringifySymbol(closureMirror.function.simpleName),
"s(doNothing42)");
Long line.

[siva, 2013-07-16 22:48:41]

lgtm

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:562: ASSERT(Dart_IsFunction(func));
Api::UnwrapFunctionHandle has a check to ensure it is a function so this ASSERT
seems redundant.

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:566: const Function& f =
Api::UnwrapFunctionHandle(isolate, func);
'f' seems like a cryptic name, maybe 'func_obj' later it would become 'func'
once you fully convert.

https://codereview.chromium.org/19299003/diff/23001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/19299003/diff/23001/tests/lib/lib.status#newc...
tests/lib/lib.status:8: mirrors/method_mirror_properties_test: Fail # Some
getters not yet implemented.
Is there an issue filed for this or will 6335 work for both?

If 6335 works add that to the comment otherwise you may have to create an
instance and add that to the comment.

https://codereview.chromium.org/19299003/diff/23001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_name_test.dart (right):

https://codereview.chromium.org/19299003/diff/23001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_name_test.dart:8: import "./stringify.dart";
why does it have to be
import "./stringify.dart"

and not just
import "stringify.dart"

[Michael Lippautz (Google), 2013-07-17 00:42:15]

https://codereview.chromium.org/19299003/diff/15001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_name_test.dart (right):

https://codereview.chromium.org/19299003/diff/15001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_name_test.dart:15:
Expect.equals(stringifySymbol(closureMirror.function.simpleName),
"s(doNothing42)");
On 2013/07/16 22:21:48, Ivan Posva wrote:
> Long line.

Done.

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:562: ASSERT(Dart_IsFunction(func));
On 2013/07/16 22:48:42, siva wrote:
> Api::UnwrapFunctionHandle has a check to ensure it is a function so this
ASSERT
> seems redundant.

Done.

https://codereview.chromium.org/19299003/diff/23001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:566: const Function& f =
Api::UnwrapFunctionHandle(isolate, func);
On 2013/07/16 22:48:42, siva wrote:
> 'f' seems like a cryptic name, maybe 'func_obj' later it would become 'func'
> once you fully convert.

Done.

https://codereview.chromium.org/19299003/diff/23001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/19299003/diff/23001/tests/lib/lib.status#newc...
tests/lib/lib.status:8: mirrors/method_mirror_properties_test: Fail # Some
getters not yet implemented.
On 2013/07/16 22:48:42, siva wrote:
> Is there an issue filed for this or will 6335 work for both?
> 
> If 6335 works add that to the comment otherwise you may have to create an
> instance and add that to the comment.

Done.

https://codereview.chromium.org/19299003/diff/23001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_name_test.dart (right):

https://codereview.chromium.org/19299003/diff/23001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_name_test.dart:8: import "./stringify.dart";
On 2013/07/16 22:48:42, siva wrote:
> why does it have to be
> import "./stringify.dart"
> 
> and not just
> import "stringify.dart"

Done.

[Michael Lippautz (Google), 2013-07-17 00:42:46]

Committed patchset #7 manually as r25081 (presubmit successful).

[ahe, 2013-07-17 14:43:57]

DBC

Nice tests!

https://codereview.chromium.org/19299003/diff/27001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_name_test.dart (right):

https://codereview.chromium.org/19299003/diff/27001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_name_test.dart:16: "s(doNothing42)");
I would have written this line as:

expect('s(doNothing42)', closureMirror.function.simpleName);