Simplify Custom Element constructors to be functions, not wrappers

Source/bindings/v8/CustomElementHelpers.cpp

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 16 matching lines @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "config.h"
 
 #include "bindings/v8/CustomElementHelpers.h"
 
 #include "HTMLNames.h"
 #include "SVGNames.h"
-#include "V8CustomElementConstructor.h"
+#include "V8Document.h"
 #include "V8HTMLElementWrapperFactory.h"
 #include "V8SVGElementWrapperFactory.h"
 #include "bindings/v8/DOMDataStore.h"
 #include "bindings/v8/DOMWrapperWorld.h"
 #include "bindings/v8/ScriptController.h"
 #include "core/dom/CustomElementRegistry.h"
+#include "core/dom/Element.h"
 #include "core/dom/Node.h"
 #include "core/html/HTMLElement.h"
 #include "core/html/HTMLUnknownElement.h"
 #include "core/svg/SVGElement.h"
 
 namespace WebCore {
 
 v8::Handle<v8::Object> CustomElementHelpers::createWrapper(PassRefPtr<Element> impl, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate, const CreateWrapperFunction& createTypeExtensionUpgradeCandidateWrapper)
 {
     ASSERT(impl);
 
     // The constructor and registered lifecycle callbacks should be visible only from main world.
-    // FIXME: This shouldn't be needed once each custom element has its own FunctionTemplate
-    // https://bugs.webkit.org/show_bug.cgi?id=108138
 
     // FIXME: creationContext.IsEmpty() should never happen. Remove
     // this when callers (like InspectorController::inspect) are fixed
     // to never pass an empty creation context.
     if (!CustomElementHelpers::isFeatureAllowed(creationContext.IsEmpty() ? v8::Context::GetCurrent() : creationContext->CreationContext())) {
         v8::Handle<v8::Object> wrapper = V8DOMWrapper::createWrapper(creationContext, &V8HTMLElement::info, impl.get(), isolate);
         if (!wrapper.IsEmpty())
             V8DOMWrapper::associateObjectWithWrapper(impl, &V8HTMLElement::info, wrapper, isolate, WrapperConfiguration::Dependent);
         return wrapper;
     }
@@ skipping 46 matching lines @@
         else {
             ASSERT(0);
             return v8::Handle<v8::Object>();
         }
     } else {
         // It's a type extension
         return createTypeExtensionUpgradeCandidateWrapper.invoke(element.get(), creationContext, isolate);
     }
 }
 
