Closed Bug 193053 Opened 22 years ago Closed 22 years ago

Merge Chimera drag and drop changes to the trunk

Categories

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

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

References

Details

(Keywords: topembed, Whiteboard: edt_x3)

Attachments

(6 files, 9 obsolete files)

3.71 KB, patch
Details | Diff | Splinter Review
46.42 KB, patch
mikepinkerton
: review+
bryner
: superreview+
Details | Diff | Splinter Review
12.68 KB, patch
mikepinkerton
: review-
bryner
: superreview+
Details | Diff | Splinter Review
8.59 KB, patch
Brade
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
12.81 KB, patch
mikepinkerton
: review+
Details | Diff | Splinter Review
38.87 KB, patch
Details | Diff | Splinter Review
I need to land drag and drop changes that happened on the chimera branch onto
the trunk.
Status: NEW → ASSIGNED
OS: MacOS X → All
Hardware: Macintosh → All
Attached patch gfx/src/mac patch (obsolete) — Splinter Review
This patch fixes a bug in nsImageMac::ConvertToPICT by always setting the
colors before the CopyBits (was bug 187290).
Attached patch widget changes (obsolete) — Splinter Review
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.
Attached patch layout changes (obsolete) — Splinter Review
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.)
Attached patch content changes (obsolete) — Splinter Review
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.
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)
Attachment #114268 - Flags: superreview?(bryner)
Attachment #114268 - Flags: review?(pinkerton)
Attachment #114269 - Flags: superreview?(bryner)
Attachment #114269 - Flags: review?(pinkerton)
Attachment #114272 - Flags: superreview?(bzbarsky)
Attachment #114272 - Flags: review?(brade)
Attachment #114273 - Flags: superreview?(kin)
Attachment #114273 - Flags: review?(pinkerton)
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...
Attachment #114268 - Flags: superreview?(bryner) → superreview+
> 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 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?
Attachment #114269 - Flags: superreview?(bryner) → superreview-
> 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?
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 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).
fwiw, #pragma mark is still very useful in projectbuilder.
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)
Attachment #114272 - Flags: review?(brade) → review+
*** Bug 193392 has been marked as a duplicate of this bug. ***
Whiteboard: edt_x3
Blocks: PhtN5
Blocks: 196698
Blocks: 196700
Comment on attachment 114268 [details] [diff] [review]
gfx/src/mac patch

r=pink
Attachment #114268 - Flags: review?(pinkerton) → review+
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.
Attachment #114273 - Flags: review?(pinkerton) → review+
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.
Attachment #114273 - Attachment is obsolete: true
Attachment #119525 - Flags: superreview?(kin)
Attachment #119525 - Flags: review?(pinkerton)
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;
}
Attachment #119525 - Attachment is obsolete: true
Attachment #119752 - Flags: superreview?(kin)
Attachment #119752 - Flags: review?(pinkerton)
Attachment #119525 - Flags: superreview?(kin)
Attachment #119525 - Flags: review?(pinkerton)
Attachment #114273 - Flags: superreview?(kin)
Comment on attachment 119752 [details] [diff] [review]
Cleaner version of the nsContentAreaDragDrop patch

r=pink
Attachment #119752 - Flags: review?(pinkerton) → review+
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.
Attachment #119752 - Attachment is obsolete: true
Attachment #119909 - Flags: superreview?(kin)
Attachment #119997 - Flags: superreview?(kin)
Attachment #119752 - Flags: superreview?(kin)
Attachment #119909 - Flags: superreview?(kin)
Comment on attachment 119997 [details] [diff] [review]
Final answer content patch. Fixes string usage, and dragging <area> elements

sr=kin@netscape.com

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);
+  }
Attachment #119997 - Flags: superreview?(kin) → superreview+
Comment on attachment 114268 [details] [diff] [review]
gfx/src/mac patch

This patch has been checked in.
Attachment #114268 - Attachment is obsolete: true
QA Contact: pmac → petersen
I checked in the parts of the content/base/src patch that are not dependent on
the other changes.
Keywords: topembed
Patch is WIP, don't review yet.
Comment on attachment 119997 [details] [diff] [review]
Final answer content patch. Fixes string usage, and dragging <area> elements

This was checked in
Attachment #119997 - Attachment is obsolete: true
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.
Attachment #114269 - Attachment is obsolete: true
Attachment #120888 - Attachment is obsolete: true
Attachment #114272 - Attachment is obsolete: true
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 on attachment 120914 [details] [diff] [review]
Final widget patch

Diffs are -w, so be prepared for some whitespace misalignment
Attachment #120914 - Flags: superreview?(bryner)
Attachment #120914 - Flags: review?(pinkerton)
Attachment #120917 - Flags: superreview?(bzbarsky)
Attachment #120917 - Flags: review?(brade)
Attachment #120916 - Flags: superreview?(bryner)
Attachment #120916 - Flags: review?(pinkerton)
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.
Attachment #120917 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 120914 [details] [diff] [review]
Final widget patch

Looks good, sr=me.
Attachment #120914 - Flags: superreview?(bryner) → superreview+
Comment on attachment 120916 [details] [diff] [review]
Final content patch

sr=me.
Attachment #120916 - Flags: superreview?(bryner) → superreview+
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
Attachment #120917 - Flags: review?(brade) → review+
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
Attachment #120914 - Flags: review?(pinkerton) → review+
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.
Attachment #120916 - Flags: review?(pinkerton) → review-
Comment on attachment 120989 [details] [diff] [review]
revised content/base/src patch addressing comments

r=pink
Attachment #120989 - Flags: review+
Blocks: 185088
Fixing widget/src/cocoa code is covered by bug 202586, which simply makes cocoa
share the files from widget/src/mac. So fixed!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Attachment #114272 - Flags: superreview?(bzbarsky)
Verified..
Status: RESOLVED → VERIFIED
Attachment #114269 - Flags: review?(pinkerton)
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: