Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor FileReader #6762 #6763 #6750 #6751 #6807

Merged
merged 1 commit into from Jul 29, 2015
Merged

Refactor FileReader #6762 #6763 #6750 #6751 #6807

merged 1 commit into from Jul 29, 2015

Conversation

farodin91
Copy link
Contributor

Review on Reviewable

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 28, 2015
@highfive
Copy link

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.

@jdm
Copy link
Member

jdm commented Jul 28, 2015

Makes sense! Thanks!
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

components/script/dom/filereader.rs, line 221 [r1] (raw file):
This can just return DOMString.

components/script/dom/filereader.rs, line 254 [r1] (raw file):
This can just return DOMString.

components/script/dom/filereader.rs, line 328 [r1] (raw file):
nit: space after :

Comments from the review on Reviewable.io

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 28, 2015
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 28, 2015
@farodin91
Copy link
Contributor Author

We need to wait until #6723 is merged!

@farodin91 farodin91 changed the title Refactor FileReader #6762 #6763 #6750 #6751 Refactor #6808 #6762 #6763 #6750 #6751 Jul 28, 2015
@farodin91 farodin91 changed the title Refactor #6808 #6762 #6763 #6750 #6751 Refactor FileReader #6762 #6763 #6750 #6751 Jul 28, 2015
@jdm
Copy link
Member

jdm commented Jul 28, 2015

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

Reviewed 2 of 2 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

@jdm jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 28, 2015
@farodin91
Copy link
Contributor Author

I will squash this. if #6803 is merged!

But humo is slow!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 28, 2015
@farodin91 farodin91 force-pushed the filereader branch 2 times, most recently from ad039db to 4e284f1 Compare July 28, 2015 18:12
@jdm
Copy link
Member

jdm commented Jul 29, 2015

@bors-servo: r+

@bors-servo
Copy link
Contributor

📌 Commit 5921af4 has been approved by jdm

@jdm jdm added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Jul 29, 2015
@bors-servo
Copy link
Contributor

⌛ Testing commit 5921af4 with merge 52c2049...

bors-servo pushed a commit that referenced this pull request Jul 29, 2015
Auto merge of #6807 - farodin91:filereader, r=jdm
52c2049 
Refactor FileReader #6762 #6763 #6750 #6751



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6807)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit 5921af4 into servo:master Jul 29, 2015
@farodin91 farodin91 deleted the filereader branch July 29, 2015 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@farodin91 @highfive @jdm @bors-servo