Bug 15221 - Make the FileSystemChooserGdk implementation properly convert the WebCore::String presentation
Summary: Make the FileSystemChooserGdk implementation properly convert the WebCore::St...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-09-15 07:20 PDT by Holger Freyther
Modified: 2007-09-17 08:09 PDT (History)
0 users

See Also:


Attachments
Properly convert the name of the file (3.09 KB, patch)
2007-09-15 07:21 PDT, Holger Freyther
aroben: review-
Details | Formatted Diff | Diff
Convert properly (3.48 KB, patch)
2007-09-17 04:25 PDT, Holger Freyther
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2007-09-15 07:20:05 PDT
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.
Comment 1 Holger Freyther 2007-09-15 07:21:37 PDT
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.
Comment 2 Adam Roben (:aroben) 2007-09-16 13:16:06 PDT
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.
Comment 3 Holger Freyther 2007-09-17 04:25:27 PDT
Created attachment 16304 [details]
Convert properly

This takes the comments of Adam from this bug report and irc into account.
Comment 4 Mark Rowe (bdash) 2007-09-17 07:51:44 PDT
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.
Comment 5 Holger Freyther 2007-09-17 08:09:23 PDT
(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.