Bug 114133

Summary: [GTK] Remove platform specific implementation of KURL::fileSystemPath()
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: New BugsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, gns, mrobinson, philn, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Patrick R. Gansterer 2013-04-07 16:04:20 PDT
[GTK] Remove platform specific implementation of KURL::fileSystemPath()
Comment 1 Patrick R. Gansterer 2013-04-07 16:07:45 PDT
Created attachment 196815 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2013-04-09 05:47:45 PDT
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.
Comment 3 Patrick R. Gansterer 2013-04-09 05:53:19 PDT
(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.
Comment 4 Martin Robinson 2013-04-09 07:11:40 PDT
(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.
Comment 5 Patrick R. Gansterer 2013-04-15 01:16:15 PDT
(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?
Comment 6 WebKit Commit Bot 2013-04-15 08:27:28 PDT
Comment on attachment 196815 [details]
Patch

Clearing flags on attachment: 196815

Committed r148440: <http://trac.webkit.org/changeset/148440>
Comment 7 WebKit Commit Bot 2013-04-15 08:27:30 PDT
All reviewed patches have been landed.  Closing bug.