460512 - Avoid AddRef/Release in scriptable helper methods for NodeList

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

This makes the dromaeo DOM tests about 2% faster (some specific tests get a bigger speedup).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Seems to me like it might be better to have a slightly more generic "this is a list" interface which returned nsISupports* and then perhaps we could have all the various other subclasses of nsArraySH implement it too?  I'm not all that happy with this sort of copy/paste if we can avoid it...

If we do want to do this, then isn't calling GetProperty() on nsNodeList guaranteed to assert?  Also, nsArraySH::doCreate should probably be private if it can.

Comment 2

[Peter Van der Beken [:peterv]]

(In reply to comment #1)
> Seems to me like it might be better to have a slightly more generic "this is a
> list" interface which returned nsISupports* and then perhaps we could have all
> the various other subclasses of nsArraySH implement it too?

Sure, seems like a lot of work for little benefit though.

> If we do want to do this, then isn't calling GetProperty() on nsNodeList
> guaranteed to assert?  Also, nsArraySH::doCreate should probably be private if
> it can.

How so? (assuming you mean nsNodeListSH::GetProperty)

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Sure, seems like a lot of work for little benefit though.

Well, the main benefit is avoiding copy/paste of fiddly code.

I'm also OK with said avoidance via macro-izing the relevant functions or something, I guess DO_SOME_STUFF(code) with |code| being two different things for the different parts.

> How so? (assuming you mean nsNodeListSH::GetProperty)

I did mean that.  I somehow missed that it implements GetProperty, so nevermind that part. :(

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v2

Patch is quite a bit bigger :-/. Did run mochitests, no regressions.

Comment 5

[Peter Van der Beken [:peterv]]

Note that I could add assertions in all the FromSupports implementations in case anyone derives a class from one of those classes and uses a different class for the canonical nsISupports pointer.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343919 [details] [diff] [review]
v2

>+++ b/content/html/content/public/nsIHTMLCollection.h
>+class nsINode;

Not needed, right?

>+++ b/content/html/content/src/nsHTMLFormElement.cpp
> nsFormControlList::Item(PRUint32 aIndex, nsIDOMNode** aReturn)
> {
>+  nsresult rv;
>+  nsISupports* item = GetNodeAt(aIndex, &rv);
> 
>+  return item ? CallQueryInterface(item, aReturn) : rv;

So if aIndex is out of bounds we'll return NS_OK (the only thing GetNodeAt() ever sticks in rv) but not set *aReturn?  That seems suboptimal.  Can we set *aReturn to null here?

>+++ b/content/html/content/src/nsHTMLSelectElement.cpp
> nsHTMLOptionCollection::Item(PRUint32 aIndex, nsIDOMNode** aReturn)

Same issue here.

>+// Returns the item at index aIndex, if available. aCount will be 0 if an item
>+// is found or the length of rows otherwise.
>+static nsINode*
> GetItemOrCountInRowGroup(nsIDOMHTMLCollection* rows,

I don't think that comment is correct.  What the function really does is:

  Returns the item at index aIndex if available.  If null is returned,
  then aCount will be set to the number of rows in this row collection.
  Otherwise, the value of aCount is undefined.


>+TableRowsCollection::Item(PRUint32 aIndex, nsIDOMNode** aReturn)

This needs to set *aReturn if GetNodeAt returns null.

>--- a/dom/src/offline/nsDOMOfflineLoadStatusList.cpp

Please check with dcamp about this stuff.

>+++ b/layout/style/nsCSSRules.cpp
>+CSSGroupRuleRuleListImpl::Item(PRUint32 aIndex, nsIDOMCSSRule** aReturn)

Need to set *aReturn if GetItemAt returns null.

> nsCSSFontFaceStyleDecl::GetParentRule(nsIDOMCSSRule** aParentRule)
>+  return static_cast<nsICSSRule*>(ContainingRule())->GetDOMRule(aParentRule);

Why is that cast needed?

>+++ b/layout/style/nsCSSStyleSheet.cpp
>+CSSRuleListImpl::Item(PRUint32 aIndex, nsIDOMCSSRule** aReturn)

Needs to always set *aReturn, no?

Adding the assertions would be nice if you can do it.

r+sr=bzbarsky with the above nits fixed and if dcamp is ok with that removal.

Comment 7

[Dave Camp (:dcamp)]

Comment on attachment 343919 [details] [diff] [review]
v2

Yeah, that should have been removed.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> > nsCSSFontFaceStyleDecl::GetParentRule(nsIDOMCSSRule** aParentRule)
> >+  return static_cast<nsICSSRule*>(ContainingRule())->GetDOMRule(aParentRule);
> 
> Why is that cast needed?

Otherwise I get:

nsCSSRules.cpp: In member function ‘virtual nsresult nsCSSFontFaceStyleDecl::GetParentRule(nsIDOMCSSRule**)’:
nsCSSRules.cpp:1753: error: no matching function for call to ‘nsCSSFontFaceRule::GetDOMRule(nsIDOMCSSRule**&)’
nsCSSRules.h:262: note: candidates are: virtual nsIDOMCSSRule* nsCSSFontFaceRule::GetDOMRule(nsresult*)

Seems I can't call the inline nsICSSRule::GetDOMRule without that cast.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, because the child class declarations hide it.  I bet you get compile warnings about that too....  <sigh>.

I guess if we want to leave the names as-is, add a comment there explaining why the cast is necessary.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Comment 11

[Peter Van der Beken [:peterv]]

I guess I could name the new method GetDOMRuleWeak.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v2.2

Renamed GetDOMRule to GetDOMRuleWeak to avoid name conflict. Boris, I'm assuming you're ok with that, speak up if you're not :-).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Sounds fine to me.