Bug 88212 - [FileAPI] FileReader should throw an InvalidStateError exception when readyState is LOADING
Summary: [FileAPI] FileReader should throw an InvalidStateError exception when readySt...
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: Li Yin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-03 23:51 PDT by Li Yin
Modified: 2012-06-07 02:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (4.58 KB, patch)
2012-06-04 00:08 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (4.80 KB, patch)
2012-06-05 22:01 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (31.87 KB, patch)
2012-06-06 18:32 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (31.97 KB, patch)
2012-06-06 21:47 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-06-03 23:51:54 PDT
From spec: http://www.w3.org/TR/FileAPI/#readAsBinaryString
If readyState = LOADING throw an InvalidStateError exception.
Currently, webkit indeed throws an exception named "NOT_ALLOWED_ERR" in this scenrio, but it hould be "InvalidStateError".
Comment 1 Li Yin 2012-06-04 00:08:17 PDT
Created attachment 145523 [details]
Patch
Comment 2 Kentaro Hara 2012-06-04 00:12:37 PDT
Comment on attachment 145523 [details]
Patch

The patch looks good to me, but want to hear opinions of File API folks.

Are you fixing this bug based on the IE test center? If so, it would be better to describe the link to the IE test in ChangeLog. It would be also better to confirm the cross-browser compatibility (i.e. how Firefox and Opera behave).
Comment 3 Li Yin 2012-06-04 00:17:20 PDT
(In reply to comment #2)
> Are you fixing this bug based on the IE test center? If so, it would be better to describe the link to the IE test in ChangeLog. It would be also better to confirm the cross-browser compatibility (i.e. how Firefox and Opera behave).

No, I don't find this test from IE test Center.
FireFox 12 can't throw exception, it should be a bug.
Comment 4 David Levin 2012-06-05 21:41:20 PDT
Kinuko and/or Eric have much more current knowledge than myself (though neither are WK reviewers), but one of them would be best to comment on the change in general, imo.
Comment 5 Li Yin 2012-06-05 21:46:14 PDT
(In reply to comment #4)
> Kinuko and/or Eric have much more current knowledge than myself (though neither are WK reviewers), but one of them would be best to comment on the change in general, imo.

Okay, let's wait for their comments.
Comment 6 Kinuko Yasuda 2012-06-05 21:52:12 PDT
Comment on attachment 145523 [details]
Patch

The change looks good to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=145523&action=review

> Source/WebCore/fileapi/FileReader.cpp:136
>      // If multiple concurrent read methods are called on the same FileReader, OperationNotAllowedException should be thrown when the state is LOADING.

Please update the comment accordingly.
Comment 7 Li Yin 2012-06-05 22:01:38 PDT
Created attachment 145932 [details]
Patch
Comment 8 Li Yin 2012-06-05 22:02:51 PDT
(In reply to comment #6)
> (From update of attachment 145523 [details])
> The change looks good to me.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=145523&action=review
> 
> > Source/WebCore/fileapi/FileReader.cpp:136
> >      // If multiple concurrent read methods are called on the same FileReader, OperationNotAllowedException should be thrown when the state is LOADING.
> 
> Please update the comment accordingly.

Done. Thanks for your review.
Comment 9 Eric U. 2012-06-06 16:52:50 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Kinuko and/or Eric have much more current knowledge than myself (though neither are WK reviewers), but one of them would be best to comment on the change in general, imo.
> 
> Okay, let's wait for their comments.

Yep, looks fine to me.
Comment 10 Jian Li 2012-06-06 17:05:43 PDT
Comment on attachment 145932 [details]
Patch

It seems that we can remove OperationNotAllowedException completely since it was only used in multi-loading scenario and we throw a different DOM error now.
Comment 11 Li Yin 2012-06-06 17:54:00 PDT
(In reply to comment #10)
> (From update of attachment 145932 [details])
> It seems that we can remove OperationNotAllowedException completely since it was only used in multi-loading scenario and we throw a different DOM error now.

Okay, I will delete the related code.
Thanks for your review.
Comment 12 Li Yin 2012-06-06 18:32:19 PDT
Created attachment 146172 [details]
Patch
Comment 13 Li Yin 2012-06-06 18:36:05 PDT
Remove OperationNotAllowedException related code completely.
Would you please have a look?
Thanks.
Comment 14 Jian Li 2012-06-06 21:37:49 PDT
Comment on attachment 146172 [details]
Patch

Only a few minor comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=146172&action=review

> Source/WebCore/ChangeLog:11
> +        And delete OperationNotAllowedException related code, because it won't be used.

nit: Also delete ... because it is not longer needed.

> Source/WebCore/ChangeLog:27
> +        (WebCore::FileReader::readInternal):

Better to add some comment after each affected method is possible. For this one, you can add the following:
  Changed to throw INVALID_STATE_ERR.

> Source/WebCore/ChangeLog:28
> +        * fileapi/FileReader.idl:

Same for this one. You can add the following comment:
  Changed to raise DOMException for all read methods.
Comment 15 Li Yin 2012-06-06 21:47:35 PDT
Created attachment 146193 [details]
Patch
Comment 16 Li Yin 2012-06-06 21:48:44 PDT
(In reply to comment #14)
> (From update of attachment 146172 [details])
> Only a few minor comments.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=146172&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        And delete OperationNotAllowedException related code, because it won't be used.
> 
> nit: Also delete ... because it is not longer needed.
> 
> > Source/WebCore/ChangeLog:27
> > +        (WebCore::FileReader::readInternal):
> 
> Better to add some comment after each affected method is possible. For this one, you can add the following:
>   Changed to throw INVALID_STATE_ERR.
> 
> > Source/WebCore/ChangeLog:28
> > +        * fileapi/FileReader.idl:
> 
> Same for this one. You can add the following comment:
>   Changed to raise DOMException for all read methods.

Done.
Thanks for you comments.
Comment 17 WebKit Review Bot 2012-06-07 02:43:19 PDT
Comment on attachment 146193 [details]
Patch

Clearing flags on attachment: 146193

Committed r119698: <http://trac.webkit.org/changeset/119698>
Comment 18 WebKit Review Bot 2012-06-07 02:43:36 PDT
All reviewed patches have been landed.  Closing bug.