ParentNode#appendChild(null) shouldn't crash

We just needed to throw the proper exception when handed null for a
non-nullable argument.

R=esprehn@chromium.org

Review URL: https://codereview.chromium.org/934863002

engine/tonic/BUILD.gn

@@ -15,6 +15,8 @@ source_set("tonic") {
     "dart_converter.h",
     "dart_error.cc",
     "dart_error.h",
+    "dart_exception_factory.cc",
+    "dart_exception_factory.h",
     "dart_gc_context.cc",
     "dart_gc_context.h",
     "dart_gc_controller.cc",

engine/tonic/dart_exception_factory.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "sky/engine/config.h"
+#include "sky/engine/tonic/dart_exception_factory.h"
+
+#include "sky/engine/tonic/dart_converter.h"
+#include "sky/engine/tonic/dart_builtin.h"
+
+namespace blink {
+
+DartExceptionFactory::DartExceptionFactory(DartState* dart_state)
+    : dart_state_(dart_state) {
+}
+
+DartExceptionFactory::~DartExceptionFactory() {
+}
+
+Dart_Handle DartExceptionFactory::CreateException(const String& class_name,
+                                                  const String& message) {
+  if (core_library_.is_empty()) {
+    Dart_Handle library = DartBuiltin::LookupLibrary("dart:core");
+    core_library_.Set(dart_state_, library);
+  }
+
+  Dart_Handle exception_class = Dart_GetType(
+      core_library_.value(), StringToDart(dart_state_, class_name), 0, 0);
+  Dart_Handle message_handle = StringToDart(dart_state_, message);
+  Dart_Handle empty_string = Dart_NewStringFromCString("");
+  return Dart_New(exception_class, empty_string, 1, &message_handle);
+}
+
+}  // namespace blink

engine/tonic/dart_exception_factory.h

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SKY_ENGINE_TONIC_DART_EXCEPTION_FACTORY_H_
+#define SKY_ENGINE_TONIC_DART_EXCEPTION_FACTORY_H_
+
+#include "dart/runtime/include/dart_api.h"
+#include "sky/engine/tonic/dart_persistent_value.h"
+#include "sky/engine/wtf/text/WTFString.h"
+
+namespace blink {
+class DartState;
+
+class DartExceptionFactory {
+ public:
+  explicit DartExceptionFactory(DartState* dart_state);
+  ~DartExceptionFactory();
+
+  Dart_Handle CreateException(const String& class_name, const String& message);
+
+ private:
+  DartState* dart_state_;
+  DartPersistentValue core_library_;
+};
+
+}  // namespace blink
+
+#endif  // SKY_ENGINE_TONIC_DART_EXCEPTION_FACTORY_H_

engine/tonic/dart_state.cc

@@ -7,6 +7,7 @@
 
 #include "sky/engine/tonic/dart_class_library.h"
 #include "sky/engine/tonic/dart_string_cache.h"
+#include "sky/engine/tonic/dart_exception_factory.h"
 #include "sky/engine/wtf/PassOwnPtr.h"
 
 namespace blink {
@@ -21,6 +22,7 @@ DartState::DartState()
     : isolate_(NULL),
       class_library_(adoptPtr(new DartClassLibrary)),
       string_cache_(adoptPtr(new DartStringCache)),
+      exception_factory_(adoptPtr(new DartExceptionFactory(this))),
       weak_factory_(this) {
 }
 

engine/tonic/dart_state.h

@@ -16,6 +16,7 @@
 namespace blink {
 class DartStringCache;
 class DartClassLibrary;
+class DartExceptionFactory;
 
 // DartState represents the state associated with a given Dart isolate. The
 // lifetime of this object is controlled by the DartVM. If you want to hold a
@@ -46,11 +47,13 @@ class DartState : public base::SupportsUserData {
 
   DartClassLibrary& class_library() { return *class_library_; }
   DartStringCache& string_cache() { return *string_cache_; }
+  DartExceptionFactory& exception_factory() { return *exception_factory_; }
 
  private:
   Dart_Isolate isolate_;
   OwnPtr<DartClassLibrary> class_library_;
   OwnPtr<DartStringCache> string_cache_;
+  OwnPtr<DartExceptionFactory> exception_factory_;
 
   base::WeakPtrFactory<DartState> weak_factory_;
 

engine/tonic/dart_wrappable.cc

@@ -7,8 +7,10 @@
 
 #include "sky/engine/tonic/dart_class_library.h"
 #include "sky/engine/tonic/dart_error.h"
+#include "sky/engine/tonic/dart_exception_factory.h"
 #include "sky/engine/tonic/dart_state.h"
 #include "sky/engine/tonic/dart_wrapper_info.h"
+#include "sky/engine/wtf/text/StringBuilder.h"
 
 namespace blink {
 
@@ -74,9 +76,17 @@ DartWrappable* DartConverterWrappable::FromArguments(Dart_NativeArguments args,
 
 DartWrappable* DartConverterWrappable::FromArgumentsWithNullCheck(
     Dart_NativeArguments args, int index, Dart_Handle& exception) {
-    Dart_Handle handle = Dart_GetNativeArgument(args, index);
-  if (Dart_IsNull(handle))
+  Dart_Handle handle = Dart_GetNativeArgument(args, index);
+  if (Dart_IsNull(handle)) {
+    DartState* state = DartState::Current();
+    StringBuilder message;
+    message.appendLiteral("Argument ");
+    message.appendNumber(index);
+    message.appendLiteral(" cannot be null.");
+    exception = state->exception_factory().CreateException("ArgumentError",
+                                                           message.toString());
     return nullptr;
+  }
   intptr_t native_fields[DartWrappable::kNumberOfNativeFields];
   Dart_Handle result = Dart_GetNativeFieldsOfArgument(
       args, index, DartWrappable::kNumberOfNativeFields, native_fields);

tests/dom/appendChild.sky

@@ -17,10 +17,9 @@ void main() {
     expect(() {
       parent.appendChild();
     }, throws);
-    // TODO(dart): This is a real bug.
-    // expect(() {
-    //   parent.appendChild(null);
-    // }, throws);
+    expect(() {
+      parent.appendChild(null);
+    }, throws);
     expect(() {
       parent.appendChild({tagName: "div"});
     }, throws);