Bug 15221 – Make the FileSystemChooserGdk implementation properly convert the WebCore::String presentation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Holger Freyther]

Currently the code assumes that the filesystem/system is using UTF-8. This assumption does not always hold true and there are the g_filename_convert_{from,to}_utf8 function to convert from the UTF-8 representation to the filesystem locale.
Also implement proper NULL checking for the filechooser get_filename method.

[Holger Freyther]

Created attachment 16297 [details]
Properly convert the name of the file

This should implement the NULL-checking and should properly convert from WebCore::String -> UTF-8 -> filesystem and from filesystem -> UTF-8 -> WebCore::String.

[Adam Roben (:aroben)]

Comment on attachment 16297 [details]
Properly convert the name of the file

+                gchar* basename = g_filename_to_utf8(basenameSystem, -1, 0, 0, 0);
+                g_free(basenameSystem);
+
+                if (basename) {
+                    string = String::fromUTF8(basename);
+                    g_free(basename);
+                }

This sequence should be factored out into a static helper function instead of repeating it twice in this file. Otherwise looks good.

[Holger Freyther]

Created attachment 16304 [details]
Convert properly

This takes the comments of Adam from this bug report and irc into account.

[Mark Rowe (bdash)]

Comment on attachment 16304 [details]
Convert properly

I think "convertToStringByAdoptingTheFilesystemRepresentation" may be a little bit wordy.  At the least, I think "The" does not add anything.  Elsewhere we also treat "filesystem" as two words.  Perhaps "stringWithFileSystemRepresentation" would be a better name?

"filenameSystem" also feels backwards.  "systemFilename" reads better to me.

Other than these naming niggles, r=me.

[Holger Freyther]

(In reply to comment #4)
> (From update of attachment 16304 [details] [edit])
> I think "convertToStringByAdoptingTheFilesystemRepresentation" may be a little
> bit wordy.  At the least, I think "The" does not add anything.  Elsewhere we
> also treat "filesystem" as two words.  Perhaps
> "stringWithFileSystemRepresentation" would be a better name?
> 
> "filenameSystem" also feels backwards.  "systemFilename" reads better to me.
> 
> Other than these naming niggles, r=me.
> 

Oops, I landed that without reading that part. I agree with systemFilename and will do a fixup (without going through review) but the function name is difficult as it takes ownership of the systemFilename and we wanted to have that inside the name.