Bug 13830 – Auto-generate JS DOM bindings for HTMLDocument and most of the rest of HTMLElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Sam Weinig]

Patch to come

[Sam Weinig]

Created attachment 14666 [details]
patch

[Sam Weinig]

There are two possible places of concern with this patch.  The first is change to generate HTMLElement.innerText which had a call to impl()->document()->updateLayoutIgnorePendingStylesheets(); and now does not.  The implementation does the same call but does some stuff before it that might cause issues. 

A second place for concern is with removing the following from the generator script:

-        if ($dataNode->extendedAttributes->{"HasNameGetter"} || $dataNode->extendedAttributes->{"HasOverridingNameGetter"}) {
-            # if it has a prototype, we need to check that first too
-            push(@implContent, "    if (prototype()->isObject() && static_cast<JSObject*>(prototype())->hasProperty(exec, propertyName))\n");
-            push(@implContent, "        return false;\n");
-        }

This code caused an incorrect prototype chain in the only other place that [HasOverridingNameGetter] was used and testing the code without it doesn't cause any regressions.

[Sam Weinig]

Created attachment 14673 [details]
updated patch

Updated to make it still build.  Above comments still hold.

[Sam Weinig]

Created attachment 14674 [details]
Just the JSHTMLElement

Splitting this into two patches for easier reviewing.  This first one deals with just the JSHTMLElement autogeneration and the comment about innerText above still holds.

[Adam Roben (:aroben)]

Comment on attachment 14674 [details]
Just the JSHTMLElement

This patch looks good, but you should talk to a rendering guru about the innerText issue.

[Sam Weinig]

Created attachment 14675 [details]
Just the JSHTMLDocument

This one just deals with JSHTMLDocument part of the first patch.  Again, my concerns regarding the changes to the generator script still hold.

[Alexey Proskuryakov]

Comment on attachment 14675 [details]
Just the JSHTMLDocument

I think most of JSHTMLDocument stuff this patch moves should go into Document, see bug 13707.

[Darin Adler]

Comment on attachment 14675 [details]
Just the JSHTMLDocument

+        return toJS(exec, node);
+    } else if (!collection->length())
+        return jsUndefined();

No need for an else here.

+    if (Window* win = Window::retrieveWindow(frame))
+        return win->location();
+    return jsUndefined();

When can retrieveWindow return 0? Do we even need this case? Should it have ASSERT_NOT_REACHED()?

+void JSHTMLDocument::setLocation(ExecState* exec, JSValue* value)
+{
+    if (Frame* frame = static_cast<HTMLDocument*>(impl())->frame()) {

I'd rather have an early return here than the entire function indented.

+        // When assigning location, IE and Mozilla both resolve the URL
+        // relative     not
+        // the target frame.

Formatting problem.

+        DeprecatedString str = value->toString(exec);

This can use String. Document has completeURL for both String and DeprecatedString, so lets use the String version.

+void JSHTMLDocument::setLocation(ExecState* exec, JSValue* value)

Why does this need to be a custom binding. This seems to just call toString and then act on the string. So it could use a standard binding if you added the necessary function to HTMLDocument.

+JSValue* JSHTMLDocument::width(ExecState*) const
+JSValue* JSHTMLDocument::height(ExecState*) const
+JSValue* JSHTMLDocument::dir(ExecState*) const
+void JSHTMLDocument::setDir(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::designMode(ExecState*) const
+void JSHTMLDocument::setDesignMode(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::bgColor(ExecState*) const
+void JSHTMLDocument::setBgColor(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::fgColor(ExecState*) const
+void JSHTMLDocument::setFgColor(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::alinkColor(ExecState*) const
+void JSHTMLDocument::setAlinkColor(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::linkColor(ExecState*) const
+void JSHTMLDocument::setLinkColor(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::vlinkColor(ExecState*) const
+void JSHTMLDocument::setVlinkColor(ExecState* exec, JSValue* value)
+JSValue* JSHTMLDocument::clear(ExecState*, const List&)
+JSValue* JSHTMLDocument::captureEvents(ExecState*, const List&)
+JSValue* JSHTMLDocument::releaseEvents(ExecState*, const List&)

None of these seem to require a custom binding.

+            push(@specials, "DontDelete") unless $attribute->signature->extendedAttributes->{"Deletatable"};

Should be "Deletable", not "Deletatable".

+#if defined(LANGUAGE_JAVASCRIPT)
+                 attribute DOMString domain;
+#else
         readonly attribute DOMString domain;
+#endif

Why do we need this to be read-only for non-JS languages?

[Darin Adler]

Comment on attachment 14674 [details]
Just the JSHTMLElement

-        case ElementInnerText:
-            impl()->document()->updateLayoutIgnorePendingStylesheets();
-            return jsString(element.innerText());

I see that innerText calls it if renderer() is non-0, but can it sometimes make renderer be non-0? If so, then this changes behavior.

+                 // So we use don't convert null to JS null.

I think you need to omit the word "use" here. It would be even easier to understand if you mentioned the attribute that we're not using.

r=me

[Sam Weinig]

Created attachment 14692 [details]
Updated JSHTMLDocument patch

Updated patch addresses most of Darin's comments.  I have left location() and setLocation() as custom because they both deal with JS related concepts in appropriate for the implementation.  I also did not make the function clear() non-custom because doing so would interfere with the function Document.clear() already in the implementation.

 I have not addressed Alexey's comment as I would like to  do that in a separate patch.

[Sam Weinig]

I left domain read-only for non-js platforms because this was the old behavior and conforms to the spec though I could certainly understand wanting to keep all the bindings in line. The change is trivial if you still think we should change it.

[Darin Adler]

Comment on attachment 14692 [details]
Updated JSHTMLDocument patch

+        return toJS(exec, node);
+    } else if (!collection->length())
+        return jsUndefined();

No need for an else here. It would also be nice not to call collection->length() twice.

+        // FIXME: Create an [NoOp] property for legacy functions that do nothing like this.

I don't agree that we want to do this. Lets keep the binding layer as simple as possible. I know it's complicated, but lets not make it more complicated except for when we have to.

r=me

[Sam Weinig]

JSHTMLDocument patch landed in r21681.  JSHTMLElement patch landed in r21683.