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).

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.

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.

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

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 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 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]

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

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.

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

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.

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?

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.

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

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?

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.

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 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?

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?

Why everyone is silent? Did I do something wrong?

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.

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 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

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...

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?

For now, use nsCopyNativeToUnicode and nsCopyUnicodeToNative

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.

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.

> 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 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).

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 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 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.

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 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.

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.

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

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

Bug 231305 and Bug 256173 look like duplicates of this.

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?

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

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.

(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 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?

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 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__

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 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.

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

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.

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.

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");

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:).

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.

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.

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.

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.

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.

+ 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 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.

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.

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

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.

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.

>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.

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 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

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

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 on attachment 206903 [details] [diff] [review] Fixed errors in comments Not doing reviews.

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)

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

+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?

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?

(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?

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?

(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"?)

+ 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())|?

(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.

(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

(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?

(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?

(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?

(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).

(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

(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?

(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.

(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?

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

(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?

I think so. Let's do it.

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?

(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)?

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

(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?

(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.

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 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.

(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.

(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 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.

I'll check the patch in.

Fix checked in to trunk.

(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

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

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 on attachment 210987 [details] [diff] [review] This fixes win98, NT4 build failure Please sr this

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?

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

(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

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

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.

Checked in. Thank you very much, Yuri!

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

this caused Bug 328077 (crash)

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

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

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 #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?

(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.

(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?

(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.

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

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

(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.

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.

(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

(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?

(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.

(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

(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.

Yuri, could you fix that?

(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?

(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)

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.

(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.

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).

(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.

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

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

(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.

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.

(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)?

(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.

(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.

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 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?

Updated, please review.

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.

+ 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?

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 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.

> 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.

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?

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!

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.

(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!

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

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

(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.

backed out

(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.

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.

(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.

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

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

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.