Closed Bug 1332592 Opened 4 years ago Closed 4 years ago

FileReader dispatch error events wrongly

Categories

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

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(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]
fr2.patch

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 amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ccb35efc96f
FileReader should dispatch onerror+onloadend when OOM occurs, r=smaug
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0191af5356e0
FileReader should dispatch onerror+onloadend when OOM occurs - text fixing, r=me CLOSED TREE
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47486c79362a
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]
fr2.patch

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]
fr2.patch

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.