unable to dnd multiple bookmarks in firefox on the mac (or multiple messages in tbird) in http://lxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsDragService.mm, for items other than files, I'm not sure how GetNumDropItems() will return something other than 1 (cyen confirms), and for GetData(), we ignore aItemIndex unless we are dealing with files. see bug #336214 comment #7 - comment #10 for more details. josh, can you investigate?

shebs, do you think your patch in bug 358093 will have any effect on this?

(In reply to comment #0) > and for GetData(), we ignore aItemIndex > unless we are dealing with files. I noticed this while I was doing clean up of the clipboard and drag stuff. It's probably just something nobody has implemented yet.

My patch for 358093 likely won't have any effect, doesn't change any single/multiple handling.

Also I see that the drop indicator for dragging bookmarks is incorrect. It I have bookmarks A, B, C, and D and I try to drag A between C and D, it actually ends up between B and C. Is there a places bug for that?

Yes, there is - the patch awaiting review for Bug 382679 should fix that off-by-one bug.

I can't even drag one bookmark, let alone multiple. Can anyone else drag a single bookmark to a new position using the current Mac OS X nightly?

Dragging single bookmarks works correctly for me using last night's nightly.

Marking this as blocking mozilla1.9alpha6 because it is pretty bad. I know exactly what the problem is, working on a fix now.

I'm putting this up to save my work and explain earlier rather than later what is going on. This bug pointed out some major problems with the drag service and thus I had to rewrite a good chunk of it. In doing so, I discovered two other serious bugs, which are fixed in this patch. In total, this patch fixes the following issues (with explanations): - Fixes this bug, dragging multiple items. We were just grabbing the last item in the list of items coming from gecko and putting it on the clipboard. With this patch we do something similar to what Carbon did, which is if a drag starts from Gecko we cache the transferable list to query for data if the drag ends in Gecko. This is the right way to do it. - Fixes a serious data corruption bug that can occur any time Mozilla-specific data is put on the clipboard. We were doing Mac->DOM newline replacement on any custom data (e.g. x-moz-place) without being able to know whether or not it was string data. In many common cases it would not have been a string. I found this when I was reading through the code. - Fixes dragging images out of Gecko. We were gathering the data for the Mac OS X image pasteboard format but not actually posting it. I found this when I was reading through the code. This patch should be fine functionally, but it isn't ready to land as there is some re-factoring left to do. I'll post that soon.

This looks good, modulo the null mDataItems issue, fixes things that were mystifying me a bit in fact.

This fixes a null pointer crash and doesn't leak mDataItems, though it'd be nice to find a fix for the leak that doesn't often retain mDataItems until the next drag that originates from Gecko.

Comment on attachment 270199 [details] [diff] [review] fix v0.9 Looks good. I note a + if (mDataItems) + NS_RELEASE(mDataItems); in InvokeDragSession that could be replaced with NS_IF_RELEASE too.

Comment on attachment 270199 [details] [diff] [review] fix v0.9 >Index: widget/src/cocoa/nsDragService.h >=================================================================== >RCS file: /cvsroot/mozilla/widget/src/cocoa/nsDragService.h,v >retrieving revision 1.10 >diff -U8 -r1.10 nsDragService.h >--- widget/src/cocoa/nsDragService.h 27 Jun 2007 05:26:18 -0000 1.10 >+++ widget/src/cocoa/nsDragService.h 28 Jun 2007 17:28:27 -0000 >@@ -57,17 +57,20 @@ > // nsIDragService > NS_IMETHOD InvokeDragSession(nsIDOMNode *aDOMNode, nsISupportsArray * anArrayTransferables, > nsIScriptableRegion * aRegion, PRUint32 aActionType); > > // nsIDragSession > NS_IMETHOD GetData(nsITransferable * aTransferable, PRUint32 aItemIndex); > NS_IMETHOD IsDataFlavorSupported(const char *aDataFlavor, PRBool *_retval); > NS_IMETHOD GetNumDropItems(PRUint32 * aNumItems); >+ NS_IMETHOD EndDragSession(PRBool aDoneDrag); This should be in the "nsIDragService" section, not the "nsIDragSession" section. >+ nsISupportsArray* mDataItems; // only valid for a drag started within gecko This should be nsCOMPtr<nsISupportsArray> >+ > nsDragService::nsDragService() >+: mDataItems(nsnull) Unneeded with the nsCOMPtr change. >+ if (mDataItems) >+ NS_RELEASE(mDataItems); >+ NS_ADDREF(aTransferableArray); >+ mDataItems = aTransferableArray; Only the last line should be needed now, with the change to nsCOMPtr. > NS_IMETHODIMP > nsDragService::GetData(nsITransferable* aTransferable, PRUint32 aItemIndex) >+ for (PRUint32 i = 0; i < acceptableFlavorCount; i++) { >+ break; >+ } >+ else if (strcmp(flavorStr, kUnicodeMime) == 0) { You should have either a break or an else, but you don't need both. >+ NSString* pString = [globalDragPboard stringForType:NSStringPboardType]; >+ if (!pString) >+ continue; >+ >+ NSData* stringData = [pString dataUsingEncoding:NSUnicodeStringEncoding]; >+ unsigned int dataLength = [stringData length]; >+ unsigned char* clipboardDataPtr = (unsigned char*)malloc(dataLength); You should handle allocation failure. Also, you'd need fewer casts if clipboardDataPtr were a void*, which it probably should be given that it's not actually an array of characters. >+ [stringData getBytes:(void*)clipboardDataPtr]; >+ >+ // The DOM only wants LF, so convert from MacOS line endings to DOM line endings. >+ nsLinebreakHelpers::ConvertPlatformToDOMLinebreaks(flavorStr, (void**)&clipboardDataPtr, (PRInt32*)&dataLength); Please don't cast to fix up errors with incompatible integer types. Use assignment to another variable. >+ // skip BOM (Byte Order Mark to distinguish little or big endian) >+ unsigned char* clipboardDataPtrNoBOM = clipboardDataPtr; Seems like this should be a PRUnichar* (or whatever mac's 2-byte character type is). >+ if ((dataLength > 2) && >+ ((clipboardDataPtr[0] == 0xFE && clipboardDataPtr[1] == 0xFF) || >+ (clipboardDataPtr[0] == 0xFF && clipboardDataPtr[1] == 0xFE))) { And then you could test clipboardDataPtrNoBOM[0] here and need to check only [0]. Also, why can't you assume that the BOM is the right way around? If it's not, you need to do byte-swapping. >+ dataLength -= sizeof(PRUnichar); >+ clipboardDataPtrNoBOM += sizeof(PRUnichar); But if it's a PRUnichar* you need to add 1 here, not sizeof(PRUnichar). >+ break; >+ } >+ // We have never supported this on Mac OS X, we should someday. Normally dragging images >+ // in is accomplished with a file path drag instead of the image data itself. >+ /* >+ else if Again, pick either break or else. >+NS_IMETHODIMP >+nsDragService::EndDragSession(PRBool aDoneDrag) >+{ >+ NS_IF_RELEASE(mDataItems); This can just be assignment to null with the change to nsCOMPtr. >+ return nsBaseDragService::EndDragSession(aDoneDrag); >+} sr=dbaron with those changes, although you don't *have* to make the type changes to clipboardDataPtr.

This is the patch I landed. dbaron - can you check this patch again even though it has already landed? it contains fixes for your comments, except we still don't do byte swapping. we never have, I'll file a bug on it.

Comment on attachment 270207 [details] [diff] [review] fix v1.0 Looks ok, except you didn't remove the (PRInt32*) cast -- although you did change the type of the variable so it wasn't needed. Did the bug you mentioned in the previous comment ever get filed? (Marking superreview+, not review+, since that's what it was.)

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008040904 Minefield/3.0pre. I can drag and drop bookmarks.

Added Litmus Test case https://litmus.mozilla.org/show_test.cgi?id=5252 into the Leopard Test Suite. https://litmus.mozilla.org/show_test.cgi?id=4524 should cover the FFT.