209866 - some members in nsGlobalWindow.h can use nsRefPtr

Status: RESOLVED FIXED

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

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

I've got a patch sort of in my tree, I just want to wait for sicking to finish
landing the stuff for bug 209667 first before I do this.

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch

Comment 2

[Johnny Stenback (:jst)]

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

@@ -454,16 +442,14 @@ GlobalWindowImpl::SetNewDocument(nsIDOMD
       } else {
	 // Different origins.	Destroy the navigator object so it gets
	 // recreated for the new document.  The plugins or mime types
	 // arrays may have changed. See bug 150087.
	 mNavigatorHolder = nsnull;

	 mNavigator->SetDocShell(nsnull);
-
-	 NS_RELEASE(mNavigator);
       }
     }

Huh? Don't you want to null out mNavigator here still?

- In GlobalWindowImpl::GetMenubar()
 {
+    if (!mMenubar) {
+      return NS_ERROR_OUT_OF_MEMORY;
+    }
+
+    nsCOMPtr<nsIWebBrowserChrome> browserChrome;
+    if (mDocShell &&
+	 NS_SUCCEEDED(GetWebBrowserChrome(getter_AddRefs(browserChrome)))) {
+      mMenubar->SetWebBrowserChrome(browserChrome);
     }
   }

You didn't write this, but I'd like to see this changed. This seems ridiculous,
no need to check for a non-null mDocShell here, nor for GetWebBrowserChrome
succeeding. Just do:

+    nsCOMPtr<nsIWebBrowserChrome> browserChrome;
+    GetWebBrowserChrome(getter_AddRefs(browserChrome));
+
+    mMenubar->SetWebBrowserChrome(browserChrome);

The worst that'll happen is that we call SetWebBrowserChrome(null), whici is a
nop in this case, so we might as well save us the code for the silly checks.

Same thing in all places where we do the same thing.

Other than that, sr=jst

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

>@@ -454,16 +442,14 @@ GlobalWindowImpl::SetNewDocument(nsIDOMD
>       } else {
>         // Different origins.  Destroy the navigator object so it gets
>         // recreated for the new document.  The plugins or mime types
>         // arrays may have changed. See bug 150087.
>         mNavigatorHolder = nsnull;
> 
>         mNavigator->SetDocShell(nsnull);
>-
>-        NS_RELEASE(mNavigator);
>       }
>     }

Shouldn't this be |mNavigator = nsnull|?

>@@ -572,15 +558,15 @@ GlobalWindowImpl::SetNewDocument(nsIDOMD
>         // has a different origin than the calling script.
>         // clear timeouts and clear the scope
>         ClearAllTimeouts();
> 
>         if (mContext && mJSObject) {
>           if (mNavigator) {
>             nsISupports* navigator =
>-              NS_REINTERPRET_CAST(nsISupports*, mNavigator);
>+              NS_REINTERPRET_CAST(nsISupports*, mNavigator.get());

I don't really like these reintperpret-casts, though you didn't add them. Mind
changing that to cast to something like NS_STATIC_CAST(nsIDOMNavigator*,
mNavigator.get());? (the cast to nsISupports can be implicit)

>     mNavigator = new NavigatorImpl(mDocShell);
>-    NS_ENSURE_TRUE(mNavigator, NS_ERROR_OUT_OF_MEMORY);
>-    NS_ADDREF(mNavigator);
>+    if (!mNavigator) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }

And i'm going to raise my voice aginst this horrible horrible practice,
NS_ENSURE_TRUE is IMHO cleaner.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(the last two comments arn't requirements for the r= )

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fixed.  Thanks for reviews.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw this checkin has caused bug 212532, now fixed