193053 - Merge Chimera drag and drop changes to the trunk

Status: VERIFIED FIXED

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

Description

[Simon Fraser [no longer active]]

I need to land drag and drop changes that happened on the chimera branch onto
the trunk.

Comment 1

[Simon Fraser [no longer active]]

Attachment: gfx/src/mac patch

This patch fixes a bug in nsImageMac::ConvertToPICT by always setting the
colors before the CopyBits (was bug 187290).

Comment 2

[Simon Fraser [no longer active]]

Attachment: widget changes

These changes add some new flavors to nsITransferable.idl, to allow us to keep
the URL, and its description, in separate flavors. This makes it much easier to
map tp the OS 'url ' and 'urld' flavors, and is cleaner.

The nsDragService.cpp changes add support for drags into the file system, by
implementing HFS promise dragging, and support for image data in drags, so that
you can drag images into PhotoShop. nsBaseDragService provides a couple of XP
utility routines that will be useful to fix this for other platforms.

nsClipboard.cpp was changed to enable putting image data on the clipboard.

Comment 3

[Simon Fraser [no longer active]]

Attachment: layout changes

In layout, nsCopySupport was changed to support copying image data to the
clipboard, which is hooked up in nsPresShell.cpp. Nothing will call this code
yet.
(If you apply the diff, the nsPresShell.cpp changes may not apply, since I had
other changes in that file which I edited out of the diff.)

Comment 4

[Simon Fraser [no longer active]]

Attachment: content changes

These are changes to nsContentAreaDragDrop.cpp, which do the following:
1. Put 'url' and 'url description' data into the transferable on drag, and
   look for 'url' data on drop.
2. Improve dragging of images, including the image URL
3. Deal with a single image element being selected, in which case we want to
   drag the image, rather than some serialized HTML.

Comment 5

[Simon Fraser [no longer active]]

In summary, these changes provide the following:
1. Ability to copy images to the clipboard (needs hooking up to a menu item)
2. Dragging of images to the desktop, to save the image file
3. Dragging of images into graphics apps.
4. Receiving of .webloc files from the Finder (they provide 'url' data)
5. Dragging links to the Dock (it wants 'url' data)

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 114268 [details] [diff] [review]
gfx/src/mac patch

I'm not really a fan of gratuitous whitespace and formatting changes, but since
the file seems to be pretty inconsistent anyway...

Comment 7

[(not reading, please use [e-mail] instead)]

> Ability to copy images to the clipboard (needs hooking up to a menu item)

very cool.  

simon tells me it "depends on stuff in gfx", and I think at least win32 should
be able to do it.  (not sure about linux or other ports)

note, in mozilla, right clicking on an image in browser (and mailnews) currently
gives you a "Copy" menuitem, but it's disabled.

sfraser's work sounds like the start of getting that enabled, but there might be
some mozilla/xpfe work we'd have to do to use it.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 114269 [details] [diff] [review]
widget changes

>--- mozilla/widget/src/mac/nsDragService.cpp	24 Oct 2002 13:39:26 -0000	1.66
>+++ mozilla/widget/src/mac/nsDragService.cpp	13 Feb 2003 00:37:39 -0000

