This is a risky thing to do, since there is no address of operator, and you'll get the address of the object, and not necessarily the address of the pointer inside. Currently they are the same so things work fine now.

Comment on attachment 111692 [details] [diff] [review] Uses raw pointers and then assigns r=adamlock Thanks for catching this, looks right to me

Whast wrong with getter_AddRefs?

The output parameter is taking a void *, and in some cases a void ** instead of an nsISupports pointer, so I figured it wouldn't work.

Comment on attachment 111692 [details] [diff] [review] Uses raw pointers and then assigns slightly corrected logic for /cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v >retrieving revision 1.459 >diff -u -r1.459 nsPluginHostImpl.cpp >--- modules/plugin/base/src/nsPluginHostImpl.cpp 8 Jan 2003 22:11:23 -0000 1.459 >+++ modules/plugin/base/src/nsPluginHostImpl.cpp 16 Jan 2003 13:09:41 -0000 >@@ -6588,9 +6588,11 @@ > mStreamConverter = finalStreamListener; > mRemoveMagicNumber = PR_TRUE; > >+ nsIStreamListener * temp = nsnull; nsIStreamListener * temp = finalStreamListener; > //get nsPluginStreamListenerPeer* ptr from finalStreamListener > nsPluginStreamListenerPeer *pslp = NS_REINTERPRET_CAST(nsPluginStreamListenerPeer*, >- *(NS_REINTERPRET_CAST(void**, &finalStreamListener))); >+ *(NS_REINTERPRET_CAST(void**, &temp))); and remove the next line >+ finalStreamListener = already_AddRefed<nsIStreamListener>(temp); > rv = pslp->ServeStreamAsFile(request, ctxt); > return rv; > }

I was blindly reproducing the pattern, and missed the fact this was a cast and not a function call. Can't this just be simplified to this, or did I miss something? Or is there some magic behind the void ** cast I missing. //get nsPluginStreamListenerPeer* ptr from finalStreamListener nsPluginStreamListenerPeer *pslp = NS_REINTERPRET_CAST(nsPluginStreamListenerPeer*, finalStreamListener.get()); rv = pslp->ServeStreamAsFile(request, ctxt); And reassigning to me, since I've been working on the patch.

finalStreamListener.get()looks good enough I do not remember what problem w/ get forced me to use double cast:(

I know what you mean, and I was a little leary of simplifying it, as I've seen similar trick to get around various compiler and other problems. Bradley, are you ok with the already_AddRefed, or do you still think I should be using getter_AddRefs. I don't mind changing it over, but was just a little leary, and felt safer in the presence of void *'s to be a little more verbose.

Ok, I dug around, and found the void** pattern for getter_AddRefs elsewhere. It's not a direct match to what the plugin API function is doing, taking a void * and then filling in the buffer, it's close though. So I verified that it does do the right thing. It's definitely cleaner looking code. So for the plugin API function that takes a void *, I cast getter_AddRef to a pointer to a pointer nsIFace **. This passes the address of the pointer into the API function, which it then fills in the pointer with a value, and all is well.

yeah, I think that thats better.

Comment on attachment 111800 [details] [diff] [review] Alternative using getter_AddRefs The patch for XPCDocument.cpp uses a C-style cast for one call to NPN_GetValue and NS_STATIC_CAST for the other. Otherwise r=adamlock

Comment on attachment 111800 [details] [diff] [review] Alternative using getter_AddRefs + NPN_GetValue(mData->pPluginInstance, NPNVDOMWindow, + NS_STATIC_CAST(nsIDOMWindow **,getter_AddRefs(mDOMWindow))); Is that cast even needed there? Wouldn't the compiler accept getter_AddRefs(mDOMWindow) w/o the cast? If it doesn't, then leave as is... sr=jst

patched checked in Changed the c-style cast. getter_AddRef won't convert to a void * automatically.

Marking Verified Fixed. Compared attached patch to checkins.