Bug 24887

Although the behavior is the same for Chromium and Safari 4 on windows (though not on Safari 4 on the Mac, which does the right thing), there is actually two different pieces of code having the same wrong behavior, so these are separate issues. The problem on the chromium side comes from a file under WebCore\platform\chromium\ClipboardChromium.cpp, and the same problem is also in the WebCore\platform\win\ClipboardWin.cpp file. Here's a snippet of that code: The ClipboardWin::declareAndWriteDragImage() method calls the static writeImageToDataObject which has the following code: HGLOBAL imageFileDescriptor = createGlobalImageFileDescriptor(url.string(), element->getAttribute(altAttr), cachedImage); which wrongly uses the altAttr for the base file name, when it should use the file name from the url. I think...

Created attachment 29086 [details] Change the way we choose the filename when drang and dropping an image to the filesystem on Windows

You shouldn't need Frame* in an argument, you can get it from the element: element->document()->frame();

Created attachment 29113 [details] Image drag and drop file name on Windows New version now uses the elements->document() as opposed to passing the frame.

Created attachment 29114 [details] Image drag and drop file name on Windows Oops... Previous upload was missing a changed file... sorry...

Comment on attachment 29114 [details] Image drag and drop file name on Windows Per ap comments on #webkit, suggestedFilename() should take precedence over actual filename, because it uses the filename value from Content-Disposition header. Also spacing nits. > + if (imageURL.isEmpty() == false) { > + fileName = KURL(element->document()->completeURL(parseURL(imageURL))).lastPathComponent(); 4 spaces > + } else { > + fileName = element->getAttribute(altAttr); > + if (fileName.isEmpty()) 4 spaces. > + if (extension_index != -1) { > + extension = fileName.substring(extension_index + 1); > + fileName.truncate(extension_index); 4 spaces. > + } else { > + extension = MIMETypeRegistry::getPreferredExtensionForMIMEType( > cachedImage->response().mimeType()); 4 spaces. > + if (fileName.isEmpty()) > + return 0; 4 spaces. > + String extension; > + if (extension_index != -1) { > + extension = fileName.substring(extension_index); > + fileName.truncate(extension_index); 4 spaces. > + } else { > + extension = image->image()->filenameExtension(); > + if (extension.isEmpty()) { 4 spaces. > + return 0; > + } > + extension.insert(".", 0); 4 spaces. > + if (imageURL.isEmpty() == false) { > + fileName = KURL(element->document()->completeURL(parseURL(imageURL))).lastPathComponent(); Ditto. > + } else { > + fileName = element->getAttribute(altAttr); > + if (fileName.isEmpty()) Ditto. > + HGLOBAL imageFileDescriptor = createGlobalImageFileDescriptor(url.string(), fileName, cachedImage); > if (!imageFileDescriptor) > return; Ditto.

OK, here is a summary of recent discussions (and a few open questions) about this: Suggestion: image->response().suggestedFilename() (which comes from the Content-Disposition) should have precedence of the file name extracted from the imageURL. Question 1: Should we also change the code that react to the contextual menu "Save image as..." to do the same? Other question: There are three ways of getting the file extension. 1) From the fileName itself (when it has one) 2) From MIMETypeRegistry::getPreferredExtensionForMIMEType(cachedImage->response().mimeType()) 3) From the image->image()->filenameExtension() What should be the precedence order?

I don't have an answer to Chromium-specific design choices, but I have some more coding style nitpicks: + if (imageURL.isEmpty() == false) { + fileName = KURL(element->document()->completeURL(parseURL(imageURL))).lastPathComponent(); + } else { We don't use braces around single line blocks (sam issue repeated multiple times). Instead of comparing to false, the style is to use "!": if (!imageURL.isEmpty()) + extension = MIMETypeRegistry::getPreferredExtensionForMIMEType( cachedImage->response().mimeType()); This line isn't really long enough to break (I know that Google has stricter requirements on line length though). + fileName = KURL(element->document()->completeURL(parseURL(imageURL))).lastPathComponent(); Document::completeURL() returns a KURL, no need to copy it again here.

@ap, he's trying to make Safari 4 and Chromium do the same thing, so it's not just Chromium-specific. Who would be a good person to deem this good? @olliej, would you like to take a peek?

Created attachment 29165 [details] More style fixes and reversed precedence If we agree on the precedence of the response().suggestedFilename(), this is what the code could be... Still unsure if we also want to change the Save As... context menu option though...

Comment on attachment 29165 [details] More style fixes and reversed precedence You can't trust the extension given by the file itself, that's why we explicitly override it with the extension we believe that mimetype should have.

Created attachment 29225 [details] Fixed extension precendence and other details... HOw about this, if we accept the precedence of the content header filename?

Oops... Wrong patch file... The new one is on its way... Sorry...

Created attachment 29234 [details] THis is the one which fixes the extension and filename precedence...

Comment on attachment 29234 [details] THis is the one which fixes the extension and filename precedence... >Index: WebCore/platform/chromium/ClipboardChromium.cpp ... >+ String fileName = cachedImage->response().suggestedFilename(); >+ if (fileName.isEmpty()) { >+ String imageURL = element->getAttribute(srcAttr); why don't you use cachedImage->url() here?

Cause I didn't know I can... I took that code going through the attribute in other parts of the code where we wanted the URL... Will this call always return the same thing as what we find in src attrib? I need to double check...

Ping?

Created attachment 29431 [details] New version with a fix to a small goof... re-ping?

Comment on attachment 29431 [details] New version with a fix to a small goof... Why is the filename fallback stuff not in some shared code? It seems like all WebKit implementations would want the same "try http header, then url, then alt tag" behavior, no?

Marc, have you given up on this bug? :)

