267426 - an empty file when dragging images (files) out of the browser to the Windows desktop or Explorer or drives other than C:

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Yuri Kovalchuk]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040910
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20040910

When dragging a large image (file) out of Mozilla browser winodw to the desktop
on Windows you can get an empty or incomplete file at the drop location and
sometimes even a "Sharing Violation" error.

Reproducible: Always
Steps to Reproduce:
1. Find some large image on the net (eg.
http://www.pakistan-tradenetwork.com/admin/images/vase-%20large.jpg)
2. Wait until it downloads. Then clear the browser's cache
(Edit->Preferences...Advanced->Cache [Clear cache] button)
3. Try to drag the image to desktop.

Actual Results:  
I get an incomplete file - not the image I was expecting.
When I try to open that newly created file I get this message in mozilla
'The image “http://www.pakistan-tradenetwork.com/admin/images/vase-%20large.jpg”
cannot be displayed, because it contains errors.'

Expected Results:  
Mozilla should download the image (file).

Comment 1

[Yuri Kovalchuk]

Attachment: Suggested changes to the drag and drop code of images and files

This is a copy from Bug#97413. I fixed problems with tabs (converted them to
whitespaces). See bug Bug#97413 for more details since this bug originated from
there.
I suggest a patch to fix this bug. Solution involves creating a hidden window
to trap drop location(drop directory) and start the download to that directory.


Here is a cpoy from bug#97413:
When dragging large images or files out of the browser window using
"application/x-moz-file-promise" with the file or image that has not been
cached yet, you can get a "Sharing Violation" or an incomplete file on Windows.

This patch fixes this issue.

Comment 2

[Yuri Kovalchuk]

