From Spec: http://www.w3.org/TR/FileAPI/#filedata-attr The list below is normative for the result attribute and is the conformance criteria for this attribute: On getting, if the readyState is EMPTY (no read method has been called) then the result attribute MUST return null. On getting, if an error in reading the File or Blob has occurred (using any read method), then the result attribute MUST return null. Currently, WebKit uses string "", not use null. We should keep the conformance with the spec.
Created attachment 144205 [details] Patch
Comment on attachment 144205 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144205&action=review > Source/WebCore/ChangeLog:12 > + WebKit should change the returned value empty string into null to keep > + conformance with the spec. What are the behaviors of other browsers? We want to also keep cross-browser compatibility. > Source/WebCore/fileapi/FileReader.cpp:248 > + if (ret == "") ret.IsEmpty() would be better. > LayoutTests/fast/files/resources/read-common.js:174 > + if (result == null) { Please use === instead of ==. == in JavaScript is "ambiguous" and thus not suitable for testing purpose. > LayoutTests/fast/files/resources/read-common.js:176 > + return ; Nit: No space needed before ';'
(In reply to comment #2) > (From update of attachment 144205 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144205&action=review > > > Source/WebCore/ChangeLog:12 > > + WebKit should change the returned value empty string into null to keep > > + conformance with the spec. > > What are the behaviors of other browsers? We want to also keep cross-browser compatibility. > Currently, Firefox 11, Opera 11, IE 10 follows the standard. Webkit based browsers don't. Thanks for your review.
Created attachment 144210 [details] Patch
You can get the test result from http://samples.msdn.microsoft.com/ietestcenter/fileapi/fileapi_harness.htm Test Method: Click the "Net" button three times, enter Test 3/3. Directly click "Run Test" button, the 14th sub-test can verify the feature.
I think using null should be more resonable, because empty string will confuse developers when reading a empty file.
Comment on attachment 144210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144210&action=review > Source/WebCore/ChangeLog:15 > + Currently, Firefox, Opera and IE 10 follows the spec, but Webkit based > + browser don't. > + WebKit should change the returned value empty string into null to keep > + conformance with the spec. Thanks for the clarification. Then the fix looks reasonable.
Comment on attachment 144210 [details] Patch Clearing flags on attachment: 144210 Committed r118620: <http://trac.webkit.org/changeset/118620>
All reviewed patches have been landed. Closing bug.
Comment on attachment 144210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144210&action=review > Source/WebCore/fileapi/FileReader.cpp:248 > + if (ret.isEmpty()) This would cause NULL being returned when reading an empty file. I think you should change it to: if (m_loader && m_loader->stringResult()) return m_loader->stringResult(); return String(); > LayoutTests/fast/files/blob-slice-test-expected.txt:3 > +Slicing from 2 to 2: null The spec does not say that null needs to be returned for empty file/blob. I think we should still return empty string for this case. You could add test cases to cover the scenarios that result is being read when readyState is EMPTY or an error occurs.
I verify that Firefox 12 returns empty string for result when reading an empty file.
Re-opened since this is blocked by 87760
> > Source/WebCore/fileapi/FileReader.cpp:248 > > + if (ret.isEmpty()) > > This would cause NULL being returned when reading an empty file. I think you should change it to: > if (m_loader && m_loader->stringResult()) > return m_loader->stringResult(); > return String(); > When the m_loader is Null or the error is occurred, both of them should return null. So I think the right change shoule be: if (!m_loader || m_error) return String(); return m_loader->stringResult; MeanWhile, the arrayBufferResult should be changed too. if (!m_loader || m_error) return 0; return m_loader->arrayBufferResult(); Thanks for reporting this regression.
Created attachment 144686 [details] Patch
Comment on attachment 144686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144686&action=review > Source/WebCore/ChangeLog:10 > + Or if an error in reading the File or Blob has occurred (using any read method), nit: two "if" can be concatenated like If the readyState is EMPTY (...) or an error occurs ... > Source/WebCore/ChangeLog:17 > + nit: extra empty line is not needed. > LayoutTests/fast/files/resources/read-file-test-cases.js:221 > + log("result before calling reading method: " + readerToTestReread.result) Why is this check being added to this test testReadAgainAfterFailedReadStep1? Why not adding a new async test case like testResultBeforeRead?
Created attachment 144705 [details] Patch
(In reply to comment #15) > nit: two "if" can be concatenated like If the readyState is EMPTY (...) or an error occurs ... Done > > > Source/WebCore/ChangeLog:17 > > + > > nit: extra empty line is not needed. > Done > > LayoutTests/fast/files/resources/read-file-test-cases.js:221 > > + log("result before calling reading method: " + readerToTestReread.result) > > Why is this check being added to this test testReadAgainAfterFailedReadStep1? Why not adding a new async test case like testResultBeforeRead? Done. Thanks for your review.
Comment on attachment 144705 [details] Patch Clearing flags on attachment: 144705 Committed r118897: <http://trac.webkit.org/changeset/118897>
Mac buildbot was seeing failures on read-blob-async.html. These did not look platform specific (additional lines in output), so I updated cross platform results: <http://trac.webkit.org/changeset/118940>.