Skip to content

/ servo

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.

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

Fixes related to File API #10873

Merged
merged 1 commit into from May 2, 2016
Merged

Fixes related to File API #10873

merged 1 commit into from May 2, 2016

Conversation

@izgzhen
Copy link
Contributor

izgzhen commented Apr 27, 2016 •
edited by larsbergstrom

Fixing problems I met when trying to resolve #10851, but most of changes here are simple fixes.

This change is Reviewable
@highfive
Copy link

highfive commented Apr 27, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/lib.rs, components/script/dom/webidls/HTMLInputElement.webidl, components/script/dom/file.rs, components/script/Cargo.toml, components/script/dom/htmlinputelement.rs, components/script/dom/blob.rs, components/script/dom/filelist.rs
@highfive
Copy link

highfive commented Apr 27, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2016

⌛ Trying commit da3d7ad with merge c0e10a2...
bors-servo added a commit that referenced this pull request Apr 27, 2016
@bors-servo
Auto merge of #10873 - izgzhen:blob, r=<try>
Loading status checks…
c0e10a2 
Fixing TODOs related to file input and File API

Fixing problems I met when trying to resolve #10851, but most of changes here are simple fixes.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2016

💔 Test failed - linux-dev
@izgzhen
Copy link
Contributor Author

izgzhen commented Apr 27, 2016

@Ms2ger Looks like the changed Cargo.lock triggered CI failure. How to deal with this?
@Ms2ger
Copy link
Contributor

Ms2ger commented Apr 27, 2016

It's not the one you changed, it's the ones you didn't change. ./mach cargo-update -p script should help.
@izgzhen izgzhen force-pushed the izgzhen:blob branch from da3d7ad to aba2203 Apr 27, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 29, 2016

☔ The latest upstream changes (presumably #10895) made this pull request unmergeable. Please resolve the merge conflicts.
@izgzhen izgzhen force-pushed the izgzhen:blob branch from a1b4771 to 95015cf Apr 29, 2016
@jdm jdm removed the S-needs-rebase label Apr 29, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2016

☔ The latest upstream changes (presumably #10929) made this pull request unmergeable. Please resolve the merge conflicts.
@izgzhen izgzhen force-pushed the izgzhen:blob branch from 95015cf to f1a5451 May 2, 2016
@izgzhen izgzhen changed the title Fixing TODOs related to file input and File API Fixes related to File API May 2, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented May 2, 2016

I simplified the content of this contrib, since my discussion with @Manishearth shows that there are some design decisions to be settled down later.

Welcome review.
components/script/dom/file.rs Outdated
@@ -18,14 +20,15 @@ pub struct File {
}

impl File {
fn new_inherited(_file_bits: &Blob, name: DOMString) -> File {
fn new_inherited(file_bits: &Blob, name: DOMString) -> File {
let mime_type = guess_mime_type(Path::new(&*name));

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth May 2, 2016

Member

Why are we guessing the type here? The spec doesn't mention this, the type is the empty string unless explicitly provided by the user through the FilePropertyBag/BlobPropertyBag.
@izgzhen
Copy link
Contributor Author

izgzhen commented May 2, 2016

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/file.rs, line 24 [r6] (raw file):
Good point! There was a "//TODO: get type from the underlying filesystem instead of "".to_string()" left in the initial version and I kind of get mislead by this.

This snippet should be used in the internal initial construction when the files are selected by user.

Comments from Reviewable
@izgzhen
Fixes related to file input and File API
Loading status checks…
e0f3cda
@izgzhen izgzhen force-pushed the izgzhen:blob branch from f1a5451 to e0f3cda May 2, 2016
@Manishearth
Copy link
Member

Manishearth commented May 2, 2016

@bors-servo r+

Reviewed 2 of 12 files at r6, 6 of 6 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/file.rs, line 24 [r6] (raw file):
Yeah, that was wrong 😄

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented May 2, 2016

📌 Commit e0f3cda has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented May 2, 2016

⌛ Testing commit e0f3cda with merge 1492652...
bors-servo added a commit that referenced this pull request May 2, 2016
@bors-servo
Auto merge of #10873 - izgzhen:blob, r=Manishearth
Loading status checks…
1492652 
Fixes related to File API

Fixing problems I met when trying to resolve #10851, but most of changes here are simple fixes.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10873)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 2, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit e0f3cda into servo:master May 2, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@izgzhen
Copy link
Contributor Author

izgzhen commented May 11, 2016

Ping issue #11131
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Add file handling functionality to Blob object
6 participants
@izgzhen @highfive @Ms2ger @bors-servo @Manishearth @jdm
You can’t perform that action at this time.