Bug 88212 – [FileAPI] FileReader should throw an InvalidStateError exception when readyState is LOADING

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Li Yin]

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

[Li Yin]

Created attachment 145523 [details]
Patch

[Kentaro Hara]

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

[Li Yin]

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

[David Levin]

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.

[Li Yin]

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

[Kinuko Yasuda]

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.

[Li Yin]

Created attachment 145932 [details]
Patch

[Li Yin]

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

[Eric U.]

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

[Jian Li]

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.

[Li Yin]

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

[Li Yin]

Created attachment 146172 [details]
Patch

[Li Yin]

Remove OperationNotAllowedException related code completely.
Would you please have a look?
Thanks.

[Jian Li]

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.

[Li Yin]

Created attachment 146193 [details]
Patch

[Li Yin]

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

[WebKit Review Bot]

Comment on attachment 146193 [details]
Patch

Clearing flags on attachment: 146193

Committed r119698: <http://trac.webkit.org/changeset/119698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.