Someone needs to pick this patch up and finish the work. It is hard to believe that dragging and dropping still gives the wrong name when a patch has been this close to being completed for months.

Chromium team has asked me to pick up this fix and finish the job. I'll look over Marc-André's last patch and see about moving it into shared code so both Chromium and WinSafari can use it.

Created attachment 34076 [details] New patch (by Jens) This is a new patch incorporating code from the previous one, but heavily rewritten. The core code for determining the filename has been moved to HTMLImageElement so it can be used in other circumstances (such as Safari for Windows; I don't feel competent to make the changes to hook it up for there, though.) I've also implemented ClipboardChromium::validateFilename for the Mac platform. I'm not aware of a way to create a layout test for this, since it would have to involve simulating a drag out of the browser to the filesystem, and then accessing the filesystem to check the name of the resulting file.

Comment on attachment 34076 [details] New patch (by Jens) > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 46768) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,26 @@ > +2009-08-04 Jens Alfke <snej@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().mimeType()); > + 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.

> You can make DRT dump the content of a pasteboard Could you elaborate, or point me at a layout test that does this kind of thing? I don't have much experience writing layout tests. > Also this code looks really similar to the code in other platforms. I don't follow -- there's no platform here, HTMLImageElement is shared code. If there's a utility I should be calling to do this, let me know what it is. >This code should not be on HTMLImageElement, it should probably be shared code >on the root Clipboard class I disagree. The filename is an attribute of the image itself, not of the clipboard. And there are things other than the clipboard that might want the filename (such as the Save As... command.) > This is wrong -- you should always use the tag constants I'll fix that. > 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. I factored as much as possible -- determining the filename -- into HTMLImageElement. The various platform implementations of Clipboard are very different. For example, ClipboardMac delegates the entire implementation of declareAndWriteDragImage to a Mac-specific helper method in DragClient. > Such a refactoring would be a Good Thing, and i think should be > done as cleanup prior to fixing this bug. The Clipboard subclasses are pretty platform-specific; I don't see a lot of other stuff that could be factored out. If you want to make that a prerequisite for fixing this, I'm not qualified to do that, as I'm new to WebKit and have no experience with GUI APIs for Windows or Linux. This bug has already been sitting around for months unfixed, despite being 90% done; the Chromium team wants to fix it for the next milestone release. I think it would be a good idea to fix this user-visible bug before planning any nice-to-have refactoring of the code.

