[webkit-reviews] review denied: [Bug 13830] Auto-generate JS DOM bindings for HTMLDocument and most of the rest of HTMLElement : [Attachment 14675] Just the JSHTMLDocument

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 13830: Auto-generate JS DOM bindings for HTMLDocument and most of the rest
of HTMLElement
http://bugs.webkit.org/show_bug.cgi?id=13830

Attachment 14675: Just the JSHTMLDocument
http://bugs.webkit.org/attachment.cgi?id=14675&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 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?