[webkit-reviews] review granted: [Bug 28975] [ChromiumMac] Can't upload images to imgur.com when "Hide Extension" set : [Attachment 39097] Adress trungl's comments as well.

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Nico Weber <thakis at chromium.org>'s
request for review:
Bug 28975: [ChromiumMac] Can't upload images to imgur.com when "Hide Extension"
set
https://bugs.webkit.org/show_bug.cgi?id=28975

Attachment 39097: Adress trungl's comments as well.
https://bugs.webkit.org/attachment.cgi?id=39097&action=review

------- Additional Comments from David Levin <levin at chromium.org>
A few last minor things.... if you find someone to fix this up and commit
great. (I may even do so later, but you could also submit a new patch with it
fixed and then I should r+ and cq+ that.)


> Index: WebCore/platform/FileSystem.h
> ===================================================================

> +#if PLATFORM(CHROMIUM)
> +String pathGetDisplayFileName(const String& path);
> +#endif

Please put this with all of the other if PLATFORM api's at the bottom of the
header.

> Index: WebCore/platform/chromium/FileChooserChromium.cpp
> +    else if (m_filenames.size() == 1) {
> +	   // See crbug.com/20857.

Let's omit this comment (and then the braces). (It is pretty atypical for
WebKit.)  Anyone who wants to know why this line looks this way can look in
revision history and see your change log and the associated bug.

> Index: WebCore/platform/chromium/FileSystemChromiumMac.mm
> @@ -38,6 +38,13 @@ namespace WebCore {
>  
>  String pathGetFileName(const String& path)
>  {
> +    // This has to return a real, existing filename with extension, see
> +    // crbug.com/20857.

This doesn't need the bug reference as it can be found from the revision
history.

> +    return [path lastPathComponent];
> +}