Closed Bug 213324 Opened 21 years ago Closed 11 years ago

Remove getter_AddRefs form that's duplicate of dont_AddRef?

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla25

People

(Reporter: jag+mozilla, Assigned: Ms2ger)

Details

Attachments

(1 file, 1 obsolete file)

Patch
15.97 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
I'm thinking about getting rid of the |getter_AddRefs| form that's wrapped
around functions that return an already AddRef'ed xpcom object through their
return value |nsCOMPtr<nsIFoo> = getter_AddRefs(GetFoo());| and replace those
forms with the |dont_AddRef| equivalent. Though I could also see us keeping that
form of |getter_AddRefs| and getting rid of |dont_AddRef|.

Anyone leaning one way or another?
fwiw, I prefer the dont_AddRef form, since it seems to me to sum up better
what's actually desired.
Attached patch Here we go (obsolete) — — Splinter Review 
Did a |grep -r -B5 -A3 "getter_AddRefs" *| and played with its output in vim
and perl to find all the |getter_AddRefs| I wanted to replace with
|dont_AddRef|.

I'll post something in n.p.m.announce and give people a chance to fix their
code before I check in the nsCOMPtr.h change.
Attachment #128386 - Flags: superreview?(dbaron)
Attachment #128386 - Flags: review?(bzbarsky)
Comment on attachment 128386 [details] [diff] [review]
Here we go