>+  else if (strcmp(actualFlavor, kFilePromiseMime) == 0)
>+  {
>+     nsCOMPtr<nsISupports> imageURLPrimitive;
>+     PRUint32 dataSize = 0;
>+     rv = item->GetTransferData(kFilePromiseURLMime, getter_AddRefs(imageURLPrimitive), &dataSize);
>+     if (NS_FAILED(rv)) return cantGetFlavorErr;
>+     
>+     nsCOMPtr<nsISupportsString> doubleByteText = do_QueryInterface(imageURLPrimitive);
>+     if (!doubleByteText) return cantGetFlavorErr;
>+
>+     nsAutoString imageURLString;
>+     PRUnichar* imageURL = nsnull;
>+     doubleByteText->ToString(&imageURL);
>+     if (imageURL) {
>+       imageURLString.Assign(imageURL);
>+       nsMemory::Free(imageURL);
>+     }

You could use .Adopt() here to avoid copying the string.


>+    image->ConvertToPICT(&picture);
>+    if (!picture) return cantGetFlavorErr; 
>+
>+    PRInt32 pictSize = ::GetHandleSize((Handle)picture);
>+    char* pictData = nsnull;
>+    if (pictSize > 0)
>+      pictData = (char*)nsMemory::Alloc(pictSize);
>+    if (pictData) {
>+      ::BlockMoveData(*picture, pictData, pictSize);    // doesn't move memory
>+      *outData = (void*)pictData;
>+      *outDataSize = pictSize;
>+      retVal = noErr;
>         }

indenting looks a bit off here.


>@@ -1029,4 +1125,126 @@
>     ::InitCursor();
>  
>   return nsBaseDragService::SetDragAction(anAction);
> }
>+
>+#pragma mark -

Can we lose the #pragma mark's? We're not building with CodeWarrior anymore; PB
and gcc don't get anything from this.