I'm sorry, but there were some network promblems and I accidentially file this
bug twice. Please remove the copy (bug#267425) - I do not know how. 

Comment 3

[Gary van der Merwe]

*** Bug 267425 has been marked as a duplicate of this bug. ***

Comment 4

[Jungshik Shin]

Please, generate a unified diff with context and function names as timeless
suggested in bug 97413 comment #54 and bug 97413 comment #58.

> please use 'cvs diff -up10' (pick 10 to be a number which gives reasonable
context)

Comment 5

[Yuri Kovalchuk]

Comment on attachment 164362 [details] [diff] [review]
Suggested changes to the drag and drop code of images and files

cvs diff -u (in directory C:\Mozilla_1.7.3\mozilla\widget\src\windows)
cvs diff: Diffing .
Index: nsDataObj.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v
retrieving revision 1.59
diff -u -r1.59 nsDataObj.cpp
--- nsDataObj.cpp	7 Oct 2003 20:39:25 -0000	1.59
+++ nsDataObj.cpp	5 Nov 2004 15:54:18 -0000
@@ -57,6 +57,9 @@
 #include "nsDirectoryServiceDefs.h"
 #include "prprf.h"
 #include "nsCRT.h"
+#include "nsIURL.h"
+#include "nsNetUtil.h"
+#include "nsEscape.h"

 #include <ole2.h>
 #ifndef __MINGW32__
@@ -763,29 +766,70 @@
 {
   HRESULT res = S_OK;
   if (strcmp(PromiseFlatCString(aDataFlavor).get(), kFilePromiseMime) == 0) {
-    // XXX We are copying the file twice below.  Once from the original
location to the temporary
-    // one (here) and then from the temporary location to the drop location
(done by the drop target
-    // when this function returns).  If the file being drag-dropped is in the
cache, we should be able to
-    // just pass back its file name to the drop target and save a copy.  Need
help here!
     nsresult rv;
+    // If we have a cached file just pass it to HGLOBAL
+    // otherwise create an empty file in the temp location
+    // after the user drops the download will start
     if (!mCachedTempFile) {
-      // Save the file to a temporary location.      
+      // Get systems' temp directory
       nsCOMPtr<nsILocalFile> dropDirectory;
       nsCOMPtr<nsIProperties> directoryService = 
	 do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
       if (!directoryService || NS_FAILED(rv)) return ResultFromScode(E_FAIL);
-      directoryService->Get(NS_OS_TEMP_DIR, 
-			     NS_GET_IID(nsIFile), 
-			     getter_AddRefs(dropDirectory));
-      nsCOMPtr<nsISupports> localFileISupports =
do_QueryInterface(dropDirectory);
-      rv = mTransferable->SetTransferData(kFilePromiseDirectoryMime,
localFileISupports, sizeof(nsILocalFile*));
-      if (NS_FAILED(rv)) return ResultFromScode(E_FAIL);
-      nsCOMPtr<nsISupports> fileDataPrimitive;
+      directoryService->Get(NS_OS_TEMP_DIR,
+	 NS_GET_IID(nsIFile),
+	 getter_AddRefs(dropDirectory));
+
+      // get the URI from the kFilePromiseURLMime flavor
+      nsCOMPtr<nsISupports> urlPrimitive;
       PRUint32 dataSize = 0;
-      rv = mTransferable->GetTransferData(kFilePromiseMime,
getter_AddRefs(fileDataPrimitive), &dataSize);
-      if (NS_FAILED(rv)) return ResultFromScode(E_FAIL);      
-      
-      mCachedTempFile = do_QueryInterface(fileDataPrimitive);
+      mTransferable->GetTransferData(kFilePromiseURLMime,
getter_AddRefs(urlPrimitive), &dataSize);
+      nsCOMPtr<nsISupportsString> srcUrlPrimitive =
do_QueryInterface(urlPrimitive);
+      if (!srcUrlPrimitive) return NS_ERROR_FAILURE;
+
+      // Get data for flavor
+      // The format of the data is (URLSTRING\nFILENAME)		        
+      nsAutoString strData;
+      srcUrlPrimitive->GetData(strData);
+      if (strData.IsEmpty()) return NS_ERROR_FAILURE;
+
+      // now figure if there is a "\n" delimeter in the data string
+      // if there is the string after the "\n" is a file name
+      // if there is no delimiter then just get the filename from the url
+      nsCOMPtr<nsILocalFile> destFile = do_QueryInterface(dropDirectory);
+      if (!destFile) return NS_ERROR_NO_INTERFACE;
+
+      nsCAutoString strFileName;
+      nsCOMPtr<nsIURI> sourceURI;
+      // New line char is used as a delimeter (hardcode)
+      PRInt32 nPos = strData.FindChar('\n');
+      PRInt32 nLen = strData.Length();
+      if (nPos != -1) { // if there is delimeter
+	 strFileName.Assign(ToNewCString(Substring(strData, nPos + 1, nLen)));
+      } else { 	 // no filename was supplied - try to get it from a URL
+	 NS_NewURI(getter_AddRefs(sourceURI), Substring(strData, 0, nPos));
+	 nsCOMPtr<nsIURL> sourceURL = do_QueryInterface(sourceURI);
+	 sourceURL->GetFileName(strFileName);
+      }
+
+      // check for an error; the URL must point to a file
+      if (strFileName.IsEmpty()) return NS_ERROR_FAILURE;
+
+      NS_UnescapeURL(strFileName);
+      NS_ConvertUTF8toUCS2 wideFileName(strFileName);
+
+      // make the name safe for the filesystem
+      wideFileName.ReplaceChar(PRUnichar('/'), PRUnichar('_'));
+      wideFileName.ReplaceChar(PRUnichar('\\'), PRUnichar('_'));
+      wideFileName.ReplaceChar(PRUnichar(':'), PRUnichar('_'));
+
+      rv = destFile->Append(wideFileName);
+      if (NS_FAILED(rv)) return rv;
+
+      rv = destFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
+      if (NS_FAILED(rv)) return rv;
+
+      mCachedTempFile = do_QueryInterface(destFile);
       if (!mCachedTempFile) return ResultFromScode(E_FAIL);
     }

@@ -810,7 +854,7 @@
       pDropFile->pt.x = 0;
       pDropFile->pt.y = 0;
       pDropFile->fWide = 0;
-      
+
       // Copy the filename right after the DROPFILES structure
       char* dest = NS_REINTERPRET_CAST(char*, pDropFile);
       dest += pDropFile->pFiles;
@@ -823,8 +867,7 @@
       dest[allocLen - 1] = '\0';

       status = ::GlobalUnlock(hGlobalMemory);
-    }
-    else {
+    } else {
       res = E_OUTOFMEMORY;
     }
     aSTG.hGlobal = hGlobalMemory;
Index: nsDragService.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsDragService.cpp,v
retrieving revision 1.36
diff -u -r1.36 nsDragService.cpp
--- nsDragService.cpp	7 Oct 2003 20:39:25 -0000	1.36
+++ nsDragService.cpp	5 Nov 2004 16:04:06 -0000
@@ -50,6 +50,18 @@

 #include "nsAutoPtr.h"

+#include "nsString.h"
+#include "nsISupportsPrimitives.h"
+#include "nsNetUtil.h"
+#include "nsIURL.h"
+#include "nsIWebProgressListener.h"
+#include "nsIWebBrowserPersist.h"
+#include "nsIDownload.h"
+#include "nsIDownloadManager.h"
+#include "nsIPrefBranch.h"
+#include "nsIPrefService.h"
+#include "nsIMIMEInfo.h"
+
 #include <ole2.h>
 #include <oleidl.h>
 #include <shlobj.h>
@@ -57,6 +69,136 @@
 // shellapi.h is needed to build with WIN32_LEAN_AND_MEAN
 #include <shellapi.h>

+//////////////////////////////////////////////////////////////////////////////
/
+// DragHook.h - defines a class to hook drag and drop operation
+// this will allow to figure drop directory
+class CDragHook
+{
+  // construction/destruction
+public:
+ 
/////////////////////////////////////////////////////////////////////////////
+  CDragHook() : m_hHiddenWnd(NULL),
+		 m_ulNotifyHandle(0)
+  {
+    lstrcpyW(m_szDropPath, L"");
+  }
+
+ 
/////////////////////////////////////////////////////////////////////////////
+  virtual ~CDragHook()
+  {
+  }
+
+  // Opreations
+public:
+ 
/////////////////////////////////////////////////////////////////////////////
+  BOOL Start()
+  {
+    // create a window and register it as notification callback
+    // Create hidden window to process shell notifications
+    WNDCLASS wc;
+    wc.style	       = CS_HREDRAW | CS_VREDRAW;
+    wc.lpfnWndProc    = (WNDPROC)HiddenWndProc;
+    wc.cbClsExtra     = 0;
+    wc.cbWndExtra     = 0;
+    wc.hInstance      = NULL;
+    wc.hIcon	       = NULL;
+    wc.hCursor        = NULL;
+    wc.hbrBackground  = (HBRUSH)GetStockObject(BLACK_BRUSH);
+    wc.lpszMenuName   = NULL;
+    wc.lpszClassName  = TEXT("DHHidden");
+
+    unsigned short res = RegisterClass(&wc);
+
+    m_hHiddenWnd = CreateWindowEx(WS_EX_TOOLWINDOW, // Extended styles
+				   TEXT("DHHidden"), // See RegisterClass()
call.
+				   TEXT("DHHidden"), // Text for window title  
bar.
+				   WS_POPUP,	     // Window style.
+				   0,		     // X position
+				   0,		     // Y position
+				   1,		     // Width
+				   1,		     // Height
+				   NULL,	     // Overlapped windows have
no parent.
+				   NULL,	     // Use the window class
menu.
+				   NULL,	     // This instance owns this
window.
+				   NULL);	     // We don't use any data
in our WM_CREATE
+
+    if (m_hHiddenWnd == NULL) {
+      return 0;
+    }
+
+    // Now let the explorer know what we want
+    // Get My Compiter PIDL
+    LPITEMIDLIST pidl;
+    HRESULT hr = ::SHGetFolderLocation(NULL, CSIDL_DESKTOP, NULL, 0, &pidl);
+    SHChangeNotifyEntry notify;
+    if (SUCCEEDED(hr)) {
+      notify.pidl = pidl;
+      notify.fRecursive = TRUE;
+      m_ulNotifyHandle = ::SHChangeNotifyRegister(m_hHiddenWnd,
+						   0x2,
+						   SHCNE_CREATE |
SHCNE_RENAMEITEM | SHCNE_UPDATEDIR,
+						   WM_USER,
+						   1,
+						   &notify);
+    }
+
+    return (m_ulNotifyHandle != 0);
+  }
+
+ 
/////////////////////////////////////////////////////////////////////////////
+  LPWSTR Stop()
+  {
+    // we failed to create window so there is no drop path
+    if (m_hHiddenWnd == NULL) {
+      return NULL;
+    }
+
+    // If we get 0 for this that means we failed to register notification
callback
+    if (m_ulNotifyHandle == 0) {
+      return NULL;
+    }
+
+    // Do clean up stuff (reverses all that's been done in Start)
+    ::SHChangeNotifyDeregister(m_ulNotifyHandle);
+    ::DestroyWindow(m_hHiddenWnd);
+
+    // if drop path is too short then there is no drop location
+    if (lstrlenW(m_szDropPath) <= 1) {
+      return NULL;
+    }
+
+    return m_szDropPath;
+  }
+
+protected:
+  static LRESULT WINAPI HiddenWndProc(HWND hWnd, UINT nMsg, WPARAM wParam,
LPARAM lParam)
+  {
+    switch (nMsg) {
+    case WM_USER:
+      if (lParam == SHCNE_RENAMEITEM || lParam == SHCNE_CREATE) {
+	 // we got notification from shell
+	 // store it for future reference
+	 //sbProcessing = TRUE;
+	 LPCITEMIDLIST* ppidl = (LPCITEMIDLIST*)wParam;
+	 ::SHGetPathFromIDListW(ppidl[1], m_szDropPath);
+	 return 0;
+      }
+      break;
+    }
+
+    return ::DefWindowProc(hWnd, nMsg, wParam, lParam);
+  }
+
+  // Implementation
+protected:
+  static WCHAR m_szDropPath[MAX_PATH];  // Drop directory
+  HWND m_hHiddenWnd;			 // Hidden window that will recieve
notifications
+					 // about shell events
+  ULONG m_ulNotifyHandle;		 // Handle to SHChangeNotify
+};
+
+// Static member declaration
+WCHAR CDragHook::m_szDropPath[MAX_PATH];

 //-------------------------------------------------------------------------
 //
@@ -89,6 +231,7 @@
   nsBaseDragService::InvokeDragSession ( aDOMNode, anArrayTransferables,
aRegion, aActionType );

   nsresult rv;
+  nsCOMPtr<nsITransferable> trans;
   PRUint32 numItemsToDrag = 0;
   rv = anArrayTransferables->Count(&numItemsToDrag);
   if ( !numItemsToDrag )
@@ -109,7 +252,7 @@
     for ( PRUint32 i=0; i<numItemsToDrag; ++i ) {
       nsCOMPtr<nsISupports> supports;
       anArrayTransferables->GetElementAt(i, getter_AddRefs(supports));
-      nsCOMPtr<nsITransferable> trans(do_QueryInterface(supports));
+      trans = (do_QueryInterface(supports));
       if ( trans ) {
	 nsRefPtr<IDataObject> dataObj;
	 if ( NS_SUCCEEDED(nsClipboard::CreateNativeDataObject(trans,
getter_AddRefs(dataObj))) ) {
@@ -123,14 +266,104 @@
   else {
     nsCOMPtr<nsISupports> supports;
     anArrayTransferables->GetElementAt(0, getter_AddRefs(supports));
-    nsCOMPtr<nsITransferable> trans(do_QueryInterface(supports));
+    trans = (do_QueryInterface(supports));
     if ( trans ) {
       if ( NS_FAILED(nsClipboard::CreateNativeDataObject(trans,
getter_AddRefs(itemToDrag))) )
	 return NS_ERROR_FAILURE;
     }
   } // else dragging a single object
-  
-  return StartInvokingDragSession ( itemToDrag, aActionType );
+
+  // Create a hidden window to recieve notifications from the shell
+  CDragHook draghook;
+  draghook.Start();
+
+  rv = StartInvokingDragSession ( itemToDrag, aActionType );
+
+  if (NS_FAILED(rv)) return rv; // See if dragsession failed
+
+  // Now that the drag session is over check if there is kFilePromiseURLMime
flavor
+  // and if there was then start the download from that URL
+  nsCOMPtr<nsISupports> urlPrimitive;
+  PRUint32 dataSize = 0;
+  rv = trans->GetTransferData(kFilePromiseURLMime,
getter_AddRefs(urlPrimitive), &dataSize);
+  if (NS_FAILED(rv)) return rv;
+  nsCOMPtr<nsISupportsString> srcUrlPrimitive =
do_QueryInterface(urlPrimitive);
+  if (!srcUrlPrimitive) return NS_ERROR_FAILURE;
+
+  // Get data for flavor
+  nsAutoString strData;
+  srcUrlPrimitive->GetData(strData);
+  if (strData.IsEmpty()) return NS_ERROR_FAILURE; // Check if someone forgot
to put the data for flavor
+
+  // Construct a new URI from string
+  nsCOMPtr<nsIURI> sourceURI;
+
+  // Figure if there is a delimiter in the data string
+  PRInt32 nPos = strData.FindChar('\n');
+  PRInt32 nLen = strData.Length();
+  nsCAutoString strURL;
+  if (nPos != -1) { // if there is delimeter
+    strURL.Assign(ToNewCString(Substring(strData, 0, nPos)));
+  } else {	     // there is no delimeter - the entire string is a URL
string
+    strURL.Assign(ToNewCString(strData));
+  }
+
+  // now construct the URI out of the url string
+  rv = NS_NewURI(getter_AddRefs(sourceURI), strURL);
+  if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+
+  // we rely on the fact that the WPB is refcounted by the channel etc,
+  // so we don't keep a ref to it. It will die when finished.
+  nsCOMPtr<nsIWebBrowserPersist> persist =
do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv); 
+  if (NS_FAILED(rv)) return rv;
+
+  // Construct target URI from nsILocalFile
+  // Init file for the download
+  nsCOMPtr<nsILocalFile>
dropLocalFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
+  if (!dropLocalFile) return NS_ERROR_FAILURE;
+
+  // init with path we get from CDragHook::Stop()
+  nsAutoString fileName;
+  fileName = draghook.Stop();
+  rv =
dropLocalFile->InitWithNativePath(NS_LossyConvertUTF16toASCII(fileName));
+  if (NS_FAILED(rv)) return rv;
+
+  // Create a new FileURI to pass to the download manager or whatever is in
the prefs
+  nsCOMPtr<nsIURI> targetURI;
+
+  nsCOMPtr<nsIFile> file = do_QueryInterface(dropLocalFile);
+  if (!file) return NS_ERROR_FAILURE;
+
+  rv = NS_NewFileURI(getter_AddRefs(targetURI), file);
+  if (NS_FAILED(rv)) return rv;
+
+  // Display the download manager or whatever the user selected in browser
prefs
+  nsCOMPtr<nsIDownloadManager> dm =
do_GetService("@mozilla.org/download-manager;1", &rv);
+  if (NS_FAILED(rv)) return rv;
+
+  nsCOMPtr<nsIDownload> download;
+
+  rv = dm->AddDownload(sourceURI, targetURI, nsnull, nsnull, 0, persist,
getter_AddRefs(download));
+  if (NS_FAILED(rv)) return rv;
+
+  PRInt32 behavior;
+  nsCOMPtr<nsIPrefBranch> branch =
do_GetService("@mozilla.org/preferences-service;1", &rv);
+  if (NS_SUCCEEDED(rv))
+    rv = branch->GetIntPref("browser.downloadmanager.behavior", &behavior);
+  if (NS_FAILED(rv))
+    behavior = 0;
+
+  if (behavior == 0)
+    rv = dm->Open(nsnull, download);
+  else if (behavior == 1)
+    rv = dm->OpenProgressDialogFor(download, nsnull, PR_TRUE);
+  if (NS_FAILED(rv)) return rv;
+
+  // Start download (I think this is wrong, but I do not know another way to
kick it off)
+  nsCOMPtr<nsISupports> fileAsSupports = do_QueryInterface(dropLocalFile);
+  rv = persist->SaveURI(sourceURI, nsnull, nsnull, nsnull, nsnull,
fileAsSupports);
+
+  return rv;
 }

 //-------------------------------------------------------------------------

***** CVS exited normally with code 1 *****

Comment 6

[Jungshik Shin]

Comment on attachment 164362 [details] [diff] [review]
Suggested changes to the drag and drop code of images and files

phew.. please, *attach* a new patch (made with 'diff -u10 -p'. don't forget
'-p' and '10') rather than editing attachment 164362 [details] [diff] [review]

Comment 7

[Yuri Kovalchuk]

Attachment: Changes to the drag and drop using shell notifications

As requested I've created a new attachment made with "diff -u10 -p".

Comment 8

[Jungshik Shin]

Comment on attachment 164815 [details] [diff] [review]
Changes to the drag and drop using shell notifications

>cvs diff -u10 -p (in directory C:\Mozilla_1.7.3\mozilla\widget\src\windows)

It'd be better if you made a patch at the top of the source tree (that is, in
mozilla directory). Btw, you have to make a patch against the trunk before
making it against 1.7.x branch unless they're the same in this case.


>+      if (nPos != -1) { // if there is delimeter
>+        strFileName.Assign(ToNewCString(Substring(strData, nPos + 1, nLen)));

You're leaking here. More importantly, ToNewCString doesn't work unless strData
is is made of characters below U+0100. Depending on what strData has, you have
to use either CopyUTF16toUTF8 or LossyCopyUTF16toASCII (if there's a guarantee
that strData is all made of ASCII characters). If you just use CopyUTF16toUTF8,
you're all right. This part is not critical in performance so that there's no
need to optimize for ASCII-only case.



>+
>+      NS_UnescapeURL(strFileName);
>+      NS_ConvertUTF8toUCS2 wideFileName(strFileName);

Pls, use NS_ConvertUTF8toUTF16 (inline forwarder NS_ConvertUTF8toUCS2 can be
removed any time).


>+      // make the name safe for the filesystem
>+      wideFileName.ReplaceChar(PRUnichar('/'), PRUnichar('_'));
>+      wideFileName.ReplaceChar(PRUnichar('\\'), PRUnichar('_'));
>+      wideFileName.ReplaceChar(PRUnichar(':'), PRUnichar('_'));

There might be a helper function for this.


>+  nsCAutoString strURL;
>+  if (nPos != -1) { // if there is delimeter
>+    strURL.Assign(ToNewCString(Substring(strData, 0, nPos)));
>+  } else {          // there is no delimeter - the entire string is a URL string
>+    strURL.Assign(ToNewCString(strData));
>+  }

The same problems here: memory leak and conversion loss.  



>+  // init with path we get from CDragHook::Stop()
>+  nsAutoString fileName;
>+  fileName = draghook.Stop();
>+  rv = dropLocalFile->InitWithNativePath(NS_LossyConvertUTF16toASCII(fileName));

You should just use InitWithPath without a conversion. Also note that
'NS_LossyConvertUTF16toASCII' has 'Lossy' in its name for a reason, which is to
alert its users that there's a conversion loss unless it's used for a string
composed of ASCII characters only.

Comment 9

[Yuri Kovalchuk]

Attachment: Updated patch with comments from previous post.

An updated patch. Fixed what was pointed out by Jungshik Shin in comment #8.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 165260 [details] [diff] [review]
Updated patch with comments from previous post.

Before going any further: I can't see anything protecting from another
completely irrelevant file creation event from ruining the whole thing. The
time interval might be short, but if it's possible, it's unacceptable.

Comment 11

[Yuri Kovalchuk]

Well, this is not what is going to happen very often (if ever). Since drag and
drop operation is syncronous and while in the drag loop only Windows and create
files. It actually does create files, well not files but pipes for its own use.
I've hooked CreateFile function to see what windows does during drag and drop
operation, so I'm pretty sure that the only file that gets created when the user
drops is our file. And I did not think it was necessary to monitor file name.
But to insure that the created file is our file I suggest to monitor the name of
the file being created. Before starting the drag get the name of the desired
file from kFilePromiseUrlMime flavour and pass it on to the CDragHook instance,
and then in the hidden window procedure we can check to see if the file created
matches our file. I do not post any code to avoid many attachments, but I have
the code that does what I described.

Also I suppose it'll be a good idea to move functionality from CDragHook class
I've created to nsDragSession class. I used CDragHook to explore possibilities
in an external application of my own (not Mozilla), so I've left it as it is.
What do you think about it?

Comment 12

[Ere Maijala (slow)]

That's true only assuming there is nothing else running on the system. What if
you are for example compiling something at the same time? That would slow down
the system leaving maybe a longer interval between the completion of the drag
event and shutting down of the notifications. Checking the filename is an
improvement, but how do you differentiate for example setup.exe from another
setup.exe? 

Yes, in my opinion there is no need for the CDragHook class. 

Comment 13

[Ere Maijala (slow)]

Btw, I understand that the probability of a problem is small, but however small
it is, sooner or later someone will stumble on it.

Comment 14

[Yuri Kovalchuk]

Sure somtime someone will trip on that...
Another solution is making a unique name and watch for that. After the drop
occurs just rename the shortcut to somthing the user expects to get?

Comment 15

[Yuri Kovalchuk]

We get a SHCNE_RENAMEITEM notification from the shell when user drops and there
are 2 PIDLs in this notification. One is from where the file is coming and the
other is where to the file is going. There will be no confusion if we check both
of them to insure that this is what we need. The file should come from OS temp
dir and have our file name.

Comment 16

[Yuri Kovalchuk]

Attachment: Patch that addreses issues stated in comment #10

Here I used the from folder and the filename to make sure that the notification
is for the event that we wait, which is the creation of file as a result of the
drag and drop operation.
I used filename and source path (which is OS temp directory) to make sure that
the the notification is the right one.
Also I've removed CDragHook class and moved all the code to nsDragService.

Comment 17

[Ere Maijala (slow)]

Comment on attachment 165679 [details] [diff] [review]
Patch that addreses issues stated in comment #10

Ok, here are some comments. Some of them are just minor nits, but some are
serious. Unfortunately they are not in order as some of them are based on the
older patch.

>+  nsCOMPtr<nsITransferable> trans;
>-      nsCOMPtr<nsITransferable> trans(do_QueryInterface(supports));
>+      trans = (do_QueryInterface(supports));

Why these? It's not worth the clutter it's causing. Same with all the early
introductions of variables (like nPos and nLen). Please introduce them as
required.

What's the do-while loop for in InvokeDragSession? I can't see the reason.

>+  rv = StartInvokingDragSession ( itemToDrag, aActionType );
>+
>+  if (NS_FAILED(rv)) return rv; // See if dragsession failed

Style issues: Keep the rows together and remove the extra spaces around the
parenthesis.



>+    // otherwise create an empty file in the temp location
>+    // after the user drops the download will start

How about:
+    // otherwise create an empty file in the temp location.
+    // The download will start after the user drops the file.

>+      // Get systems' temp directory

Make it:
+      // Get system temp directory 

>+      // now figure if there is a "\n" delimeter in the data string
>+      // if there is the string after the "\n" is a file name
>+      // if there is no delimiter then just get the filename from the url

It's easier to understand these if you make them sentences ending in a period.
Something like: 

+      // Now figure if there is a "\n" delimeter in the data string.
+      // If there is, the string after the "\n" is a filename.
+      // If there is no delimiter then just get the filename from the url.

>+      // New line char is used as a delimeter (hardcode)

=> hardcoded

>+  // Create a hidden window to recieve notifications from the shell

=> receive

>+      if (!srcUrlPrimitive) return NS_ERROR_FAILURE;

You should not return NS_ERROR_FAILURE or other NS_RESULT values as a HRESULT.

>+  // we rely on the fact that the WPB is refcounted by the channel etc,
>+  // so we don't keep a ref to it. It will die when finished.
>+  nsCOMPtr<nsIWebBrowserPersist> persist = do_CreateInstance("@mozilla.org/embedding/browser/nsWebBrowserPersist;1", &rv);	
>+  if (NS_FAILED(rv)) return rv;

>+  nsCOMPtr<nsILocalFile> dropLocalFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
>+  if (!dropLocalFile) return NS_ERROR_FAILURE;

Remove the comment, it's not true. It would be cleaner to use
NS_WEBBROWSERPERSIST_CONTRACT instead of the string. Please create the objects
using a consistent style, preferable a style prevalent in the existing code.

>+      // make the name safe for the filesystem
>+      wideFileName.ReplaceChar(PRUnichar('/'), PRUnichar('_'));
>+      wideFileName.ReplaceChar(PRUnichar('\\'), PRUnichar('_'));
>+      wideFileName.ReplaceChar(PRUnichar(':'), PRUnichar('_'));

You should do something like |wideFileName.ReplaceChar(FILE_PATH_SEPARATOR
FILE_ILLEGAL_CHARACTERS, '-');| instead (This was taken from
ConvertAndSanitizeFileName in nsMessenger.cpp).

>+      // If the file already exits then just use it to pass into CF_HDROP
>+      // if fail is due to some other reason that the existance of a file then quit

Suggestions:
// Try to create the file.
// Bail out if it fails for a reason other the existence of the file.

Maybe you should use nsIDownload instead of nsIDownloadManager to save the
file. See
http://lxr.mozilla.org/seamonkey/source/xpfe/communicator/resources/content/con
tentAreaUtils.js#266 for code in js. I'm not sure though.

>+  // Get My Compiter PIDL

=> Get PIDL for My Computer

>+PRBool nsDragService::StartWatchingShell(const nsAString& strFileName)

Start parameter names with a, such as aFileName. No need for str prefix. Ditto
for other functions.

>+  return (PRBool)(m_nNotifyHandle != 0);

No need to cast.

>+// Static member declaration
>+nsString nsDragService::m_strDropPath;
>+nsString nsDragService::m_strFileName;

Why are these static?

Comment 18

[Yuri Kovalchuk]

Attachment: Patch that addresses comment #17

Here is an updated patch that fixes issues stated in comment #17.

>>+// Static member declaration
>>+nsString nsDragService::m_strDropPath;
>>+nsString nsDragService::m_strFileName;
>
>Why are these static?

I made those vars static to avoid sending additional messages to the window
procedure. It is static and cannot access regular members, so I thought it'll
be a good idea to make varables that this window procedure uses static too.
That is why.
If this needs fixing also, I suggest using WM_COPYDATA to pass this pointer of
the nsDragSession. So that window procedure could access member of the instance
of nsDragSession. Is it a good idea or should I use some other mechanism?

Comment 19

[Yuri Kovalchuk]

Why everyone is silent? Did I do something wrong?

Comment 20

[Ere Maijala (slow)]

Ok, I think statics are ok. I somehow missed the point earlier. Nothing wrong
here, I'm just quite busy and the patch is fairly big to review.

Comment 21

[Yuri Kovalchuk]

Ok, I unerstand. But please do not forget about it.
I've fixed all of the previous issues and left statics as tey were in the new patch.

Comment 22

[Ere Maijala (slow)]

Comment on attachment 165978 [details] [diff] [review]
Patch that addresses comment #17

I still didn't have time for a thorough review, but there are still some things
I didn't realize before:

- follow the convention with variable naming (for example m_strFileName ->
mFileName)
- you shouldn't assume ::SHGetPathFromIDListW and other wide string versions
work. See for example bug 239279 and the ones there are mentioned for more
information
- avoid WCHAR arrays whenever you. You can use string classes using
SetLength(), BeginWriting() and trimming the result for example by calling
.Truncate(strlen(.get())). If you really want to use WCHAR [], just use it
temporarily and assign to a string immediately after
- "Computer" spelled "Compiter" in a comment
- "recieved" should be "received"
- "winodw" -> "window"
- "SHould" -> "Should"
- remove tabs from the files

Comment 23

[Yuri Kovalchuk]

Ok. About the W API functions. Is it OK to use macros like CreateFile instead of
CreateFileW or CreateFileA and TCHARs to go with them if needed?
I've read through the bug 239279 and it seem that the work on it is still in
progress, so It is not clear what to use...

Comment 24

[Yuri Kovalchuk]

And by the way how do I handle conversions from ASCII to Unicode and the other
way around? Or maybe I should just use A versions of API and plain char? Any
idea about it?

Comment 25

[Jungshik Shin]

For now, use nsCopyNativeToUnicode and nsCopyUnicodeToNative

Comment 26

[Ere Maijala (slow)]

Don't use TCHAR. Use W versions if possible. See for example
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#5600 for
one way to do it. See also nsToolkit.mSHGetPathFromIDList and others in
nsToolkit. I think the needed functionality should be eventually added to MZLU,
but for now you could do it in the drag code.

I just noticed there is nsDataObj::MangleTextToValidFilename(). You should
probably use it to ensure the file name is legal. 

Comment 27

[Yuri Kovalchuk]

Attachment: Patch that uses only Mozilla's native string classes and nsToolkit

It's been a long while since I've posted a patch for this bug. Finally here it
is. I've removed all of the TCHARs I used in the previous patch, also now I use
nsToolkit::nsSHGetPathFromIDList. Please take a look at it and let me know what
you think.

Comment 28

[Yuri Kovalchuk]

> I just noticed there is nsDataObj::MangleTextToValidFilename(). You should
> probably use it to ensure the file name is legal. 

Sorry, I could not find that function in nsDataObj, probably because it is in
the newer version of the code. Currently I have 1.7.3 release...

Comment 29

[Ere Maijala (slow)]

Comment on attachment 177132 [details] [diff] [review]
Patch that uses only Mozilla's native string classes and nsToolkit

The patch is missing at least nsDragService.cpp.

By the way, MangleTextToValidFilename is there in nsDataObj.cpp, it just
doesn't belong to the class (my fault, sorry).

Comment 30

[Yuri Kovalchuk]

What do you mean? I've looked through it and nsDragService.cpp is there. Please
explain more clearly what you mean by "the patch is missing at least
nsDragService.cpp" I do not underatand that...

Comment 31

[Ere Maijala (slow)]

Comment on attachment 177132 [details] [diff] [review]
Patch that uses only Mozilla's native string classes and nsToolkit

Obviously it is there and I can't find any explanation for my missing it. Sorry
about that.

Comment 32

[Ere Maijala (slow)]

Comment on attachment 177132 [details] [diff] [review]
Patch that uses only Mozilla's native string classes and nsToolkit

I wasn't completely confused the first time. There is indeed nsDragService.cpp,
but where is the implementation for StartWatchingShell, GetDropPath etc? The
patch can't be applied to the current cvs tip.

Comment 33

[Yuri Kovalchuk]

Attachment: Patch that addresses comments #29.

Sorry about the last patch. Do not know why it turned out that way. The new one
has all the neccessary updates in it i.e. all the missing functions from the
previous patch. And besides I've made it against 1.7.5 version of the sources.

Comment 34

[Ere Maijala (slow)]

Comment on attachment 179028 [details] [diff] [review]
Patch that addresses comments #29.

(sorry for the slow answer)

Please create a patch against the current trunk. This kind of a patch won't get
to a branch without going in to trunk first.

Comment 35

[Yuri Kovalchuk]

I'm not sure how to do this. Does this mean that I have to make a patch against
the current version in the cvs tree? Which tag sould I use to checkout the
latest trunk? Please help.

Comment 36

[Ere Maijala (slow)]

Don't use a tag at all. Just check out mozilla/client.mk and get the rest using 
make -f client.mk checkout

Comment 37

[Yuri Kovalchuk]

Attachment: A new patch against the current trunk

This is a new patch I've made against the current trunk. Also I've made some
modifications to it.

Comment 38

[OstGote!]

Bug 231305 and Bug 256173 look like duplicates of this.

Comment 39

[Yuri Kovalchuk]

I've just noted a equrement for win32 build: "For older compilers, you need to
download SDKs from Microsoft to have the GDI+ headers and library."
If it states that in order to build one must have "latest" Platform SDK, then
there is no need to use ::GetProcAddress routines to get shell functions
addresses for older systems like win98 (the ones that do not have IE5+ installed).
Can any one comment this please?

Comment 40

[Yuri Kovalchuk]

What about the patch? It's been a while since I heard anything from anyone here.
Why is this happening?

Comment 41

[Ere Maijala (slow)]

Because I suck. It's a fairly big patch and I'm swamped (in both lifes). Sorry
about the lack of response, I'll tackle this as soon as possible.

Comment 42

[Ere Maijala (slow)]

(In reply to comment #39)

IMO: The latest SDK might be a requirement for compilation, but there is only so
much we can expect from the target system that finally runs the binaries. That's
why it's good to keep the requirements as low as possible (without sacrificing
basic functionality). 

Comment 43

[Ere Maijala (slow)]

Comment on attachment 183491 [details] [diff] [review]
A new patch against the current trunk

Unfortunately the patch has bit-rotten, and nsDataObj won't compile anymore.
While this is my fault, could you, Yuri, try to update the patch to work
against the current trunk, please?

Comment 44

[Yuri Kovalchuk]

I tried to copy my patch to firefox widget code and it wont compile because of
errors (wrong number of params to some functions). Are the different? I mean
mozilla's source and firefox's one. If the are should I make a patch against
firefox tree or mozilla's. I guess firefox would be better, since there will be
no mere releases of mozilla (as I've heard). Any suggestions?

Comment 45

[Yuri Kovalchuk]

Comment on attachment 183491 [details] [diff] [review]
A new patch against the current trunk

Index: widget/src/windows/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/Makefile.in,v
retrieving revision 3.27
diff -u -w -b -u -1 -0 -p -r3.27 Makefile.in
--- widget/src/windows/Makefile.in	23 Aug 2005 02:11:54 -0000	3.27
+++ widget/src/windows/Makefile.in	26 Sep 2005 10:39:59 -0000
@@ -60,20 +60,22 @@ REQUIRES	= xpcom \
			timer \
			uconv \
			intl \
			locale \
			webshell \
			docshell \
			layout \
			xuldoc \
			view \
			imglib2 \
+			uriloader \
+			webbrowserpersist \
			$(NULL)

 CPPSRCS		= \
	L_Ienumfe.cpp  \
	nsWindow.cpp	     \
	nsAppShell.cpp	     \
	nsLookAndFeel.cpp    \
	nsToolkit.cpp	     \
	nsDataObj.cpp  \
	nsDataObjCollection.cpp  \
Index: widget/src/windows/nsDataObj.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsDataObj.cpp,v
retrieving revision 1.69
diff -u -w -b -u -1 -0 -p -r1.69 nsDataObj.cpp
--- widget/src/windows/nsDataObj.cpp	11 Aug 2005 00:37:53 -0000	1.69
+++ widget/src/windows/nsDataObj.cpp	26 Sep 2005 10:26:07 -0000
@@ -61,20 +61,23 @@
 #include "nsXPIDLString.h"
 #include "nsIImage.h"
 #include "nsImageClipboard.h"
 #include "nsIDirectoryService.h"
 #include "nsILocalFile.h"
 #include "nsDirectoryServiceDefs.h"
 #include "prprf.h"
 #include "nsCRT.h"
 #include "nsPrintfCString.h"
 #include "nsIStringBundle.h"
+#include "nsEscape.h"
+#include "nsIURL.h"
+#include "nsNetUtil.h"

 #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
@@ -934,43 +937,82 @@ HRESULT nsDataObj::GetText(const nsACStr
   nsMemory::Free(data);

   return ResultFromScode(S_OK);
 }

 //-----------------------------------------------------
 HRESULT nsDataObj::GetFile(const nsACString & aDataFlavor, FORMATETC& aFE,
STGMEDIUM& aSTG)
 {
   HRESULT res = S_OK;
   if (strcmp(PromiseFlatCString(aDataFlavor).get(), kFilePromiseMime) == 0) {
-    // XXX We are copying the file twice below.  Once from the original
location to the temporary
-    // one (here) and then from the temporary location to the drop location
(done by the drop target
-    // when this function returns).  If the file being drag-dropped is in the
cache, we should be able to
-    // just pass back its file name to the drop target and save a copy.  Need
help here!
+    // If we have a cached file just pass it to HGLOBAL
+    // otherwise create an empty file in the temp location.
+    // The download will start after the user drops the file.
     nsresult rv;
     if (!mCachedTempFile) {
-      // Save the file to a temporary location.      
+      // Get system temp directory
       nsCOMPtr<nsILocalFile> dropDirectory;
       nsCOMPtr<nsIProperties> directoryService = 
	 do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID, &rv);
       if (!directoryService || NS_FAILED(rv)) return ResultFromScode(E_FAIL);
       directoryService->Get(NS_OS_TEMP_DIR, 
			     NS_GET_IID(nsIFile), 
			     getter_AddRefs(dropDirectory));
-      nsCOMPtr<nsISupports> localFileISupports =
do_QueryInterface(dropDirectory);
-      rv = mTransferable->SetTransferData(kFilePromiseDirectoryMime,
localFileISupports, sizeof(nsILocalFile*));
-      if (NS_FAILED(rv)) return ResultFromScode(E_FAIL);
-      nsCOMPtr<nsISupports> fileDataPrimitive;
+      // get the URI from the kFilePromiseURLMime flavor
+      nsCOMPtr<nsISupports> urlPrimitive;
       PRUint32 dataSize = 0;
-      rv = mTransferable->GetTransferData(kFilePromiseMime,
getter_AddRefs(fileDataPrimitive), &dataSize);
-      if (NS_FAILED(rv)) return ResultFromScode(E_FAIL);      
+      mTransferable->GetTransferData(kFilePromiseURLMime,
getter_AddRefs(urlPrimitive), &dataSize);
+      nsCOMPtr<nsISupportsString> srcUrlPrimitive =
do_QueryInterface(urlPrimitive);
+      if (!srcUrlPrimitive) return E_FAIL;
+
+      // Get data for flavor
+      // The format of the data is (URLSTRING\nFILENAME)
+      nsAutoString strData;
+      srcUrlPrimitive->GetData(strData);
+      if (strData.IsEmpty()) return E_FAIL;
+
+      // Now figure if there is a "\n" delimeter in the data string.
+      // If there is, the string after the "\n" is a filename.
+      // If there is no delimiter then just get the filename from the url.
+      nsCOMPtr<nsILocalFile> destFile = do_QueryInterface(dropDirectory);
+      if (!destFile) return E_FAIL;
+
+      nsCAutoString strFileName;
+      nsCOMPtr<nsIURI> sourceURI;
+      // New line char is used as a delimeter (hardcoded)
+      PRInt32 nPos = strData.FindChar('\n');
+      PRInt32 nLen = strData.Length();
+      if (nPos != -1) { // if there is delimeter
+	 CopyUTF16toUTF8(Substring(strData, nPos + 1, nLen), strFileName);
+      } else { 	 // no filename was supplied - try to get it from a URL
+	 NS_NewURI(getter_AddRefs(sourceURI), Substring(strData, 0, nPos));
+	 nsCOMPtr<nsIURL> sourceURL = do_QueryInterface(sourceURI);
+	 sourceURL->GetFileName(strFileName);
+      }
+      // check for an error; the URL must point to a file
+      if (strFileName.IsEmpty()) return E_FAIL;
+
+      NS_UnescapeURL(strFileName);
+      NS_ConvertUTF8toUTF16 wideFileName(strFileName);
+
+      // make the name safe for the filesystem
+      wideFileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS,
'-');
+
+      rv = destFile->Append(wideFileName);
+      if (NS_FAILED(rv)) return E_FAIL;
+
+      // Try to create the file.
+      rv = destFile->Create(nsIFile::NORMAL_FILE_TYPE, 0600);
+      // Bail out if it fails for a reason other the existence of the file.
+      if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS) return E_FAIL;

-      mCachedTempFile = do_QueryInterface(fileDataPrimitive);
+      mCachedTempFile = do_QueryInterface(destFile);
       if (!mCachedTempFile) return ResultFromScode(E_FAIL);
     }

     // Pass the file name back to the drop target so that it can copy to the
drop location
     nsCAutoString path;
     rv = mCachedTempFile->GetNativePath(path);
     if (NS_FAILED(rv)) return ResultFromScode(E_FAIL);
     const char* pFileName = path.get();
     // Two null characters are needed to terminate the file name list
     PRUint32 allocLen = path.Length() + 2;
Index: widget/src/windows/nsDragService.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsDragService.cpp,v
retrieving revision 1.42
diff -u -w -b -u -1 -0 -p -r1.42 nsDragService.cpp
--- widget/src/windows/nsDragService.cpp	16 Mar 2005 00:40:21 -0000     
1.42
+++ widget/src/windows/nsDragService.cpp	26 Sep 2005 11:36:38 -0000
@@ -52,20 +52,33 @@
 #include "nsWidgetsCID.h"
 #include "nsNativeDragTarget.h"
 #include "nsNativeDragSource.h"
 #include "nsClipboard.h"
 #include "nsISupportsArray.h"
 #include "nsIDocument.h"
 #include "nsDataObjCollection.h"

 #include "nsAutoPtr.h"

+#include "nsString.h"
+#include "nsEscape.h"
+#include "nsISupportsPrimitives.h"
+#include "nsNetUtil.h"
+#include "nsIURL.h"
+#include "nsCWebBrowserPersist.h"
+#include "nsIDownload.h"
+#include "nsToolkit.h"
+#include "nsCRT.h"
+
+// Static member declaration
+nsString nsDragService::mDropPath;
+nsString nsDragService::mFileName;

 //-------------------------------------------------------------------------
 //
 // DragService constructor
 //
 //-------------------------------------------------------------------------
 nsDragService::nsDragService()
   : mNativeDragSrc(nsnull), mNativeDragTarget(nsnull), mDataObject(nsnull)
 {
 }
@@ -139,21 +152,122 @@ nsDragService::InvokeDragSession(nsIDOMN
     anArrayTransferables->GetElementAt(0, getter_AddRefs(supports));
     nsCOMPtr<nsITransferable> trans(do_QueryInterface(supports));
     if (trans) {
       rv = nsClipboard::CreateNativeDataObject(trans,
						getter_AddRefs(itemToDrag),
						uri);
       NS_ENSURE_SUCCESS(rv, rv);
     }
   } // else dragging a single object

-  return StartInvokingDragSession(itemToDrag, aActionType);
+  // Before starting get the file name to monitor if there is any
+  nsAutoString strData;	 // holds kFilePromiseURLMime flavour data
+  PRBool bDownload = PR_FALSE;  // Used after drag ends to fugure if the
download should start
+
+  nsCOMPtr<nsISupports> urlPrimitive;
+  nsCOMPtr<nsISupports> transSupports;
+  anArrayTransferables->GetElementAt(0, getter_AddRefs(transSupports));
+  nsCOMPtr<nsITransferable> trans = (do_QueryInterface(transSupports));
+  if (trans) {
+    PRUint32 dataSize = 0;
+    rv = trans->GetTransferData(kFilePromiseURLMime,
getter_AddRefs(urlPrimitive), &dataSize);
+    // If there is no kFilePromiseURLMime - it is not a file drag from a URL
+    // so stop further processing for this format
+    if (NS_SUCCEEDED(rv)) {
+      nsCOMPtr<nsISupportsString> srcUrlPrimitive =
do_QueryInterface(urlPrimitive);
+      if (!srcUrlPrimitive) return NS_ERROR_FAILURE;
+
+      // Get data for flavor
+      srcUrlPrimitive->GetData(strData);
+      if (strData.IsEmpty()) return NS_ERROR_FAILURE; // Check if someone
forgot to put the data for flavor
+
+      nsCAutoString strFileName;
+      // New line char is used as a delimeter (hardcode)
+      PRInt32 nPos = strData.FindChar('\n');
+      PRInt32 nLen = strData.Length();
+      if (nPos != -1) { // if there is delimeter
+	 CopyUTF16toUTF8(Substring(strData, nPos + 1, nLen), strFileName);
+      } else { 	 // no filename was supplied - try to get it from a URL
+	 nsCOMPtr<nsIURI> sourceURI;
+	 NS_NewURI(getter_AddRefs(sourceURI), Substring(strData, 0, nPos));
+	 nsCOMPtr<nsIURL> sourceURL = do_QueryInterface(sourceURI);
+	 sourceURL->GetFileName(strFileName);
+      }
+
+      // check for an error; the URL must point to a file
+      if (strFileName.IsEmpty()) return NS_ERROR_FAILURE;
+
+      NS_UnescapeURL(strFileName);
+      NS_ConvertUTF8toUTF16 wideFileName(strFileName);
+
+      // make the name safe for the filesystem
+      wideFileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS,
'-');
+      // Start listening to the shell notifications
+      if (StartWatchingShell(wideFileName)) {
+	 // Set download flag if all went ok so far
+	 bDownload = PR_TRUE;
+      }
+    }
+  }
+
+  // Kick off the native drag session
+  rv = StartInvokingDragSession (itemToDrag, aActionType);
+  if (NS_FAILED(rv)) return rv; // See if dragsession failed
+  // Check if the download flag was set
+  // if not then we are done
+  if (!bDownload) return rv;
+
+  // Construct a new URI from string
+  // Figure if there is a delimiter in the data string
+  PRInt32 nPos = strData.FindChar('\n');
+  PRInt32 nLen = strData.Length();
+  nsCAutoString strURL;
+  if (nPos != -1) { // if there is delimeter
+    CopyUTF16toUTF8(Substring(strData, 0, nPos), strURL);
+  } else {	     // there is no delimeter - the entire string is a URL
string
+    CopyUTF16toUTF8(strData, strURL);
+  }
+
+  // now construct the URI out of the url string
+  nsCOMPtr<nsIURI> sourceURI;
+  rv = NS_NewURI(getter_AddRefs(sourceURI), strURL);
+  if (NS_FAILED(rv)) return NS_ERROR_FAILURE;
+
+  nsCOMPtr<nsIWebBrowserPersist> persist =
do_CreateInstance(NS_WEBBROWSERPERSIST_CONTRACTID, &rv);
+  if (NS_FAILED(rv)) return rv;
+
+  // Construct target URI from nsILocalFile
+  // Init file for the download
+  nsCOMPtr<nsILocalFile>
dropLocalFile(do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
+  if (!dropLocalFile) return NS_ERROR_FAILURE;
+
+  // init with path we get from GetDropPath
+  // if it fails then something went wrong and we can not continue
+  nsAutoString fileName;
+  if (!GetDropPath(fileName)) return NS_ERROR_FAILURE;
+  rv = dropLocalFile->InitWithPath(fileName);
+  if (NS_FAILED(rv)) return rv;
+
+  // Create a new FileURI to pass to the nsIDownload
+  nsCOMPtr<nsIURI> targetURI;
+
+  nsCOMPtr<nsIFile> file = do_QueryInterface(dropLocalFile);
+  if (!file) return NS_ERROR_FAILURE;
+
+  rv = NS_NewFileURI(getter_AddRefs(targetURI), file);
+  if (NS_FAILED(rv)) return rv;
+
+  // Start download
+  nsCOMPtr<nsISupports> fileAsSupports = do_QueryInterface(dropLocalFile);
+  rv = persist->SaveURI(sourceURI, nsnull, nsnull, nsnull, nsnull,
fileAsSupports);
+
+  return rv;
 }

 //-------------------------------------------------------------------------
 NS_IMETHODIMP
 nsDragService::StartInvokingDragSession(IDataObject * aDataObj,
					 PRUint32 aActionType)
 {
   // To do the drag we need to create an object that
   // implements the IDataObject interface (for OLE)
   NS_IF_RELEASE(mNativeDragSrc);
@@ -444,10 +558,176 @@ nsDragService::IsCollectionObject(IDataO
 // w/out crashing when we're still holding onto their data
 //
 NS_IMETHODIMP
 nsDragService::EndDragSession()
 {
   nsBaseDragService::EndDragSession();
   NS_IF_RELEASE(mDataObject);

   return NS_OK;
 }
+
+// Start monitoring shell events - when user drops we'll get a notification
from shell
+// containing drop location
+// aFileName is a filename to monitor
+// returns true if all went ok 
+// if hidden window was created and shell is watching out for files being
created
+PRBool nsDragService::StartWatchingShell(const nsAString& aFileName)
+{
+  // init member varable with the file name to watch
+  mFileName = aFileName;
+  // Create hidden window to process shell notifications
+  WNDCLASS wc;
+
+  wc.style	     = CS_HREDRAW | CS_VREDRAW;
+  wc.lpfnWndProc    = (WNDPROC)HiddenWndProc;
+  wc.cbClsExtra     = 0;
+  wc.cbWndExtra     = 0;
+  wc.hInstance      = NULL;
+  wc.hIcon	     = NULL;
+  wc.hCursor	     = NULL;
+  wc.hbrBackground  = (HBRUSH)GetStockObject(BLACK_BRUSH);
+  wc.lpszMenuName   = NULL;
+  wc.lpszClassName  = TEXT("DHHidden");
+
+  unsigned short res = RegisterClass(&wc);
+
+  mHiddenWnd = CreateWindowEx(WS_EX_TOOLWINDOW,
+			       TEXT("DHHidden"),
+			       TEXT("DHHidden"),
+			       WS_POPUP,
+			       0,
+			       0,
+			       1,
+			       1,
+			       NULL,
+			       NULL,
+			       NULL,
+			       NULL);
+  if (mHiddenWnd == NULL) {
+    return PR_FALSE;
+  }
+
+  // Now let the explorer know what we want
+  // Get My Computer PIDL
+  LPITEMIDLIST pidl;
+  HRESULT hr = ::SHGetFolderLocation(NULL, CSIDL_DESKTOP, NULL, 0, &pidl);
+
+  SHChangeNotifyStruct notify;
+  if (SUCCEEDED(hr)) {
+    notify.pidl = pidl;
+    notify.fRecursive = TRUE;
+
+    HMODULE hShell32 = ::GetModuleHandleA("SHELL32.DLL");
+
+    if (hShell32 == NULL) {
+      return PR_FALSE;
+    }
+
+    DWORD dwOrdinal = 2;
+
+    // get reg func
+    SHCNRegPtr reg;
+    reg = (SHCNRegPtr)::GetProcAddress(hShell32, (LPCSTR)dwOrdinal);
+    if (reg == NULL) return PR_FALSE;
+
+    mNotifyHandle = (reg)(mHiddenWnd,
+			   0x2,
+			   0x1,
+			   WM_USER,
+			   1,
+			   &notify);
+  }
+
+  return (mNotifyHandle != 0);
+}
+
+// Get the drop path if there is one:)
+// returns true if there is a drop path
+PRBool nsDragService::GetDropPath(nsAString& aDropPath) const
+{
+  // we failed to create window so there is no drop path
+  if (mHiddenWnd == NULL) {
+    return PR_FALSE;
+  }
+
+  // If we get 0 for this that means we failed to register notification
callback
+  if (mNotifyHandle == 0) {
+    return PR_FALSE;
+  }
+
+  // Do clean up stuff (reverses all that's been done in Start)
+  HMODULE hShell32 = ::GetModuleHandleA("SHELL32.DLL");
+
+  if (hShell32 == NULL) {
+    return PR_FALSE;
+  }
+
+  DWORD dwOrdinal = 4;
+  SHCNDeregPtr dereg;
+  dereg = (SHCNDeregPtr)::GetProcAddress(hShell32, (LPCSTR)dwOrdinal);
+  if (dereg == NULL) return PR_FALSE;
+  (dereg)(mNotifyHandle);
+
+  ::DestroyWindow(mHiddenWnd);
+
+  // if drop path is too short then there is no drop location
+  if (mDropPath.IsEmpty()) {
+    return PR_FALSE;
+  }
+
+  aDropPath = mDropPath;
+
+  return PR_TRUE;
+}
+
+// Hidden window procedure - this is where shell sends notifications
+// When notification is received, perform some checking and copy path to the
member variable
+LRESULT WINAPI nsDragService::HiddenWndProc(HWND aWnd, UINT aMsg, WPARAM
awParam, LPARAM alParam)
+{
+  switch (aMsg) {
+  case WM_USER:
+    if (alParam == SHCNE_RENAMEITEM) {
+      // Check if there is an empty filename
+      // if so stop further processing
+      if (mFileName.IsEmpty()) {
+	 // Empty drop path
+	 mDropPath = NS_LITERAL_STRING("");
+	 return 0;
+      }
+
+      // we got notification from shell
+      // as wParam we get 2 PIDL
+      LPCITEMIDLIST* ppidl = (LPCITEMIDLIST*)awParam;
+      // Get From path (Consider:
+      WCHAR szPathFrom[MAX_PATH + 1];
+      WCHAR szPathTo[MAX_PATH + 1];
+      nsToolkit::mSHGetPathFromIDList(ppidl[0], szPathFrom);
+      // Get To path
+      nsToolkit::mSHGetPathFromIDList(ppidl[1], szPathTo);
+
+      // first is from where the file is coming
+      // and the second is where the file is going
+      nsAutoString pathFrom(szPathFrom);  // where the file is comming from
+      nsAutoString pathTo(szPathTo);	 // where the file is going to
+      // Get OS Temp directory
+      WCHAR szTempPath[MAX_PATH + 1];
+      ::GetTempPathW(MAX_PATH, szTempPath);
+      // append file name to Temp directory string
+      nsAutoString tempPath(szTempPath);
+      tempPath.Append(mFileName);
+      // Now check if there is our filename in the path
+      // and also check for the source directory - it should be OS Temp dir
+      // this way we can ensure that this is the file that we need
+      if (wcsstr(pathTo.get(), mFileName.get()) != NULL &&
+	 lstrcmpiW(tempPath.get(), pathFrom.get()) == 0)
+      {
+	 // This is what we wanted to get
+	 mDropPath = pathTo;
+      }
+      return 0;
+    }
+    break;
+  }
+
+  return ::DefWindowProc(aWnd, aMsg, awParam, alParam);
+}
Index: widget/src/windows/nsDragService.h
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsDragService.h,v
retrieving revision 1.19
diff -u -w -b -u -1 -0 -p -r1.19 nsDragService.h
--- widget/src/windows/nsDragService.h	15 Mar 2005 00:26:15 -0000	1.19
+++ widget/src/windows/nsDragService.h	26 Sep 2005 10:38:02 -0000
@@ -32,25 +32,50 @@
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */

 #ifndef nsDragService_h__
 #define nsDragService_h__

 #include "nsBaseDragService.h"
+#include <windows.h>
+#include <shlobj.h>

 struct IDropSource;
 struct IDataObject;
 class	nsNativeDragTarget;
 class	nsDataObjCollection;
+class	nsString;
+
+// some older versions of MS PSDK do not have definitions for theese, even
though
+// the functions are there and exported from the shell32.dll,
+// so I had to add them manualy. For example if one tries to build without
those
+// using VC6 standard libs one will get an error.
+#define SHCNE_RENAMEITEM_DEF 0x00000001
+
+// Structure
+typedef struct {
+    LPCITEMIDLIST pidl;
+    BOOL fRecursive;
+} SHChangeNotifyStruct;
+
+// Register for shell notifications func ptr
+typedef ULONG (*SHCNRegPtr)(HWND hWnd, 		     // Ordinal of 2
+			     int fSources,
+			     LONG fEvents,
+			     UINT wMsg,
+			     int cEntries,
+			     SHChangeNotifyStruct *pschn);
+// Unregister form from the shell notifications func ptr
+typedef BOOL (*SHCNDeregPtr)(ULONG ulID);		     // Ordinal of 4

 /**
  * Native Win32 DragService wrapper
  */

 class nsDragService : public nsBaseDragService
 {
 public:
   nsDragService();
   virtual ~nsDragService();
@@ -72,16 +97,30 @@ public:
   NS_IMETHOD StartInvokingDragSession(IDataObject * aDataObj,
				       PRUint32 aActionType);

 protected:
   nsDataObjCollection* GetDataObjCollection(IDataObject * aDataObj);

   // determine if we have a single data object or one of our private
   // collections
   PRBool IsCollectionObject(IDataObject* inDataObj);

+  // Begin monitoring the shell for events (this would allow to figure drop
location)
+  PRBool StartWatchingShell(const nsAString& aFileName);
+
+  // Should be called after drag is over to get the drop path (if any)
+  PRBool GetDropPath(nsAString& aDropPath) const;
+
+  // Hidden winodow procedure - here we shall receive notification from the
shell
+  static LRESULT WINAPI HiddenWndProc(HWND aWnd, UINT aMsg, WPARAM awParam,
LPARAM alParam);
+
   IDropSource * mNativeDragSrc;
   nsNativeDragTarget * mNativeDragTarget;
   IDataObject * mDataObject;
+
+  static nsString mFileName;	   // File name to look for
+  static nsString mDropPath;	   // Drop path
+  HWND 	   mHiddenWnd;	   // Handle to a hidden window for
notifications
+  PRInt32	   mNotifyHandle;  // Handle to installed hook (SHChangeNotify)
 };

 #endif // nsDragService_h__

Comment 46

[Yuri Kovalchuk]

Sorry about this long post? but I thought that I'd just edit my previous patch
and it'd be fine, well it didn't work as I expected:) Sorry again. This is a
patch against the latest trunk.

Comment 47

[Kathleen :Brade]

Comment on attachment 183491 [details] [diff] [review]
A new patch against the current trunk

Yuri--you should mark a patch as obsolete and "Create a new attachment" to the
bug.

I've taken care of marking the current patch as obsolete.  You should click
"Create a New Attachment" link at the bottom of the list of attachments and
upload your diffs/patch.

Comment 48

[Yuri Kovalchuk]

Attachment: Patch against the current trunk

Created a new attachment to the bug as suggested.
Again, sorry about that long post, thought it'd work...

Comment 49

[Ere Maijala (slow)]

Comment on attachment 197526 [details] [diff] [review]
Patch against the current trunk

Generally looks quite good. A few things still need to be changed, though. I
couldn't test it in action but will do when I get my build working. Here are
the review notes in no order at all:

Please change all if (...) return ...; lines so that the actual return is on
its own line, for example:

  if (strData.IsEmpty()) 
    return E_FAIL;

Could you change nsDragService::HiddenWndProc so that it uses the directory
service to get the temp path like in nsDataObj::GetFile?

+      PRInt32 nPos = strData.FindChar('\n');
+      PRInt32 nLen = strData.Length();

You don't need nLen here and you can put into the Substring call.

+  PRInt32 nPos = strData.FindChar('\n');
+  PRInt32 nLen = strData.Length();
+  nsCAutoString strURL;

Please remove nLen from nsDragService::InvokeDragSession, it is unused. 

+      // New line char is used as a delimeter (hardcode)

==> ... delimiter (hardcoded)

+      // make the name safe for the filesystem
+      wideFileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS,
'-');

Please use MangleTextToValidFilename (it's in nsDataObj, you could make it a
static member of the nsDataObj class). It's way more complete.

+      } else { 	 // no filename was supplied - try to get it from a URL

This would look cleaner if you moved the comment to the next line

+      if (NS_FAILED(rv) && rv != NS_ERROR_FILE_ALREADY_EXISTS) return E_FAIL;

Return the actual error code so as not to mask the real reason.

+  rv = StartInvokingDragSession (itemToDrag, aActionType);

Remove the extra space after the function name.

+    reg = (SHCNRegPtr)::GetProcAddress(hShell32, (LPCSTR)dwOrdinal);

Could you use the function names instead of ordinals here and in dereg? 

+#define SHCNE_RENAMEITEM_DEF 0x00000001

Enclose this inside #ifndef .. #endif.

Comment 50

[Yuri Kovalchuk]

I'll fix what you've pointed out. But before I do,
here is the the answer to the question about using function names instead of
ordinals - if I use function names and related headers it will only compille
with the 2003 SDK from Microsoft? the way it is now it compilles even with the
VS98's headers and libs, the only prerequsture to compille is ho have
shell32.dll ver 5.0 or soething like that. Should I use the headers - it will
not compile without the recent PSDK.

Comment 51

[Ere Maijala (slow)]

I didn't mean to imply you should use the headers etc, just to use names instead
of ordinals in GetProcAddress, for example 
reg = (SHCNRegPtr)::GetProcAddress(hShell32, "SHChangeNotifyRegister");

Comment 52

[Yuri Kovalchuk]

I was not exactly correct when I said that I should use headers. The point is if
do not use PSDK 2003 I can't use function names. In dll's there simply no names
for those functions, even though they are there. SSO should I stick to the PSDK
2003 and use it's header or resort to VC6 style? where I have to use ordinals.
Sorry for my bad english (hope you can understand it:).

Comment 53

[Ere Maijala (slow)]

Please see the example in comment #51 again. There's nothing requiring a certain
platform SDK in changing the ordinal number to a string in the call to
GetProcAddress. I don't mean predefined function prototypes, just the call to
GetProcAddress.

Comment 54

[Yuri Kovalchuk]

No? agin you don't get it. Take a look at shell32.dll with some tool like
depends.exe or the like. Notice there is no function names for the first couple
of functions - theese are the ones we need for this patch. But the libs and
header of the PSDK 2003 (I haven't looked at the older ones - I do not have
them) define those functions.
What I'm trying to say is that if I use function names (strings) instead of
ordinals the source would not compille with plain vc6 setup (without the psdk
installed).
If that's ok? I shall as well remove a bunch of defines and typedefs of function
pointers altogether. Again that is if it is fine to use (as a minimum) PSDK 2003
for the compilation, althoug the copilled product should(!not tested) work with
earlier release windows.
Now again to avoid confusion we have 2 choices:
1. Use latest PSDK and avoid function pointers altogether.
2. Use ordinals (can't use strings) and compille even under win98 with plain vc6
setup.

Comment 55

[Ere Maijala (slow)]

My bad, sorry. They are named in WinXP but not in some older versions. Got to
use ordinals then, but please add a comment about the reason.

Comment 56

[Yuri Kovalchuk]

I sure will. Thanks for the comments. I think tomorrow I can manage to make a
new patch with all the fixes from comment #49.

Comment 57

[Yuri Kovalchuk]

Attachment: Patch fixes issus from comment #49

This patch fixes comments #49.
I also had to modify nsDataObj - added one public static function
GetDownloadDetails - the best name I could come up with :). Had to do this to
avoid code duplication - the same code is used in nsDataObj::GetFile and in
nsDragService::InvokeDragSession - hence this function.

Comment 58

[Ere Maijala (slow)]

+      if (NS_FAILED(rv))
+          return E_FAIL;

Fix this and all "delimeter" words to "delimiter" and I'm quite satisfied. I'd
still like to test this in practise before giving r+, but it seems dragging is
completely broken at the moment :(

Comment 59

[Ere Maijala (slow)]

Comment on attachment 198293 [details] [diff] [review]
Patch fixes issus from comment #49

Hmm.. it's this patch causing the problem. For example trying to drag the site
icon from the url bar tries to call nsDataObj::GetDownloadDetails in
nsDragService::InvokeDragSession and fails.

Comment 60

[Yuri Kovalchuk]

Attachment: This patch fixes my own drag bug and previous comments

I fixed the you've described. I returned failure from GetDownloadDetails if it
was not a file drag, my fault. And fixed what you stated in prevois comments.
Now it works as expected.

Comment 61

[Nickolay_Ponomarev]

*** Bug 298856 has been marked as a duplicate of this bug. ***

Comment 62

[Ere Maijala (slow)]

Comment on attachment 199392 [details] [diff] [review]
This patch fixes my own drag bug and previous comments

Unfortunately I found a couple of issues. 

The critical one is that the drag doesn't honor canceling overwrite prompt. I
took a quick look and it seems that DoDragDrop returns same result reagrdless
of the answer to the prompt.

The second one is that I think we should show a progress dialog, download
manager or whatever downloading would normally show, because the download could
take a good while and having no feedback on the progress would be intolerable. 

Then the easy one: 
+  // Hidden winodow procedure - here we shall receive notification from the
shell

Fix the spelling error in window.

Comment 63

[Ere Maijala (slow)]

One more issue (in my opinion just a bit confusing, not a show-stopper): the
overwrite prompt gives 0 as the size of the dragged file.

Comment 64

[Yuri Kovalchuk]

>The critical one is that the drag doesn't honor canceling overwrite prompt. I
>took a quick look and it seems that DoDragDrop returns same result reagrdless
>of the answer to the prompt.

that's really odd, guess it should return DRAGDROP_S_CANCEL. Is it a bug or by design, any ideas anyone?
This one will be hard to overcome. Right now I have no ideas how to make drag honour the prompt.

Comment 65

[Yuri Kovalchuk]

Attachment: SHows download mgr and hohours drag cancel prompt

Made the download manager (or whatever is selected) to appear. Also figured how to  make the drag honour the "Confirm file replace" prompt - now it does honour the prompt.
The only thing that I was unable to fix - is the file size. The reason for this is that There is no quick way to get file size from a remote server.
I thought about making that file unique, but that is not possible too. We get the prompt just before hidden window receives notification - so there is no way of telling where the file would be created and no way to create a unique file there.
If anyone has any ideas about this..

Comment 66

[Ere Maijala (slow)]

Comment on attachment 205030 [details] [diff] [review]
SHows download mgr and hohours drag cancel prompt

Nice :) Because I couldn't find anything better to complain about, please fix these minor issues:

Remove or add tabs from/to the lines you added to Makefile.in, just make it uniform.

+  // Have to use ordinals in the call to GetProcAddres instead of function names.

Add the missing 's' to GetProcAddress in both comments.

+// some older versions of MS PSDK do not have definitions for theese, even though

theese -> these

+// so I had to add them manualy. For example if one tries to build without those

manualy -> manually

With these fixed, r=emaijala

Comment 67

[Yuri Kovalchuk]

Attachment: Fixed errors in comments

Fixed errors in comments.
By the way? do I need superreview for this and who should I ask it from?

Comment 68

[Ere Maijala (slow)]

Comment on attachment 206903 [details] [diff] [review]
Fixed errors in comments

Yes, you also need super-review. See http://www.mozilla.org/hacking/reviewers.html for more information. It's probably useful to check history to see who would be an appropriate super-reviewer. Bonsai can help here, see for example:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/widget/src/windows/nsWindow.cpp

Comment 69

[Christopher Blizzard (:blizzard)]

Comment on attachment 206903 [details] [diff] [review]
Fixed errors in comments

Not doing reviews.

Comment 70

[Jungshik Shin]

Comment on attachment 206903 [details] [diff] [review]
Fixed errors in comments

The most frequent superreviewer of the file is roc (and I wonder why you picked blizzard)

Comment 71

[Yuri Kovalchuk]

Sorry, about that. It is the first time I'm doing this...

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+nsresult nsDataObj::GetDownloadDetails(nsITransferable *aTransferable,
+                                       nsIURI **aSourceURI,
+                                       nsAString &aFilename)
+{
+    if (!aTransferable)
+        return E_FAIL;

Returning HRESULT error codes from a function that returns nsresult is not a good idea, please fix.

+  SHChangeNotifyStruct notify;
+  if (SUCCEEDED(hr)) {

You can move 'notify' inside the if.

In nsDragService::InvokeDragSession, there are many ways it can call StartWatchingShell and then fail to call GetDropPath. In that case mHiddenWnd is not destroyed and if StartWatchingShell is called again, mHiddenWnd will be leaked.

+      // Get From path (Consider:

Is there supposed to be more in this comment?

+      if (mFileName.IsEmpty()) {
+        // Empty drop path
+        mDropPath = NS_LITERAL_STRING("");
+        return 0;
+      }

You don't need to set mDropPath here, right? It should already be empty. Why do we need this check at all? It seems to me mFileName can't be empty.

+      if (wcsstr(pathTo.get(), mFileName.get()) != NULL

This just checks whether mFileName occurs anywhere in the path, right? so you if you're searching for SETUP.EXE it would match on C:\SETUP.EXE\FOO.TXT and stuff like that. Is this really what you want?

Comment 73

[Yuri Kovalchuk]

I agrre with all comments except for the last one:

>+      if (wcsstr(pathTo.get(), mFileName.get()) != NULL
>
>This just checks whether mFileName occurs anywhere in the path, right? so you
>if you're searching for SETUP.EXE it would match on C:\SETUP.EXE\FOO.TXT and
>stuff like that. Is this really what you want?

This cannot happen. When notification is delivered to the hidden window, it contains the target drop path - which has our filename in it. it does not matter where it is in the path - if the file name is there this is what we want. All we do is get the target drop path, where the file is downloaded after the drag is done.

in short the OS takes care of that - why would it deliver invalid notifications to the registered observers.

Probably it would be better to check the end of the path (the filename itself), but I think it does not matter. Should I fix this?

Comment 74

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #73)
> This cannot happen. When notification is delivered to the hidden window, it
> contains the target drop path - which has our filename in it. it does not
> matter where it is in the path - if the file name is there this is what we
> want. All we do is get the target drop path, where the file is downloaded
> after the drag is done.
> 
> in short the OS takes care of that - why would it deliver invalid
> notifications to the registered observers.

OK, then why is the wcsstr check needed at all?

Comment 75

[Yuri Kovalchuk]

This is needed to make shure that we get the right notification. Notification is delivered several times and sometimes it is (the file name is empty).
I can change it to check the filename portion of the path, do you think this will be better?

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #75)
> This is needed to make shure that we get the right notification. Notification
> is delivered several times and sometimes it is (the file name is empty).

Then this would go wrong if the filename was, say, the string "empty".

> I can change it to check the filename portion of the path, do you think this
> will be better?

Yes, as long as that does not match any notification that you don't want (e.g., what if there's a notification with message "error on file SETUP.EXE"?)

Comment 77

[Jungshik Shin]

+      if (wcsstr(pathTo.get(), mFileName.get()) != NULL &&
+          lstrcmpiW(tempPath.get(), pathFrom.get()) == 0)

lstrcmpiW is a DirectX API, isn't it? Can we assume that it's always available? Why don't you just use _wcsicmp or |tempPath.Equals(pathFrom, nsCaseInsensitiveStringComparator())|? 

Comment 78

[Yuri Kovalchuk]

Attachment: A new patch that fixes issues from comment #72

(reply to comment #76)
We cannot get text notifications. We can get only paths in this kind of notification, well not paths but PIDL (wich are shell objects in this case they contain paths). So we only need to check for the presence of the filename in the path.

(reply to comment #77)
lstrcmpiW is not a DirectX API it is a kernel API see:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/strings/stringreference/stringfunctions/lstrcmpi.asp
So I think I think we can assume it will be present anywhere.
The later (one with nsCaseInsensitive) is better but I cannot find it anywhere in the code.

Comment 79

[Jungshik Shin]

(In reply to comment #78)

> (reply to comment #77)
> lstrcmpiW is not a DirectX API it is a kernel API see:

Hmm... I saw it in Direct X API... So the same API is available in multiple places... Nonetheless, you'd better not use it (see below)
 http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui/windowsuserinterface/resources/strings/stringreference/stringfunctions/lstrcmpi.asp
> So I think I think we can assume it will be present anywhere.

No, it's not. I'm afraid you didn't read the fine details at the end of the page:

[quote]
Windows 95/98/Me: lstrcmpiW is supported by the Microsoft Layer for Unicode. To use this, you must add certain files to your application, as outlined in Microsoft Layer for Unicode on Windows 95/98/Me Systems.
[/quote]

We can't make mozilla depend on the presence of MSLU on Win 9x/ME. 
(see a long thread of discussion in bug 162361)

> The later (one with nsCaseInsensitive) is better but I cannot find it anywhere
> in the code.

See http://lxr.mozilla.org/seamonkey/ident?i=nsCaseInsensitiveStringComparator

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #78)
> (reply to comment #76)
> We cannot get text notifications. We can get only paths in this kind of
> notification, well not paths but PIDL (wich are shell objects in this case
> they contain paths). So we only need to check for the presence of the
> filename in the path.

So what can it be other than the path that we want? An empty string?

Comment 81

[Yuri Kovalchuk]

(In reply to comment #79)

Thank you for the hint. Now I have another problem - I get uresolved external symbols during link stage of gkwidget.dll (nsCaseInsensitiveStringComparator).
I think I have to link against unicharutil_s.lib but I do not know how to change the makefile or whatever I have to change to link against that lib...
can anybody give me a hint on doing this the right way?

Comment 82

[Yuri Kovalchuk]

(In reply to comment #80)

> So what can it be other than the path that we want? An empty string?

Thank you for the hint. Now I have another problem - I get uresolved external
symbols during link stage of gkwidget.dll (nsCaseInsensitiveStringComparator).
I think I have to link against unicharutil_s.lib but I do not know how to
change the makefile or whatever I have to change to link against that lib...
can anybody give me a hint on doing this the right way?

Comment 83

[Yuri Kovalchuk]

(In reply to comment #80)

> So what can it be other than the path that we want? An empty string?

Yes we can get almost anything there (I mean any path) and sometimes during debugging I even got an empty path in the notification.
This is why we have to make sure we get the filename and the source of the drag? hence the chaek for the filename and the source (OS temp dir).

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #83)
> Yes we can get almost anything there (I mean any path) and sometimes during
> debugging I even got an empty path in the notification.

So couldn't you get a notification with a path C:\SETUP.EXE\FOO.TXT while you were looking for a filename SETUP.EXE?

Try adding unicharutil to the REQUIRES section of gfx/src/windows/Makefile.in

Comment 85

[Yuri Kovalchuk]

(In reply to comment #84)

> So couldn't you get a notification with a path C:\SETUP.EXE\FOO.TXT while you
> were looking for a filename SETUP.EXE?
Yes this can happen, but I've fixed it now (I'm checking the filename portion of the path).

> Try adding unicharutil to the REQUIRES section of gfx/src/windows/Makefile.in
> 
I did this but it still complains. When unicharutil wasn't there build couldn't find nsUnicharUtils.h, now I added unicharutil to the Makefile.in, but still fails to link with unresolved external nsCaseInsensitiveStringComparator.
May be I should change something in th build system? Or does it looks like a new bug in the build system?

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #85)
> I did this but it still complains. When unicharutil wasn't there build couldn't
> find nsUnicharUtils.h, now I added unicharutil to the Makefile.in, but still
> fails to link with unresolved external nsCaseInsensitiveStringComparator.
> May be I should change something in th build system? Or does it looks like a
> new bug in the build system?

Beats me, it should have just worked. Almost certainly not a build system bug but an error in the Makefile.in. Try rebuilding from scratch just in case that fixes it.

Comment 87

[Yuri Kovalchuk]

(In reply to comment #86)
> Beats me, it should have just worked. Almost certainly not a build system bug
> but an error in the Makefile.in. Try rebuilding from scratch just in case that
> fixes it.
> 

Tried to rebuild it from scratch - it failed with the same error. Carefully checked my Makefile.in - it seems to be correct. It finds nsUnicharUtils.h header but fails to link against unicharutil_s.lib.

Here is what's happening:
when I run make command from OBJ_DIR/widget/src/windows - it links the library ok (windows_widget.lib)? but when linking gkwidget.dll it fails with unresolved external (nsCaseInsensitiveStringCamparator::operator()...).

Does anyone have any idea how to fix this?

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try adding unicharutil to the REQUIRES in widget/src/build? (which seems to be Windows specific)

Comment 89

[Yuri Kovalchuk]

(In reply to comment #88)
> Try adding unicharutil to the REQUIRES in widget/src/build? (which seems to be
> Windows specific)
> 
That didn't help but I figured how to make it build:
 
 EXTRA_DSO_LDOPTS = \
 		$(LIBS_DIR) \
 		$(EXTRA_DSO_LIBS) \
 		$(MOZ_COMPONENT_LIBS) \
+		$(MOZ_UNICHARUTIL_LIBS) \
 		$(NULL)
Added a single line to widget/src/build/Makefile.in I've seen it done in other modules that make use of unicharutil_s.lib. Tested it with static and shared builds (both from scratch).

Now I wonder is this solution correct?

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think so. Let's do it.

Comment 91

[Yuri Kovalchuk]

Attachment: Path that fixes issues from comment #72 (improved)

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 209526 [details] [diff] [review]
Path that fixes issues from comment #72 (improved)

+      if (Substring(pathTo, (pathToLength - mFileName.Length()), pathToLength).Equals(mFileName) &&

the last parameter of Substring should be mFileName.Length(), right?

Comment 93

[Yuri Kovalchuk]

(In reply to comment #92)

> the last parameter of Substring should be mFileName.Length(), right?
> 
I think it should be pathToLength (if I get it right) - or the right parameter is the length of the fragment and I was wrong (then maybe I should add this to the string reference)?

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

the last parameter is the length of the substring you want to extract, which is mFileName.Length() characters.

Comment 95

[Yuri Kovalchuk]

(In reply to comment #94)
> the last parameter is the length of the substring you want to extract, which is
> mFileName.Length() characters.
> 

Ok I'll fix that. DO I need to submit the patch for another super/review after that?

And this question:
Since it's been reviewed and superreviewed (It hasn't changed much since the last review from Ere) I have this question - what do I need to do th check this into the tree and make it resolved?

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #95)
> Ok I'll fix that. DO I need to submit the patch for another super/review
> after that?

No, not for something that small. However, you will need to post the final patch for me or Ere to check in.

> And this question:
> Since it's been reviewed and superreviewed (It hasn't changed much since the
> last review from Ere) I have this question - what do I need to do th check
> this into the tree and make it resolved?

I would like Ere to sign off one last time, that shouldn't take long. Then someone can check it in for you --- Ere or me. Thanks for your work.

Comment 97

[Yuri Kovalchuk]

Attachment: Final patch that fixes Substring error.

Fixed Substring's 3rd argument error. No other modifications, not requesting review for this since this is the only change (if it get review from Ere it can be checked in).

Comment 98

[Ere Maijala (slow)]

Comment on attachment 209536 [details] [diff] [review]
Final patch that fixes Substring error.

One anomaly needs to be resolved. Why does it sometimes show a "Moving..." dialog briefly even if I answer No to the overwrite prompt. If I saw correctly it says it's moving the file from Temp to Desktop where I had the original file already. It's not overwritten, but it's still strange.

Comment 99

[Yuri Kovalchuk]

(In reply to comment #98)
> One anomaly needs to be resolved. Why does it sometimes show a "Moving..."
> dialog briefly even if I answer No to the overwrite prompt. If I saw correctly
> it says it's moving the file from Temp to Desktop where I had the original file
> already. It's not overwritten, but it's still strange.
> 

It shows this dialog even in unpatched browser. I'm pretty sure that this happens because there is DROPEFFECT_MOVE flag added to the IDataObject. The file gets deleted from the temp dir after the drag is complete - this is why this progress dialog appears. I think this issue doesn't belong to this bug this is  a new bug.
The original bug for this issue is http://bugzilla.mozilla.org/show_bug.cgi?id=203847, it is fixed now. Maybe we should file a new bug for this issue?
If yes, assign me to fix it, I'll do it.

Comment 100

[Yuri Kovalchuk]

(In reply to comment #99)
> this progress dialog appears. I think this issue doesn't belong to this bug
> this is  a new bug.
> The original bug for this issue is
> http://bugzilla.mozilla.org/show_bug.cgi?id=203847, it is fixed now. Maybe we
> should file a new bug for this issue?
> If yes, assign me to fix it, I'll do it.
> 
Replying to myself, huh. I investigated this issue a little closer.
It seem like to fix this (make the moving dialog appear only when really moving) I'll need to move few lines of code from nsDragService::StartInvokingDragSession
to nsDragService::InvokeDragSession.
But it really needs more testing and debugging, just to make sure it works as expected.
I still think a new bug will be better.

Comment 101

[Ere Maijala (slow)]

Comment on attachment 209536 [details] [diff] [review]
Final patch that fixes Substring error.

Ok, let's do it in a separate bug. Transferring also sr to this patch.

Comment 102

[Ere Maijala (slow)]

I'll check the patch in.

Comment 103

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 104

[Joey Minta]

(In reply to comment #103)
> Fix checked in to trunk.
> 
Sunbird solaria is burning. http://tinderbox.mozilla.org/showlog.cgi?log=Sunbird/1139144940.2823.gz

Building deps for /cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build/nsWinWidgetFactory.cpp
/cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/build/cygwin-wrapper cl -FonsWinWidgetFactory.obj -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"WINNT5.2\" -DOSARCH=\"WINNT\" -DBUILD_ID=2006020505 -D_IMPL_NS_WIDGET -DMOZ_UNICODE -I/cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build -I/cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build/../xpwidgets -I/cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build/../windows  -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/include/necko -I../../../dist/include/uconv -I../../../dist/include/gfx -I../../../dist/include/dom -I../../../dist/include/timer -I../../../dist/include/accessibility -I../../../dist/include   -I../../../dist/include/widget -I../../../dist/include/nspr    -I../../../dist/sdk/include       -TP -nologo -W3 -Gy -FdnsWinWidgetFactory.pdb  -DNDEBUG -DTRIMMED -Zi -O1 -UDEBUG -DNDEBUG -MD            -DX_DISPLAY_MISSING=
1 -DMOZILLA_VERSION=\"1.9a1\" -DMOZILLA_VERSION_U=1.9a1 -DHAVE_SNPRINTF=1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWINVER=0x400 -D_WIN32_WINNT=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11=1 -D_X86_=1 -DD_INO=d_ino -DMOZ_DEFAULT_TOOLKIT=\"windows\" -DMOZ_THUNDERBIRD=1 -DMOZ_BUILD_APP=mail -DMOZ_XUL_APP=1 -DMOZ_STATIC_MAIL_BUILD=1 -DMOZ_DISTRIBUTION_ID=\"org.mozilla\" -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DACCESSIBILITY=1 -DMOZ_XPINSTALL=1 -DMOZ_JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_XTF=1 -DMOZ_UPDATE_CHANNEL=default -DMOZ_LOGGING=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_XUL=1 -DMOZ_PROFILELOCKING=1 -DMOZ_MORK=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1 -DMOZILLA_LOCALE_VERSION=\"1.9a1\" -DMOZILLA_REGION_VERSION=\"1.9a1\" -DMOZILLA_SKIN_VERSION=\"1.8\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CLIENT /cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/widget/src/build/nsWinWidgetFactory.cpp
nsWinWidgetFactory.cpp
/cygdrive/c/builds/tinderbox/Lt-Trunk/WINNT_5.2_Depend/mozilla/build/cygwin-wrapper link -NOLOGO -DLL -OUT:gkwidget.dll -PDB:gkwidget.pdb -SUBSYSTEM:WINDOWS  nsWinWidgetFactory.obj   widget.res -DEBUG -OPT:REF -OPT:nowin98  -IMPLIB:fake.lib  ../../../dist/lib/widget_windows.lib ../../../dist/lib/xpwidgets_s.lib    ../../../dist/lib/gkgfx.lib ../../../dist/lib/xpcom.lib ../../../dist/lib/xpcom_core.lib ../../../dist/lib/nspr4.lib ../../../dist/lib/plc4.lib ../../../dist/lib/plds4.lib  ../../../dist/lib/unicharutil_s.lib   kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib advapi32.lib uuid.lib ole32.lib oleaut32.lib comctl32.lib comdlg32.lib shell32.lib gdi32.lib imm32.lib   
   Creating library fake.lib and object fake.exp
widget_windows.lib(nsDragService.obj) : error LNK2001: unresolved external symbol __imp__SHGetFolderLocation@20
gkwidget.dll : fatal error LNK1120: 1 unresolved externals
make[6]: *** [gkwidget.dll] Error 96

Comment 105

[Ere Maijala (slow)]

Patch backed out. SHGetFolderLocation is only available beginning with Win2K or WinME.

Comment 106

[Yuri Kovalchuk]

Attachment: This fixes win98, NT4 build failure

Yes this is true, changed SHGetFolderLocation to SHGetSpecialFolderLocation which is available on Win95 & NT4, that should do the trick.
Checked other functions for compatibility with older versions of windows everything seems to be alright.
Please review.

Comment 107

[Yuri Kovalchuk]

Comment on attachment 210987 [details] [diff] [review]
This fixes win98, NT4 build failure

Please sr this

Comment 108

[Yuri Kovalchuk]

Sorry to bug you again people but the patch passed r & sr who's to check this in now?
One more thing, Ere about the moving dialog after the drop - should I file a bug myself or there is one filed somewhere already?

Comment 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can check this in for you if Ere does't get to it first.

Comment 110

[Yuri Kovalchuk]

(In reply to comment #109)
> I can check this in for you if Ere does't get to it first.
> 

Does it matter who checks this in? I just wanted to make sure it's there before moving on. Thank you

Comment 111

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It doesn't matter, as long as it gets checked in ASAP. Thanks!

Comment 112

[Ere Maijala (slow)]

Roc, go ahead, I can't watch the tree right now. Yuri, file a new bug for the Moving dialog if you can't find an old one. I don't know of any.

Comment 113

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in. Thank you very much, Yuri!

Comment 114

[IU]

This has caused a regression.  Now any image that is dragged causes a crash.  Follow the thread here (starting with the 4th post down): http://forums.mozillazine.org/viewtopic.php?t=384078&postdays=0&postorder=asc&postsperpage=15&start=30

Comment 115

[Peter van der Woude [:Peter6]]

this caused Bug 328077 (crash)

Comment 116

[Yuri Kovalchuk]

Submitted a patch to fix the crash. see bug# 328077

Comment 117

[Yuri Kovalchuk]

Can this get into Firefox 2.0? What should I do to make it happen, help please

Comment 118

[Ryuichi KUBUKI]

I think the FF trunk still has this bug. Not only large images, it happens also for 100KB small images.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060404 Firefox/1.6a1 (non-Cairo trunk, no extensions)

Comment 119

[Yuri Kovalchuk]

(In reply to comment #118)
> I think the FF trunk still has this bug. Not only large images, it happens also
> for 100KB small images.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060404
> Firefox/1.6a1 (non-Cairo trunk, no extensions)
> 

This is why I'm asking how to get this into firefox? Anyone?

Comment 120

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #118)
> I think the FF trunk still has this bug. Not only large images, it happens
> also for 100KB small images.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060404
> Firefox/1.6a1 (non-Cairo trunk, no extensions)

(In reply to comment #119)
> This is why I'm asking how to get this into firefox? Anyone?

Comment 118's build ID (trunk from 20060404) would include the patch from this bug, so if the bug is present in that build, it's because the patch didn't entirely fix the issue, not because it hasn't reached the right branch. It could also be a different bug.

Comment 121

[Yuri Kovalchuk]

(In reply to comment #120)
>...so if the bug is present in that build, it's because the patch didn't
> entirely fix the issue, not because it hasn't reached the right branch. It
> could also be a different bug.
> 

What do you mean by didn't etirely fix the issue?  Who can comment on that? 

Comment 122

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #121)
> What do you mean by didn't etirely fix the issue?  Who can comment on that? 

All I'm saying is that the build in comment 118 includes the patch from this bug.

Comment 123

[Ryuichi KUBUKI]

So SeaMonkey works OK now? Or is it fixed only in Win 2000?
Anyway it's still not working in this non-Cairo trunk build without an extension, all image files I dragged from FF to Explorer create 0-byte files. How about reopening this for FF and Windows XP?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060405 Firefox/1.6a1

Comment 124

[Ryuichi KUBUKI]

I recall in an older version of FF you could drag an image and create a file when it's in the cache, but now all images create empty files regardless of their size.

As Gavin suggests I assume this build contains the patch but it only creates an empty file, so I reopen this for WinXP.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060414 Firefox/3.0a1

Comment 125

[Yuri Kovalchuk]

(In reply to comment #123)
> So SeaMonkey works OK now? Or is it fixed only in Win 2000?
> Anyway it's still not working in this non-Cairo trunk build without an
> extension, all image files I dragged from FF to Explorer create 0-byte files.

Updated the tree and built Firefox, but could not reproduce this.
I'm using: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20060321 Firefox/2.0a1. - The date is wrong becuse I just pulled updated sources and replaced files in my BonEchoAlpha1 build (to speed up the build). The patch is there and is working well.

What are you trying to do? If you check Work Offline option, clear the cache and then try to drag an image - you will get 0 size file - but that's because you deleted all instances of the downloaded file and there is no way to get it from the server. Are you trying to do this?

Haven't had a chance to test it on WinXP though, but I'm sure it should work there as well.

Comment 126

[Yuri Kovalchuk]

it works on XP as well, used Minefield for testing (build from 2006-17-04).
Don't get zero size images except when I'm offline and the cache is cleared...
Changing back to fixed.

Comment 127

[Ryuichi KUBUKI]

(In reply to comment #126)
> it works on XP as well, used Minefield for testing (build from 2006-17-04).
> Don't get zero size images except when I'm offline and the cache is cleared...
> Changing back to fixed.
> 

Weird, it still doesn't work for me (non-Cairo build without an extension).
I tried
http://www.mozilla.com/firefox/central/firefox-central-title.png
in http://www.mozilla.com/firefox/central/ and still get an empty file.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060418 Firefox/3.0a1

Comment 128

[Ryuichi KUBUKI]

(In reply to comment #127)
> (In reply to comment #126)
> > it works on XP as well, used Minefield for testing (build from 2006-17-04).
> > Don't get zero size images except when I'm offline and the cache is cleared...
> > Changing back to fixed.
> > 
> 
> Weird, it still doesn't work for me (non-Cairo build without an extension).
> I tried
> http://www.mozilla.com/firefox/central/firefox-central-title.png
> in http://www.mozilla.com/firefox/central/ and still get an empty file.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060418
> Firefox/3.0a1
 
OK I think I could narrow it down. It always creates an empty file if you drag an image onto a drive other than C:\, while it always works for C:\. Can anyone confirm this?

Comment 129

[Yuri Kovalchuk]

(In reply to comment #128)
> OK I think I could narrow it down. It always creates an empty file if you drag
> an image onto a drive other than C:\, while it always works for C:\. Can anyone
> confirm this?
> 
Yes it does, looking at it now.

Comment 130

[Ryuichi KUBUKI]

(In reply to comment #129)
> (In reply to comment #128)
> > OK I think I could narrow it down. It always creates an empty file if you drag
> > an image onto a drive other than C:\, while it always works for C:\. Can anyone
> > confirm this?
> > 
> Yes it does, looking at it now.

Then let's reopen it. Other people could confirm it
http://forums.mozillazine.org/viewtopic.php?t=406948&start=45

Comment 131

[Axel Hecht]

(In reply to comment #18)

> I made those vars static to avoid sending additional messages to the window
> procedure. It is static and cannot access regular members, so I thought it'll
> be a good idea to make varables that this window procedure uses static too.
> That is why.

That crashes my windows VC8 Express debug build on shutdown.

Comment 132

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yuri, could you fix that?

Comment 133

[Yuri Kovalchuk]

(In reply to comment #132)
> Yuri, could you fix that?

Been thinking about it for while now. I can fix it but I need some advice.
First, why this happens - this is beccause that for different drives shell issues different notification. There is no way of telling where the file came from -  something else could create a file with the same name as the one being dragged.
Another way is to restore to the code that was there earlier - if the file is in cache then pass it on and do not mess with it anymore. But that will still show violation dialogs etc.
Yet another way to go is to start supporting IStream as a storage medium. But this requires the IDataObject to support IAsyncOperation which is only available on Win 2000 and Win Me or later (shell50.dll or later).

It seems to me that the best way to go is the last one, but the second one is ok too, only a little limited...

What should I do?

Comment 134

[Ryuichi KUBUKI]

(In reply to comment #133)
> Yet another way to go is to start supporting IStream as a storage medium. But
> this requires the IDataObject to support IAsyncOperation which is only
> available on Win 2000 and Win Me or later (shell50.dll or later).

If it's the only way please implement it.
With Bug 330276 Win9X/Me were already dropped from the support, now you can forget these older platforms (the issue in Comment 79 too)

Comment 135

[Axel Hecht]

Well, the shortest path to get rid of the static strings is to make them static
string pointers and to delete the strings in the desctructor of the drag service.

That said, I don't know anything about this bug in particular, so I can't really
give an opinion on whether that information should be stored in the service, or
somewhere else, and if there is such a thing as a "right lifetime" for that data.

Comment 136

[Yuri Kovalchuk]

(In reply to comment #134)
> With Bug 330276 Win9X/Me were already dropped from the support, now you can
> forget these older platforms (the issue in Comment 79 too)
> 
If there is no support for pre W2K anymore then I guess I can implement this.
But this is a different bug, don't remember it's number though. I'll post it here when I find it.

Comment 137

[Benjamin Smedberg]

This patch causes a crash in windows xulrunner builds running the destructor for the static strings. The problem with the static strings is that you're not guaranteed that the string statics (most notably nsObsoleteAString::sCanonicalVTable are initialized first).

Comment 138

[Yuri Kovalchuk]

(In reply to comment #137)
Now working on implementing D&D via IStream - this will remove all static strings and fix the original issue of this bug I hope.

Comment 139

[Yuri Kovalchuk]

Have a fix for this bug (using IStream and IAsyncOperation). But it will only work on w2k or later. Also I'd like to see it in Firefox 2.0, so what do I have to do make it all happen :) ? I mean against what branch should I make a patch. Please help

Comment 140

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we use GetProcAddress etc to make it work on Win2K but still run on Win9x?

Comment 141

[Yuri Kovalchuk]

(In reply to comment #140)
> Can we use GetProcAddress etc to make it work on Win2K but still run on Win9x?
> 
The new patch I'm working on is not using GetProcAddress at all. I do not know if it is possible at all. I'm taking advantage of the IAsyncOperation which is available only on w2k or WinMe (requies shell.dll v5+):
http://windowssdk.msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/ifaces/iasyncoperation/iasyncoperation.asp

It's the shell itself that queries this interface from DataObject if it is not available the operation's synchronous and will hang the browser while the download is in progress. Although I do not think this is going to happen very often, if at all, but it is possible. All the normal file drags are from cache anyway.

I heard that support for pre-w2k was dropped.

Comment 142

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're dropping pre-Win2K support for FF3. For FF2 we still want to support Win9x.

If we just disable these drags for Win9x in FF2, I think that would be OK.

Comment 143

[Yuri Kovalchuk]

(In reply to comment #142)
> If we just disable these drags for Win9x in FF2, I think that would be OK.
> 
By disbale you mean something like:
if (DllMajorVersion("shell32.dll") >= 5)
  DoAsyncDrag();  // use IAsyncOperation
else
  DoSyncDrag();   // do normal drag and drop. But if the file is not cached fail as it was before.

Is this good enough?
What branch should I use (the main trunk(HEAD) or some other)?

Comment 144

[Masatoshi Kimura [:emk]]

(In reply to comment #143)
> What branch should I use (the main trunk(HEAD) or some other)?

Use MOZILLA_1_8_BRANCH for Fx2.
However you should make a trunk patch first. Then port it to the branch.
Any branch patch wouldn't be approved to land unless you baked the corresponding patch on the trunk.

Comment 145

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #143)
> (In reply to comment #142)
> > If we just disable these drags for Win9x in FF2, I think that would be OK.
> > 
> By disbale you mean something like:
> if (DllMajorVersion("shell32.dll") >= 5)
>   DoAsyncDrag();  // use IAsyncOperation
> else
>   DoSyncDrag();   // do normal drag and drop. But if the file is not cached
> fail as it was before.

That sounds good to me.

Comment 146

[Yuri Kovalchuk]

Attachment: patch using IStream and IAsyncOperation

This is a working version of the patch, it can go unchanged to the Firefox 2.0 branch (I added the shell32.dll version check in nsDragService.cpp just before the DoDragDrop call).
Please r&sr this.

Comment 147

[Ere Maijala (slow)]

Comment on attachment 225689 [details] [diff] [review]
patch using IStream and IAsyncOperation

Remove PRNTDEBUG stuff if you don't need them anymore or at least get rid of unused PRNTDEBUG2 and PRNTDEBUG3.

Variable naming:
m_IsAsyncMode -> mIsAsyncMode. Likewise for others.

+  if (isAsyncAvailable)
+  {
+    // do async drag
+    if (SUCCEEDED(aDataObj->QueryInterface(IID_IAsyncOperation,
+                                          (void**)&pAsyncOp)))
+    pAsyncOp->SetAsyncMode(TRUE);
+  }

Indentation is wrong for pAsyncOp->...

+  if (NS_FAILED(rv))
+    return rv;

Change to NS_ENSURE_SUCCESS(rv, rv); Likewise for return E_FAIL;

+  if (outStream == NULL)
+    return E_INVALIDARG;

Use NS_ENSURE_TRUE(outStream, E_INVALIDARG); Likewise for other parameter checks.

Make nsDataObj::CStream::Read return explicitly S_OK (not just the result of the comparison).

+    return NOERROR;

Shouldn't QueryInterface return S_OK?

Comment 148

[Yuri Kovalchuk]

Attachment: IStream and IAsyncOperation implementation

Updated, please review.

Comment 149

[Ere Maijala (slow)]

Comment on attachment 226307 [details] [diff] [review]
IStream and IAsyncOperation implementation

+  nsDataObj::CStream *pStream = new nsDataObj::CStream();
+  NS_ENSURE_TRUE(pStream, E_OUTOFMEMORY);
+  rv = pStream->Init(sourceURI);
+  NS_ENSURE_SUCCESS(rv, E_FAIL);
+  *outStream = pStream;

The above would leak pStream if Init fails. With that fixed, r=emaijala.

One additional thing I'd like you to check is if you could use DROPEFFECT_COPY instead of DROPEFFECT_MOVE as copying is what the action for the user really is so the "Moving" dialog would not be there to mislead the user. This isn't anything mandatory but would look better.

Comment 150

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRUint64 maji64 = versionInfo.dwMajorVersion;
+    PRUint64 mini64 = versionInfo.dwMinorVersion;

These fields are DWORDs, so only 32bit, so use PRUint32.

Loading the library every time to check the version might not be a good idea, but we're not actually loading the library, right? because it's already loaded, almost certainly. Might want to mention that in a comment.

+  rv = GetDownloadDetails(getter_AddRefs(sourceURI),
+                          wideFileName);

Check rv here.

+  ULONG refCnt = Release();

Remove the unused variable.

+  strcpy(fileGroupDescA->fgd[0].cFileName, nativeFileName.get());
...
+  wcscpy(fileGroupDescW->fgd[0].cFileName, wideFileName.get());

How do you know these won't overflow?

Comment 151

[Yuri Kovalchuk]

Attachment: Fixes issues from comments #149 & #150

Tried to use DROPEFFECT_COPY intead of DROPEFFECT_MOVE, but the dialog's caption still reads Moving... Could not find the reason for this, but I guess it is windows' way of doing things. Or perhaps it has something to do with removing the original file from the temp folder. In any case I failed to change the caption.
Please review.

Comment 152

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 228657 [details] [diff] [review]
Fixes issues from comments #149 & #150

+    PRUint32 maji64 = versionInfo.dwMajorVersion;
+    PRUint32 mini64 = versionInfo.dwMinorVersion;
+    lVersion = LL_INIT(maj, min);

maji64/mini64 should be renamed to maji32/mini32 or just maj/min, I guess. But er, how did this compile?

You're not handling the result of GetDownloadDetails in nsDataObj::GetFileDescriptor_IStreamA.

Fix those, and it's good.

Comment 153

[Yuri Kovalchuk]

Attachment: Fixes issuse from comment #152

> maji64/mini64 should be renamed to maji32/mini32 or just maj/min, I guess. But
> er, how did this compile?
If I change maji64/mini64 to 32 it would not compile. The reason for this LL_INIT macro. For some unknown reason it adds i64 to the names of the vars passed to it:
in prlong.h
#define LL_INIT(hi, lo)  ((hi ## i64 << 32) + lo ## i64)
Could not find any explanation for this, so I leave it as is.

Comment 154

[Yuri Kovalchuk]

Good. Who's going to check this is?
Can this patch go to the Firefox 2.0 branch? Do I have to make a different patch against MOZILLA_1_8_BRANCH for that?

Comment 155

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Your best bet is to find someone on IRC to check it in for you. Gavin Sharp is usually helpful :-)

It needs to land on the trunk and wait for a bit before we can think about putting it on branch. We will need a patch against MOZILLA_1_8_BRANCH for that.

If there are any more regressions or problems please open new bugs for them. This bug is too long.

Thanks for all this!

Comment 156

[:Gavin Sharp [email: [e-mail]]]

I can check this is once the tree is green and I get CVS working again (hopefully later today), if you find no one else before then.

Comment 157

[Yuri Kovalchuk]

(In reply to comment #156)
> I can check this is once the tree is green and I get CVS working again
> (hopefully later today), if you find no one else before then.
> 
Yes please do it. Thank you!

Comment 158

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/windows/nsDragService.h 	1.25
mozilla/widget/src/windows/nsDragService.cpp 	1.50
mozilla/widget/src/windows/nsDataObj.h 	1.30
mozilla/widget/src/windows/nsDataObj.cpp 	1.80

Comment 159

[Scott MacGregor]

This checkin appears to have broken the tree.

The Thunderbird patrocles build is red:

e:/builds/tinderbox/Tb-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsDataObj.h(119) : error C2504: 'IAsyncOperation' : base class undefined

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird

Comment 160

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #159)
> This checkin appears to have broken the tree
> : error C2504: 'IAsyncOperation' : base class undefined

I suspect it is caused by bug 336337. I can't reproduce the problem locally, my attempts to fix the bustage got nowhere, the amount of time prometheus takes to cycle is limiting my ability to debug, and I need to go to bed, so I'm going to back the patch out.

Comment 161

[:Gavin Sharp [email: [e-mail]]]

backed out

Comment 162

[Yuri Kovalchuk]

(In reply to comment #160)

> I suspect it is caused by bug 336337. I can't reproduce the problem locally, my
> attempts to fix the bustage got nowhere, the amount of time prometheus takes to
> cycle is limiting my ability to debug, and I need to go to bed, so I'm going to
> back the patch out.
> 
Yes it is caused by using VC6 without the latest platform SDK installed.
Is Thunderbird going to be built with VC71 any time soon,
or, perhaps I should define missing identifiers myself (like suggested for bug #344504)?
Which is the best way to go, please comment.

Comment 163

[Yuri Kovalchuk]

I suggest we move discussions to bug #203307 - since this is what it is. And this bug is really too long :). Is Scott doesn't mind I'll take the new bug, assign it to me please. 

Comment 164

[Yuri Kovalchuk]

(In reply to comment #161)
> backed out
> 

This makes me wonder what to am I to do if I want this to be on the trunk? Help, please.

Comment 165

[Yuri Kovalchuk]

marking as fixed, use bug 203307 if you find any issuses.

Comment 168

[zoltee]

I still get this problem, however instead of an empty image, I get I partial image. 

Here's a quick
mention from the author of Total Commander:

Total Commander :: View topic - Drag & drop images from Firefox?
http://www.ghisler.ch/board/viewtopic.php?t=5772

"Sorry, I haven't found a way yet to correct this problem. When FF reports the
drop to Total Commander, the file exists, but it is empty."


Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

Comment 169

[Steffen Wilberg]

Looks like the 2006-02-20 12:48 checkin (attachment 210987 [details] [diff] [review]) broke dragging attachments out of Thunderbird on Windows Vista (not XP), see bug 458159.