(In reply to comment #25) > > You can make DRT dump the content of a pasteboard > > Could you elaborate, or point me at a layout test that does this kind of thing? > I don't have much experience writing layout tests. Look at the other drag and drop tests in webkit -- they should show you how to initiate a drag, etc, then all you need to do is add an API along the lines of getFilenameForClipboardContent() or whatever that returns the filename. Then you can have a test case and we can ensure this does not regress. > > > Also this code looks really similar to the code in other platforms. > > I don't follow -- there's no platform here, HTMLImageElement is shared code. If > there's a utility I should be calling to do this, let me know what it is. That comment was made before i realised it was in htmlimagelement > > >This code should not be on HTMLImageElement, it should probably be shared code > >on the root Clipboard class > > I disagree. The filename is an attribute of the image itself, not of the > clipboard. And there are things other than the clipboard that might want the > filename (such as the Save As... command.) CachedImage would seem to be a better place then, as you're ruling out SVGImageElement as well in that case. > > 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. > > I factored as much as possible -- determining the filename -- into > HTMLImageElement. The various platform implementations of Clipboard are very > different. For example, ClipboardMac delegates the entire implementation of > declareAndWriteDragImage to a Mac-specific helper method in DragClient. However the only difference between windows and chromium is how they write to the system clipboard -- everything leading up to that point is identical. The correct fix for this would be to refactor the initial platform independent code to be the general entry point for all platforms other that mac (which still has a few pieces of cruft where we call out to a client to preserve historic behaviour -- this is not a good thing, it just hasn't yet been fixed). Then use a (new) low level function on Clipboard to do the final write -- providing the data and appropriate filename -- which can be per-platform. This should not be a hard thing to do. > > > Such a refactoring would be a Good Thing, and i think should be > > done as cleanup prior to fixing this bug. > > The Clipboard subclasses are pretty platform-specific; I don't see a lot of > other stuff that could be factored out. If you want to make that a prerequisite > for fixing this, I'm not qualified to do that, as I'm new to WebKit and have no > experience with GUI APIs for Windows or Linux. They already have the code the does the writing to clipboard, all you're doing is factoring out the name selection. > > This bug has already been sitting around for months unfixed, despite being 90% > done; the Chromium team wants to fix it for the next milestone release. I think > it would be a good idea to fix this user-visible bug before planning any > nice-to-have refactoring of the code. And if you do some minimal refactoring you can actually fix the bug in webkit, rather than fixing the bug in chromium.

> CachedImage would seem to be a better place then Actually I looked at that earlier, but it doesn't have a reference to the Element so it can't get the 'src' or 'alt' attributes. > the only difference between windows and chromium is how they write to > the system clipboard -- everything leading up to that point is identical. Less identical than it looks, unfortunately. Chromium "m_dataObject" and WIn "m_writableDataObject" are entirely different classes (the Win one appears to be a COM object!) The parts that look like they could be factored out are: 1. The filename 2. The image data 3. The absolute form of the source URL I could have the base class derive these, and add them as extra parameters to the declareAndWriteDragImage method. To avoid breaking existing code, I can implement the existing four-parameter method in the base class, and have it derive the above values and call a new seven-parameter version of the method. ClipboardChromium and ClipboardWin can then just implement the latter method. Other subclasses won't be affected, but later on they can simplify their code in the same way.

I'm investigating the layout test. It's a lot more involved than it looks, because it gets into the always-tricky area of "file promises" (as they're called on Mac OS), i.e. dragging a file that doesn't exist yet. I can use the HTML5 drag-n-drop API to examine the drag's dataTransfer object. However, this doesn't actually contain any information about the filename. Even the raw NSPasteboard doesn't have the info -- as Apple's "Drag & Drop Programming Topics For Cocoa" puts it: "Before the drag is actually dropped, a potential dragging destination does not have access to the filenames of the files being promised. Only the file types are available from the pasteboard." To get the filenames, I'd need some native code that calls -namesOfPromisedFilesDroppedAtDestination: on the NSDraggingInfo object. I can add a JS glue method to do this, but I don't know how this method would acquire the NSDraggingInfo corresponding to the current drag operation... Of course, all the above is specific to Mac OS. Someone would need to implement similar functionality for every other platform (that goes both for WebKit and for Chromium, which has its own very different implementation of DRT.)

I've experimented with UIDelegate.mm in DRT and there doesn't seem to be any way, with existing APIs, to get the actual NSDraggingInfo. When the fake drag starts, the -webView:(dragImage:at:... delegate method is called, but it isn't given the draggingInfo object. During the drag, -webView:(dragDestinationActionMaskForDraggingInfo: is called, but it's passed the fake DumpRenderTreeDraggingInfo object that the previous delegate method created. -webView:willPerformDragSourceAction:fromPoint:withPasteboard: is called on drop, but it isn't passed the dragging info. So it seems like, at a minimum, some kind of SPI would need to be plumbed into WebView to make the real draggingInfo accessible. Then it could be captured by DRT, and a new JS accessor could be written so the tests could look at it. This is seeming kind of excessive for just a test for a minor bug-fix.

DRT does not use that standard system pasteboard, that's how we do things like the addFileToPasteboard() tests. It should be trivial to add logic to that to be able to query sensibly. (In reply to comment #29) > I've experimented with UIDelegate.mm in DRT and there doesn't seem to be any > way, with existing APIs, to get the actual NSDraggingInfo. > When the fake drag starts, the -webView:(dragImage:at:... delegate method is > called, but it isn't given the draggingInfo object. > During the drag, -webView:(dragDestinationActionMaskForDraggingInfo: is called, > but it's passed the fake DumpRenderTreeDraggingInfo object that the previous > delegate method created. > -webView:willPerformDragSourceAction:fromPoint:withPasteboard: is called on > drop, but it isn't passed the dragging info. > > So it seems like, at a minimum, some kind of SPI would need to be plumbed into > WebView to make the real draggingInfo accessible. Then it could be captured by > DRT, and a new JS accessor could be written so the tests could look at it. > > This is seeming kind of excessive for just a test for a minor bug-fix.

Created attachment 34160 [details] Updated patch, refactoring Clipboard class As I described above, this patch moves most of the code into the base Clipboard class. The existing declareAndWriteDragImage method is retained for compatibility (to avoid having to modify every subclass) but it now has a base implementation that invokes a new overloaded variant that's passed the important info like the image data, filename, and HTML markup. Subclasses can now just override the new method; I've changed ClipboardChromium to do this. No layout tests are added. See above comments on the difficulty of testing this functionality.

Comment on attachment 34160 [details] Updated patch, refactoring Clipboard class > +#include "Element.h" > #include "Frame.h" > #include "FrameLoader.h" > +#include "HTMLImageElement.h" Since Element is a base class of HTMLImageElement, you never need to add both these includes. Please add the minimum number of includes. For some reason this is marked review- so I won't try to review any more of it.

> DRT does not use that standard system pasteboard, that's how we do things like > the addFileToPasteboard() tests. It's not the pasteboard that matters -- the filename is never put on the pasteboard. it's the DraggingInfo object's -namesOfPromisedFilesDroppedAtDestination: method that needs to be called, and I don't see how to get ahold of that object, which is created down inside WebKit. > It should be trivial to Please stop using the word 'trivial' unless you also back it up by providing detailed info about the 'trivial' changes to be made. Thanks.

Created attachment 34163 [details] Patch with extra include removed Removed redundant #include found by Darin.

Comment on attachment 34163 [details] Patch with extra include removed I know you're just moving code, but I'm reviewing it as if newly written. > + No tests; would be unreasonably difficult to add the plumbing to access the proposed > + filename from within DRT. Tab in change log will make it impossible to land it as-is. > +static CachedImage* getCachedImage(Element* element) In WebKit we normally don't use get in function names when the function has a result. For those we just use nouns or adjectives. Instead get is reserved for functions with out parameters. However, there's a good chance you moved this from elsewhere and the name is not new. Also, this function doesn't do anything that requires an Element* so you might want to consider making its argument a Node* instead. > + RenderImage* image = toRenderImage(renderer); > + if (image->cachedImage() && !image->cachedImage()->errorOccurred()) > + return image->cachedImage(); Normally we like to make return early for the error, and put the success case in the main line of the function. > + String srcURL; > + AtomicString imageURL = element->getAttribute(HTMLNames::srcAttr); The optimal type for this sort of thing is const AtomicString& -- it avoids a bit of reference count churn. We normally do using namespace HTMLNames at the top of source files rather than qualifying the names within the file. The names have distinctive naming that means there's rarely a cross-namespace conflict. > + if (!imageURL.isEmpty()) > + srcURL = frame->document()->completeURL(deprecatedParseURL(imageURL)); Is this check needed? Document::completeURL already returns KURL() if the passed-in string is null. Maybe the check is needed because of non-null empty strings? > + markup.append("<img src=\""); > + markup.append(srcURL); > + markup.append("\""); Is the quoting right here. Does this work for URLs with quote marks in them? Ampersands? > + // Copy over attributes. If we are dragging an image, we expect things like > + // the id to be copied as well. If this was new code I'd point out we should not > + if (attr->localName() == "src") > + continue; This can be done more efficiently with the srcAttr value from HTMLNames rather than doing a string comparison. > + escapedAttr.replace("\"", "&quot;"); Is this the only escaping that's needed? I'd expect that at least "&" would need escaping to "&amp;" as well. > + // Get the image data (abort if there is none). The word "abort" seems a little strange here. > + SharedBuffer* imageBuffer = NULL; New code in WebKit uses 0 for this, not NULL. > + String htmlMarkup = imageToMarkup(element,frame); Missing space after comma. > + this->declareAndWriteDragImage(url, title, imageBuffer, fileName, htmlMarkup); No need for "this->" here. > +void Clipboard::declareAndWriteDragImage(const KURL&, const String&, > + SharedBuffer*, const String&, > + const String&) We normally don't indent like this for multiple line function declarations. > +{ > + // This method is abstract; a subclass must override it, or the public version that calls it. > + ASSERT_NOT_REACHED(); > +} This is an anti-pattern for WebKit. We prefer to use pure virtual functions when possible instead. But I suppose there's no avoiding it if there are two possible functions you could override. Yuck. > #include "HTMLFormElement.h" > #include "HTMLNames.h" > #include "MappedAttribute.h" > +#include "MIMETypeRegistry.h" > #include "RenderImage.h" > #include "ScriptEventListener.h" These are supposed to be case-sensitive sorted, so I think MIME comes before Mapped. Sorry. That's kind of insane but we want to stay specific. You could try the new check-webkit-style script perhaps. > + KURL src = this->src(); > + if (src.isValid()) > + fileName = src.lastPathComponent(); The isValid check here is not needed, since lastPathComponent is guaranteed to return the null string if the URL is not valid. > + fileName = this->altText(); No need for this-> here. > + while (fileName.startsWith(".")) > + fileName.remove(0,1); > + while (fileName.endsWith(".")) > + fileName.remove(fileName.length()-1, 1); Need spaces around the "-1". This is a unnecessarily inefficient way to strip these dots, but perhaps that doesn't really matter. A more efficient way would mutate the string only once using substring. > +static void splitFileName(const String &fileName, String &outBaseName, String &outExtension) WebKit coding style puts the & characters next to the type name, not the variable name. > + int extension_index = fileName.reverseFind('.'); WebKit coding style forbids the use of underscores in multiple word variable names like this. Instead we would call it extensionIndex. > -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) This multi-line style with the subsequent lines lined up with the "(" is not the WebKit style I don't think. I could be wrong on this one. I never do it, but it might be considered an acceptable style. > class ChromiumDataObject; > + class Element; > class IntPoint; I'm surprised this needed to be added. The old file already used Element* but didn't have it. > + protected: > + virtual void declareAndWriteDragImage(const KURL& url, const String& title, > + SharedBuffer* imageBuffer, const String& fileName, > + const String& htmlMarkup); It seems to me this should be private, not protected. The general rule would be to make things as private as possible, and not make something protected just because it was protected in the base class. Also, it is WebKit style to omit names like "url" in this declaration that add nothing beyond what the type adds. > + void writeImageToDataObject(Element*); Why are you adding this declaration? I don't see any definition of this newly-added member function. > +// On HFS+ the maximum length of a filename is 255 UTF-8 bytes. Other filesystems > +// have different limits, but this is the most common, so use it. > +static const int MaxFilenameLengthBytes = 255; Doesn't sound quite right to me, but I guess you're enough of a Mac expert that I'll bow to your judgement on this. > + String result = title.removeCharacters(&isInvalidFileCharacter); > + String extension = dataObject->fileExtension.removeCharacters(&isInvalidFileCharacter); No need for the "&" characters here. review- for now

> > It should be trivial to > > Please stop using the word 'trivial' unless you also back it up by providing > detailed info about the 'trivial' changes to be made. Thanks. Alas you are indeed right, it is trivial to get the extension, but not any of the rest of the filename :-/ I'd still be tempted to test this, even if it's a windows only test

Responding to comments -- other than as noted below, I've made the changes, and will have a new patch ready for review in a few minutes. >Is this check needed? Document::completeURL already returns KURL() if the >passed-in string is null. Maybe the check is needed because of non-null empty >strings? I don't know; this came from the earlier patch I was adapting. I don't know the String/URL classes well so I'm loath to touch this unless you think it's important to remove the check. >Is the quoting right here. Does this work for URLs with quote marks in them? >Ampersands? Likewise, I don't know, but that's a very good point. This code seems to have been expanded from imageToMarkup() in ClipboardWin.cpp, which has the same issue. The URL string shouldn't have quotes in it, but that assumes the URL class made sure to encode them; I don't know if it does. Both the URL and attribute values can contain ampersands. These ought to be quoted (although browsers seem to be lenient about it; a lot of HTML coders don't seem to know about that rule.) Is there any utility code elsewhere in WebCore that generates HTML, that could be called here instead? >New code in WebKit uses 0 for this, not NULL. I just saw that in the style guide. Mixing up pointers and integers this way seems like a bad idea to me, but I'll follow it. >We normally don't indent like this for multiple line function declarations. I can't find anything in the style guide about breaking up long lines, and I looked around but didn't see any multiline declarations. Does WebKit just keep everything on one long line? (This is the way Xcode indents it by default, and I find it more readable.) >This is an anti-pattern for WebKit. We prefer to use pure virtual functions >when possible instead. But I suppose there's no avoiding it if there are two >possible functions you could override. Yuck. I wanted to preserve the existing method, to avoid having to modify every platform's subclass of Clipboard, and that led to this structure. Sorry. Maybe when/if the other platforms move to using the new method, it can then be made pure-virtual. >This is a unnecessarily inefficient way to strip these dots Yes, but this isn't performance-sensitive code, and this way is extremely simple and safe. >It seems to me this should be private, not protected. The general rule would be >to make things as private as possible, and not make something protected just >because it was protected in the base class. In general OOP design it doesn't make sense for a subclass to make an overridden method more private, because it can still be called through the base class interface. I believe it's actually a compile error in Java, for example. But I've changed it to 'private' and GCC seemed to allow it.

Created attachment 34290 [details] patch, responding to Darin's review This addresses the points made in Darin's review. In particular, imageToMarkup() now escapes both double-quotes and ampersands, in all attributes including src. Also, I had doubts about the HFS+ filename limits, so I looked them up and found it's actually 255 Unicode characters, not bytes. (I think I was mis-remembering an old thread on macosx-dev-internal; the UTF-8 limit might be in ext3.)

Comment on attachment 34290 [details] patch, responding to Darin's review Isn't the plumbing already there? You do a drag of an image, with eventSender and then look at what the URLs are on the clipboard. This should all be possible from DRT today, no? There are lots of examples of starting drags in DRT using eventSender.mouseDown(), eventSender.leapForward(), eventSender.mouseMove(), and there are also examples of inspecting the clipboard (event.dataTransfer), which you can do on a drop event. What I don't understand is why this is such a huge amount of code. Is this more than one fix? It's very difficult to understand what's going on in a patch this large, making it difficult to review. Is a bunch of this refactoring which coudl be done first before the actual code change? Also, welcome (back to?) WebKit Jens. :) r- for lack of test (or at least in need of more explaination as to why the above proposed testing strategy would not work.) The code i'm proposing: var dragStart = document.createElement('img'); dragStart.src = "imageName.png"; document.body.appendChild(dragStart); var dragEnd = document.createElement('div'); dragEnd.style.cssText = "width: 100px; height: 100px;" document.body.appendChild(dragEnd); // get the x/y middle of dragStart and dragEnd (see examples in the layout tests). dragEnd.ondrop = function() { event.dataTransfer.getData("URLS"); // do whatever you need to do here. } eventSender.mouseMove(startX, startY); eventSender.mouseDown(); eventSender.leapForward(1000); // to bypass the mac drag delay eventSender.mouseMove(endX, endY); eventSender.mouseUp(); Ideally the test would be dumpAsText with a bunch of nice pass/fail messages, which you would get for free if you use the fast/js testing framework. Search for TEMPLATE.html in the LayoutTests for examples of these types of tests. You write a .js file and then make-js-test-wrappers generates a wrapper for you using the TEMPLATE.html file found in the same directory.

>Isn't the plumbing already there? You do a drag of an image, with eventSender >and then look at what the URLs are on the clipboard. This should all be >possible from DRT today, no? No. :) You're confusing the source URL with the promised filename. The promised filename is what's being affected by this patch, and it does not appear on the dragging pasteboard. The system drag APIs do not make it available until the drop occurs, and even then only via the DraggingInfo object, not the pasteboard. If you're still doubtful, you could go talk to Oliver about it, since he seemed convinced after a couple of iterations of these same arguments. >What I don't understand is why this is such a huge amount of code. Is this >more than one fix? It's very difficult to understand what's going on in a >patch this large, making it difficult to review. Is a bunch of this >refactoring which coudl be done first before the actual code change? It _was_ a reasonable size patch written by somebody else a few months ago, which was rejected for putting the logic into the ClipboardChromium class (which is where the bug was found) instead of somewhere that it could be shared by other implementations. So I added that. Then it was rejected for not factoring out more logic into the base Clipboard class. So I added that too. The changes to ClipboardChromium could be broken into a later patch. However, that would put the first patch in the weird situation of consisting entirely of unreachable code, since it's only ClipboardChromium that uses the new stuff added to the base class (so far). You could pretend this is two patches by just reading down through HTMLImageElement.h (line 232) as a self-contained patch; taking a coffee break; and then coming back and reading the rest as though it were a dependent follow-on patch. :-)

Comment on attachment 34290 [details] patch, responding to Darin's review Setting review status back to "?" after responding to Eric's comments.

Comment on attachment 34290 [details] patch, responding to Darin's review I said it still needs a test, it just can't be tested on mac -- on windows we provide the filename explicitly so are perfectly able to test (similar may be true of the gtk and qt builds)

Comment on attachment 34290 [details] patch, responding to Darin's review > + markup.append(" />"); Does this make HTML markup or XHTML? I'm asking because in HTML there is no meaning to "/>" and using it is probably not a good idea. > + fileName.remove(0,1); Missing space after comma here.

> Does this make HTML markup or XHTML? This came from the earlier patch. My understanding is that the "/>" is harmlessly ignored in HTML mode by all modern browsers (provided the tag is defined as self-closing in HTML.) I can take this out though. And I'll add a space after the comma.

Created attachment 34300 [details] Patch, fixing 2 more issues from Darin Fixed the "/>" and added a space after a comma. To respond to Oliver: Point taken. But the required test code would be platform-specific (adding native code to DRT to inspect the Windows drag object, or at least using JS to check for some Windows-specific drag data type) and I do not have the knowledge of those APIs needed to write such a test.

> To respond to Oliver: Point taken. But the required test code would be > platform-specific (adding native code to DRT to inspect the Windows drag > object, or at least using JS to check for some Windows-specific drag data type) > and I do not have the knowledge of those APIs needed to write such a test. Given this bug is actually specific to windows you cannot *not* test on windows, anyhoo it will be a learning experience :D You can learn to hate windows as we do :D

It's not specific to Windows. It occurs on all platforms of Chrome, since they share the same Clipboard subclass. I made sure I could reproduce it on my Mac before I started working on it.

Forgot to add: There's also a difference between testing on Windows, and writing Windows-specific test code. I have VMWare installed and can run Chromium on Windows. What I don't have is the knowledge of Windows APIs.

(In reply to comment #48) > Forgot to add: There's also a difference between testing on Windows, and > writing Windows-specific test code. I have VMWare installed and can run > Chromium on Windows. What I don't have is the knowledge of Windows APIs. This is testable on windows, therefore it needs a test, there is no point in arguing over this.

Then we appear to be stymied. I have no idea how to write such a Windows-specific test. Eric's example above isn't correct -- I assume he meant "URL" as the parameter to getData(), but that returns the source URL of the original image, not the proposed filename. If the proposed filename does show up in the DataTransfer object on Windows, then please tell me the type name to access it with, and I can write a test and try it out on WIndows. Similarly for GTK; I suppose the test could check for multiple platform-specific types. Then maybe the test could check the User-Agent header to see if it's running on Mac, and in that case skip the check. Thanks.

Comment on attachment 34300 [details] Patch, fixing 2 more issues from Darin > + value.replace("&", "&amp;"); > + value.replace("\"", "&quot;"); There is a faster version of replace that takes a character for the first parameter. You should use that. > + markup.append(" "); > + markup.append(name); > + markup.append("=\""); > + markup.append(value); > + markup.append("\""); There is a version of append that takes a single character. You should use that when possible. > + markup.append(">"); Here too. > +} > + > + > > } // namespace WebCore Although there's no firm policy against it, I prefer not to add this additional vertical space. I guess we can live with this without any further testing. Normally we work very hard to make such things testable when fixing bugs in them. r=me

Comment on attachment 34300 [details] Patch, fixing 2 more issues from Darin We really *do* need a Windows test case here, and I also think it would be worthwhile to make it testable on Mac. Your "would be unreasonably difficult" comment seems wrong to me. We normally make things testable rather than making excuses like that. It seems you've decided it's too difficult without spending much time on it. Is there a big hurry here?

I spent the better part of a day digging through the DRT source code (both the WebKit and Chromium versions) and the upper layers of WebKit, trying to figure out how to get to this data. More time than it took to write the code for the patch. I agree that it's important to make everything testable. It's also important to make everything run fast, and have lots of features, and readable source code, and ship on time. Engineering is trade-offs, we all know that, so you can't meet all those goals at once. My determination was that the expense of adding testability for this was well in excess of the value of that testability, given that this is a fairly minor bug I'm fixing; that's what I meant by "unfeasible difficult". I can write a platform-specific (non-Mac) test case if Oliver (or someone else) can tell me the exact data-type string to check for on various platforms.

Created attachment 34318 [details] patch Uses char versions of String operations.

> I agree that it's important to make everything testable. It's also important to > make everything run fast, and have lots of features, and readable source code, > and ship on time. Engineering is trade-offs, we all know that, so you can't > meet all those goals at once. My determination was that the expense of adding > testability for this was well in excess of the value of that testability, given > that this is a fairly minor bug I'm fixing; that's what I meant by "unfeasible > difficult". I've spent 3 days writing support for testing a change that took half an hour. More over adding support for this testing may prove helpful in the future as well.

I did some more digging; here's what I found. For Mac WebKit (i.e. Safari), the actual logic for computing the filename isn't in Clipboard, or WebCore, at all. It's up in -[WebHTMLView namesOfPromisedFilesDroppedAtDestination:]. This code isn't reachable in DRT, because in order to make drag-and-drop programmatically controllable, DRT fakes out the entire operation, using its own mock draggingSource and draggingInfo. The real draggingInfo implementation (which is in WebHTMLView) doesn't get involved. So, to actually test this would require rewriting the drag-n-drop support in DRT to make the WebHTMLView think it's involved in a real drag, but still without really starting a drag. But! This wouldn't actually test my patch, because as I said, the Mac-WebKit code that determines the file name isn't in Clipboard and doesn't use the code I changed. So writing a Mac test that actually tests my patch would _also_ require rewriting the MacClipboard (and WebHTMLView) code for managing dragging an image, to make it more in line with the other platform implementations. That looks like a big job, and would also make this already-large patch much larger. And to be realistic, I don't think rewriting MacClipboard is something my management at Google would want me to spend time on, since Chrome doesn't even use that code at all. As I said above, I think I can write a Windows-specific test if someone tells me what data type name to look for in the datatransfer object.

(In reply to comment #56) > I did some more digging; here's what I found. > For Mac WebKit (i.e. Safari), the actual logic for computing the filename isn't > in Clipboard, or WebCore, at all. It's up in -[WebHTMLView > namesOfPromisedFilesDroppedAtDestination:]. This code isn't reachable in DRT, > because in order to make drag-and-drop programmatically controllable, DRT fakes > out the entire operation, using its own mock draggingSource and draggingInfo. > The real draggingInfo implementation (which is in WebHTMLView) doesn't get > involved. > So, to actually test this would require rewriting the drag-n-drop support in > DRT to make the WebHTMLView think it's involved in a real drag, but still > without really starting a drag. > But! This wouldn't actually test my patch, because as I said, the Mac-WebKit > code that determines the file name isn't in Clipboard and doesn't use the code > I changed. > So writing a Mac test that actually tests my patch would _also_ require > rewriting the MacClipboard (and WebHTMLView) code for managing dragging an > image, to make it more in line with the other platform implementations. That > looks like a big job, and would also make this already-large patch much larger. > And to be realistic, I don't think rewriting MacClipboard is something my > management at Google would want me to spend time on, since Chrome doesn't even > use that code at all. Actually i suspect they wouldn't mind, however i agree that given this code doesn't effect mac it is unnecessary to do such refactoring for this patch (although a bug suggesting this refactoring would probably be a good thing). Like i said though a windows test is a must because this patch does have an observable impact on windows. Anyhoo, the type i think you want to be querying the data object for on windows is CFSTR_FILEDESCRIPTOR -- you can probably get a good idea of what is necessary in ClipboardWin and PasteboardWin's writeImage* method (look at writeImageToDataObject for the writing side)... actually ClipboardUtilitiesWin has code that reads a filename in getURL, you can probably crib that and copy to drt's win and chromium/win ports. I quick search through the repository shows that there's a WCDataObject used by the main windows port, but there's also a ChromiumDataObject so there may be people at google who can help you more directly.

Comment on attachment 34318 [details] patch I believe you meant to mark this as a patch file. Note if you use a script like "bugzilla-tool post-diff" or "bugzilla-tool post-commits" or git-send-bugzilla it will automatically setup the attachment flags (including review) for you.

Comment on attachment 34318 [details] patch Given Oliver's comment above about what to look for on the clipboard, marking r- while we wait for a response or test case. I wonder if we really want the suggested filename code on HTMLImageElement. Seems like something that would apply for <video> and <audio> elements eventually too, no? Depends on what browsers chose to make the drag behavior for these objects. Also, I would think <svg> image elements would need this same image code. If you're successful in creating a test case, and add an SVG image element to your test, I think you'll find that we "fail" the SVG image element drags after this patch.

I've been bringing up a Windows dev environment and building WebKit on it. Unfortunately the first thing I discovered is that all dragging on Windows is currently broken: dragging anything inside a web view causes a crash. This was reported a month ago as 27332 but no one seems to have investigated it yet. That pretty much stymies me from writing any test cases until that bug is fixed. >I wonder if we really want the suggested filename code on HTMLImageElement. >Seems like something that would apply for <video> and <audio> elements Sure, but the logic for which attribute(s) to check is different for each element. For <img> you check the "src" and "alt" attributes; I don't know what the corresponding attributes for the other elements would be. So there'd need to be a base implementation of suggestedFilename on Element, which probably does nothing, and then concrete implementations for the various elements that support it. Sounds like a good idea for a future commit, but this patch is big enough already.

Since we're blocked on Win crash fix to write a good test, can we submit this fix and file a bug about writing a test? Seems sad that this bug-fixing patch is just sitting here.

Created attachment 95223 [details] Patch

The original issue has been separately fixed in ClipboardWin it appears. I've taken the last patch and simplified it a bit and removed the refactoring of Clipboard. As much as I'd like to share code between ports, I'd rather address the brokenness in Chromium first since it's been around for more than 2 years.

Comment on attachment 95223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95223&action=review > Source/WebCore/ChangeLog:8 > + No new tests since iamge drags aren't accessible through event.dataTransfer.files. iamge -> image Is it possible to add plumbing to the WebKit layer to test this (e.g., by adding a method to eventSender)?

(In reply to comment #64) > (From update of attachment 95223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95223&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests since iamge drags aren't accessible through event.dataTransfer.files. > > iamge -> image > Oops. I'll make sure to fix that before checking in. > Is it possible to add plumbing to the WebKit layer to test this (e.g., by adding a method to eventSender)? It'd have to be a method that takes a event.DataTransfer; presumably there is some way of getting a C++ object from the JS object, and then we'd be able to read the image file name in the drop. It doesn't seem like it belongs on EventSender though, since it seems to be primarily used to trigger DOM events...

(In reply to comment #65) > (In reply to comment #64) > > Is it possible to add plumbing to the WebKit layer to test this (e.g., by adding a method to eventSender)? > > It'd have to be a method that takes a event.DataTransfer; presumably there is some way of getting a C++ object from the JS object, and then we'd be able to read the image file name in the drop. It doesn't seem like it belongs on EventSender though, since it seems to be primarily used to trigger DOM events... It could be on layoutTestController if you think that makes more sense. It could be a dump method (e.g., dumpFilenameCurrentlyBeingDragged()) so it doesn't have to take the current event.DataTransfer. Alternately, you could make a mode that printfs information whenever a drag event fires. For example, something like layoutTestController.dumpEditingCallbacks().

Created attachment 95510 [details] Patch

(In reply to comment #66) > (In reply to comment #65) > > (In reply to comment #64) > > > Is it possible to add plumbing to the WebKit layer to test this (e.g., by adding a method to eventSender)? > > > > It'd have to be a method that takes a event.DataTransfer; presumably there is some way of getting a C++ object from the JS object, and then we'd be able to read the image file name in the drop. It doesn't seem like it belongs on EventSender though, since it seems to be primarily used to trigger DOM events... > > It could be on layoutTestController if you think that makes more sense. > > It could be a dump method (e.g., dumpFilenameCurrentlyBeingDragged()) so it doesn't have to take the current event.DataTransfer. Alternately, you could make a mode that printfs information whenever a drag event fires. For example, something like layoutTestController.dumpEditingCallbacks(). I think EventSender is an awkward place to put this, but I would really like to see this checked in rather than linger for another 2 years.

Comment on attachment 95510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95510&action=review Seems OK. Mostly small nits. > LayoutTests/fast/events/drag-image-filename.html:50 > +<p>This test requires DumpRenderTree. > + > +<div id="target" ondragenter="dragEnterOrOver(event)" ondragover="dragEnterOrOver(event)" ondrop="drop(event)"></div> > +<img id="source" src="resources/onload-image.png" alt="Does it work?"> Please include instructions for manually testing (e.g., drag the image to the desktop. The filename should be onload-image.png). > LayoutTests/platform/gtk/Skipped:1147 > +# EventSender::dumpFilenameBeingDragged not implemented. > +fast/events/drag-image-filename.html Please file bugs for each platform that fails and include the bug link in the Skipped files before landing. > Source/WebCore/ChangeLog:6 > + [chromium] Use correct file name for dragging out images. > + https://bugs.webkit.org/show_bug.cgi?id=24887 Please be more specific here. Like, "Prefer the last component of the URL for the filename over alt text when dragging out images." > Tools/DumpRenderTree/chromium/EventSender.cpp:348 > +void EventSender::dumpFilenameBeingDragged(const CppArgumentList& arguments, CppVariant* result) Nit: I think webkit style is to not name params that are unused.

Created attachment 95526 [details] Patch

Created attachment 95528 [details] Patch

(In reply to comment #69) > (From update of attachment 95510 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95510&action=review > > Seems OK. Mostly small nits. > > > LayoutTests/fast/events/drag-image-filename.html:50 > > +<p>This test requires DumpRenderTree. > > + > > +<div id="target" ondragenter="dragEnterOrOver(event)" ondragover="dragEnterOrOver(event)" ondrop="drop(event)"></div> > > +<img id="source" src="resources/onload-image.png" alt="Does it work?"> > > Please include instructions for manually testing (e.g., drag the image to the desktop. The filename should be onload-image.png). Done. > > > LayoutTests/platform/gtk/Skipped:1147 > > +# EventSender::dumpFilenameBeingDragged not implemented. > > +fast/events/drag-image-filename.html > > Please file bugs for each platform that fails and include the bug link in the Skipped files before landing. Done. > > > Source/WebCore/ChangeLog:6 > > + [chromium] Use correct file name for dragging out images. > > + https://bugs.webkit.org/show_bug.cgi?id=24887 > > Please be more specific here. Like, "Prefer the last component of the URL for the filename over alt text when dragging out images." > Done. > > Tools/DumpRenderTree/chromium/EventSender.cpp:348 > > +void EventSender::dumpFilenameBeingDragged(const CppArgumentList& arguments, CppVariant* result) > > Nit: I think webkit style is to not name params that are unused. Done.

The commit-queue encountered the following flaky tests while processing attachment 95528 [details]: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 95528 [details] Patch Clearing flags on attachment: 95528 Committed r87848: <http://trac.webkit.org/changeset/87848>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 95528 [details]: inspector/debugger/debugger-scripts.html bug 59921 (authors: pfeldman@chromium.org and podivilov@chromium.org) The commit-queue is continuing to process your patch.