Bug 114133 – [GTK] Remove platform specific implementation of KURL::fileSystemPath()

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

[Patrick R. Gansterer]

[GTK] Remove platform specific implementation of KURL::fileSystemPath()

[Patrick R. Gansterer]

Created attachment 196815 [details]
Patch

[Gustavo Noronha (kov)]

There is one thing to be considered here: the GTK+ implementation of KRUL::fileSystemPath ends up calling FileSystemGtk's filenameToString(). That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything. We will lose that feature by adopting the shared implementation. I don't mind, but I'd like to hear other opinions.

[Patrick R. Gansterer]

(In reply to comment #2)
> That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything.

For what is the encoding stuff needed? Input of KRUL::fileSystemPath is UTF16 and output is UTF16? Any special encondig of the filesytem has to be done somewhere else (e.g. at http://trac.webkit.org/browser/trunk/Source/WebCore/platform/gtk/FileSystemGtk.cpp?rev=148014#L56) IMHO.

[Martin Robinson]

(In reply to comment #2)
> There is one thing to be considered here: the GTK+ implementation of KRUL::fileSystemPath ends up calling FileSystemGtk's filenameToString(). That code uses glib's file naming functions which are smart enough to deal with systems that use encodings other than UTF-8 and even names that cannot be encoded to anything. We will lose that feature by adopting the shared implementation. I don't mind, but I'd like to hear other opinions.

The old code looks a bit wrong too:

RefPtr<GFile> file = adoptGRef(g_file_new_for_path(path().utf8().data()));
GOwnPtr<char> filename(g_file_get_path(file.get()));
return filenameToString(filename.get());

Here it's using a UTF-8 string to produce a filename, while it should instead be using a system-encoded string. As long as translation to a system-encoded string happens later, I think it might be okay to use the shared implementation.

[Patrick R. Gansterer]

(In reply to comment #4)
> (In reply to comment #2)
> The old code looks a bit wrong too:
> 
> RefPtr<GFile> file = adoptGRef(g_file_new_for_path(path().utf8().data()));
> GOwnPtr<char> filename(g_file_get_path(file.get()));
> return filenameToString(filename.get());
> 
> Here it's using a UTF-8 string to produce a filename, while it should instead be using a system-encoded string. As long as translation to a system-encoded string happens later, I think it might be okay to use the shared implementation.

The more importent failure is, that path() returns a percent-encoded string and needs a decodeURLEscapeSequences(). So this code never worked with non-ASCII file names anyway.
So what about landing it?

[WebKit Commit Bot]

Comment on attachment 196815 [details]
Patch

Clearing flags on attachment: 196815

Committed r148440: <http://trac.webkit.org/changeset/148440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.