Bug 528731 - Cleanup nsDataObj drag code. r=roc.
authorJim Mathies <jmathies@mozilla.com>
Mon, 21 Dec 2009 14:48:12 -0600
changeset 36517 b53e720e26e6
parent 36516 b45c4ef383b2
child 36518 06dd18a36470
push id10853
push userjmathies@mozilla.com
push dateMon, 21 Dec 2009 20:49:05 +0000
treeherdermozilla-central@b53e720e26e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersroc
bugs528731
milestone1.9.3a1pre
Bug 528731 - Cleanup nsDataObj drag code. r=roc.
widget/src/windows/nsDataObj.cpp
widget/src/windows/nsDataObj.h
--- a/widget/src/windows/nsDataObj.cpp
+++ b/widget/src/windows/nsDataObj.cpp
@@ -92,26 +92,16 @@ MakeRandomString(char *buf, PRInt32 bufL
 
 // XXX for older version of PSDK where IAsyncOperation and related stuff is not available
 // but this should be removed when parocles config is updated
 // IAsyncOperation interface GUID
 #ifndef __IAsyncOperation_INTERFACE_DEFINED__
   const IID IID_IAsyncOperation = {0x3D8B0590, 0xF691, 0x11d2, {0x8E, 0xA9, 0x00, 0x60, 0x97, 0xDF, 0x5B, 0xD4}};
 #endif
 
-#if 0
-#define PRNTDEBUG(_x) printf(_x);
-#define PRNTDEBUG2(_x1, _x2) printf(_x1, _x2);
-#define PRNTDEBUG3(_x1, _x2, _x3) printf(_x1, _x2, _x3);
-#else
-#define PRNTDEBUG(_x) // printf(_x);
-#define PRNTDEBUG2(_x1, _x2) // printf(_x1, _x2);
-#define PRNTDEBUG3(_x1, _x2, _x3) // printf(_x1, _x2, _x3);
-#endif
-
 //-----------------------------------------------------------------------------
 // CStream implementation
 nsDataObj::CStream::CStream() : mRefCount(1)
 {
 }
 
 //-----------------------------------------------------------------------------
 nsDataObj::CStream::~CStream()
@@ -145,17 +135,17 @@ STDMETHODIMP nsDataObj::CStream::QueryIn
   }
 
   if (NULL != *ppvResult)
   {
     ((LPUNKNOWN)*ppvResult)->AddRef();
     return S_OK;
   }
 
-  return ResultFromScode(E_NOINTERFACE);
+  return E_NOINTERFACE;
 }
 
 //-----------------------------------------------------------------------------
 STDMETHODIMP_(ULONG) nsDataObj::CStream::AddRef(void)
 {
   return ++mRefCount;
 }
 
@@ -421,35 +411,33 @@ STDMETHODIMP nsDataObj::QueryInterface(R
 		AddRef();
 		return S_OK;
   } else if (IID_IAsyncOperation == riid) {
     *ppv = static_cast<IAsyncOperation*>(this);
     AddRef();
     return S_OK;
   }
 
