Bug 13830

Created attachment 14666 [details] patch

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.

Created attachment 14673 [details] updated patch Updated to make it still build. Above comments still hold.

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 on attachment 14674 [details] Just the JSHTMLElement This patch looks good, but you should talk to a rendering guru about the innerText issue.

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 on attachment 14675 [details] Just the JSHTMLDocument I think most of JSHTMLDocument stuff this patch moves should go into Document, see bug 13707.

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?

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

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.

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 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

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