-bool CustomElementHelpers::initializeConstructorWrapper(CustomElementConstructor* constructor, const ScriptValue& prototype, ScriptState* state)
+static void constructCustomElement(const v8::FunctionCallbackInfo<v8::Value>& args)
 {
-    ASSERT(isFeatureAllowed(state));
-    ASSERT(!prototype.v8Value().IsEmpty() && prototype.v8Value()->IsObject());
-    v8::Handle<v8::Value> wrapperValue = toV8(constructor, state->context()->Global(), state->context()->GetIsolate());
-    if (wrapperValue.IsEmpty() || !wrapperValue->IsObject())
-        return false;
-    v8::Handle<v8::Function> wrapper = v8::Handle<v8::Function>::Cast(wrapperValue);
-    // - Object::ForceSet() nor Object::SetAccessor Doesn't work against the "prototype" property of function objects.
-    // - Set()-ing here is safe because
-    //   - Hooking Object.prototype's defineProperty() with "prototype" or "constructor" also doesn't affect on these properties of function objects and
-    //   - Using Set() is okay becaues each function has "prototype" property from start and Objects.prototype cannot intercept the property access.
-    v8::Handle<v8::String> prototypeKey = v8String("prototype", state->context()->GetIsolate());
-    ASSERT(wrapper->HasOwnProperty(prototypeKey));
-    wrapper->Set(prototypeKey, prototype.v8Value(), v8::ReadOnly);
+    v8::Isolate* isolate = args.GetIsolate();
 
-    v8::Handle<v8::String> constructorKey = v8String("constructor", state->context()->GetIsolate());
-    v8::Handle<v8::Object> prototypeObject = v8::Handle<v8::Object>::Cast(prototype.v8Value());
-    ASSERT(!prototypeObject->HasOwnProperty(constructorKey));
-    prototypeObject->ForceSet(constructorKey, wrapper, v8::ReadOnly);
-    return true;
+    if (!args.IsConstructCall()) {
+        throwTypeError("DOM object constructor cannot be called as a function.", isolate);
+        return;
+    }
+

[haraken, 2013/06/10 09:11:03]

I think you need to add the following check here:

  if (ConstructorMode::current() == ConstructorMode::WrapExistingObject) {
    v8SetReturnValue(args, args.Holder());
    return;
  }

[dominicc (has gone to gerrit), 2013/06/11 01:29:02]

This is different to a wrapper constructor; this function is *only* called if
author script is doing

new FooElement()

where FooElement is a custom element. Basically we're using the facility of
JavaScript where a function, even when invoked as a constructor, can return a
different object.

We'll invoke the HTMLElement wrapper constructor as usual as the element is
being wrapped to be returned. But that won't call this function.

+    if (args.Length() > 0) {

[haraken, 2013/06/10 09:11:03]

Nit: Do you need this check?

[dominicc (has gone to gerrit), 2013/06/11 01:29:02]

This is per <http://www.w3.org/TR/WebIDL/#es-interface-call> step 7.

+        throwTypeError(0, isolate);
+        return;
+    }
+
+    Document* document = V8Document::toNative(v8::Handle<v8::Object>::Cast(args.Callee()->GetHiddenValue(v8String("document", isolate))));
+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, namespaceURI, args.Callee()->GetHiddenValue(v8String("namespaceURI", isolate)));

[abarth-chromium, 2013/06/10 17:42:19]

Please use V8HiddenPropertyNames rather than allocating new strings every time.

+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, name, args.Callee()->GetHiddenValue(v8String("name", isolate)));
+    v8::Handle<v8::Value> maybeType = args.Callee()->GetHiddenValue(v8String("type", isolate));
+    V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, type, maybeType);
+
+    ExceptionCode ec = 0;
+    CustomElementRegistry::CallbackDeliveryScope deliveryScope;
+    RefPtr<Element> element = document->createElementNS(namespaceURI, name, maybeType->IsNull() ? nullAtom : type, ec);
+    if (ec) {
+        setDOMException(ec, isolate);
+        return;
+    }
+    v8SetReturnValue(args, toV8Fast(element.release(), args, document));
+}
+
+ScriptValue CustomElementHelpers::createConstructor(ScriptState* state, const ScriptValue& prototypeValue, Document* document, const AtomicString& namespaceURI, const AtomicString& name, const AtomicString& type)

[haraken, 2013/06/10 09:11:03]

Not related to this CL: won't prototypeValue cause a circular reference? If
prototypeValue holds a reference to a window object (I'm not sure if it's
possible or not in terms of the semantics of custom elements), a circle is
produced: window => V8PerContextData => prototypeValue => window.

[dominicc (has gone to gerrit), 2013/06/11 01:29:02]

We discussed this offline: 

- We think that the FunctionTemplate->Function etc. references are OK.

- I am separately going to try to remove the reference from C++ Document->C++
Custom Element Registry->C++ Custom Element Definition->prototypeValue by
weakening it/moving it to V8PerContextData.

