Start checking inputs to dart api functions.

Start using UNWRAP_NONNULL to check inputs to dart api functions.

Improve a few error messages.

Lots of testing.

[siva, 2011-11-15 22:09:49]

library already loaded check needs some changes.

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:321: UNWRAP_NONNULL(source, source_str, String);
This seems pretty hard to read in my opinion, the declaration and the unwrapping
are disjoint and hard to associate.

It should be possible for us to do some macro magic using the macro CLASS_LIST
to have something like

const String& url_str = Api::UnwrapStringHandle(url);
if (url_str.IsNull()) {
  return REPORT_ERROR(url);  // Reports the proper error.
}

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:447: } else if (library.name() != library.url()) {
This is not something that we can rely to check that the library has already
been loaded, one could use the same name as the URL in the #library tag.

You should use the loaded_ field in RawLibrary to check this. We have a bug
currently which is not setting the loaded_ field, maybe that should first be
done in a CL and then we could check against that here.

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1807: EXPECT(!Dart_IsValid(result));
I guess in the new API style this would be EXPECT(Dart_IsError(result));

[turnidge, 2011-11-15 23:30:41]

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:321: UNWRAP_NONNULL(source, source_str, String);
On 2011/11/15 22:09:49, asiva wrote:
> This seems pretty hard to read in my opinion, the declaration and the
unwrapping
> are disjoint and hard to associate.
> 
> It should be possible for us to do some macro magic using the macro CLASS_LIST
> to have something like
> 
> const String& url_str = Api::UnwrapStringHandle(url);
> if (url_str.IsNull()) {
>   return REPORT_ERROR(url);  // Reports the proper error.
> }

Done.

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:447: } else if (library.name() != library.url()) {
Ok.  I'll wait for the fix.

On 2011/11/15 22:09:49, asiva wrote:
> This is not something that we can rely to check that the library has already
> been loaded, one could use the same name as the URL in the #library tag.
> 
> You should use the loaded_ field in RawLibrary to check this. We have a bug
> currently which is not setting the loaded_ field, maybe that should first be
> done in a CL and then we could check against that here.

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:1807: EXPECT(!Dart_IsValid(result));
On 2011/11/15 22:09:49, asiva wrote:
> I guess in the new API style this would be EXPECT(Dart_IsError(result));

Yes.  Synced and resolved.

[turnidge, 2011-11-18 00:50:15]

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8528018/diff/5001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:447: } else if (library.name() != library.url()) {
Updated.

On 2011/11/15 23:30:42, turnidge wrote:
> Ok.  I'll wait for the fix.
> 
> On 2011/11/15 22:09:49, asiva wrote:
> > This is not something that we can rely to check that the library has already
> > been loaded, one could use the same name as the URL in the #library tag.
> > 
> > You should use the loaded_ field in RawLibrary to check this. We have a bug
> > currently which is not setting the loaded_ field, maybe that should first be
> > done in a CL and then we could check against that here.
>

[siva, 2011-11-18 07:04:34]

I think the changelist is missing object.h which has the change for NotLoaded

[turnidge, 2011-11-21 17:01:39]

On 2011/11/18 07:04:34, asiva wrote:
> I think the changelist is missing object.h which has the change for NotLoaded

Oops.  I have added the two missing files.  Please take another look.

[siva, 2011-11-21 18:24:59]

lgtm

http://codereview.chromium.org/8528018/diff/21001/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8528018/diff/21001/runtime/vm/object.h#newcode...
runtime/vm/object.h:1535: bool NotLoaded() const {
The name NotLoaded() sounds like a negation of Loaded() but it is not quite a
pure negation.
How about NoLoadsIssued() or LoadNotIssued() or NoLoadsAttemptedYet()

[turnidge, 2011-11-21 18:52:16]

http://codereview.chromium.org/8528018/diff/21001/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/8528018/diff/21001/runtime/vm/object.h#newcode...
runtime/vm/object.h:1535: bool NotLoaded() const {
On 2011/11/21 18:24:59, asiva wrote:
> The name NotLoaded() sounds like a negation of Loaded() but it is not quite a
> pure negation.
> How about NoLoadsIssued() or LoadNotIssued() or NoLoadsAttemptedYet()

Going with LoadNotStarted for now.