[webkit-reviews] review denied: [Bug 24887] Wrong filename when dragging an image to the Desktop. : [Attachment 34076] New patch (by Jens)

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Jens Alfke <snej at chromium.org>'s
request for review:
Bug 24887: Wrong filename when dragging an image to the Desktop.
https://bugs.webkit.org/show_bug.cgi?id=24887

Attachment 34076: New patch (by Jens)
https://bugs.webkit.org/attachment.cgi?id=34076&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 46768)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,26 @@
> +2009-08-04  Jens Alfke  <snej at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 24887 - Wrong filename when dragging an image to the Desktop.
> +	   Adds functionality to HTMLImageElement to determine the filename to
use.
> +	   Modifies ClipboardChromium to use it when setting up the drag.
> +	   Implements validateFileName for Mac Chromium (limits name to 255
UTF-8 bytes.)
> +	   https://bugs.webkit.org/show_bug.cgi?id=24887
> +	   
> +	   No tests; I don't know if this behavior can be simulated for a test.

You can make DRT dump the content of a pasteboard -- it should not be hard to
add support if DRT does not already support this.

> +    
> +String HTMLImageElement::suggestedFilename() const
> +{
> +    String fileName;
> +    
> +    // We try to use the suggested filename in the http header first.
> +    CachedImage* cachedImage = this->cachedImage();
> +    if (cachedImage && cachedImage->image() && cachedImage->isLoaded())
> +	   fileName = cachedImage->response().suggestedFilename();
> +    
> +    if (fileName.isEmpty()) {
> +	   // Then, if there's none, we try to extract the filename from
> +	   // the image URL stored in the src attribute.
> +	   KURL src = this->src();
> +	   if (src.isValid())
> +	       fileName = src.lastPathComponent();
> +	   // If there's none, we then try the alt tag if one exists.
> +	   if (fileName.isEmpty())
> +	       fileName = this->altText();
> +    }
> +    
> +    // Leading or trailing '.'s are naughty.
In appropriate comment.  Also this code looks really similar to the code in
other platforms.

> +    while (fileName.startsWith("."))
> +	   fileName.remove(0,1);
> +    while (fileName.endsWith("."))
> +	   fileName.remove(fileName.length()-1, 1);
> +    
> +    // We can't rely on the filename extension, so we must remove it
> +    // and use the one registered with the MIME type.
> +    int extension_index = fileName.reverseFind('.');
> +    if (extension_index != -1)
> +	   fileName.truncate(extension_index);
> +    if (!fileName.isEmpty()) {
> +	   String extension =
MIMETypeRegistry::getPreferredExtensionForMIMEType(cachedImage->response().mime
Type());
> +	   fileName += ".";
> +	   fileName += extension;
> +    }
> +    return fileName;
> +}

This code should not be on HTMLImageElement, it should probably be shared code
on the root Clipboard class so that it's available to all ports in a simple
way. 

> Index: WebCore/platform/chromium/ClipboardChromium.cpp
> ===================================================================
> --- WebCore/platform/chromium/ClipboardChromium.cpp	(revision 46767)
> +++ WebCore/platform/chromium/ClipboardChromium.cpp	(working copy)
> @@ -36,6 +36,7 @@
>  #include "FileList.h"
>  #include "Frame.h"
>  #include "HTMLNames.h"
> +#include "HTMLImageElement.h"
>  #include "NamedAttrMap.h"
>  #include "MIMETypeRegistry.h"
>  #include "markup.h"
> @@ -259,33 +260,33 @@ static CachedImage* getCachedImage(Eleme
>      return 0;
>  }
>  
> -static void writeImageToDataObject(ChromiumDataObject* dataObject, Element*
element,
> -				      const KURL& url)
> +void ClipboardChromium::writeImageToDataObject(Element* element)
>  {
> -    // Shove image data into a DataObject for use as a file
> +    if (element->tagName() != "IMG")
> +	   return;
This is wrong -- you should always use the tag constants, this requires
encoding and a string compare rather than a simple pointer check and matches no
other code anyway in webcore.  It is also unsafe due to

> +    // Pick a filename and split out the extension.
> +    String fileName = ((HTMLImageElement*)element)->suggestedFilename();
This.  Your code will attempt to treat anything with the tag "IMG" to
HTMLImageElement which is unsafe -- foo:img is not the same as html:img.

Also WebKit coding standards prohibit C-style casts.
>  
Given all this code was fairly directly copied from ClipboardWin i would expect
it to be fairly obvious how to refactor this to share code.

Realistically Clipboard should really be devirtualised and have more code
shared between the ports, that way each port doesn't have to fixed these bugs
separately.  Such a refactoring would be a Good Thing, and i think should be
done as cleanup prior to fixing this bug.