Implement read methods on FileReaderSync #19511

DonatJR · 2017-12-07T17:49:35Z

Implemented the read methods on the FileReaderSync struct according to https://w3c.github.io/FileAPI/#dfn-FileReaderSync

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15114

There are tests for these changes

This change is

highfive · 2017-12-07T17:49:41Z

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

highfive · 2017-12-07T17:49:42Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/filereadersync.rs, components/script/dom/webidls/FileReaderSync.webidl, components/script/dom/bindings/error.rs, components/script/dom/webidls/DOMException.webidl, components/script/dom/domexception.rs
@fitzgen: components/script/dom/filereadersync.rs, components/script/dom/webidls/FileReaderSync.webidl, components/script/dom/bindings/error.rs, components/script/dom/webidls/DOMException.webidl, components/script/dom/domexception.rs
@KiChjang: components/script/dom/filereadersync.rs, components/script/dom/webidls/FileReaderSync.webidl, components/script/dom/bindings/error.rs, components/script/dom/webidls/DOMException.webidl, components/script/dom/domexception.rs

highfive · 2017-12-07T17:49:43Z

Warning

These commits modify unsafe code. Please review it carefully!

DonatJR · 2017-12-07T21:24:17Z

I tried to update the manifest with ./mach update-manifest but this changed almost every entry in the files. What is the correct way to only change the entry for the changed test (/tests/wpt/web-platform-tests/FileAPI/FileReaderSync.worker.js) ?

jdm · 2017-12-08T14:50:29Z

That's the correct way to update it; what were the changes it tried to make?

DonatJR · 2017-12-08T18:53:43Z

