Adding proper handling for MethodMirror.owner

If we have the owner mirror at creation time (which right now is a lazy mirror), we will use it. Eventually lazy mirrors will go away and we will not have to resolve() in dart code anymore.

If we don't have it, for example for a nested closure, we set it null and construct it lazily at access time.

Example:

outer() {
  inner() {}
  var closureMirror = reflect(inner);
  print(closureMirror.function.owner);  // Should be a MethodMirror on outer, that is constructed lazily.
}

Since we need to be able to construct the chain of owners (back up to library), we need to handle all cases in the lazy creation.

The CL also adds wrappers that can be called with native objects. At some point these wrapper will create the mirrors without referring to the ones using embedded API.

BUG=
R=asiva@google.com

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

[rmacnak, 2013-07-17 22:27:50]

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

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:20: static RawObject* CreateClassMirror(const Class&
cls);
RawInstance* --- we know they will be mirrors

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:23: const Instance& owner);
owner_mirror, no?

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1027: // default and one using Dart_Handles will
disappear.
will get default ... => will replace the version that uses API handles

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1059: // default and one using Dart_Handles will
disappear.
will replace

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1073: // default and one using Dart_Handles will
disappear.
will replace

[Michael Lippautz (Google), 2013-07-17 22:43:19]

Also fixed the call to CreateMethodMirror using handles to include the provided
owner_mirror. (Was Dart_Null().)

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

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:20: static RawObject* CreateClassMirror(const Class&
cls);
On 2013/07/17 22:27:50, Ryan Macnak wrote:
> RawInstance* --- we know they will be mirrors

Done.

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:23: const Instance& owner);
On 2013/07/17 22:27:50, Ryan Macnak wrote:
> owner_mirror, no?

Done.

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1027: // default and one using Dart_Handles will
disappear.
On 2013/07/17 22:27:50, Ryan Macnak wrote:
> will get default ... => will replace the version that uses API handles

Done.

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1059: // default and one using Dart_Handles will
disappear.
On 2013/07/17 22:27:50, Ryan Macnak wrote:
> will replace

Done.

https://codereview.chromium.org/19579006/diff/7001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1073: // default and one using Dart_Handles will
disappear.
On 2013/07/17 22:27:50, Ryan Macnak wrote:
> will replace

Done.

[siva, 2013-07-18 20:32:34]

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

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1028: static RawInstance* CreateClassMirror(const Class&
cls) {
Change the signature of the method to the final form that we want for this
function.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1031: DARTSCOPE(isolate);
As discussed it is not necessary to have this DARTSCOPE check here.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1047: }
I think the TODO comment above should end up here stating that this call will go
away and be replaced by inlined functionality.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1051: lib_mirror);
As discussed offline we should probably call this version of
CreateClassMirror, CreateClassMirrorUsingAPI or something like that so that it
is explicit that this method is going away.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1063: static RawInstance* CreateLibraryMirror(const
Library& lib) {
Ditto comment about the signature here, you want it to be in the final form.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1875: func ^= func_ref.referent();
I see this pattern a lot I am wondering if it makes sense to
have some helper functions

e.g:

const Function func =
Function::Handle(MirrorReference::GetFunctionReferent(arguments->NativeArgAt(0)));

And GetFunctionReferent could be something like:
GetFunctionReferent(RawObject* raw_ptr) {
  const MirrorReference& ref = MirrorReference::CheckedHandle(raw_ptr);
  ASSERT(Object::Handle(ref.referent().IsFunction());
  return ref.referent();

}

maybe do this in a different CL.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:906: // TODO(mlippautz): This will go away, as
soon as lazy mirrors go away.
Do you have an issue ID for this?

[siva, 2013-07-18 20:38:45]

lgtm

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

https://codereview.chromium.org/19579006/diff/17001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:24: const Instance& owner_mirror);
Why are these forward declarations needed?

https://codereview.chromium.org/19579006/diff/17001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1028: // the version that uses API handles.
This comment here is misleading it seems to imply that this is not the final
version...

Move it down to the call of CreateLibraryMirrorUsingApi stating that this will
be replaced by actual VM code.

[Michael Lippautz (Google), 2013-07-18 22:01:23]

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

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1028: static RawInstance* CreateClassMirror(const Class&
cls) {
On 2013/07/18 20:32:34, siva wrote:
> Change the signature of the method to the final form that we want for this
> function.

Done.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1031: DARTSCOPE(isolate);
On 2013/07/18 20:32:34, siva wrote:
> As discussed it is not necessary to have this DARTSCOPE check here.

Done.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1047: }
On 2013/07/18 20:32:34, siva wrote:
> I think the TODO comment above should end up here stating that this call will
go
> away and be replaced by inlined functionality.

Done.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1051: lib_mirror);
On 2013/07/18 20:32:34, siva wrote:
> As discussed offline we should probably call this version of
> CreateClassMirror, CreateClassMirrorUsingAPI or something like that so that it
> is explicit that this method is going away.

Done.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1063: static RawInstance* CreateLibraryMirror(const
Library& lib) {
On 2013/07/18 20:32:34, siva wrote:
> Ditto comment about the signature here, you want it to be in the final form.

Done.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1875: func ^= func_ref.referent();
On 2013/07/18 20:32:34, siva wrote:
> I see this pattern a lot I am wondering if it makes sense to
> have some helper functions
> 
> e.g:
> 
> const Function func =
>
Function::Handle(MirrorReference::GetFunctionReferent(arguments->NativeArgAt(0)));
> 
> And GetFunctionReferent could be something like:
> GetFunctionReferent(RawObject* raw_ptr) {
>   const MirrorReference& ref = MirrorReference::CheckedHandle(raw_ptr);
>   ASSERT(Object::Handle(ref.referent().IsFunction());
>   return ref.referent();
> 
> }
> 
> maybe do this in a different CL.

Will try to incorporate in another CL.

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors_impl....
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/19579006/diff/15001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:906: // TODO(mlippautz): This will go away, as
soon as lazy mirrors go away.
On 2013/07/18 20:32:34, siva wrote:
> Do you have an issue ID for this?

Created www.dartbug.com/11897

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

https://codereview.chromium.org/19579006/diff/17001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:24: const Instance& owner_mirror);
On 2013/07/18 20:38:45, siva wrote:
> Why are these forward declarations needed?

Are not needed anymore. (At some point code was ordered differently.)

https://codereview.chromium.org/19579006/diff/17001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1028: // the version that uses API handles.
On 2013/07/18 20:38:45, siva wrote:
> This comment here is misleading it seems to imply that this is not the final
> version...
> 
> Move it down to the call of CreateLibraryMirrorUsingApi stating that this will
> be replaced by actual VM code.

Done.

[Michael Lippautz (Google), 2013-07-18 22:05:10]

Committed patchset #10 manually as r25186 (presubmit successful).