+{
+    v8::Isolate* isolate = state->context()->GetIsolate();
+
+    v8::TryCatch tryCatch;
+    v8::Local<v8::FunctionTemplate> constructorTemplate = v8::FunctionTemplate::New();
+    constructorTemplate->SetCallHandler(constructCustomElement);
+    v8::Handle<v8::Function> constructor = constructorTemplate->GetFunction();
+    if (tryCatch.HasCaught()) {
+        state->setException(tryCatch.Exception());
+        return ScriptValue();
+    }
+
+    v8::Handle<v8::String> v8Name = v8String(name, isolate);
+    v8::Handle<v8::Value> v8Type;
+
+    if (type.isNull()) {
+        v8Type = v8::Null(isolate);
+        constructor->SetName(v8Name);

[haraken, 2013/06/10 09:11:03]

You need to somehow handle a case where constructor is Empty.

[dominicc (has gone to gerrit), 2013/06/11 01:29:02]

When would that happen? I think we early return on line 161. I added an
assertion.

[haraken, 2013/06/11 01:39:21]

I just wanted to make sure here that the constructor is not empty. The ASSERT is
fine.

In general, given:

  v8::TryCatch tryCatch;
  v8::Handle<v8::Value> ret = v8::SomeAPI();

tryCatch.HasCaught() and ret.IsEmpty() are not equivalent for some V8 APIs. It's
a good idea to check both of them.

[dominicc (has gone to gerrit), 2013/06/11 02:41:33]

Done.

+    } else {
+        v8::Handle<v8::String> v8TypeString = v8String(type, isolate);

[haraken, 2013/06/10 09:11:03]

By using v8StringOrNull() here, I think you can kill the if branch.

+        v8Type = v8TypeString;
+        constructor->SetName(v8TypeString);
+    }
+
+    constructor->SetHiddenValue(v8String("document", isolate), toV8(document, state->context()->Global(), isolate));

[haraken, 2013/06/10 09:11:03]

Can't you use toV8Fast(), given that we already know a creation context here?

[dominicc (has gone to gerrit), 2013/06/11 01:29:02]

Not sure... ScriptState is not a "HolderContainer", but I could make a wrapper.
What would .Holder be?

[haraken, 2013/06/11 01:39:21]

You're right. Please ignore my comment. You can use toV8Fast() only when you
have 'info' or 'args' passed by V8.

+    constructor->SetHiddenValue(v8String("namespaceURI", isolate), v8String(namespaceURI, isolate));
+    constructor->SetHiddenValue(v8String("name", isolate), v8Name);
+    constructor->SetHiddenValue(v8String("type", isolate), v8Type);
+
+    // Neither Object::ForceSet nor Object::SetAccessor can set the
+    // "prototype" property of function objects, so we use Set()
+    // instead. This is safe because each function has "prototype"
+    // property from birth so the Function, etc. prototypes will not
+    // intercept the property access.
+    v8::Handle<v8::Object> prototype = v8::Handle<v8::Object>::Cast(prototypeValue.v8Value());
+    v8::Handle<v8::String> prototypeKey = v8String("prototype", isolate);
+    ASSERT(constructor->HasOwnProperty(prototypeKey));
+    constructor->Set(prototypeKey, prototype, v8::ReadOnly);
+
+    prototype->ForceSet(v8String("constructor", isolate), constructor, v8::ReadOnly);
+
+    ASSERT(!tryCatch.HasCaught());
+
+    return ScriptValue(constructor);
 }
 
 static bool hasValidPrototypeChainFor(v8::Handle<v8::Object> prototypeObject, WrapperTypeInfo* typeInfo, v8::Handle<v8::Context> context)
 {
     // document.register() sets the constructor property, so the prototype shouldn't have one.
     if (prototypeObject->HasOwnProperty(v8String("constructor", context->GetIsolate())))
         return false;
 
     v8::Handle<v8::Object> elementConstructor = v8::Handle<v8::Object>::Cast(V8PerContextData::from(context)->constructorForType(typeInfo));
     if (elementConstructor.IsEmpty())
@@ skipping 135 matching lines @@
         return;
     v8::Context::Scope scope(context);
 
     for (size_t i = 0; i < invocations.size(); ++i) {
         ASSERT(executionContext == invocations[i].element()->document());
         invokeReadyCallbackIfNeeded(invocations[i].element(), context);
     }
 }
 
 } // namespace WebCore