Bug 13830 - Auto-generate JS DOM bindings for HTMLDocument and most of the rest of HTMLElement
Summary: Auto-generate JS DOM bindings for HTMLDocument and most of the rest of HTMLEl...
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 13779
  Show dependency treegraph
Reported: 2007-05-22 14:18 PDT by Sam Weinig
Modified: 2007-05-23 16:57 PDT (History)
0 users

See Also:

patch (55.20 KB, patch)
2007-05-22 14:38 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
updated patch (52.17 KB, patch)
2007-05-22 19:37 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Just the JSHTMLElement (8.19 KB, patch)
2007-05-22 20:15 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff
Just the JSHTMLDocument (44.98 KB, patch)
2007-05-22 20:57 PDT, Sam Weinig
darin: review-
Details | Formatted Diff | Diff
Updated JSHTMLDocument patch (51.43 KB, patch)
2007-05-23 11:43 PDT, Sam Weinig
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2007-05-22 14:18:40 PDT
Patch to come
Comment 1 Sam Weinig 2007-05-22 14:38:51 PDT
Created attachment 14666 [details]
Comment 2 Sam Weinig 2007-05-22 14:43:06 PDT
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.

Comment 3 Sam Weinig 2007-05-22 19:37:45 PDT
Created attachment 14673 [details]
updated patch

Updated to make it still build.  Above comments still hold.
Comment 4 Sam Weinig 2007-05-22 20:15:41 PDT
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.
Comment 5 Adam Roben (:aroben) 2007-05-22 20:19:31 PDT
Comment on attachment 14674 [details]
Just the JSHTMLElement

This patch looks good, but you should talk to a rendering guru about the innerText issue.
Comment 6 Sam Weinig 2007-05-22 20:57:15 PDT
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.
Comment 7 Alexey Proskuryakov 2007-05-23 02:57:52 PDT
Comment on attachment 14675 [details]
Just the JSHTMLDocument

I think most of JSHTMLDocument stuff this patch moves should go into Document, see bug 13707.
Comment 8 Darin Adler 2007-05-23 09:11:16 PDT
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".

+                 attribute DOMString domain;
         readonly attribute DOMString domain;

Why do we need this to be read-only for non-JS languages?
Comment 9 Darin Adler 2007-05-23 09:18:10 PDT
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.

Comment 10 Sam Weinig 2007-05-23 11:43:47 PDT
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.
Comment 11 Sam Weinig 2007-05-23 11:51:39 PDT
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.
Comment 12 Darin Adler 2007-05-23 15:54:47 PDT
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.

Comment 13 Sam Weinig 2007-05-23 16:57:56 PDT
JSHTMLDocument patch landed in r21681.  JSHTMLElement patch landed in r21683.