Closed Bug 1332592 Opened 4 years ago Closed 4 years ago

FileReader dispatch error events wrongly


(Core :: DOM: Core & HTML, defect)

49 Branch
Not set



Tracking Status
firefox52 --- fixed
firefox53 --- fixed


(Reporter: baku, Assigned: baku)



(1 file)

No description provided.
Attached patch fr2.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8828756 - Flags: review?(bugs)
Comment on attachment 8828756 [details] [diff] [review]

Want to explain this a bit? No test changes or anything, so I guess I need to read the spec.
This is not about the spec, it's about the fact that:

1. FileReader::DoOnLoadEnd() doesn't set aSuccessEvent and aTerminationEvent before returning errors. But still those strings (empty) are used for dispatching events.
2. Why do we have aSuccessEvent and aTerminationEvent in the first place? They are always set to the same value.
3. If FileReader::DoOnLoadEnd() returns an error, we don't dispatch the error event.
Ok, FileReader::DoOnLoadEnd() used to be saner, but then over time got regressed, at least by
Bug 507805, and partially also Bug 1198095.
Attachment #8828756 - Flags: review?(bugs) → review+
Pushed by
FileReader should dispatch onerror+onloadend when OOM occurs, r=smaug
Pushed by
FileReader should dispatch onerror+onloadend when OOM occurs - text fixing, r=me CLOSED TREE
Pushed by
FileReader should dispatch onerror+onloadend when OOM occurs - text fixing, r=me
Andrea, I uplifted this to Beta with jcristau's blessing because bug 1332602 was hitting conflicts otherwise. Please request Beta approval on this retroactively so we can at least have a record of it. Or yell at me about what a terrible idea it was if that's the case :)
Flags: needinfo?(amarchesini)
Comment on attachment 8828756 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1198095.
[User impact if declined]: some events are not correctly dispatched when FileReader is used
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no, there is a mochitest
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk.
[Why is the change risky/not risky?]: There is just a better event dispatching algorithm.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8828756 - Flags: approval-mozilla-beta?
Comment on attachment 8828756 [details] [diff] [review]

after-the-fact-approval for beta :)
Attachment #8828756 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.