-	return ResultFromScode(E_NOINTERFACE);
+	return E_NOINTERFACE;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP_(ULONG) nsDataObj::AddRef()
 {
 	++m_cRef;
 	NS_LOG_ADDREF(this, m_cRef, "nsDataObj", sizeof(*this));
-  //PRNTDEBUG3("nsDataObj::AddRef  >>>>>>>>>>>>>>>>>> %d on %p\n", (m_cRef+1), this);
 	return m_cRef;
 }
 
 
 //-----------------------------------------------------
 STDMETHODIMP_(ULONG) nsDataObj::Release()
 {
-  //PRNTDEBUG3("nsDataObj::Release >>>>>>>>>>>>>>>>>> %d on %p\n", (m_cRef-1), this);
-
 	--m_cRef;
+	
 	NS_LOG_RELEASE(this, m_cRef, "nsDataObj");
 	if (0 != m_cRef)
 		return m_cRef;
 
   // We have released our last ref on this object and need to delete the
   // temp file. External app acting as drop target may still need to open the
   // temp file. Addref a timer so it can delay deleting file and destroying
   // this object. Delete file anyway and destroy this obj if there's a problem.
@@ -468,299 +456,266 @@ STDMETHODIMP_(ULONG) nsDataObj::Release(
 	delete this;
 
 	return 0;
 }
 
 //-----------------------------------------------------
 BOOL nsDataObj::FormatsMatch(const FORMATETC& source, const FORMATETC& target) const
 {
-	if ((source.cfFormat == target.cfFormat) &&
-		 (source.dwAspect  & target.dwAspect)  &&
-		 (source.tymed     & target.tymed))       {
-		return TRUE;
-	} else {
-		return FALSE;
-	}
+  if ((source.cfFormat == target.cfFormat) &&
+      (source.dwAspect & target.dwAspect) &&
+      (source.tymed & target.tymed)) {
+    return TRUE;
+  } else {
+    return FALSE;
+  }
 }
 
 //-----------------------------------------------------
 // IDataObject methods
 //-----------------------------------------------------
-STDMETHODIMP nsDataObj::GetData(LPFORMATETC pFE, LPSTGMEDIUM pSTM)
+STDMETHODIMP nsDataObj::GetData(LPFORMATETC aFormat, LPSTGMEDIUM pSTM)
 {
-  PRNTDEBUG("nsDataObj::GetData\n");
-  PRNTDEBUG3("  format: %d  Text: %d\n", pFE->cfFormat, CF_HDROP);
-  if ( !mTransferable )
-	  return ResultFromScode(DATA_E_FORMATETC);
+  if (!mTransferable)
+    return DV_E_FORMATETC;
 
   PRUint32 dfInx = 0;
 
   static CLIPFORMAT fileDescriptorFlavorA = ::RegisterClipboardFormat( CFSTR_FILEDESCRIPTORA ); 
   static CLIPFORMAT fileDescriptorFlavorW = ::RegisterClipboardFormat( CFSTR_FILEDESCRIPTORW ); 
   static CLIPFORMAT uniformResourceLocatorA = ::RegisterClipboardFormat( CFSTR_INETURLA );
   static CLIPFORMAT uniformResourceLocatorW = ::RegisterClipboardFormat( CFSTR_INETURLW );
   static CLIPFORMAT fileFlavor = ::RegisterClipboardFormat( CFSTR_FILECONTENTS ); 
   static CLIPFORMAT PreferredDropEffect = ::RegisterClipboardFormat( CFSTR_PREFERREDDROPEFFECT );
 
-  // Arbitrary system formats
+  // Arbitrary system formats are used for image feedback during drag
+  // and drop. We are responsible for storing these internally during
+  // drag operations.
   LPDATAENTRY pde;
-  HRESULT hres = FindFORMATETC(pFE, &pde, FALSE);
-  if (SUCCEEDED(hres)) {
-      return AddRefStgMedium(&pde->stgm, pSTM, FALSE);
+  if (LookupArbitraryFormat(aFormat, &pde, FALSE)) {
+    return CopyMediumData(pSTM, &pde->stgm, aFormat, FALSE);
   }
 
   // Firefox internal formats
   ULONG count;
   FORMATETC fe;
   m_enumFE->Reset();
   while (NOERROR == m_enumFE->Next(1, &fe, &count)
          && dfInx < mDataFlavors.Length()) {
     nsCString& df = mDataFlavors.ElementAt(dfInx);
-    if (FormatsMatch(fe, *pFE)) {
+    if (FormatsMatch(fe, *aFormat)) {
       pSTM->pUnkForRelease = NULL;        // caller is responsible for deleting this data
-      CLIPFORMAT format = pFE->cfFormat;
+      CLIPFORMAT format = aFormat->cfFormat;
       switch(format) {
 
       // Someone is asking for plain or unicode text
       case CF_TEXT:
       case CF_UNICODETEXT:
-      return GetText(df, *pFE, *pSTM);
+      return GetText(df, *aFormat, *pSTM);
 
       // Some 3rd party apps that receive drag and drop files from the browser
       // window require support for this.
       case CF_HDROP:
-        return GetFile(*pFE, *pSTM);
+        return GetFile(*aFormat, *pSTM);
 
       // Someone is asking for an image
       case CF_DIB:
-        return GetDib(df, *pFE, *pSTM);
-
-      // ... not yet implemented ...
-      //case CF_BITMAP:
-      //  return GetBitmap(*pFE, *pSTM);
-      //case CF_METAFILEPICT:
-      //  return GetMetafilePict(*pFE, *pSTM);
+        return GetDib(df, *aFormat, *pSTM);
 
       default:
         if ( format == fileDescriptorFlavorA )
-          return GetFileDescriptor ( *pFE, *pSTM, PR_FALSE );
+          return GetFileDescriptor ( *aFormat, *pSTM, PR_FALSE );
         if ( format == fileDescriptorFlavorW )
-          return GetFileDescriptor ( *pFE, *pSTM, PR_TRUE);
+          return GetFileDescriptor ( *aFormat, *pSTM, PR_TRUE);
         if ( format == uniformResourceLocatorA )
-          return GetUniformResourceLocator( *pFE, *pSTM, PR_FALSE);
+          return GetUniformResourceLocator( *aFormat, *pSTM, PR_FALSE);
         if ( format == uniformResourceLocatorW )
-          return GetUniformResourceLocator( *pFE, *pSTM, PR_TRUE);
+          return GetUniformResourceLocator( *aFormat, *pSTM, PR_TRUE);
         if ( format == fileFlavor )
-          return GetFileContents ( *pFE, *pSTM );
+          return GetFileContents ( *aFormat, *pSTM );
         if ( format == PreferredDropEffect )
-          return GetPreferredDropEffect( *pFE, *pSTM );
-        PRNTDEBUG2("***** nsDataObj::GetData - Unknown format %u\n", format);
-        return GetText(df, *pFE, *pSTM);
+          return GetPreferredDropEffect( *aFormat, *pSTM );
+        //printf("***** nsDataObj::GetData - Unknown format %u\n", format);
+        return GetText(df, *aFormat, *pSTM);
       } //switch
     } // if
     dfInx++;
   } // while
 
-  return ResultFromScode(DATA_E_FORMATETC);
+  return DATA_E_FORMATETC;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::GetDataHere(LPFORMATETC pFE, LPSTGMEDIUM pSTM)
 {
-  PRNTDEBUG("nsDataObj::GetDataHere\n");
-		return ResultFromScode(E_FAIL);
+  return E_FAIL;
 }
 
 
 //-----------------------------------------------------
 // Other objects querying to see if we support a 
 // particular format
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::QueryGetData(LPFORMATETC pFE)
 {
-  PRNTDEBUG("nsDataObj::QueryGetData  ");
-  PRNTDEBUG2("format: %d\n", pFE->cfFormat);
-
-  // Arbitrary system formats
+  // Arbitrary system formats are used for image feedback during drag
+  // and drop. We are responsible for storing these internally during
+  // drag operations.
   LPDATAENTRY pde;
-  if (SUCCEEDED(FindFORMATETC(pFE, &pde, FALSE)))
+  if (LookupArbitraryFormat(pFE, &pde, FALSE))
     return S_OK;
 
   // Firefox internal formats
   ULONG count;
   FORMATETC fe;
   m_enumFE->Reset();
   while (NOERROR == m_enumFE->Next(1, &fe, &count)) {
     if (fe.cfFormat == pFE->cfFormat) {
       return S_OK;
     }
   }
-
-  PRNTDEBUG2("***** nsDataObj::QueryGetData - Unknown format %d\n", pFE->cfFormat);
-	return ResultFromScode(E_FAIL);
+  return E_FAIL;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::GetCanonicalFormatEtc
 	 (LPFORMATETC pFEIn, LPFORMATETC pFEOut)
 {
-  PRNTDEBUG("nsDataObj::GetCanonicalFormatEtc\n");
-		return ResultFromScode(E_FAIL);
-}
-
-HGLOBAL nsDataObj::GlobalClone(HGLOBAL hglobIn)
-{
-  HGLOBAL hglobOut = NULL;
-
-  LPVOID pvIn = GlobalLock(hglobIn);
-  if (pvIn) {
-    SIZE_T cb = GlobalSize(hglobIn);
-    HGLOBAL hglobOut = GlobalAlloc(GMEM_FIXED, cb);
-    if (hglobOut) {
-      CopyMemory(hglobOut, pvIn, cb);
-    }
-    GlobalUnlock(hglobIn);
-  }
-  return hglobOut;
-}
-
-IUnknown* nsDataObj::GetCanonicalIUnknown(IUnknown *punk)
-{
-  IUnknown *punkCanonical;
-  if (punk && SUCCEEDED(punk->QueryInterface(IID_IUnknown,
-                                             (LPVOID*)&punkCanonical))) {
-    punkCanonical->Release();
-  } else {
-    punkCanonical = punk;
-  }
-  return punkCanonical;
+  return E_FAIL;
 }
 
 //-----------------------------------------------------
-STDMETHODIMP nsDataObj::SetData(LPFORMATETC pFE, LPSTGMEDIUM pSTM, BOOL fRelease)
+STDMETHODIMP nsDataObj::SetData(LPFORMATETC aFormat, LPSTGMEDIUM aMedium, BOOL shouldRel)
 {
-  PRNTDEBUG("nsDataObj::SetData\n");
-  // Store arbitrary system formats
+  // Arbitrary system formats are used for image feedback during drag
+  // and drop. We are responsible for storing these internally during
+  // drag operations.
   LPDATAENTRY pde;
-  HRESULT hres = FindFORMATETC(pFE, &pde, TRUE); // add
-  if (SUCCEEDED(hres)) {
+  if (LookupArbitraryFormat(aFormat, &pde, TRUE)) {
+    // Release the old data the lookup handed us for this format. This
+    // may have been set in CopyMediumData when we originally stored the
+    // data.
     if (pde->stgm.tymed) {
       ReleaseStgMedium(&pde->stgm);
-      ZeroMemory(&pde->stgm, sizeof(STGMEDIUM));
+      memset(&pde->stgm, 0, sizeof(STGMEDIUM));
     }
 
-    if (fRelease) {
-      pde->stgm = *pSTM;
-      hres = S_OK;
+    PRBool result = PR_TRUE;
+    if (shouldRel) {
+      // If shouldRel is TRUE, the data object called owns the storage medium
+      // after the call returns. Store the incoming data in our data array for
+      // release when we are destroyed. This is the common case with arbitrary
+      // data from explorer.
+      pde->stgm = *aMedium;
     } else {
-      hres = AddRefStgMedium(pSTM, &pde->stgm, TRUE);
+      // Copy the incoming data into our data array. (AFAICT, this never gets
+      // called with arbitrary formats for drag images.)
+      result = CopyMediumData(&pde->stgm, aMedium, aFormat, TRUE);
     }
     pde->fe.tymed = pde->stgm.tymed;
 
-    // Break circular reference loop (see msdn)
-    if (GetCanonicalIUnknown(pde->stgm.pUnkForRelease) ==
-        GetCanonicalIUnknown(static_cast<IDataObject*>(this))) {
-      pde->stgm.pUnkForRelease->Release();
-      pde->stgm.pUnkForRelease = NULL;
-    }
-    return hres;
+    return result ? S_OK : DV_E_TYMED;
   }
 
-  if (fRelease)
-    ReleaseStgMedium(pSTM);
+  if (shouldRel)
+    ReleaseStgMedium(aMedium);
 
-  return ResultFromScode(S_OK);
+  return S_OK;
 }
 
-HRESULT
-nsDataObj::FindFORMATETC(FORMATETC *pfe, LPDATAENTRY *ppde, BOOL fAdd)
+PRBool
+nsDataObj::LookupArbitraryFormat(FORMATETC *aFormat, LPDATAENTRY *aDataEntry, BOOL aAddorUpdate)
 {
-  *ppde = NULL;
+  *aDataEntry = NULL;
 
-  if (pfe->ptd != NULL) return DV_E_DVTARGETDEVICE;
+  if (aFormat->ptd != NULL)
+    return PR_FALSE;
 
-  // See if it's in our list
+  // See if it's already in our list. If so return the data entry.
   for (PRUint32 idx = 0; idx < mDataEntryList.Length(); idx++) {
-    if (mDataEntryList[idx]->fe.cfFormat == pfe->cfFormat &&
-        mDataEntryList[idx]->fe.dwAspect == pfe->dwAspect &&
-        mDataEntryList[idx]->fe.lindex == pfe->lindex) {
-      if (fAdd || (mDataEntryList[idx]->fe.tymed & pfe->tymed)) {
-        *ppde = mDataEntryList[idx];
-        return S_OK;
+    if (mDataEntryList[idx]->fe.cfFormat == aFormat->cfFormat &&
+        mDataEntryList[idx]->fe.dwAspect == aFormat->dwAspect &&
+        mDataEntryList[idx]->fe.lindex == aFormat->lindex) {
+      if (aAddorUpdate || (mDataEntryList[idx]->fe.tymed & aFormat->tymed)) {
+        // If the caller requests we update, or if the 
+        // medium type matches, return the entry. 
+        *aDataEntry = mDataEntryList[idx];
+        return PR_TRUE;
       } else {
-        return DV_E_TYMED;
+        // Medium does not match, not found.
+        return PR_FALSE;
       }
     }
   }
 
-  if (!fAdd)
-    return DV_E_FORMATETC;
+  if (!aAddorUpdate)
+    return PR_FALSE;
 
-  LPDATAENTRY pde = (LPDATAENTRY)CoTaskMemAlloc(sizeof(DATAENTRY));
-  if (pde) {
-    pde->fe = *pfe;
-    *ppde = pde;
-    ZeroMemory(&pde->stgm, sizeof(STGMEDIUM));
+  // Add another entry to mDataEntryList
+  LPDATAENTRY dataEntry = (LPDATAENTRY)CoTaskMemAlloc(sizeof(DATAENTRY));
+  if (!dataEntry)
+    return PR_FALSE;
+  
+  dataEntry->fe = *aFormat;
+  *aDataEntry = dataEntry;
+  memset(&dataEntry->stgm, 0, sizeof(STGMEDIUM));
 
-    m_enumFE->AddFormatEtc(pfe);
-    mDataEntryList.AppendElement(pde);
+  // Add this to our IEnumFORMATETC impl. so we can return it when
+  // it's requested.
+  m_enumFE->AddFormatEtc(aFormat);
 
-    return S_OK;
-  } else {
-    return E_OUTOFMEMORY;
-  }
+  // Store a copy internally in the arbitrary formats array.
+  mDataEntryList.AppendElement(dataEntry);
+
+  return PR_TRUE;
 }
 
-HRESULT
-nsDataObj::AddRefStgMedium(STGMEDIUM *pstgmIn, STGMEDIUM *pstgmOut, BOOL fCopyIn)
+PRBool
+nsDataObj::CopyMediumData(STGMEDIUM *aMediumDst, STGMEDIUM *aMediumSrc, LPFORMATETC aFormat, BOOL aSetData)
 {
-  HRESULT hres = S_OK;
-  STGMEDIUM stgmOut = *pstgmIn;
-
-  if (pstgmIn->pUnkForRelease == NULL &&
-      !(pstgmIn->tymed & (TYMED_ISTREAM | TYMED_ISTORAGE))) {
-    if (fCopyIn) {
-      // Object needs to be cloned
-      if (pstgmIn->tymed == TYMED_HGLOBAL) {
-        stgmOut.hGlobal = GlobalClone(pstgmIn->hGlobal);
-        if (!stgmOut.hGlobal) {
-          hres = E_OUTOFMEMORY;
+  STGMEDIUM stgmOut = *aMediumSrc;
+  
+  switch (stgmOut.tymed) {
+    case TYMED_ISTREAM:
+      stgmOut.pstm->AddRef();
+    break;
+    case TYMED_ISTORAGE:
+      stgmOut.pstg->AddRef();
+    break;
+    case TYMED_HGLOBAL:
+      if (!aMediumSrc->pUnkForRelease) {
+        if (aSetData) {
+          if (aMediumSrc->tymed != TYMED_HGLOBAL)
+            return PR_FALSE;
+          stgmOut.hGlobal = OleDuplicateData(aMediumSrc->hGlobal, aFormat->cfFormat, 0);
+          if (!stgmOut.hGlobal)
+            return PR_FALSE;
+        } else {
+          // We are returning this data from LookupArbitraryFormat, indicate to the
+          // shell we hold it and will free it.
+          stgmOut.pUnkForRelease = static_cast<IDataObject*>(this);
         }
-      } else {
-        hres = DV_E_TYMED;
       }
-    } else {
-      stgmOut.pUnkForRelease = static_cast<IDataObject*>(this);
-    }
+    break;
+    default:
+      return PR_FALSE;
   }
 
-  if (SUCCEEDED(hres)) {
-    switch (stgmOut.tymed) {
-    case TYMED_ISTREAM:
-      stgmOut.pstm->AddRef();
-      break;
-    case TYMED_ISTORAGE:
-      stgmOut.pstg->AddRef();
-      break;
-    }
-    if (stgmOut.pUnkForRelease) {
-      stgmOut.pUnkForRelease->AddRef();
-    }
-    *pstgmOut = stgmOut;
-  }
+  if (stgmOut.pUnkForRelease)
+    stgmOut.pUnkForRelease->AddRef();
 
-  return hres;
+  *aMediumDst = stgmOut;
+
+  return PR_TRUE;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::EnumFormatEtc(DWORD dwDir, LPENUMFORMATETC *ppEnum)
 {
-  PRNTDEBUG("nsDataObj::EnumFormatEtc\n");
-
   switch (dwDir) {
     case DATADIR_GET:
       m_enumFE->Clone(ppEnum);
       break;
     case DATADIR_SET:
       // fall through
     default:
       *ppEnum = NULL;
@@ -772,33 +727,30 @@ STDMETHODIMP nsDataObj::EnumFormatEtc(DW
   // Clone already AddRefed the result so don't addref it again.
   return NOERROR;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::DAdvise(LPFORMATETC pFE, DWORD dwFlags,
 										            LPADVISESINK pIAdviseSink, DWORD* pdwConn)
 {
-  PRNTDEBUG("nsDataObj::DAdvise\n");
-	return ResultFromScode(E_FAIL);
+  return E_FAIL;
 }
 
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::DUnadvise(DWORD dwConn)
 {
-  PRNTDEBUG("nsDataObj::DUnadvise\n");
-	return ResultFromScode(E_FAIL);
+  return E_FAIL;
 }
 
 //-----------------------------------------------------
 STDMETHODIMP nsDataObj::EnumDAdvise(LPENUMSTATDATA *ppEnum)
 {
-  PRNTDEBUG("nsDataObj::EnumDAdvise\n");
-	return ResultFromScode(E_FAIL);
+  return E_FAIL;
 }
 
 // IAsyncOperation methods
 STDMETHODIMP nsDataObj::EndOperation(HRESULT hResult,
                                      IBindCtx *pbcReserved,
                                      DWORD dwEffects)
 {
   mIsInOperation = FALSE;
@@ -838,46 +790,34 @@ STDMETHODIMP nsDataObj::StartOperation(I
   return S_OK;
 }
 
 //-----------------------------------------------------
 // GetData and SetData helper functions
 //-----------------------------------------------------
 HRESULT nsDataObj::AddSetFormat(FORMATETC& aFE)
 {
-  PRNTDEBUG("nsDataObj::AddSetFormat\n");
-	return ResultFromScode(S_OK);
+  return S_OK;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::AddGetFormat(FORMATETC& aFE)
 {
-  PRNTDEBUG("nsDataObj::AddGetFormat\n");
-	return ResultFromScode(S_OK);
+  return S_OK;
 }
 
-//-----------------------------------------------------
-HRESULT 
-nsDataObj::GetBitmap ( const nsACString& , FORMATETC&, STGMEDIUM& )
-{
-  PRNTDEBUG("nsDataObj::GetBitmap\n");
-	return ResultFromScode(E_NOTIMPL);
-}
-
-
 //
 // GetDIB
 //
 // Someone is asking for a bitmap. The data in the transferable will be a straight
 // imgIContainer, so just QI it.
 //
 HRESULT 
 nsDataObj :: GetDib ( const nsACString& inFlavor, FORMATETC &, STGMEDIUM & aSTG )
 {
-  PRNTDEBUG("nsDataObj::GetDib\n");
   ULONG result = E_FAIL;
   PRUint32 len = 0;
   nsCOMPtr<nsISupports> genericDataWrapper;
   mTransferable->GetTransferData(PromiseFlatCString(inFlavor).get(), getter_AddRefs(genericDataWrapper), &len);
   nsCOMPtr<imgIContainer> image ( do_QueryInterface(genericDataWrapper) );
   if ( !image ) {
     // Check if the image was put in an nsISupportsInterfacePointer wrapper.
     // This might not be necessary any more, but could be useful for backwards
@@ -1406,17 +1346,17 @@ HRESULT nsDataObj::DropFile(FORMATETC& a
 
   nsAutoString path;
   rv = file->GetPath(path);
   if (NS_FAILED(rv))
     return E_FAIL;
 
   PRUint32 allocLen = path.Length() + 2;
   HGLOBAL hGlobalMemory = NULL;
-  PRUnichar *dest, *dest2;
+  PRUnichar *dest;
 
   hGlobalMemory = GlobalAlloc(GMEM_MOVEABLE, sizeof(DROPFILES) +
                                              allocLen * sizeof(PRUnichar));
   if (!hGlobalMemory)
     return E_FAIL;
 
   DROPFILES* pDropFile = (DROPFILES*)GlobalLock(hGlobalMemory);
 
@@ -1683,29 +1623,29 @@ HRESULT nsDataObj::DropTempFile(FORMATET
   aSTG.hGlobal = hGlobalMemory;
 
   return S_OK;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::GetMetafilePict(FORMATETC&, STGMEDIUM&)
 {
-	return ResultFromScode(E_NOTIMPL);
+	return E_NOTIMPL;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::SetBitmap(FORMATETC&, STGMEDIUM&)
 {
-	return ResultFromScode(E_NOTIMPL);
+	return E_NOTIMPL;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::SetDib(FORMATETC&, STGMEDIUM&)
 {
-	return ResultFromScode(E_FAIL);
+	return E_FAIL;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::SetText  (FORMATETC& aFE, STGMEDIUM& aSTG)
 {
   if (aFE.cfFormat == CF_TEXT && aFE.tymed ==  TYMED_HGLOBAL) {
 		HGLOBAL hString = (HGLOBAL)aSTG.hGlobal;
 		if(hString == NULL)
@@ -1715,23 +1655,23 @@ HRESULT nsDataObj::SetText  (FORMATETC& 
 		char *  pString = (char *) GlobalLock(hString);    
 		if(!pString)
 			return(FALSE);
 
 		GlobalUnlock(hString);
     nsAutoString str; str.AssignWithConversion(pString);
 
   }
-	return ResultFromScode(E_FAIL);
+	return E_FAIL;
 }
 
 //-----------------------------------------------------
 HRESULT nsDataObj::SetMetafilePict(FORMATETC&, STGMEDIUM&)
 {
-	return ResultFromScode(E_FAIL);
+	return E_FAIL;
 }
 
 
 
 //-----------------------------------------------------
 //-----------------------------------------------------
 CLSID nsDataObj::GetClassID() const
 {
--- a/widget/src/windows/nsDataObj.h
+++ b/widget/src/windows/nsDataObj.h
@@ -214,57 +214,56 @@ class nsDataObj : public IDataObject,
 
 	public: // other methods
 
     // Gets the filename from the kFilePromiseURLMime flavour
     nsresult GetDownloadDetails(nsIURI **aSourceURI,
                                 nsAString &aFilename);
 
 	protected:
-	  // help determine the kind of drag
+    // help determine the kind of drag
     PRBool IsFlavourPresent(const char *inFlavour);
 
-		virtual HRESULT AddSetFormat(FORMATETC&  FE);
-		virtual HRESULT AddGetFormat(FORMATETC&  FE);
+    virtual HRESULT AddSetFormat(FORMATETC&  FE);
+    virtual HRESULT AddGetFormat(FORMATETC&  FE);
 
-		virtual HRESULT GetFile ( FORMATETC& aFE, STGMEDIUM& aSTG );
-		virtual HRESULT GetText ( const nsACString& aDF, FORMATETC& aFE, STGMEDIUM & aSTG );
-		virtual HRESULT GetBitmap ( const nsACString& inFlavor, FORMATETC&  FE, STGMEDIUM&  STM);
-		virtual HRESULT GetDib ( const nsACString& inFlavor, FORMATETC &, STGMEDIUM & aSTG );
-		virtual HRESULT GetMetafilePict(FORMATETC&  FE, STGMEDIUM&  STM);
+    virtual HRESULT GetFile ( FORMATETC& aFE, STGMEDIUM& aSTG );
+    virtual HRESULT GetText ( const nsACString& aDF, FORMATETC& aFE, STGMEDIUM & aSTG );
+    virtual HRESULT GetDib ( const nsACString& inFlavor, FORMATETC &, STGMEDIUM & aSTG );
+    virtual HRESULT GetMetafilePict(FORMATETC&  FE, STGMEDIUM&  STM);
         
-        virtual HRESULT DropImage( FORMATETC& aFE, STGMEDIUM& aSTG );
-        virtual HRESULT DropFile( FORMATETC& aFE, STGMEDIUM& aSTG );
-        virtual HRESULT DropTempFile( FORMATETC& aFE, STGMEDIUM& aSTG );
+    virtual HRESULT DropImage( FORMATETC& aFE, STGMEDIUM& aSTG );
+    virtual HRESULT DropFile( FORMATETC& aFE, STGMEDIUM& aSTG );
+    virtual HRESULT DropTempFile( FORMATETC& aFE, STGMEDIUM& aSTG );
 
     virtual HRESULT GetUniformResourceLocator ( FORMATETC& aFE, STGMEDIUM& aSTG, PRBool aIsUnicode ) ;
     virtual HRESULT ExtractUniformResourceLocatorA ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT ExtractUniformResourceLocatorW ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetFileDescriptor ( FORMATETC& aFE, STGMEDIUM& aSTG, PRBool aIsUnicode ) ;
     virtual HRESULT GetFileContents ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetPreferredDropEffect ( FORMATETC& aFE, STGMEDIUM& aSTG );
-   
-		virtual HRESULT SetBitmap(FORMATETC&  FE, STGMEDIUM&  STM);
-		virtual HRESULT SetDib   (FORMATETC&  FE, STGMEDIUM&  STM);
-		virtual HRESULT SetText  (FORMATETC&  FE, STGMEDIUM&  STM);
-		virtual HRESULT SetMetafilePict(FORMATETC&  FE, STGMEDIUM&  STM);
+
+    virtual HRESULT SetBitmap(FORMATETC&  FE, STGMEDIUM&  STM);
+    virtual HRESULT SetDib   (FORMATETC&  FE, STGMEDIUM&  STM);
+    virtual HRESULT SetText  (FORMATETC&  FE, STGMEDIUM&  STM);
+    virtual HRESULT SetMetafilePict(FORMATETC&  FE, STGMEDIUM&  STM);
 
       // Provide the structures needed for an internet shortcut by the shell
     virtual HRESULT GetFileDescriptorInternetShortcutA ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetFileDescriptorInternetShortcutW ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetFileContentsInternetShortcut ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
 
     // IStream implementation
     virtual HRESULT GetFileDescriptor_IStreamA ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetFileDescriptor_IStreamW ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
     virtual HRESULT GetFileContents_IStream ( FORMATETC& aFE, STGMEDIUM& aSTG ) ;
 
     nsresult ExtractShortcutURL ( nsString & outURL ) ;
     nsresult ExtractShortcutTitle ( nsString & outTitle ) ;
-    
+
       // munge our HTML data to win32's CF_HTML spec. Will null terminate
     nsresult BuildPlatformHTML ( const char* inOurHTML, char** outPlatformHTML ) ;
 
     // Used for the SourceURL part of CF_HTML
     nsCString mSourceURL;
 
     BOOL FormatsMatch(const FORMATETC& source, const FORMATETC& target) const;
 
@@ -318,28 +317,25 @@ class nsDataObj : public IDataObject,
       STDMETHOD(UnlockRegion)(ULARGE_INTEGER nStart, ULARGE_INTEGER nBytes, DWORD dwFlags);
       STDMETHOD(Write)(const void* pvBuffer, ULONG nBytesToRead, ULONG* nBytesRead);
     };
 
     HRESULT CreateStream(IStream **outStream);
 
   private:
 
-    // Drag and drop helper data for implementing drag and drop image support 
+    // Drag and drop helper data for implementing drag and drop image support
     typedef struct {
       FORMATETC   fe;
       STGMEDIUM   stgm;
     } DATAENTRY, *LPDATAENTRY;
 
     nsTArray <LPDATAENTRY> mDataEntryList;
+    nsCOMPtr<nsITimer> mTimer;
 
-    HRESULT FindFORMATETC(FORMATETC *pfe, LPDATAENTRY *ppde, BOOL fAdd);
-    HRESULT AddRefStgMedium(STGMEDIUM *pstgmIn, STGMEDIUM *pstgmOut,
-                            BOOL fCopyIn);
-    IUnknown* GetCanonicalIUnknown(IUnknown *punk);
-    HGLOBAL GlobalClone(HGLOBAL hglobIn);
+    PRBool LookupArbitraryFormat(FORMATETC *aFormat, LPDATAENTRY *aDataEntry, BOOL aAddorUpdate);
+    PRBool CopyMediumData(STGMEDIUM *aMediumDst, STGMEDIUM *aMediumSrc, LPFORMATETC aFormat, BOOL aSetData);
     static void RemoveTempFile(nsITimer* aTimer, void* aClosure);
-    nsCOMPtr<nsITimer> mTimer;
 };
 
 
 #endif  // _NSDATAOBJ_H_