manifests.zip
For tests/wpt/metadata/MANIFEST.json there are a few timeout changes in the WebCryptoAPI tests and a lot of changed hashes for the tests themselves.
For tests/wpt/mozilla/meta/MANIFEST.json there are only changes to the test hashes.
I atteched all 4 files so you can have a look at them (they also have different line endings - probably because I'm on Windows).

jdm

This is great! I think it should be possible to avoid duplicating some of the less straightforward parts of the implementation, though.

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+
+        // step 2
+        let blob_contents = blob.get_bytes().unwrap_or(vec![]);
+        if blob_contents.is_empty() {


What happens if we run a test like readerSync.readAsBinaryString(new Blob([""]));? Won't this incorrectly throw an exception?

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+
+        // step 2
+        let blob_contents = blob.get_bytes().unwrap_or(vec![]);
+        if blob_contents.is_empty() {


Same question as readAsBinaryString.

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+        let blob_type = String::from(blob.Type());
+
+        //https://w3c.github.io/FileAPI/#encoding-determination
+        let mut encoding = blob_label.as_ref()


Could we share this code with filereader.rs instead of duplicating it?

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+
+        // step 2
+        let blob_contents = blob.get_bytes().unwrap_or(vec![]);
+        if blob_contents.is_empty() {


Same question as other methods.

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+        }
+
+        // step 3
+        let base64 = base64::encode(&blob_contents);


Let's share this code with filereader.rs.

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

+
+        // step 2
+        let blob_contents = blob.get_bytes().unwrap_or(vec![]);
+        if blob_contents.is_empty() {


Same question as other methods.

jdm · 2018-01-29T23:20:53Z

tests/wpt/web-platform-tests/FileAPI/FileReaderSync.worker.js

+  var data = readerSync.readAsBinaryString(blob);
+  assert_equals(data, "test");
+}, "readAsBinaryString");
+


Let's add tests with empty blobs in this file as well.

jdm · 2018-01-29T23:20:53Z

components/script/dom/filereadersync.rs

 }

 impl FileReaderSync {
    pub fn new_inherited() -> FileReaderSync {
        FileReaderSync {
            eventtarget: EventTarget::new_inherited(),
+            ready_state: Cell::new(FileReaderReadyState::Empty),


Since the spec doesn't actually tell us to modify the readyState for synchronous file readers, nor does it describe a way where the LOADING check could actually succeed, let's remove it from our implementation.

I've filed w3c/FileAPI#96 against the spec.

jdm · 2018-01-29T23:21:30Z

Sorry about the unnecessary delay before this code was reviewed. I promise shorter review cycles if you want to keep working on this!

DonatJR · 2018-01-30T17:39:56Z

@jdm No worries about the delay, I'm rather short on time myself right now, so I probably wouldn't have been able to work on this any sooner anyway. That said, I do want to keep working on it! :)

DonatJR · 2018-05-23T17:41:16Z

Now I'm the one having to apologize for taking SO long to update this...
I adressed your comments but I'm not completely happy with my solution for sharing the code between FileReader and FileReaderSync - if you have any suggestions on how to improve it I'd be happy to make the necessary changes.

I am also still unable to properly update the manifest files for the updated tests (same issues as described in an earlier comment)

servo-wpt-sync · 2018-05-23T17:41:18Z

Error syncing changes upstream. Logs saved in error-snapshot-1527098417542.

servo-wpt-sync · 2018-05-23T18:58:41Z

Error syncing changes upstream. Logs saved in error-snapshot-1527103060139.

jdm

Great work! For sharing code, it's a toss-up as to whether it's worth creating static methods on a type like this PR does, or using free functions. I'm not bothered either way. Please squash all the commits together, too!

jdm · 2018-05-29T16:03:15Z

components/script/dom/filereader.rs

+        result: &DomRefCell<Option<FileReaderResult>>,
+        data: ReadMetaData,
+        blob_bytes: &[u8],
+    ) {
        let blob_label = &data.label;
        let blob_type = &data.blobtype;

        //https://w3c.github.io/FileAPI/#encoding-determination


This comment should be moved into the text_decode function.

Its in text_decode (link to line) as well, do you want me to remove it here anyway?

Ah, my mistake. You can just remove it from this location, in that case.

DonatJR · 2018-06-03T21:33:37Z

This is the first time I'm attempting to squash commits together, so I just wanted to be sure if I'm doing it correctly. I searched the log for the commit hash that came right before my first change, ran git rebase -i <hash> and replaced pick with squash for all commits except the first. So far this seems to be easy enough, but because of the fact that I merged the master branch back into this one to resolve conflicts the rebase is now also processing these merged commits. Is this correct or should I have only picked my commits from this PR in the list at the beginning of the rebase?

Thanks for your patience and help :)

KiChjang · 2018-06-03T21:48:33Z

@DonatJR What happens if you do git rebase origin master first before you squash?

DonatJR · 2018-06-03T22:49:15Z

@KiChjang git rebase origin master says that master is already up to date:

Current branch master is up to date.

And it changes nothing about the number of commits when squashing.

KiChjang · 2018-06-03T23:01:50Z

@DonatJR Oh sorry, I meant git rebase origin/master.

DonatJR · 2018-06-03T23:11:24Z

With git rebase origin/master my commits from this PR are getting rebased:

First, rewinding head to replay your work on top of it...
Applying: implemented readAsText, readAsBinaryString and readAsDataUrl on FileReaderSync
...

But again, no changes to before when squashing.

jdm · 2018-06-04T13:18:49Z

It's probably easiest to make a new branch based off of master and cherry-pick all of the non-merge commits like:

git cherry-pick b290a5b
git cherry-pick a343a77
git cherry-pick 90db19a
git cherry-pick e2abae0
git cherry-pick 7b7b3d1
git cherry-pick 5550449
git cherry-pick d0a5e2b
git cherry-pick 00700fa
git cherry-pick a0748c3
git cherry-pick 8ffa7a4
git cherry-pick 371c770

Then you can do git push [fork] [newbranchname]:implement-read-methods-on-filereadersync -f to overwrite the changes in this PR, where [fork] is the remote pointing at your github fork, and [newbranchname] is the name of the new branch you created locally.

bors-servo · 2018-06-04T18:43:28Z

⌛ Testing commit 21bf9c3 with merge 49b2367...


        Auto merge of #19511 - DonatJR:implement-read-methods-on-filereadersy…

…nc, r=jdm Implement read methods on FileReaderSync  Implemented the read methods on the FileReaderSync struct according to https://w3c.github.io/FileAPI/#dfn-FileReaderSync ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15114  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19511)

bors-servo · 2018-06-04T19:00:51Z

💔 Test failed - android

jdm · 2018-06-04T19:08:28Z

 0:24.76 INFO Diffing old and new manifests /Users/servo/buildbot/slave/mac-dev-unit/build/tests/wpt/mozilla/meta/MANIFEST.json
 0:25.38 INFO Diffing old and new manifests /Users/servo/buildbot/slave/mac-dev-unit/build/tests/wpt/metadata/MANIFEST.json
 0:29.87 WARNING Manifest /Users/servo/buildbot/slave/mac-dev-unit/build/tests/wpt/metadata/MANIFEST.json contains correct tests but file hashes changed.
 0:29.95 ERROR Manifest /Users/servo/buildbot/slave/mac-dev-unit/build/tests/wpt/metadata/MANIFEST.json is outdated, use |./mach update-manifest| to fix.

DonatJR · 2018-06-04T19:28:16Z

This is because of the issues I talked about earlier in the PR (here, here and here). I am not sure how to correctly generate the manifests, ./mach update-manifest changes a lot of hashes (among some other changes) for tests I never touched. :(

jdm · 2018-06-04T19:46:25Z

Oh, right. Yeah, that's super weird, and I don't have any good suggestions. I'll make the change locally and update the PR.


        Implement read methods on FileReaderSync

jdm · 2018-06-04T19:53:39Z

@bors-servo r+

bors-servo · 2018-06-04T19:53:40Z

📌 Commit 0fdafb0 has been approved by jdm

servo-wpt-sync · 2018-06-04T19:53:53Z

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at jdm/web-platform-tests#65.

bors-servo · 2018-06-04T19:55:16Z

⌛ Testing commit 0fdafb0 with merge 652a177...


        Auto merge of #19511 - DonatJR:implement-read-methods-on-filereadersy…

…nc, r=jdm Implement read methods on FileReaderSync  Implemented the read methods on the FileReaderSync struct according to https://w3c.github.io/FileAPI/#dfn-FileReaderSync ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #15114  - [x] There are tests for these changes    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19511)

bors-servo · 2018-06-04T20:33:48Z

💔 Test failed - mac-rel-css2

jdm · 2018-06-04T21:00:51Z

@bors-servo retry
#20917

bors-servo · 2018-06-04T21:27:30Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-rel-css2...

bors-servo · 2018-06-04T22:10:44Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 652a177 to master...


        Implement read methods on FileReaderSync

Upstreamed from servo/servo#19511 [ci skip]

highfive assigned wafflespeanut Dec 7, 2017

highfive added the S-awaiting-review label Dec 7, 2017

jdm assigned jdm and unassigned wafflespeanut Jan 29, 2018

jdm requested changes Jan 29, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 29, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels May 23, 2018

jdm requested changes May 29, 2018

View changes

jdm added S-needs-squash and removed S-awaiting-review labels May 29, 2018

DonatJR force-pushed the DonatJR:implement-read-methods-on-filereadersync branch from 371c770 to 21bf9c3 Jun 4, 2018

highfive added S-awaiting-merge and removed S-needs-squash labels Jun 4, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 4, 2018

Implement read methods on FileReaderSync

Loading status checks…

0fdafb0

jdm force-pushed the DonatJR:implement-read-methods-on-filereadersync branch from 21bf9c3 to 0fdafb0 Jun 4, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Jun 4, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jun 4, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 4, 2018

bors-servo merged commit 0fdafb0 into servo:master Jun 4, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2018

Implement read methods on FileReaderSync

320e4fd

Upstreamed from servo/servo#19511 [ci skip]

servo-wpt-sync mentioned this pull request Jun 7, 2018

Implement read methods on FileReaderSync web-platform-tests/wpt#11412

Merged

servo / servo

Join GitHub today

Implement read methods on FileReaderSync #19511

Implement read methods on FileReaderSync #19511

Conversation

DonatJR commented Dec 7, 2017 • edited by jdm

highfive commented Dec 7, 2017

highfive commented Dec 7, 2017

highfive commented Dec 7, 2017

DonatJR commented Dec 7, 2017

jdm commented Dec 8, 2017

DonatJR commented Dec 8, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jan 29, 2018

DonatJR commented Jan 30, 2018

DonatJR commented May 23, 2018

servo-wpt-sync commented May 23, 2018

servo-wpt-sync commented May 23, 2018

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DonatJR commented Jun 3, 2018

KiChjang commented Jun 3, 2018

DonatJR commented Jun 3, 2018

KiChjang commented Jun 3, 2018

DonatJR commented Jun 3, 2018

jdm commented Jun 4, 2018

bors-servo commented Jun 4, 2018

bors-servo commented Jun 4, 2018

jdm commented Jun 4, 2018

DonatJR commented Jun 4, 2018 • edited

jdm commented Jun 4, 2018

jdm commented Jun 4, 2018

bors-servo commented Jun 4, 2018

servo-wpt-sync commented Jun 4, 2018

bors-servo commented Jun 4, 2018

bors-servo commented Jun 4, 2018

jdm commented Jun 4, 2018

bors-servo commented Jun 4, 2018

bors-servo commented Jun 4, 2018

DonatJR commented Dec 7, 2017 •

edited by jdm

DonatJR commented Jun 4, 2018 •

edited