Comment on attachment 674589 [details] [diff] [review] v1 >+++ b/content/base/src/nsDOMAttributeMap.cpp >+nsDOMAttributeMap::GetNamedItemNS(const nsAString& aNamespaceURI, >+ const nsAString& aLocalName, >+ mozilla::ErrorResult& aError) Fix indent, please. >+++ b/content/base/src/nsGenericElement.cpp > nsGenericElement::GetPreviousElementSibling() >+ const nsAttrAndChildArray& children = > static_cast<nsGenericElement*>(parent)->mAttrsAndChildren; This is a preexisting problem, but that cast should be to FragmentOrElement*. > nsGenericElement::GetNextElementSibling() >+ const nsAttrAndChildArray& children = > static_cast<nsGenericElement*>(parent)->mAttrsAndChildren; Same. That said, I don't understand why these functions can't just GetNextSibling() and avoid this casting. Followup bug for that is probably fine. >+nsGenericElement::GetElementsByTagName(const nsAString& aQualifiedName) aLocalName, please. >+nsGenericElement::GetElementsByTagName(const nsAString& aQualifiedName, And here. >+nsGenericElement::ScrollIntoView(bool aTop) Can you please make nsGenericHTMLElement::ScrollIntoView call this instead of duplicating the code? > nsGenericElement::SetAttribute(const nsAString& aName, >+ if (!nameAtom) { >+ aError.Throw(NS_ERROR_OUT_OF_MEMORY); Need to return there. >+nsGenericElement::SetAttributeNode(nsIDOMAttr* aNewAttr, ErrorResult& aError) >+ if (!aNewAttr) { It can't be null coming in via WebIDL. Would it make sense to move this null-check to the forwarding macro instead? Alternately, add a comment in the forwarding macro to remove this check when that macro goes away. >+nsGenericElement::RemoveAttributeNode(nsIDOMAttr* aAttribute, >+ if (!aAttribute) { Likewise. >+already_AddRefed<nsIDOMAttr> >+nsGenericElement::SetAttributeNodeNS(nsIDOMAttr* aNewAttr, >+ return map->SetNamedItemNS(aNewAttr, aError).get(); You shouldn't need teh .get() there. >+++ b/content/base/src/nsGenericElement.h >+ void GetClassName(nsAString& aClassName) Since we're moving this to Element, can you file a followup to make ClassList() not use GetClassAttributeName() either? >+#define NS_FORWARD_NSIDOMELEMENT_TO_GENERIC \ >+NS_IMETHOD GetAttribute(const nsAString& name, nsAString& _retval) MOZ_FINAL \ >+{ \ >+ nsString attr; \ >+ GetAttribute(name, attr); \ Hmm. Why does GetAttribute not just take nsAString& as second arg? Seems like that would allow us to forward directly here.... >+NS_IMETHOD SetAttribute(const nsAString& name, \ >+ nsGenericElement::SetAttribute(name, value, rv); \ That won't call the subclass SetAttribute if any of them override. Is that OK? Looks like for RemoveAttribute you went with an approach that calls overrides as needed; why not do that here? By the way, we should strongly consider removing XTF. :( If your editor makes it easy to line up all the '\\' here, maybe do that? Otherwise, don't worry aboout it. >+++ b/content/base/src/nsINode.cpp >+ error = SetOn##name_(cx, OBJECT_TO_JSVAL(listener)); \ JS::ObjectOrNullValue(listener) ? >- return elm->SetEventHandlerToJsval(nsGkAtoms::on##name_, cx, obj, v, \ >- true); \ >+ return elm-> SetEventHandlerToJsval(nsGkAtoms::on##name_, cx, obj, v, \ >+ true); \ That looks unnecessary. Undo it, please. >+++ b/dom/webidl/Element.webidl I assume you updated this to whatever is in http://dom.spec.whatwg.org/#element now? > interface Element : Node { >+/* >+ [Throws] > readonly attribute DOMString? namespaceURI; > readonly attribute DOMString? prefix; > readonly attribute DOMString localName; >+*/ Plese document why these are commented (specifically, that we have not moved them from Node yet... frankly, I doubt this move is web-compatible >+/* > attribute DOMString className; >+*/ This should probably have a FIXME and bug number. >+ //void prepend((Node or DOMString)... nodes); >+ //void append((Node or DOMString)... nodes); >+ //void before((Node or DOMString)... nodes); >+ //void after((Node or DOMString)... nodes); >+ //void replace((Node or DOMString)... nodes); >+ //void remove(); Perhaps document that we don't have them yet? >+/* > }; >+ >+partial interface Element { >+*/ Cute. Maybe document for each partial interface where it comes from, of the things in the license header? >+partial interface Element { >+ [Throws] >+ attribute DOMString innerHTML; e.g. this one doesn't seem to come from any of the things in the license header. >+++ b/js/xpconnect/src/dom_quickstubs.qsconf > 'nsIDOMNodeSelector_QuerySelector': { >+ 'self->QuerySelector(arg0, error);' Should have a \n at the end there. r=me with the nits picked.

> Fix indent, please. Done. > That said, I don't understand why these functions can't just > GetNextSibling() and avoid this casting. Followup bug for that is probably > fine. Changed it in this patch. > aLocalName, please. > And here. Done. > Can you please make nsGenericHTMLElement::ScrollIntoView call this instead > of duplicating the code? Done. > Need to return there. Done. > It can't be null coming in via WebIDL. Would it make sense to move this > null-check to the forwarding macro instead? > Likewise. Done. > You shouldn't need teh .get() there. Done. > Since we're moving this to Element, can you file a followup to make > ClassList() not use GetClassAttributeName() either? Actually, I opted for removing nsINode::Get/SetClassName for now. I'll make a note in bug 810677. > Hmm. Why does GetAttribute not just take nsAString& as second arg? Seems > like that would allow us to forward directly here.... That won't work, we'd have two virtual functions differing just in return type. > That won't call the subclass SetAttribute if any of them override. Is that > OK? Looks like for RemoveAttribute you went with an approach that calls > overrides as needed; why not do that here? No one curently overrides SetAttribute, right? I'd rather remove the other overrides at some point instead of allowing more :-). > By the way, we should strongly consider removing XTF. :( Yup. > If your editor makes it easy to line up all the '\\' here, maybe do that? Done. > JS::ObjectOrNullValue(listener) ? > That looks unnecessary. Undo it, please. This all changed a bit after merging with the new event handler stuff. > I assume you updated this to whatever is in > http://dom.spec.whatwg.org/#element now? Yes. > Plese document why these are commented (specifically, that we have not moved > them from Node yet... frankly, I doubt this move is web-compatible Done. > This should probably have a FIXME and bug number. Bug 810677. > Perhaps document that we don't have them yet? Done. > Cute. Maybe document for each partial interface where it comes from, of the > things in the license header? Done. > >+partial interface Element { > >+ [Throws] > >+ attribute DOMString innerHTML; > > e.g. this one doesn't seem to come from any of the things in the license > header. Added http://domparsing.spec.whatwg.org/ > Should have a \n at the end there. Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/b2bdbfe06b10

> No one curently overrides SetAttribute, right? Right, afaict. Can we enforce that with MOZ_FINAL even though it's non-virtual? > I'd rather remove the other overrides at some point instead of allowing more :-). Sold! > This all changed a bit after merging with the new event handler stuff. The merge doesn't look right to me. error events on nodes shouldn't get an EventHandlerNonNull: that's only for window.onerror. Please undo this; with this fix as-is <script onerror="something"> doesn't behave the same way as setting .onerror on a script element directly...

(In reply to Boris Zbarsky (:bz) from comment #3) > The merge doesn't look right to me. error events on nodes shouldn't get an > EventHandlerNonNull: that's only for window.onerror. They don't get an EventHandlerNonNull, they get an OnErrorEventHandlerNonNull. So I'm not sure what you're saying, they shouldn't get a OnErrorEventHandlerNonNull?

> they shouldn't get a OnErrorEventHandlerNonNull? Correct. The spec here is bogus, in my opinion. I thought Olli and I had filed a spec bug on that, but I can't find it right now.

And I _think_ the difference is detectable from web content via redispatching actual error events that went to window to an element that has an onerror set on it.