Attachment 34300 Details for Bug 24887 – Patch, fixing 2 more issues from Darin

[patch] Patch, fixing 2 more issues from Darin

imageFilename3.patch (text/plain), 16.36 KB, created by Jens Alfke on 2009-08-07 11:38:50 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Jens Alfke

Created: 2009-08-07 11:38:50 PDT

Size: 16.36 KB

patch

obsolete

>Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog	(revision 46894)
>+++ WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,27 @@
>+2009-08-07  Jens Alfke  <snej@chromium.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Bug 24887 - Wrong filename when dragging an image to the Desktop.
>+            https://bugs.webkit.org/show_bug.cgi?id=24887
>+        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.)
>+
>+        No tests; would be unreasonably difficult to add the plumbing to access the proposed
>+        filename from within DRT.
>+
>+        * html/HTMLImageElement.cpp:
>+        (WebCore::HTMLImageElement::suggestedFilename):
>+        * html/HTMLImageElement.h:
>+        * platform/chromium/ClipboardChromium.cpp:
>+        (WebCore::ClipboardChromium::writeImageToDataObject):
>+        (WebCore::ClipboardChromium::declareAndWriteDragImage):
>+        * platform/chromium/ClipboardChromium.h:
>+        * platform/chromium/ClipboardChromiumMac.cpp:
>+        (WebCore::isInvalidFileCharacter):
>+        (WebCore::ClipboardChromium::validateFileName):
>+
> 2009-08-05  Dimitri Glazkov  <dglazkov@chromium.org>
> 
>         Unreviewed, build fix.
>Index: WebCore/dom/Clipboard.cpp
>===================================================================
>--- WebCore/dom/Clipboard.cpp	(revision 46894)
>+++ WebCore/dom/Clipboard.cpp	(working copy)
>@@ -30,9 +30,15 @@
> #include "DOMImplementation.h"
> #include "Frame.h"
> #include "FrameLoader.h"
>+#include "HTMLImageElement.h"
>+#include "HTMLNames.h"
> #include "Image.h"
>+#include "RenderImage.h"
>+#include "StringBuilder.h"
> 
> namespace WebCore {
>+    
>+using namespace HTMLNames;
> 
> Clipboard::Clipboard(ClipboardAccessPolicy policy, bool isForDragging) 
>     : m_policy(policy) 
>@@ -139,5 +145,87 @@ void Clipboard::setEffectAllowed(const S
>     if (m_policy == ClipboardWritable)
>         m_effectAllowed = effect;
> }
>+    
>+static CachedImage* cachedImageOfNode(Node* node)
>+{
>+    ASSERT(node);
>+    RenderObject* renderer = node->renderer();
>+    if (!renderer || !renderer->isImage())
>+        return 0;
>+    
>+    RenderImage* image = toRenderImage(renderer);
>+    if (!image->cachedImage() || image->cachedImage()->errorOccurred())
>+        return 0;
>+    
>+    return image->cachedImage();
>+}
>+    
>+static void appendAttribute(StringBuilder& markup, const String& name, String value)
>+{
>+    value.replace("&", "&amp;");
>+    value.replace("\"", "&quot;");
>+    markup.append(" ");
>+    markup.append(name);
>+    markup.append("=\"");
>+    markup.append(value);
>+    markup.append("\"");
>+}
>+
>+static String imageToMarkup(Element* element, Frame* frame)
>+{
>+    String srcURL;
>+    const AtomicString& imageURL = element->getAttribute(srcAttr);
>+    if (!imageURL.isEmpty())
>+        srcURL = frame->document()->completeURL(deprecatedParseURL(imageURL));
>+    
>+    StringBuilder markup;
>+    markup.append("<img");
>+    appendAttribute(markup, srcAttr.localName(), srcURL);
>+
>+    // Copy over attributes.  If we are dragging an image, we expect things like
>+    // the id to be copied as well.
>+    NamedNodeMap* attrs = element->attributes();
>+    unsigned length = attrs->length();
>+    for (unsigned i = 0; i < length; ++i) {
>+        Attribute* attr = attrs->attributeItem(i);
>+        if (attr->name() != srcAttr)
>+            appendAttribute(markup, attr->localName(), attr->value());
>+    }
>+    
>+    markup.append(">");
>+    return markup.toString();
>+}
>+
>+void Clipboard::declareAndWriteDragImage(Element* element, const KURL& url, const String& title, Frame* frame)
>+{
>+    // Get the image data (return early if there is none).
>+    SharedBuffer* imageBuffer = 0;
>+    CachedImage* cachedImage = cachedImageOfNode(element);
>+    if (cachedImage && cachedImage->image() && cachedImage->isLoaded())
>+        imageBuffer = cachedImage->image()->data();
>+    if (!imageBuffer || !imageBuffer->size())
>+        return;
>+    
>+    // Get the filename to use.
>+    String fileName;
>+    if (element->hasTagName(imgTag))
>+        fileName = static_cast<HTMLImageElement*>(element)->suggestedFilename();
>+    
>+    // Create HTML <img> markup:
>+    String htmlMarkup = imageToMarkup(element, frame);
>+    
>+    // Now call the abstract fully-specified version of this method.
>+    declareAndWriteDragImage(url, title, imageBuffer, fileName, htmlMarkup);
>+}
>+    
>+void Clipboard::declareAndWriteDragImage(const KURL&, const String&,
>+                                         SharedBuffer*, const String&,
>+                                         const String&)
>+{
>+    // This method is abstract; a subclass must override it, or the public version that calls it.
>+    ASSERT_NOT_REACHED();
>+}
>+    
>+
> 
> } // namespace WebCore
>Index: WebCore/dom/Clipboard.h
>===================================================================
>--- WebCore/dom/Clipboard.h	(revision 46894)
>+++ WebCore/dom/Clipboard.h	(working copy)
>@@ -64,7 +64,7 @@ namespace WebCore {
>         virtual void setDragImageElement(Node*, const IntPoint&) = 0;
>         
>         virtual DragImageRef createDragImage(IntPoint& dragLocation) const = 0;
>-        virtual void declareAndWriteDragImage(Element*, const KURL&, const String& title, Frame*) = 0;
>+        virtual void declareAndWriteDragImage(Element*, const KURL&, const String& title, Frame*);
>         virtual void writeURL(const KURL&, const String&, Frame*) = 0;
>         virtual void writeRange(Range*, Frame*) = 0;
> 
>@@ -85,6 +85,10 @@ namespace WebCore {
>         ClipboardAccessPolicy policy() const { return m_policy; }
>         bool dragStarted() const { return m_dragStarted; }
>         
>+        virtual void declareAndWriteDragImage(const KURL& url, const String& title,
>+                                              SharedBuffer* imageBuffer, const String& fileName,
>+                                              const String& markup);
>+        
>     private:
>         ClipboardAccessPolicy m_policy;
>         String m_dropEffect;
>Index: WebCore/html/HTMLImageElement.cpp
>===================================================================
>--- WebCore/html/HTMLImageElement.cpp	(revision 46894)
>+++ WebCore/html/HTMLImageElement.cpp	(working copy)
>@@ -30,6 +30,7 @@
> #include "HTMLDocument.h"
> #include "HTMLFormElement.h"
> #include "HTMLNames.h"
>+#include "MIMETypeRegistry.h"
> #include "MappedAttribute.h"
> #include "RenderImage.h"
> #include "ScriptEventListener.h"
>@@ -441,5 +442,42 @@ void HTMLImageElement::addSubresourceAtt
>     addSubresourceURL(urls, src());
>     addSubresourceURL(urls, document()->completeURL(useMap()));
> }
>+    
>+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.
>+        fileName = src().lastPathComponent();
>+        // If there's none, we then try the alt tag if one exists.
>+        if (fileName.isEmpty())
>+            fileName = altText();
>+    }
>+    
>+    // Remove leading or trailing '.'s.
>+    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().mimeType());
>+        fileName += ".";
>+        fileName += extension;
>+    }
>+    return fileName;
>+}
> 
> }
>Index: WebCore/html/HTMLImageElement.h
>===================================================================
>--- WebCore/html/HTMLImageElement.h	(revision 46894)
>+++ WebCore/html/HTMLImageElement.h	(working copy)
>@@ -116,6 +116,8 @@ public:
>     bool haveFiredLoadEvent() const { return m_imageLoader.haveFiredLoadEvent(); }
> 
>     virtual void addSubresourceAttributeURLs(ListHashSet<KURL>&) const;
>+    
>+    String suggestedFilename() const;
> 
> private:
>     HTMLImageLoader m_imageLoader;
>Index: WebCore/platform/chromium/ClipboardChromium.cpp
>===================================================================
>--- WebCore/platform/chromium/ClipboardChromium.cpp	(revision 46894)
>+++ 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"
>@@ -216,99 +217,35 @@ DragImageRef ClipboardChromium::createDr
>     }
>     return result;
> }
>-
>-static String imageToMarkup(const String& url, Element* element)
>+    
>+static void splitFileName(const String& fileName, String& outBaseName, String& outExtension)
> {
>-    StringBuilder markup;
>-    markup.append("<img src=\"");
>-    markup.append(url);
>-    markup.append("\"");
>-    // Copy over attributes.  If we are dragging an image, we expect things like
>-    // the id to be copied as well.
>-    NamedNodeMap* attrs = element->attributes();
>-    unsigned length = attrs->length();
>-    for (unsigned i = 0; i < length; ++i) {
>-        Attribute* attr = attrs->attributeItem(i);
>-        if (attr->localName() == "src")
>-            continue;
>-        markup.append(" ");
>-        markup.append(attr->localName());
>-        markup.append("=\"");
>-        String escapedAttr = attr->value();
>-        escapedAttr.replace("\"", "&quot;");
>-        markup.append(escapedAttr);
>-        markup.append("\"");
>+    int extensionIndex = fileName.reverseFind('.');
>+    if (extensionIndex != -1) {
>+        outBaseName = fileName.left(extensionIndex);
>+        outExtension = fileName.substring(extensionIndex);
>+    } else {
>+        outBaseName = fileName;
>+        outExtension = "";
>     }
>-
>-    markup.append("/>");
>-    return markup.toString();
>-}
>-
>-static CachedImage* getCachedImage(Element* element)
>-{
>-    // Attempt to pull CachedImage from element
>-    ASSERT(element);
>-    RenderObject* renderer = element->renderer();
>-    if (!renderer || !renderer->isImage())
>-        return 0;
>-
>-    RenderImage* image = toRenderImage(renderer);
>-    if (image->cachedImage() && !image->cachedImage()->errorOccurred())
>-        return image->cachedImage();
>-
>-    return 0;
> }
> 
>-static void writeImageToDataObject(ChromiumDataObject* dataObject, Element* element,
>-                                   const KURL& url)
>-{
>-    // Shove image data into a DataObject for use as a file
>-    CachedImage* cachedImage = getCachedImage(element);
>-    if (!cachedImage || !cachedImage->image() || !cachedImage->isLoaded())
>-        return;
>-
>-    SharedBuffer* imageBuffer = cachedImage->image()->data();
>-    if (!imageBuffer || !imageBuffer->size())
>-        return;
>-
>-    dataObject->fileContent = imageBuffer;
>-
>-    // Determine the filename for the file contents of the image.  We try to
>-    // use the alt tag if one exists, otherwise we fall back on the suggested
>-    // filename in the http header, and finally we resort to using the filename
>-    // in the URL.
>-    String extension = MIMETypeRegistry::getPreferredExtensionForMIMEType(
>-        cachedImage->response().mimeType());
>-    dataObject->fileExtension = extension.isEmpty() ? "" : "." + extension;
>-    String title = element->getAttribute(altAttr);
>-    if (title.isEmpty())
>-        title = cachedImage->response().suggestedFilename();
>-
>-    title = ClipboardChromium::validateFileName(title, dataObject);
>-    dataObject->fileContentFilename = title + dataObject->fileExtension;
>-}
>-
>-void ClipboardChromium::declareAndWriteDragImage(Element* element, const KURL& url, const String& title, Frame* frame)
>+void ClipboardChromium::declareAndWriteDragImage(const KURL& url, const String& title,
>+                                                 SharedBuffer* imageBuffer, const String& fileName,
>+                                                 const String& htmlMarkup)
> {
>     if (!m_dataObject)
>         return;
> 
>     m_dataObject->url = url;
>     m_dataObject->urlTitle = title;
>-
>-    // Write the bytes in the image to the file format.
>-    writeImageToDataObject(m_dataObject.get(), element, url);
>-
>-    AtomicString imageURL = element->getAttribute(srcAttr);
>-    if (imageURL.isEmpty())
>-        return;
>-
>-    String fullURL = frame->document()->completeURL(deprecatedParseURL(imageURL));
>-    if (fullURL.isEmpty())
>-        return;
>-
>-    // Put img tag on the clipboard referencing the image
>-    m_dataObject->textHtml = imageToMarkup(fullURL, element);
>+    m_dataObject->fileContent = imageBuffer;
>+    m_dataObject->textHtml = htmlMarkup;
>+    
>+    String baseName;
>+    splitFileName(fileName, baseName,m_dataObject->fileExtension);
>+    baseName = ClipboardChromium::validateFileName(baseName,m_dataObject.get());
>+    m_dataObject->fileContentFilename = baseName + m_dataObject->fileExtension;
> }
> 
> void ClipboardChromium::writeURL(const KURL& url, const String& title, Frame*)
>Index: WebCore/platform/chromium/ClipboardChromium.h
>===================================================================
>--- WebCore/platform/chromium/ClipboardChromium.h	(revision 46894)
>+++ WebCore/platform/chromium/ClipboardChromium.h	(working copy)
>@@ -71,17 +71,20 @@ namespace WebCore {
>         }
> 
>         virtual DragImageRef createDragImage(IntPoint& dragLoc) const;
>-        virtual void declareAndWriteDragImage(Element*, const KURL&, const String& title, Frame*);
>         virtual void writeURL(const KURL&, const String&, Frame*);
>         virtual void writeRange(Range*, Frame*);
> 
>         virtual bool hasData();
>-
>+        
>     private:
>         ClipboardChromium(bool, PassRefPtr<ChromiumDataObject>, ClipboardAccessPolicy);
> 
>         void resetFromClipboard();
>         void setDragImage(CachedImage*, Node*, const IntPoint&);
>+        virtual void declareAndWriteDragImage(const KURL&, const String& title,
>+                                              SharedBuffer* imageBuffer, const String& fileName,
>+                                              const String& htmlMarkup);
>+
>         RefPtr<ChromiumDataObject> m_dataObject;
>     };
> 
>Index: WebCore/platform/chromium/ClipboardChromiumMac.cpp
>===================================================================
>--- WebCore/platform/chromium/ClipboardChromiumMac.cpp	(revision 46894)
>+++ WebCore/platform/chromium/ClipboardChromiumMac.cpp	(working copy)
>@@ -28,14 +28,46 @@
> #include "ClipboardChromium.h"
> 
> #include "ChromiumDataObject.h"
>-#include "NotImplemented.h"
> 
> namespace WebCore {
>+    
>+
>+// Filename length and character-set limits vary by filesystem, and there's no
>+// real way to tell what filesystem is in use. Since HFS+ is by far the most
>+// common, we'll abide by its limits, which are 255 Unicode characters (slightly
>+// simplified; really it uses Normalization Form D, but the differences aren't
>+// really worth dealing with here.)
>+static const int MaxHFSFilenameLength = 255;
>+
>+
>+static bool isInvalidFileCharacter(UChar c)
>+{
>+    // HFS+ basically allows anything but '/'. For sanity's sake we'll disallow
>+    // control characters.
>+    return c < ' ' || c == 0x7F || c == '/';
>+}
> 
> String ClipboardChromium::validateFileName(const String& title, ChromiumDataObject* dataObject)
> {
>-    notImplemented();
>-    return title;
>+    // Remove any invalid file system characters, especially "/".
>+    String result = title.removeCharacters(isInvalidFileCharacter);
>+    String extension = dataObject->fileExtension.removeCharacters(&isInvalidFileCharacter);
>+
>+    // Leading "."s would make the file invisible -- strip them.
>+    while (result.startsWith("."))
>+        result.remove(0,1);
>+
>+    // Remove a ridiculously-long extension.
>+    if (extension.length() >= MaxHFSFilenameLength)
>+        extension = "";
>+    
>+    // Truncate an overly-long filename.
>+    int overflow = result.length() + extension.length() - MaxHFSFilenameLength;
>+    if (overflow > 0) 
>+        result.remove(result.length() - overflow, overflow);
>+    
>+    dataObject->fileExtension = extension;
>+    return result;
> }
> 
> }  // namespace WebCore

Flags:

darin: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 24887: 29086 | 29113 | 29114 | 29165 | 29225 | 29234 | 29431 | 34076 | 34160 | 34163 | 34290 | 34300 | 34318 | 95223 | 95510 | 95526 | 95528