Implement FileReader #6172

[farodin91]

Finished

• readAsText

ToDo

• readAsArrayBuffer - currently it is impossible Spidermonkey Interfaces
• readAsDataUrl - I'm work on it
• readAsText, use blob type doesn't work
• error, attribute - I'm work on it

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5116

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[nox]

Thanks for the contribution @farodin91! Could you rebase and squash everything into a single commit (or multiple self-contained commits) to ease reviewing?

[nox]

Also, Travis complained:

1.69s$ ./mach test-tidy
components/script/dom/filereader.rs:46: (much) overlong line
components/script/dom/filereader.rs:97: trailing whitespace
components/script/dom/filereader.rs:118: (much) overlong line
components/script/dom/filereader.rs:160: trailing whitespace
components/script/dom/filereader.rs:163: trailing whitespace
components/script/dom/filereader.rs:164: trailing whitespace
components/script/dom/filereader.rs:180: trailing whitespace
components/script/dom/filereader.rs:217: trailing whitespace
components/script/dom/filereader.rs:268: trailing whitespace
components/script/dom/filereader.rs:284: trailing whitespace
components/script/dom/filereader.rs:349: trailing whitespace
components/script/dom/filereader.rs:430: trailing whitespace

[Manishearth]

https://reviewable.io/reviews/servo/servo/6205

[Manishearth]

Review status: 18 of 20 files reviewed, 7 unresolved discussions, all commit checks successful.
Reviewed files:

• components/script/dom/eventtarget.rs @ r1, r5
• components/script/dom/mod.rs @ r1
• components/script/dom/webidls/FileReader.webidl @ r11
• components/servo/Cargo.lock @ r12
• tests/wpt/metadata/FileAPI/blob/Blob-constructor.html.ini @ r7
• tests/wpt/metadata/FileAPI/blob/Blob-slice.html.ini @ r7
• tests/wpt/metadata/FileAPI/fileReader.html.ini @ r11
• tests/wpt/metadata/FileAPI/FileReader/Progress_event_bubbles_cancelable.html.ini @ r7
• tests/wpt/metadata/FileAPI/historical.html.ini @ r4
• tests/wpt/metadata/FileAPI/idlharness.html.ini @ r10
• tests/wpt/metadata/FileAPI/reading-data-section/Determining-Encoding.html.ini @ r10
• tests/wpt/metadata/FileAPI/reading-data-section/FileReader-event-handler-attributes.html.ini @ r1
• tests/wpt/metadata/FileAPI/reading-data-section/FileReader-multiple-reads.html.ini @ r11
• tests/wpt/metadata/FileAPI/reading-data-section/filereader_abort.html.ini @ r1
• tests/wpt/metadata/FileAPI/reading-data-section/filereader_error.html.ini @ r1
• tests/wpt/metadata/FileAPI/reading-data-section/filereader_readAsText.html.ini @ r1
• tests/wpt/metadata/FileAPI/reading-data-section/filereader_readystate.html.ini @ r11
• tests/wpt/metadata/FileAPI/reading-data-section/filereader_result.html.ini @ r11

---

^{components/script/dom/filereader.rs, line 154 [r13] (raw file):}
Nit: All methods here should have spec links, e.g. https://w3c.github.io/FileAPI/#dfn-readAsArrayBuffer

---

^{components/script/dom/filereader.rs, line 164 [r13] (raw file):}
Nit: Such comments should be on their own line

---

^{components/script/dom/filereader.rs, line 181 [r13] (raw file):}
Nit: ditto (comments), same for below

---

^{components/script/dom/filereader.rs, line 197 [r13] (raw file):}
nit: remove this

---

^{components/script/dom/webidls/FileReader.webidl, line 32 [r13] (raw file):}
The w3c spec mentions (DOMString or ArrayBuffer)?, and I can't find a whatwg spec. Are we looking at the same spec?

I'm looking at https://w3c.github.io/FileAPI/#APIASynch

---

^{components/script/dom/webidls/FileReader.webidl, line 35 [r13] (raw file):}
DOMException / DOMError ? (again, different specs?)

---

^{tests/wpt/metadata/FileAPI/fileReader.html.ini, line 2 [r13] (raw file):}
Shouldn't these files be deleted altogether?

How did you update the text expectations -- using update-wpt, or by hand?

(the format may have changed since I last had a close look so ICBW)

---

Comments from the review on Reviewable.io

[Manishearth]

