Bug 87578 – [FileAPI] The result attribute of FileReader should use null to replace empty string

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Li Yin]

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.

[Li Yin]

Created attachment 144205 [details]
Patch

[Kentaro Hara]

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 ';'

[Li Yin]

(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.

[Li Yin]

Created attachment 144210 [details]
Patch

[Li Yin]

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.

[Li Yin]

I think using null should be more resonable, because empty string will confuse developers when reading a empty file.

[Kentaro Hara]

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.

[WebKit Review Bot]

Comment on attachment 144210 [details]
Patch

Clearing flags on attachment: 144210

Committed r118620: <http://trac.webkit.org/changeset/118620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jian Li]

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.

[Jian Li]

I verify that Firefox 12 returns empty string for result when reading an empty file.

[WebKit Review Bot]

Re-opened since this is blocked by 87760

[Li Yin]

> > 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.

[Li Yin]

Created attachment 144686 [details]
Patch

[Jian Li]

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?

[Li Yin]

Created attachment 144705 [details]
Patch

[Li Yin]

(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.

[WebKit Review Bot]

Comment on attachment 144705 [details]
Patch

Clearing flags on attachment: 144705

Committed r118897: <http://trac.webkit.org/changeset/118897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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>.