Bug 87578 - [FileAPI] The result attribute of FileReader should use null to replace empty string
Summary: [FileAPI] The result attribute of FileReader should use null to replace empty...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 87760
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-26 08:31 PDT by Li Yin
Modified: 2012-05-30 11:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (10.08 KB, patch)
2012-05-26 11:01 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2012-05-26 16:28 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2012-05-29 20:28 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2012-05-29 22:48 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-05-26 08:31:45 PDT
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.
Comment 1 Li Yin 2012-05-26 11:01:58 PDT
Created attachment 144205 [details]
Patch
Comment 2 Kentaro Hara 2012-05-26 15:05:45 PDT
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 ';'
Comment 3 Li Yin 2012-05-26 16:17:41 PDT
(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.
Comment 4 Li Yin 2012-05-26 16:28:19 PDT
Created attachment 144210 [details]
Patch
Comment 5 Li Yin 2012-05-26 16:36:16 PDT
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.
Comment 6 Li Yin 2012-05-26 17:37:10 PDT
I think using null should be more resonable, because empty string will confuse developers when reading a empty file.
Comment 7 Kentaro Hara 2012-05-26 18:02:58 PDT
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 8 WebKit Review Bot 2012-05-26 18:51:21 PDT
Comment on attachment 144210 [details]
Patch

Clearing flags on attachment: 144210

Committed r118620: <http://trac.webkit.org/changeset/118620>
Comment 9 WebKit Review Bot 2012-05-26 18:51:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Jian Li 2012-05-29 10:36:45 PDT
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.
Comment 11 Jian Li 2012-05-29 11:00:53 PDT
I verify that Firefox 12 returns empty string for result when reading an empty file.
Comment 12 WebKit Review Bot 2012-05-29 11:12:18 PDT
Re-opened since this is blocked by 87760
Comment 13 Li Yin 2012-05-29 18:29:08 PDT
> > 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.
Comment 14 Li Yin 2012-05-29 20:28:02 PDT
Created attachment 144686 [details]
Patch
Comment 15 Jian Li 2012-05-29 22:12:26 PDT
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?
Comment 16 Li Yin 2012-05-29 22:48:46 PDT
Created attachment 144705 [details]
Patch
Comment 17 Li Yin 2012-05-29 22:50:42 PDT
(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 18 WebKit Review Bot 2012-05-29 23:34:37 PDT
Comment on attachment 144705 [details]
Patch

Clearing flags on attachment: 144705

Committed r118897: <http://trac.webkit.org/changeset/118897>
Comment 19 WebKit Review Bot 2012-05-29 23:34:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Alexey Proskuryakov 2012-05-30 11:08:35 PDT
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>.