This looks good! I left some comments on the review for some peripheral changes (If you don't understand what I meant by any one of them or have other questions; feel free to ask!). I'll get to the bulk of the review later today 😄

I'm a bit unsure of which spec we're looking at here, could you edit in spec links for all the methods?

[farodin91]

Review status: 18 of 20 files reviewed, 4 unresolved discussions, all commit checks successful.

---

^{components/script/dom/webidls/FileReader.webidl, line 32 [r13] (raw file):}
ArrayBuffer is SpiderMonkey Interface, that's support currently by codegen alg.

---

^{components/script/dom/webidls/FileReader.webidl, line 35 [r13] (raw file):}
DOMError will be replaced by DOMException http://www.w3.org/TR/domcore/#domexception

---

^{tests/wpt/metadata/FileAPI/fileReader.html.ini, line 2 [r13] (raw file):}
By hand

---

Comments from the review on Reviewable.io

[Manishearth]

Review status: 18 of 20 files reviewed, 1 unresolved discussion, all commit checks successful.

---

^{tests/wpt/metadata/FileAPI/fileReader.html.ini, line 2 [r13] (raw file):}
Okay, eventually you should do https://github.com/servo/servo/blob/master/tests/wpt/README.md#updating-test-expectations to update them automatically and to keep the diffs clean.

---

Comments from the review on Reviewable.io

[farodin91]

Review status: 18 of 20 files reviewed, 1 unresolved discussion, all commit checks successful.

---

^{tests/wpt/metadata/FileAPI/fileReader.html.ini, line 2 [r13] (raw file):}
i excute ./mach test-wpt and ./mach update-wpt, but nothing changed.

---

Comments from the review on Reviewable.io

[Manishearth]

You need to do it with --log-raw

If nothing changes then we're good 👍

[farodin91]

During the run crash by this reason glx::MakeCurrent failed #6042

---

Review status: 18 of 20 files reviewed, 1 unresolved discussion, all commit checks successful.

---

Comments from the review on Reviewable.io

[farodin91]

I forgot to add the log-raw

---

Review status: 18 of 20 files reviewed, 1 unresolved discussion, all commit checks successful.

---

Comments from the review on Reviewable.io

[farodin91]

I found a redirect to the correct spec site!

[farodin91]

Review status: 10 of 19 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/filereader.rs, line 58 [r31] (raw file):}
filereader.rs:433
error: the trait core::clone::Clone is not implemented for the type dom::filereader::BlobBody [E0277]
ProcessReadEOF(TrustedFileReader, GenerationId, Option)

---

^{components/script/dom/filereader.rs, line 228 [r31] (raw file):}
enc.decode can cause errors

---

^{components/script/dom/filereader.rs, line 241 [r31] (raw file):}
converting Vec to &[u8]

let (_, convert) = blob_body.bytes.split_at(0);

---

^{components/script/dom/filereader.rs, line 267 [r31] (raw file):}
I looked up in Firefox

---

^{components/script/dom/filereader.rs, line 481 [r31] (raw file):}
Thank You

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 2 files at r32.
Review status: 10 of 19 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{components/script/dom/filereader.rs, line 228 [r31] (raw file):}
I guess I was wrong, so let's make it return Result<(), DOMErrorName> and have the caller call process_read_error instead.

---

^{components/script/dom/filereader.rs, line 242 [r33] (raw file):}
Instead of this match, we can use

resultmime.and_then(|Mime(_, _, ref parameters)| {
  parameters.iter()
             .find(|&(ref k, _)| &Attr::Charset == k)
             .and_then(|&(_, ref v)| encoding_from_whatwg_label(&v.to_string()))
}

---

^{components/script/dom/filereader.rs, line 244 [r31] (raw file):}
This still needs to be addressed.

---

^{components/script/dom/filereader.rs, line 247 [r33] (raw file):}
This doesn't do anything with the return value.

---

Comments from the review on Reviewable.io

[jdm]

Reviewed 1 of 1 files at r34.
Review status: 11 of 19 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[farodin91]

Review status: 11 of 19 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{components/script/dom/filereader.rs, line 265 [r31] (raw file):}
is this right

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r35.
Review status: 11 of 19 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

---

^{components/script/dom/filereader.rs, line 246 [r35] (raw file):}
If we add .ok() here, we can avoid the conversion to Result and then back to Option below.

---

^{components/script/dom/filereader.rs, line 265 [r31] (raw file):}
Please pass DecoderTrap::Replacement instead of DecoderTrap::Strict. We can then call unwrap on the result and the following match is not needed.

---

Comments from the review on Reviewable.io

[jdm]

Great work! I think this is good enough to merge, and then I'll probably file some more issues to continue improving the readibility of the code. Thanks for sticking with this; go ahead and squash all of these commits together into one, please :)

-S-awaiting-review +S-needs-squash

---

Reviewed 1 of 1 files at r36.
Review status: 11 of 19 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[jdm]

Oh, and you'll need to modify http://mxr.mozilla.org/servo/source/tests/wpt/mozilla/tests/mozilla/interfaces.html?force=1 to allow the test to pass now.

[farodin91]

How i can easy squash commits

---

Review status: 11 of 19 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from the review on Reviewable.io

[jdm]

Squashed in #6716.