[webkit-reviews] review denied: [Bug 24887] Wrong filename when dragging an image to the Desktop. : [Attachment 34163] Patch with extra include removed

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 34163: Patch with extra include removed
https://bugs.webkit.org/attachment.cgi?id=34163&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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