Bug 28975 – [ChromiumMac] Can't upload images to imgur.com when "Hide Extension" set

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nico Weber]

Can't upload images with "Hide Extension" set to imgur.com on chromium/mac

[Nico Weber]

Created attachment 39072 [details]
Patch.

[Nico Weber]

Slightly more detailed bug report at http://crbug.com/20857 .

[David Levin]

Comment on attachment 39072 [details]
Patch.

Just some minor nits... Please fix and then I can go through this one more time.


> Index: WebCore/platform/chromium/FileChooserChromium.cpp
> ===================================================================
> +#if PLATFORM(DARWIN)
> +        // See crbug.com/20857.
> +        string = pathGetPresentationalName(m_filenames[0]);
> +#else
>          string = pathGetFileName(m_filenames[0]);
> +#endif

Add braces around this since there is more than one physical line here.




> Index: WebCore/platform/chromium/FileSystemChromiumMac.mm
> ===================================================================

> +  // This has to return a real, existing filename with extension, see
> +  // crbug.com/20857.
> +  return [path lastPathComponent];

Use a 4 space indent throughout.


> +String pathGetPresentationalName(const String& path)
> +{
> +  return [[NSFileManager defaultManager] displayNameAtPath:path];

Use a 4 space indent throughout.

[Viet-Trung Luu]

IANAWKR, but some comments anyway:

> Index: WebCore/platform/chromium/FileChooserChromium.cpp

> +#if PLATFORM(DARWIN)
> +String pathGetPresentationalName(const String& path);
> +#endif

I don't think having an explicit prototype to something in another file is a good idea; if it's that specific to this file, maybe the entire function belongs in this file? However, see also my comments below.

Nit: I would prefer |pathGetDisplayFileName()|. (Why? I want the "File" in there to indicate that it's just a display form of the file name, not the entire path; "Display" is shorter and maybe also more standard than "Presentational".)

> +#if PLATFORM(DARWIN)
> +        // See crbug.com/20857.
> +        string = pathGetPresentationalName(m_filenames[0]);
> +#else
>          string = pathGetFileName(m_filenames[0]);
> +#endif

I don't like this #ifdef; I'd rather see |pathGetPresentationalName()| just do the same thing as |pathGetFileName()| on other platforms (again, see below).

> Index: WebCore/platform/chromium/FileSystemChromiumMac.mm

>  String pathGetFileName(const String& path)
[...]
> +String pathGetPresentationalName(const String& path)
[...]

This is fine in itself, but |pathGetFileName| has prototype in WebCore/platform/FileSystem.h, so |pathGetPresentationalName()| (or whatever it ends up being called) should logically be there too. I wouldn't be opposed to that, even with an |#if (PLATFORM(CHROMIUM) && PLATFORM(DARWIN) [prototype ...] #else [inline definition which makes pathGetPresentationName() equivalent to pathGetFileName ...] #endif|. I can see that others may be opposed to this, though.

Just my opinion -- as I said, IANAWKR.

[Nico Weber]

Created attachment 39096 [details]
Address review comments.

[Nico Weber]

Created attachment 39097 [details]
Adress trungl's comments as well.

This addresses trungl's comments as well. I think his remarks make sense, but if you like the previous version better, feel free to go with that.

[David Levin]

Comment on attachment 39097 [details]
Adress trungl's comments as well.

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];
> +}

[Nico Weber]

Created attachment 39109 [details]
Address more review comments.

[Eric Seidel (no email)]

Comment on attachment 39109 [details]
Address more review comments.

Clearing flags on attachment: 39109

Committed r48095: <http://trac.webkit.org/changeset/48095>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.