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".
Created attachment 145523 [details] Patch
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).
(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.
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.
(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 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.
Created attachment 145932 [details] Patch
(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.
(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 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.
(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.
Created attachment 146172 [details] Patch
Remove OperationNotAllowedException related code completely. Would you please have a look? Thanks.
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.
Created attachment 146193 [details] Patch
(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 on attachment 146193 [details] Patch Clearing flags on attachment: 146193 Committed r119698: <http://trac.webkit.org/changeset/119698>
All reviewed patches have been landed. Closing bug.