Closed Bug 460512 Opened 11 years ago Closed 11 years ago

Avoid AddRef/Release in scriptable helper methods for NodeList

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: peterv, Assigned: peterv)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This makes the dromaeo DOM tests about 2% faster (some specific tests get a bigger speedup).
Attachment #343594 - Flags: superreview?(bzbarsky)
Attachment #343594 - Flags: review?(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.
(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)
> 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. :(
Attached patch v2 (obsolete) — Splinter Review
Patch is quite a bit bigger :-/. Did run mochitests, no regressions.
Attachment #343594 - Attachment is obsolete: true
Attachment #343919 - Flags: superreview?(bzbarsky)
Attachment #343919 - Flags: review?(bzbarsky)
Attachment #343594 - Flags: superreview?(bzbarsky)
Attachment #343594 - Flags: review?(bzbarsky)
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 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.
Attachment #343919 - Flags: superreview?(bzbarsky)
Attachment #343919 - Flags: superreview+
Attachment #343919 - Flags: review?(dcamp)
Attachment #343919 - Flags: review?(bzbarsky)
Attachment #343919 - Flags: review+
Comment on attachment 343919 [details] [diff] [review]
v2

Yeah, that should have been removed.
Attachment #343919 - Flags: review?(dcamp) → review+
(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.
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.
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #343919 - Attachment is obsolete: true
Attachment #344152 - Flags: superreview+
Attachment #344152 - Flags: review+
I guess I could name the new method GetDOMRuleWeak.
Attached patch v2.2Splinter Review
Renamed GetDOMRule to GetDOMRuleWeak to avoid name conflict. Boris, I'm assuming you're ok with that, speak up if you're not :-).
Attachment #344152 - Attachment is obsolete: true
Attachment #344284 - Flags: superreview+
Attachment #344284 - Flags: review+
Sounds fine to me.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.