39882 2010-05-28 10:05:57 -0700 openFile(...) in FIleSystemPOSIX does not call fileSystemRepresentation 2010-05-28 13:08:24 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 1 benm benm android-webkit-unforking darin oldest_to_newest 231754 0 benm 2010-05-28 10:05:57 -0700 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. 231762 1 57338 benm 2010-05-28 10:15:04 -0700 Created attachment 57338 Patch. 231771 2 57338 darin 2010-05-28 10:26:34 -0700 Comment on attachment 57338 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 231862 3 benm 2010-05-28 12:16:41 -0700 (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 231893 4 benm 2010-05-28 13:08:24 -0700 Sending WebCore/ChangeLog Sending WebCore/platform/posix/FileSystemPOSIX.cpp Transmitting file data .. Committed revision 60374. 57338 2010-05-28 10:15:04 -0700 2010-05-28 10:26:33 -0700 Patch. 39882.txt text/plain 1580 benm SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA2MDM2NCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDUtMjggIEJlbiBNdXJkb2NoICA8YmVubUBnb29nbGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIG9wZW5G aWxlKC4uLikgaW4gRklsZVN5c3RlbVBPU0lYIGRvZXMgbm90IGNhbGwgZmlsZVN5c3RlbVJlcHJl c2VudGF0aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0zOTg4MgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gRXhpc3RpbmcgdGVzdHMgaW4gZmFzdC9m aWxlcyBzaG91bGQgc3VmZmljZS4KKworICAgICAgICAqIHBsYXRmb3JtL3Bvc2l4L0ZpbGVTeXN0 ZW1QT1NJWC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpvcGVuRmlsZSk6IHBhc3MgdGhlIHBhdGgg cGFyYW1ldGVyIHRocm91Z2ggZmlsZVN5c3RlbVJlcHJlc2VudGF0aW9uIGJlZm9yZSB1c2luZyBp dC4KKwogMjAxMC0wNS0yNyAgRGFyaW4gQWRsZXIgIDxkYXJpbkBhcHBsZS5jb20+CiAKICAgICAg ICAgUmV2aWV3ZWQgYnkgRGF2aWQgTGV2aW4uCkluZGV4OiBXZWJDb3JlL3BsYXRmb3JtL3Bvc2l4 L0ZpbGVTeXN0ZW1QT1NJWC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9wb3Np eC9GaWxlU3lzdGVtUE9TSVguY3BwCShyZXZpc2lvbiA2MDM1MCkKKysrIFdlYkNvcmUvcGxhdGZv cm0vcG9zaXgvRmlsZVN5c3RlbVBPU0lYLmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjksMTIgKzY5 LDE3IEBAIGJvb2wgZGVsZXRlRmlsZShjb25zdCBTdHJpbmcmIHBhdGgpCiAKIFBsYXRmb3JtRmls ZUhhbmRsZSBvcGVuRmlsZShjb25zdCBTdHJpbmcmIHBhdGgsIEZpbGVPcGVuTW9kZSBtb2RlKQog eworICAgIENTdHJpbmcgZnNSZXAgPSBmaWxlU3lzdGVtUmVwcmVzZW50YXRpb24ocGF0aCk7CisK KyAgICBpZiAoIWZzUmVwLmRhdGEoKSB8fCBmc1JlcC5kYXRhKClbMF0gPT0gJ1wwJykKKyAgICAg ICAgcmV0dXJuIGludmFsaWRQbGF0Zm9ybUZpbGVIYW5kbGU7CisKICAgICBpbnQgcGxhdGZvcm1G bGFnID0gMDsKICAgICBpZiAobW9kZSA9PSBPcGVuRm9yUmVhZCkKICAgICAgICAgcGxhdGZvcm1G bGFnIHw9IE9fUkRPTkxZOwogICAgIGVsc2UgaWYgKG1vZGUgPT0gT3BlbkZvcldyaXRlKQogICAg ICAgICBwbGF0Zm9ybUZsYWcgfD0gKE9fV1JPTkxZIHwgT19DUkVBVCB8IE9fVFJVTkMpOwotICAg IHJldHVybiBvcGVuKHBhdGgudXRmOCgpLmRhdGEoKSwgcGxhdGZvcm1GbGFnLCAwNjY2KTsKKyAg ICByZXR1cm4gb3Blbihmc1JlcC5kYXRhKCksIHBsYXRmb3JtRmxhZywgMDY2Nik7CiB9CiAKIHZv aWQgY2xvc2VGaWxlKFBsYXRmb3JtRmlsZUhhbmRsZSYgaGFuZGxlKQo=