>+OSErr
>+nsDragService::HandleHFSPromiseDrop(DragReference inDragRef, unsigned int inItemIndex, 
>+        FlavorType inFlavor, const nsAString& inSourceURL, void** outData, unsigned int* outDataSize)
>+{
>+  *outData = NULL;
>+  *outDataSize = 0;
>+  
>+  OSErr err;
>+  nsresult rv;
>+  
>+  nsCOMPtr<nsIURI> sourceURI;
>+  rv = NS_NewURI(getter_AddRefs(sourceURI), inSourceURL);
>+  if (NS_FAILED(rv)) return paramErr;

You could use NS_ENSURE_SUCCESS(rv, paramErr).	Up to you.


>Index: mozilla/widget/src/xpwidgets/nsBaseDragService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/widget/src/xpwidgets/nsBaseDragService.cpp,v
>retrieving revision 1.27
>diff -b -u -4 -r1.27 nsBaseDragService.cpp
>--- mozilla/widget/src/xpwidgets/nsBaseDragService.cpp	8 Jan 2003 23:02:43 -0000	1.27
>+++ mozilla/widget/src/xpwidgets/nsBaseDragService.cpp	13 Feb 2003 00:37:37 -0000
>@@ -52,8 +52,15 @@
> #include "nsIPresShell.h"
> #include "nsIDOMNode.h"
> #include "nsIPresContext.h"
> 
>+// file downloading stuff
>+#include "nsNetUtil.h"
>+#include "nsEscape.h"
>+#include "nsIURL.h"
>+#include "nsIFile.h"
>+#include "nsIWebBrowserPersist.h"
>+
> 

How is it that you didn't have to add webbrowserpersist to REQUIRES in
Makefile.in?  (I really don't like that dependency, either.)

>+
>+nsresult
>+nsBaseDragService::CreateFileInDirectory(nsIURI* inSourceURI, nsILocalFile* inParentDir, nsILocalFile** outFile)
>+{
>+  NS_ENSURE_ARG(inSourceURI);
>+  NS_ENSURE_ARG(inParentDir);
>+  NS_ENSURE_ARG_POINTER(outFile);
>+  
>+  *outFile = nsnull;
>+  
>+  nsresult rv;
>+  
>+  nsCOMPtr<nsIFile> clonedFile;
>+  rv = inParentDir->Clone(getter_AddRefs(clonedFile));
>+  if (NS_FAILED(rv)) return rv;
>+
>+  nsCOMPtr<nsILocalFile> destFile = do_QueryInterface(clonedFile);
>+  if (!destFile) return NS_ERROR_NO_INTERFACE;

I think NS_ERROR_NO_INTERFACE is normally reserved as a QueryInterface or
GetInterface return value.

>+
>+  *outFile = destFile.get();

.get() is not necessary here.

>+// SaveURIToFile
>+// used on platforms where it's possible to drag items (e.g. images)
>+// into the file system
>+nsresult
>+nsBaseDragService::SaveURIToFile(nsIURI* inURI, nsILocalFile* inFile)
>+{
>+  nsresult rv;
>+  // 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;
>+

If you change to #including nsCWebBrowserPersist.idl, you get a nice macro for
that contract id.  But like I said, is there a way we can avoid using this
interface from here?

Comment 9

[Simon Fraser [no longer active]]

> How is it that you didn't have to add webbrowserpersist to REQUIRES in
> Makefile.in?  (I really don't like that dependency, either.)

Oops, missed that. Still hacking in CFM  :)

I'd love to avoid that dependency too; it's not pretty. Maybe the drag service
code could fire off a command (adding dependencies on
embedding/components/commandhandler), or call via an API on the document?

Comment 10

[Simon Fraser [no longer active]]

Attachment: xpfe/components patch to hook up context menu item for Copy Image

This patch enables a Copy Image item in the context menu, which replaces the
disabled Copy item when an image is clicked on.

Note that we should add code somewhere to disable the cmd_copyImageContents
commnad for those platforms that are too lame to support it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114272 [details] [diff] [review]
layout changes

> +    static nsresult GetContents(const nsAString& aMimeType, PRUint32 aFlags, nsISelection *aSel, nsIDocument *aDoc, nsAString& outdata);

Why make the MIME type nsAString instead of nsACString?  MIME types are
guaranteed to be ASCII.... (and you assume they are anyway in the
implementation).

>+    static nsresult ImageCopy(nsIDOMHTMLImageElement* imageElement, PRInt16 aClipboardID);

What about copying <input type="image"> or objects that load images?

>+    // these are ripped from nsContentAreaDragDrop. This so needs factoring.    
>+    static nsresult GetImageFromDOMNode(nsIDOMNode* inNode, nsIImage**outImage);
>+
>+    static nsresult GetImageFrame(nsIContent* aContent, nsIDocument *aDocument,
>+                          nsIPresContext *aPresContext, nsIPresShell *aPresShell,
>+                          nsIImageFrame** aImageFrame);

Note that the imageframe stuff is being removed from nsContentAreaDragDrop. 
See bug 83774 -- once that lands you will be able to get the image data
directly from the DOM node (and ImageCopy could take an nsIImageLoadingContent*
instead of nsIDOMHTMLImageElement*).  What's the timeframe you have for this
patch?	I don't mind merging after you check this in if you're trying to get it
in soon... If you're aiming for 1.4a, we should coordinate a landing order for
the two patches.

>+  nsCOMPtr<nsIDOMNode> imageNode = do_QueryInterface(imageElement, &rv);
>+  if (NS_FAILED(rv)) return rv;
>+  if (!imageNode) return NS_ERROR_FAILURE;

I'd shorten that to 

if (!imageNode) return rv;

since do_QueryInterface is guaranteed to only return non-null on success and
null on failure.

Similarly for do_GetService, and do_CreateInstance.

>+  nsCOMPtr<nsIDOMHTMLImageElement> img(do_QueryInterface(aNode, &rv));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (img) {

Same here.

Looks pretty good otherwise (I didn't read the imageframe code too carefully
since I assume that you copied it correctly and all and since it will hopefully
not exist in the tree for long, if at all).

Comment 12

[Mike Pinkerton (not reading bugmail)]

fwiw, #pragma mark is still very useful in projectbuilder.

Comment 13

[Kathleen :Brade]

Comment on attachment 114272 [details] [diff] [review]
layout changes

r=brade

extra space on this line (nsCopySupport::ImageCopy):
+  if (!ptrPrimitive)  return NS_ERROR_FAILURE;

Looks like nsCopySupport::GetImageFrame and nsCopySupport::GetImageFromDOMNode
use tabs unlike the rest of the file.

Is there some reason you are nesting the coe in
nsCopySupport::GetImageFromDOMNode unlike the checking in
nsCopySupport::ImageCopy?

Somme of the lines in GetImageFromDOMNode have different whitespace around
parens:
  if ( imgFrame )
vs 
  if (presContext)

Comment 14

[Michael Lefevre]

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

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 114268 [details] [diff] [review]
gfx/src/mac patch

r=pink

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 114273 [details] [diff] [review]
content changes

r=pink

fix the 128+256 to use the appropriate constants and you're good to go.

Comment 17

[Simon Fraser [no longer active]]

Attachment: content/base/src patch fixes bad merge, and cleaned

This patch fixes a bad merge, and tries to clarify the code that decides what
to put in the transferable. Images with links should be handled correctly. I
cleaned up some string stuff too.

Comment 18

[Simon Fraser [no longer active]]

Attachment: Cleaner version of the nsContentAreaDragDrop patch

I rewrote nsContentAreaDragDrop::BuildDragData so it's more easily understood,
factoring out some code into a new method. For ease of reading, here's the
rewritten method in its entirety:

PRBool
nsContentAreaDragDrop::BuildDragData(nsIDOMEvent* inMouseEvent, nsAString &
outURLString, nsAString & outTitleString,
					nsAString & outHTMLString, nsAString &
outImageSourceString, nsIImage** outImage, PRBool* outIsAnchor)
{
  if ( !outIsAnchor || !outImage )
    return PR_FALSE;
 
  outURLString.Truncate();
  outTitleString.Truncate();
  outHTMLString.Truncate();
  outImageSourceString.Truncate();

  *outImage = nsnull; 
  *outIsAnchor = PR_FALSE;
  
  nsCOMPtr<nsIDOMUIEvent> uiEvent(do_QueryInterface(inMouseEvent));
  if ( !uiEvent )
    return PR_FALSE;

  nsCOMPtr<nsIDOMEventTarget> target;
  inMouseEvent->GetTarget(getter_AddRefs(target));

  PRBool isAltKeyDown = PR_FALSE;
  nsCOMPtr<nsIDOMMouseEvent> mouseEvent(do_QueryInterface(inMouseEvent));
  if ( mouseEvent )
    mouseEvent->GetAltKey(&isAltKeyDown);
  
  // only drag form elements by using the alt key,
  // otherwise buttons and select widgets are hard to use
  nsCOMPtr<nsIFormControl> form(do_QueryInterface(target));
  if (form && !isAltKeyDown)
    return PR_FALSE;
  
  // the resulting strings from the beginning of the drag
  nsAutoString urlString;

  nsXPIDLString titleString;
  nsXPIDLString htmlString;	   // will be filled automatically if you fill
urlstring
  PRBool startDrag = PR_TRUE;
  nsCOMPtr<nsIDOMNode> draggedNode(do_QueryInterface(target));
  
  // find the selection to see what we could be dragging and if
  // what we're dragging is in what is selected.
  nsCOMPtr<nsIDOMAbstractView> view;
  uiEvent->GetView(getter_AddRefs(view));
  nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(view));
  if (!window)
    return PR_FALSE;

  // Get the real target and see if it is in the selection
  nsCOMPtr<nsIDOMNode> realTargetNode;
  nsCOMPtr<nsIDOMNSEvent> internalEvent = do_QueryInterface(inMouseEvent);
  if (internalEvent)
  {
    nsCOMPtr<nsIDOMEventTarget> realTarget;
    internalEvent->GetExplicitOriginalTarget(getter_AddRefs(realTarget));
    realTargetNode = do_QueryInterface(realTarget);
  }
  
  nsCOMPtr<nsISelection> selection;
  window->GetSelection(getter_AddRefs(selection));
  
  PRBool getFormattedStrings = PR_FALSE;
  PRBool haveSelectedContent = PR_FALSE;
  
  nsCOMPtr<nsIDOMNode> selectedImageOrLinkNode;
  GetDraggableSelectionData(selection, realTargetNode,
getter_AddRefs(selectedImageOrLinkNode), &haveSelectedContent);

  nsCOMPtr<nsIDOMNode> linkNode;		// set for linked images, and
links
  nsCOMPtr<nsIDOMNode> parentLink;		// possible parent link node
  nsCOMPtr<nsIDOMNode> selectionNormalizeNode;	// if set, normalize the
selection around this node

  nsCOMPtr<nsIDOMHTMLAreaElement>   area;   // client-side image map
  nsCOMPtr<nsIDOMHTMLImageElement>  image;
  nsCOMPtr<nsIDOMHTMLLinkElement>   link;

  if (selectedImageOrLinkNode)
  {
    image = do_QueryInterface(selectedImageOrLinkNode);
    link  = do_QueryInterface(selectedImageOrLinkNode);
    getFormattedStrings = !image && link;
  }
  else
  {
    // we're not using a selected element. Look for draggable elements
    // under the mouse
    
    // if the alt key is down, don't start a drag if we're in an anchor because

    // we want to do selection.
    FindParentLinkNode(draggedNode, getter_AddRefs(parentLink));
    if (parentLink && isAltKeyDown)
      return NS_OK;
    
    area  = do_QueryInterface(draggedNode);
    image = do_QueryInterface(draggedNode);
    link  = do_QueryInterface(draggedNode);
  }
  
  if (area)
  {
    *outIsAnchor = PR_TRUE;
    // grab the href as the url, use alt text as the title of the area if it's
there.
    // the drag data is the image tag and src attribute.
    area->GetAttribute(NS_LITERAL_STRING("href"), urlString);
    area->GetAttribute(NS_LITERAL_STRING("alt"), titleString);
    if (titleString.IsEmpty())
      titleString = urlString;
    htmlString = NS_LITERAL_STRING("<img src=\"") +
      urlString +
      NS_LITERAL_STRING("\">");
  }
  else if (image)
  {
    *outIsAnchor = PR_TRUE;
    // grab the href as the url, use alt text as the title of the area if it's
there.
    // the drag data is the image tag and src attribute.
    image->GetSrc(urlString);
    image->GetAttribute(NS_LITERAL_STRING("alt"), titleString);
    if (titleString.IsEmpty())
      titleString = urlString;
    htmlString = NS_LITERAL_STRING("<img src=\"") +
      urlString +
      NS_LITERAL_STRING("\">");

    // pass out the image source string
    outImageSourceString = urlString;

    // also grab the image data
    GetImageFromDOMNode(draggedNode, outImage);

    // If we are dragging around an image in an anchor, then we
    // are dragging the entire anchor (but also return the image)
   if (parentLink)
   {
      linkNode = parentLink;
      selectionNormalizeNode = linkNode;
    }
    else
    {
      // select siblings up to and including the selected link.
      selectionNormalizeNode = draggedNode;
    }
  }
  else if (link)
  {
    // set linkNode. The code below will handle this
    linkNode = draggedNode;
    GetNodeString(draggedNode, titleString);
  }
  else if (parentLink)
  {
    linkNode = parentLink;
    selectionNormalizeNode = linkNode;
    getFormattedStrings = PR_TRUE;
  }
  else if (!haveSelectedContent)
  {
    // nothing draggable
    startDrag = PR_FALSE;
  }

  if (linkNode)
  {
    *outIsAnchor = PR_TRUE;
    GetAnchorURL(linkNode, urlString);
  }
  
#ifdef CHANGE_SELECTION_ON_DRAG
  if (selectionNormalizeNode)
    NormalizeSelection(selectionNormalizeNode, selection);
#endif

  if (getFormattedStrings && selection)
  {
    // find the title for the drag and any associated html
    nsCOMPtr<nsISelectionPrivate> privSelection(do_QueryInterface(selection));
    if (privSelection)
    {
      // the window has a selection so we should grab that rather
      // than looking for specific elements
      privSelection->ToStringWithFormat("text/html",
					nsIDocumentEncoder::OutputAbsoluteLinks
|
				       
nsIDocumentEncoder::OutputEncodeW3CEntities,
					0, getter_Copies(htmlString));
      privSelection->ToStringWithFormat("text/plain", 0, 0,
getter_Copies(titleString));
    }
    else 
      selection->ToString(getter_Copies(titleString));
  }

  if (startDrag)
  {
    // default text value is the URL
    if (titleString.IsEmpty())
      titleString = urlString;
    
    // if we haven't constructed a html version, make one now
    if (htmlString.IsEmpty() && !urlString.IsEmpty())
      CreateLinkText(urlString, titleString, htmlString);
  }
  
  outURLString = urlString;
  outTitleString = titleString;
  outHTMLString = htmlString;
  
  return startDrag;
}

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 119752 [details] [diff] [review]
Cleaner version of the nsContentAreaDragDrop patch

r=pink

Comment 20

[Simon Fraser [no longer active]]

Attachment: More nsContentAreaDragDrop cleaning

This patch fixes the dragging of selected text, does the right thing when you
drag selected images and text (if you click on the image or the text to drag),
fixes dragging of <area> nodes (now uses absolute urls), and no longer messes
with the selection in the document at all.

Comment 21

[Simon Fraser [no longer active]]

Attachment: Final answer content patch. Fixes string usage, and dragging <area> elements

Comment 22

[kinmoz]

Comment on attachment 119997 [details] [diff] [review]
Final answer content patch. Fixes string usage, and dragging <area> elements

sr=[e-mail]

Looks good to me, just change |draggedNode| to |link|, like we discussed
earlier, for this tidbit:

+  else if (link)
+  {
+    // set linkNode. The code below will handle this
+    linkNode = draggedNode;
+    GetNodeString(draggedNode, titleString);
+  }

Comment 23

[Simon Fraser [no longer active]]

Comment on attachment 114268 [details] [diff] [review]
gfx/src/mac patch

This patch has been checked in.

Comment 24

[Simon Fraser [no longer active]]

I checked in the parts of the content/base/src patch that are not dependent on
the other changes.

Comment 25

[Simon Fraser [no longer active]]

Attachment: content/widget changes for file drag and drop, using nsIFormatConverter

Patch is WIP, don't review yet.

Comment 26

[Simon Fraser [no longer active]]

Comment on attachment 119997 [details] [diff] [review]
Final answer content patch. Fixes string usage, and dragging <area> elements

This was checked in

Comment 27

[Simon Fraser [no longer active]]

Attachment: Final widget patch

This patch uses a new lazy data provider API on nsITransferable to do the file
saving. I think it's a bit neater than the last method. This patch is ready for
review.

Comment 28

[Simon Fraser [no longer active]]

Attachment: Final content patch

Comment 29

[Simon Fraser [no longer active]]

Attachment: Final layout patch

Comment 30

[Simon Fraser [no longer active]]

Here's how dragging files to the Finder works with these changes.

Drag code in nsContentAreaDragDrop.cpp adds a kFilePromiseMime flavor to the
transferable, with an nsIFlavorDataProvider. It also adds a kFilePromiseURLMime
flavor, with a string containing the source url.

We map that to the OS drag flavor kDragFlavorTypePromiseHFS, which promises the
flavor kDragPromisedFlavor. The OS asks for kDragPromisedFlavor, and we map that
back to kFilePromiseMime.

When asked for kFilePromiseMime data (here), we figure out the drop location
from the OS, and set that as an nsILocalFile on the kFilePromiseDirectoryMime
flavor. We then call GetTransferData() for the kFilePromiseMime flavor, which
triggers the nsIFlavorDataProvider to do the save.

Comment 31

[Simon Fraser [no longer active]]

Comment on attachment 120914 [details] [diff] [review]
Final widget patch

Diffs are -w, so be prepared for some whitespace misalignment

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 120917 [details] [diff] [review]
Final layout patch

Basically, all the comments from comment 11 still apply.  To clarify some of
them:

>Index: base/src/nsCopySupport.cpp

Check whether the caller actually has to pass in an nsAString MIME type to
GetContents, please.  If not, pass an nsACString and convert when calling
Init() on the encoder...

>+nsCopySupport::ImageCopy(nsIDOMHTMLImageElement* imageElement, PRInt16 aClipboardID)

Make this take nsIImageLoadingContent as the first arg.  That eliminates the
need for that first QI if you make GetImageFromDOMNode also take
nsIImageLoadingContent (and rename it if desired; maybe
GetImageFromImageContent?).

>+  nsCOMPtr<nsIDOMNode> imageNode = do_QueryInterface(imageElement, &rv);
>+  if (NS_FAILED(rv)) return rv;
>+  if (!imageNode) return NS_ERROR_FAILURE;

See comment 11.

>+// GetImageFrame
>+//
>+// Finds the image from from a content node
>+//
>+nsresult 
>+nsCopySupport::GetImageFrame(nsIContent* aContent, nsIDocument 

This is no longer needed and can be removed.  Includes can be reduced
appropriately.

>+nsCopySupport::GetImageFromDOMNode(nsIDOMNode* inNode, nsIImage**outImage)

Space before "outImage"?


>Index: html/base/src/nsPresShell.cpp
>+  // are we an image?
>+  nsCOMPtr<nsIDOMHTMLImageElement> img(do_QueryInterface(aNode, &rv));

QI to nsIImageLoadingContent instead; you want to change ImageCopy to take that
anyway.  ;)

sr=me with these changes.

Comment 33

[Brian Ryner (not reading)]

Comment on attachment 120914 [details] [diff] [review]
Final widget patch

Looks good, sr=me.

Comment 34

[Brian Ryner (not reading)]

Comment on attachment 120916 [details] [diff] [review]
Final content patch

sr=me.

Comment 35

[Kathleen :Brade]

Comment on attachment 120917 [details] [diff] [review]
Final layout patch

spacing in GetImageFrame looks wacky (tabs?); please fix.

remove this line or make it have the method name:
+// GetImage

looks great!
r=brade

Comment 36

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 120914 [details] [diff] [review]
Final widget patch

+    * @param  aDataLen the length of the data, or 0 if passing a
nsIFlavorDataProvider

can you make a constant for zero? just so in the places where it's intended to
be a flavor provider callers can be explicit about their intentions.

looks good other than that. you should probably also close out 39748 as a dupe
of this, or at least mark a dependency.

r=pink

Comment 37

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 120916 [details] [diff] [review]
Final content patch

+    nsCOMPtr<nsIFlavorDataProvider> thisAsProvider =
NS_STATIC_CAST(nsIFlavorDataProvider*, this);
+    trans->SetTransferData(kFilePromiseMime, thisAsProvider, 0);

what's the difference between doing it this way and just passing the
static_cast'ed |this| directly to SetTransferData()? If it's an object slicing
issue, won't you still get bit on the first line?

+#if 0
+#pragma mark -
+#endif

should do something useful with this, one way or the other.

+// this is our nsIFlavorDataProvider callback

mention in the comments that we assume that the dest dir has been placed as a
nsILocalFile in the transferable.

+    if (!destDirectory) return  NS_ERROR_FAILURE;

you never init *aData or *aDataLen, nor do any callers of this check the return
value. it just so happens that we get here because *aDataLen is 0 so it'll
work, but that seems more lucky than correct.

Comment 38

[Simon Fraser [no longer active]]

Attachment: revised content/base/src patch addressing comments

Comment 39

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 120989 [details] [diff] [review]
revised content/base/src patch addressing comments

r=pink

Comment 40

[Simon Fraser [no longer active]]

Attachment: revised widget patch, addressing pink's comments. diff is -w

Comment 41

[Simon Fraser [no longer active]]

Fixing widget/src/cocoa code is covered by bug 202586, which simply makes cocoa
share the files from widget/src/mac. So fixed!

Comment 42

[Chris Petersen]

Verified..

Comment 43

[Wayne Woods]

For reference, the kURLDataMime definition that was added here is involved in some Terminal paste weirdness in bug 336012. Not saying that this definition is wrong, but it may have uncovered an underlying problem elsewhere. Just wanted to add a link to it.