Summary: | openFile(...) in FIleSystemPOSIX does not call fileSystemRepresentation | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Murdoch <benm> | ||||
Component: | WebCore Misc. | Assignee: | Ben Murdoch <benm> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | android-webkit-unforking, darin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Android | ||||||
OS: | Android | ||||||
Attachments: |
|
Description
Ben Murdoch
2010-05-28 10:05:57 PDT
Created attachment 57338 [details]
Patch.
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 (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 Sending WebCore/ChangeLog Sending WebCore/platform/posix/FileSystemPOSIX.cpp Transmitting file data .. Committed revision 60374. |