Comment on attachment 676734 [details] [diff] [review] v1 Review of attachment 676734 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLObjectElement.cpp @@ +310,5 @@ > } > > + return IsEditableRoot() || > + (Type() == eType_Document && > + nsContentUtils::IsSubDocumentTabbable(const_cast<nsHTMLObjectElement*>(this))); I don't think we should make TabIndex() const because of this part ::: content/html/content/src/nsHTMLUnknownElement.h @@ +8,5 @@ > +#include "nsGenericHTMLElement.h" > +#include "nsIDOMHTMLUnknownElement.h" > + > +class nsHTMLUnknownElement : public nsGenericHTMLElement > + , public nsIDOMHTMLUnknownElement You don't actually include this in nsHTMLUnknownElement.cpp

Comment on attachment 676734 [details] [diff] [review] v1 >+++ b/content/html/content/src/nsGenericHTMLElement.cpp >-NS_IMPL_BOOL_ATTR(nsGenericHTMLElement, Hidden, hidden) I lightly wonder whether it's worth having macros for the new stuff too. Maybe it's not. >-nsGenericHTMLElement::GetClassName(nsAString& aClassName) It's worth documenting in the header where you declare all the WebIDL stuff that we have a className getter/setter returning nsresult that always return null, so we can just use those. >@@ -545,80 +473,18 @@ nsGenericHTMLElement::GetOffsetRect(nsRe >+ return static_cast<nsGenericElement*>(offsetParent); If we can't easily make offsetParent an nsGenericElement, can we at least make this a cast from offsetParent->AsElement() so it will assert if somehow it's not an element? Once we merge Element and nsGenericElement, we can then get rid of the cast. >+nsGenericHTMLElement::SetItemValue(JSContext* aCx, JS::Value aValue, Hrm. We should consider seeing if a union would work here, after we implement union return values. >+ // XXX How do we signal failure here? By throwing something on aError. The codegen will then not throw anything of its own if there's an exception on the JSContext already. >+++ b/content/html/content/src/nsHTMLSelectElement.cpp >+ nsGenericHTMLElement::AppendChild(*aElement, aError); aElement could be null there, as far as I can tell. We need to handle that. >+ while (ancestor != this) { nsContentUtils::ContentIsDescendantOf? >+ parent->InsertBefore(*aElement, aBefore, aError); Again, aElement can be null. > nsHTMLSelectElement::Add(nsIDOMHTMLElement* aElement, >+ nsGenericHTMLElement* htmlElement = >+ nsGenericHTMLElement::FromContentOrNull(element); This is where I think we should null-check and then throw if null or whatever the right behavior is. >+++ b/content/html/content/src/nsHTMLSelectElement.h > nsHTMLOptionCollection::Add(const HTMLOptionOrOptGroupElement& aElement, >+ nsCOMPtr<nsIContent> content = >+ do_QueryInterface(aBefore.Value().GetAsHTMLElement()); This is needed because the codegen still maps HTMLElement to nsIDOMHTMLElement? Please file a followup on making this better and mark it dependend on the bug for flipping on the HTMLElement binding? >+++ b/content/html/content/src/nsHTMLUnknownElement.cpp Like ms2ger said, you're not using the header here... >+++ b/dom/webidl/HTMLElement.webidl >+ // This should move to Element >+ attribute DOMString className; Please file a bug, reference it here? >+ // These should move to Element >+ attribute DOMString innerHTML; >+ attribute DOMString outerHTML; >+ void insertAdjacentHTML(DOMString position, DOMString text); Likewise. r=me with those nits.

Backed out for: https://tbpl.mozilla.org/php/getParsedLog.php?id=17030278&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=17030155&tree=Mozilla-Inbound etc Backout: http://hg.mozilla.org/integration/mozilla-inbound/rev/a6305a66f8c1

Also browser-chrome assertions: https://tbpl.mozilla.org/php/getParsedLog.php?id=17032080&tree=Mozilla-Inbound

(In reply to Ed Morley [:edmorley UTC+0] from comment #4) > Also browser-chrome assertions: > https://tbpl.mozilla.org/php/getParsedLog.php?id=17032080&tree=Mozilla- > Inbound Actually these assertions may be unrelated (also occurring on another tree).