(In reply to comment #57)

Hej Karl,

> It makes it much easier for reviewers to understand the changes and work out
> which code is being modified when patches show function names and have
> additional lines of context.
Ok, fixed, thanks for the hint!

> >+    // We can not delete the temporary files immediately after the
> >+    // drag has finished, because the target application might have not
> >+    // copied the temporary file yet. Instead we collect all temporary
> >+    // files in mTemporaryFiles array and remove them here in the destructor.
> 
> The destination app should not indicate that it is finished until it has
> copied the file.  This seems to be implied by both the XDND and Motif
> protocols:
Right, unfortunately the KDE applications have a bug there (well, it is actually a bug/missing feature in Qt) and they return the XDNDFinished signal
too early before they have started copying the file. I talked with the
Qt developers already, however it won't be fixed in near future, therefor
that delayed deleting would be a helpfull workaround.
(See last entry on http://www.freedesktop.org/wiki/Specifications/XDNDRevision)

> We don't really want to keep accumulating these files when the application
> runs for months and the files would not be cleaned up if the app doesn't get a
> chance to shut down nicely.
Is it so common that a Thunderbird app runs for months?

> In GetSourceList():
> 
> >+                        // check if application/x-moz-file-promise url is supported.
> >+                        // If so, advertise text/uri-list.
> >+                        if (strcmp(flavorStr, kFilePromiseURLMime) == 0) {
> >+                            GdkAtom urlAtom = gdk_atom_intern(gTextUriListType, FALSE);
> >+                            GtkTargetEntry *urlTarget =
> >+                             (GtkTargetEntry *)g_malloc(sizeof(GtkTargetEntry));
> >+                            urlTarget->target = g_strdup(gTextUriListType);
> >+                            urlTarget->flags = 0;
> >+                            /* Bug 331198 */
> >+                            urlTarget->info = NS_PTR_TO_UINT32(urlAtom);
> >+                            targetArray.AppendElement(urlTarget);
> >+                        }
> 
> It looks like this can be done even when numDragItems > 1.
Ok, will check

> I don't think handling multiple files would be too difficult, but even if you
> prefer to only implement this for one file right now, make sure that
> SourceDataGet() is consistent with GetSourceList().
Ok, will have a look.

> In SourceDataGet():
> 
> >+                void    *tmpData = NULL;
> >+                nsresult rv;
> >+                nsCOMPtr<nsISupports> data;
> >+                rv = item->GetTransferData(kFilePromiseURLMime,
> >+                                           getter_AddRefs(data),
> >+                                           &tmpDataLen);
> >+                if (NS_SUCCEEDED(rv)) {
> >+                    foundFilePromiseFlavor = PR_TRUE;
> >+                    if (tmpData) {
> 
> tmpData is not used.
> 
> >+            if (foundFilePromiseFlavor == PR_FALSE) {
> >+                PR_LOG(sDragLm, PR_LOG_DEBUG, ("Transferable is missing kFilePromiseURLMime flavor\n"));
> >+                return;
> >+            }
> >+
> 
> I expect the transferable may often not have kFilePromiseURLMime even though
> someone asks for target type mimeFlavor == gTextUriListType.  (If kURLMime is
> a supported flavour, then gTextUriListType will be in the GtkTargetList.)  This
> function should continue to return gTextUriListType data even if there is no
> kFilePromiseURLMime flavour.
Ok, have to check.

> >+            rv = NS_NewFileURI(getter_AddRefs(uri), tmpDir);
> 
> The uri will need to contain the hostname.  Will NS_NewFileURI do this?
Good to know...

> http://newplanetsoftware.com/xdnd/dragging_files.html
> 
> Feel free to use glib functions (g_filename_to_uri() perhaps) when dealing
> with GTK as in this function (and others in this file), if that makes things
> easier.
> 
> >+                    // remove and re-add the text/uri-list flavor to discard any previous
> >+                    // data of that flavor
> >+                    item->RemoveDataFlavor(gTextUriListType);
> >+                    item->AddDataFlavor(gTextUriListType);
> >+
> >+                    // set the file:///tmp/dnd_file/<filename> URL for text/uri-list flavor
> >+                    item->SetTransferData(gTextUriListType, genericDataWrapper, ucs2Str.Length() * 2);
> 
> The side-effect of modifying the transferable here doesn't feel right to me.
> The transferable is an app-internal reference to the data, while the external
> uri list, generated here for the GtkSelectionData for other apps, is likely to
> be different.  (Internally the data can be referenced from the original URI,
> rather than from the file intended for external use.)
> 
> There is lot of code specific to kFilePromiseURLMime added to SourceDataGet()
> here.  This dominates the function (which is actually much more general) and
> makes it harder to follow the flow.  Most of the new code should be split out
> into one or more static functions or private methods.
Ok, then I'll try to separate it more and send an updated Patch