Bug 39882 - openFile(...) in FIleSystemPOSIX does not call fileSystemRepresentation
Summary: openFile(...) in FIleSystemPOSIX does not call fileSystemRepresentation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-28 10:05 PDT by Ben Murdoch
Modified: 2010-05-28 13:08 PDT (History)
2 users (show)

See Also:


Attachments
Patch. (1.54 KB, patch)
2010-05-28 10:15 PDT, Ben Murdoch
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 2010-05-28 10:05:57 PDT
In WebCore/platform/posix/FileSystemPOSIX.cpp all the functions that access the filesystem (e.g. fileExists, deleteFile) and take a WebCore::String representing the file path run that string through fileSystemRepresentation() before using it, except for openFile(). This causes openFile to fail on Android, where the call to fileSystemRepresentation is very important.

I'd like to change openFile() to make this call. I think the existing FileReader tests in LayoutTests/fast/files should suffice here.

Patch to follow.
Comment 1 Ben Murdoch 2010-05-28 10:15:04 PDT
Created attachment 57338 [details]
Patch.
Comment 2 Darin Adler 2010-05-28 10:26:34 PDT
Comment on attachment 57338 [details]
Patch.

> +    if (!fsRep.data() || fsRep.data()[0] == '\0')
> +        return invalidPlatformFileHandle;

I understand the check for null here, but:

   1) I suggest using the isNull function instead.
   2) Why the check for the empty string? I don't think there's a need to optimize that case, and I expect open to give an error.

r=me
Comment 3 Ben Murdoch 2010-05-28 12:16:41 PDT
(In reply to comment #2)
> (From update of attachment 57338 [details])
> > +    if (!fsRep.data() || fsRep.data()[0] == '\0')
> > +        return invalidPlatformFileHandle;
> 
> I understand the check for null here, but:
> 
>    1) I suggest using the isNull function instead.
>    2) Why the check for the empty string? I don't think there's a need to optimize that case, and I expect open to give an error.
> 
> r=me

Thanks for the review Darin! When checking the value of the return of the fileSystemRepresentation function I was following the standard pattern of other functions in the file - they all perform a null and empty string check in this way.

Having said that, I think you're correct - isNull() is better and open should be able to handle the empty string. I'll make your suggested changes and land this patch manually.

Cheers, Ben
Comment 4 Ben Murdoch 2010-05-28 13:08:24 PDT
Sending        WebCore/ChangeLog
Sending        WebCore/platform/posix/FileSystemPOSIX.cpp
Transmitting file data ..
Committed revision 60374.