I'll try to get to this tomorrow, but in the meantime it looks to me like a
goodly number of these callsites should be returning already_AddRefed and not
using either getter_AddRefs or dont_AddRef...
bz: I totally agree. Should I do that as part of this, or could we do that as a
next step (they should be easier to find once they're limited to |dont_AddRef|).
Either way is fine with me.
Okay, two-pass it is then.
Comment on attachment 128386 [details] [diff] [review]
Here we go

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>-        nsCOMPtr<nsISupports> supports = getter_AddRefs(value.GetISupportsValue());
>+        nsCOMPtr<nsISupports> supports = dont_AddRef(value.GetISupportsValue());

GetISupportsValue returns already_AddRefed, so you could just skip on the
dont_AddRef here.

>Index: layout/html/base/src/nsTextFrame.cpp

I think I made exactly these changes as part of the patch for bug 213823.... 
you may not even have merge issues.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

>     // Create first-letter style rule
>-    nsRefPtr<nsStyleContext> sc = getter_AddRefs(
>+    nsRefPtr<nsStyleContext> sc = dont_AddRef(
>       GetFirstLetterStyle(aPresContext, blockContent, parentStyleContext));

GetFirstLetterStyle returns already_AddRefed<>, no?  So that dont_AddRef can go
away?

The rest looks great; nice leak fix for that option element leak... r=bzbarsky.
Attachment #128386 - Flags: review?(bz-vacation) → review+
Comment on attachment 128386 [details] [diff] [review]
Here we go

>Index: content/base/src/nsDocumentViewer.cpp

It would be better to make GetPresShellFor return already_AddRefed.


>Index: content/xbl/src/nsBindingManager.cpp
>@@ -600,7 +600,7 @@
>       anonymousElements->Count(&count);
> 
>       while (PRInt32(--count) >= 0) {

How about replacing this:
>-        nsCOMPtr<nsISupports> isupports( getter_AddRefs(anonymousElements->ElementAt(count)) );
>+        nsCOMPtr<nsISupports> isupports( dont_AddRef(anonymousElements->ElementAt(count)) );
>         nsCOMPtr<nsIContent> content( do_QueryInterface(isupports) );

with:
	  nsCOMPtr<nsIContent> content = do_QueryElementAt(anonymousElements,
count);

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>@@ -714,8 +714,8 @@
>   // Check our IID table.
>   if (mInterfaceTable) {
>     nsIIDKey key(aIID);
>-    nsCOMPtr<nsISupports> supports = getter_AddRefs(NS_STATIC_CAST(nsISupports*, 
>-                                                                   mInterfaceTable->Get(&key)));
>+    nsCOMPtr<nsISupports> supports = dont_AddRef(NS_STATIC_CAST(nsISupports*, 
>+                                                                mInterfaceTable->Get(&key)));
>     return supports != nsnull;
>   }
> 

This cast is not needed.

>Index: content/xbl/src/nsXBLPrototypeResources.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeResources.cpp,v
>retrieving revision 1.8
>diff -u -r1.8 nsXBLPrototypeResources.cpp
>--- content/xbl/src/nsXBLPrototypeResources.cpp	6 Mar 2003 23:59:15 -0000	1.8
>+++ content/xbl/src/nsXBLPrototypeResources.cpp	23 Jul 2003 23:03:08 -0000
>@@ -125,7 +125,7 @@
>   mStyleSheetList->Count(&count);
>   PRUint32 i;
>   for (i = 0; i < count; i++) {

You can replace this:
>-    nsCOMPtr<nsISupports> supp = getter_AddRefs(mStyleSheetList->ElementAt(i));
>+    nsCOMPtr<nsISupports> supp = dont_AddRef(mStyleSheetList->ElementAt(i));
>     nsCOMPtr<nsICSSStyleSheet> oldSheet(do_QueryInterface(supp));
with this:
      nsCOMPtr<nsICSSStyleSheet> oldSheet = do_QueryElementAt(mStyleSheetList,
i);


>Index: content/xbl/src/nsXBLResourceLoader.cpp
>@@ -201,7 +201,7 @@
>     PRUint32 count;
>     mResources->mStyleSheetList->Count(&count);
>     for (PRUint32 i = 0; i < count; i++) {
>-      nsCOMPtr<nsISupports> supp = getter_AddRefs(mResources->mStyleSheetList->ElementAt(i));
>+      nsCOMPtr<nsISupports> supp = dont_AddRef(mResources->mStyleSheetList->ElementAt(i));
>       nsCOMPtr<nsICSSStyleSheet> sheet(do_QueryInterface(supp));
> 
>       nsCOMPtr<nsIStyleRuleProcessor> processor;
>@@ -256,7 +256,7 @@
>   PRUint32 eltCount;
>   mBoundElements->Count(&eltCount);
>   for (PRUint32 j = 0; j < eltCount; j++) {
>-    nsCOMPtr<nsISupports> supp = getter_AddRefs(mBoundElements->ElementAt(j));
>+    nsCOMPtr<nsISupports> supp = dont_AddRef(mBoundElements->ElementAt(j));
>     nsCOMPtr<nsIContent> content(do_QueryInterface(supp));
>     
>     PRBool ready = PR_FALSE;

And the same thing for both of these.

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>@@ -12316,7 +12315,7 @@
>     nsIContent* blockContent = aState.mFloatedItems.containingBlock->GetContent();
> 
>     // Create first-letter style rule
>-    nsRefPtr<nsStyleContext> sc = getter_AddRefs(
>+    nsRefPtr<nsStyleContext> sc = dont_AddRef(
>       GetFirstLetterStyle(aPresContext, blockContent, parentStyleContext));
>     if (sc) {
>       // Create a new text frame (the original one will be discarded)

No need for the |dont_AddRef|.


>Index: widget/src/cocoa/nsMenuBarX.cpp
>@@ -122,7 +122,7 @@
>   
>   for (PRUint32 i = numItems; i > 0; --i)
>   {
>-    nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+    nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>     nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(menuSupports);


nsCOMPtr<nsIMenuListener> menuListener = do_QueryElementAt(&mMenusArray, i-1);

>@@ -162,7 +162,7 @@
>     mMenusArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; --i)
>     {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>       nsCOMPtr<nsIMenuListener> thisListener = do_QueryInterface(menuSupports);

Same here.

>Index: widget/src/cocoa/nsMenuX.cpp
>@@ -527,7 +527,7 @@
>     PRUint32    numItems;
>     mMenuItemsArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; i--) {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenuItemsArray.ElementAt(i - 1));    
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenuItemsArray.ElementAt(i - 1));    
>       nsCOMPtr<nsIMenu>         submenu = do_QueryInterface(menuSupports);

and here.

>Index: widget/src/mac/nsMenu.cpp
>@@ -648,7 +648,7 @@
>     // Call MenuItemSelected on the correct nsMenuItem
>     PRInt16 menuItemID = LoWord(((nsMenuEvent)aMenuEvent).mCommand);
>     
>-    nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenuItemsArray.ElementAt(menuItemID - 1));
>+    nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenuItemsArray.ElementAt(menuItemID - 1));
>     nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(menuSupports);
>     if (menuListener)
>     {
>@@ -668,7 +668,7 @@
>   mMenuItemsArray.Count(&numItems);
>   for (PRUint32 i = numItems; i > 0; i--)
>   {
>-    nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenuItemsArray.ElementAt(i - 1));    
>+    nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenuItemsArray.ElementAt(i - 1));    
>     nsCOMPtr<nsIMenu>         submenu = do_QueryInterface(menuSupports);
>     nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(submenu);
>     if (menuListener)
>@@ -747,7 +747,7 @@
>     mMenuItemsArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; i--)
> 	  {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenuItemsArray.ElementAt(i - 1));    
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenuItemsArray.ElementAt(i - 1));    
>       nsCOMPtr<nsIMenu>         submenu = do_QueryInterface(menuSupports);
> 	    nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(submenu);
> 	    if(menuListener) {
>Index: widget/src/mac/nsMenuBar.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/mac/nsMenuBar.cpp,v
>retrieving revision 1.77
>diff -u -r1.77 nsMenuBar.cpp
>--- widget/src/mac/nsMenuBar.cpp	20 Jul 2003 07:47:46 -0000	1.77
>+++ widget/src/mac/nsMenuBar.cpp	23 Jul 2003 23:03:21 -0000
>@@ -180,7 +180,7 @@
>   
>   for (PRUint32 i = numItems; i > 0; --i)
>   {
>-    nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+    nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>     nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(menuSupports);
>     if(menuListener)
>     {
>@@ -216,7 +216,7 @@
>     mMenusArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; --i)
>     {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>       nsCOMPtr<nsIMenuListener> thisListener = do_QueryInterface(menuSupports);
> 	    if (thisListener)
> 	    {

And all of these.

>@@ -588,7 +588,7 @@
>   
>   for (PRInt32 i = numItems - 1; i >= 0; --i)
>   {
>-    nsCOMPtr<nsISupports> thisItem = getter_AddRefs(mMenusArray.ElementAt(i));
>+    nsCOMPtr<nsISupports> thisItem = dont_AddRef(mMenusArray.ElementAt(i));
>     nsCOMPtr<nsIMenu>     menu = do_QueryInterface(thisItem);
>     PRBool isHelpMenu = PR_FALSE;
>     if (menu)

And this.

>Index: widget/src/mac/nsMenuBarX.cpp
>@@ -127,7 +127,7 @@
>   
>   for (PRUint32 i = numItems; i > 0; --i)
>   {
>-    nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+    nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>     nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(menuSupports);
>     if(menuListener)
>     {
>@@ -167,7 +167,7 @@
>     mMenusArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; --i)
>     {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenusArray.ElementAt(i - 1));
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenusArray.ElementAt(i - 1));
>       nsCOMPtr<nsIMenuListener> thisListener = do_QueryInterface(menuSupports);
> 	    if (thisListener)
> 	    {

And these.

>Index: widget/src/mac/nsMenuX.cpp
>@@ -533,7 +533,7 @@
>     PRUint32    numItems;
>     mMenuItemsArray.Count(&numItems);
>     for (PRUint32 i = numItems; i > 0; i--) {
>-      nsCOMPtr<nsISupports>     menuSupports = getter_AddRefs(mMenuItemsArray.ElementAt(i - 1));    
>+      nsCOMPtr<nsISupports>     menuSupports = dont_AddRef(mMenuItemsArray.ElementAt(i - 1));    
>       nsCOMPtr<nsIMenu>         submenu = do_QueryInterface(menuSupports);
>       nsCOMPtr<nsIMenuListener> menuListener = do_QueryInterface(submenu);
>       if (menuListener) {

and this.

>Index: xpcom/glue/nsCOMPtr.h

Maybe wait a bit to make this change?
Attachment #128386 - Flags: superreview?(dbaron) → superreview+
Assignee: jag → nobody
QA Contact: scc → xpcom
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INCOMPLETE
Attached patch Patch — — Splinter Review 
Assignee: nobody → Ms2ger

        Attachment #128386 -
        Attachment is obsolete: true
Status: RESOLVED → REOPENED

        Attachment #769799 -
        Flags: review?(ehsan)
Resolution: INCOMPLETE → ---

    
    

    
  
Comment on attachment 769799 [details] [diff] [review]
Patch

Review of attachment 769799 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/time/TimeChangeObserver.cpp
@@ +143,1 @@
>    mWindowListeners.RemoveElement(NS_GetWeakReference(aWindow));

Please file a follow-up bug to investigate this.

        Attachment #769799 -
        Flags: review?(ehsan) → review+

    
    

    
  
>    mWindowListeners.RemoveElement(NS_GetWeakReference(aWindow));

That looks like a leak to me at first glance!

    
    

    
  
Filed bug 890239.
Status: REOPENED → ASSIGNED

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/344c716aefa6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: