Last Comment Bug 804950 - New DOM binding APIs for Element
: New DOM binding APIs for Element
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla19
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on: 811069 811226
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-10-24 01:35 PDT by Peter Van der Beken [:peterv]
Modified: 2012-12-19 21:59 PST (History)
2 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (222.71 KB, patch)
2012-10-24 01:35 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2012-10-24 01:35:30 PDT
Created attachment 674589 [details] [diff] [review]
v1
Comment 1 Boris Zbarsky [:bz] (TPAC) 2012-10-24 20:04:11 PDT
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.
Comment 2 Peter Van der Beken [:peterv] 2012-11-11 05:18:46 PST
> 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
Comment 3 Boris Zbarsky [:bz] (TPAC) 2012-11-11 08:25:35 PST
> 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...
Comment 4 Peter Van der Beken [:peterv] 2012-11-11 08:40:38 PST
(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?
Comment 5 Boris Zbarsky [:bz] (TPAC) 2012-11-11 08:57:03 PST
> 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.
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-11-11 08:59:54 PST
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.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-11-11 11:30:45 PST
https://hg.mozilla.org/mozilla-central/rev/b2bdbfe06b10

Note You need to log in before you can comment on or make changes to this bug.