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

Implement file related functionalities in htmlinputelement and related #11225

Merged
merged 1 commit into from
May 23, 2016
Merged

Implement file related functionalities in htmlinputelement and related #11225

merged 1 commit into from
May 23, 2016

Conversation

izgzhen
Copy link
Contributor

@izgzhen izgzhen commented May 17, 2016 •
edited
Loading

  • ./mach build -d does not report any errors
  • ./mach test-tidy --faster does not report any errors
  • These changes is related to Implement File API and related support #11131
  • These changes do not require tests because it is a partial implementation

Major changes

  1. Improve the filemanager_thread by adding type string and create SelectedFile
  2. Fill several gaps in htmlinputelement implementation related to file type
  3. Improve the File interface to accommodate the above changes
  4. Integrate changes introduced by PR Refactor resource thread code #11189

This change is Reviewable

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/filemanager_thread.rs, components/script/dom/file.rs, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs, components/script/dom/htmlformelement.rs, components/script/dom/htmlinputelement.rs, components/script/dom/filelist.rs

@highfive
Copy link

warning Warning warning

  • These commits modify net and script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 17, 2016
@izgzhen
Copy link
Contributor Author

izgzhen commented May 17, 2016

@Manishearth yet another piece of code :P

Manishearth
Manishearth reviewed May 17, 2016
View reviewed changes
components/net/filemanager_thread.rs
(Ok(epoch), Some(filename)) => Some((id, Path::new(filename).to_path_buf(), epoch)),
(Ok(epoch), Some(filename)) => {
let filename_path = Path::new(filename);
let mime = guess_mime_type_opt(filename_path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be guessing the MIME here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://w3c.github.io/FileAPI/#dfn-type it said that:

User agents can also determine the type of a Blob, especially if the byte sequence is from an on-disk file

And also, I can't image other source of type instead of determining it at the time of reading.

@mbrubeck mbrubeck assigned Manishearth and unassigned pcwalton May 19, 2016
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #11270) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label May 19, 2016
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #11293) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label May 20, 2016
Manishearth
Manishearth reviewed May 20, 2016
View reviewed changes
components/net_traits/filemanager_thread.rs
pub filename: PathBuf,
pub modified: u64,
// https://w3c.github.io/FileAPI/#dfn-type
pub type_string: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for an option, type is "" if there is no type (according to the spec)

@Manishearth
Copy link
Member

@bors-servo delegate+

r=me after you remove that comment on the cast (feel free to replace it with a non-xxx comment explaining that the cast is okay)

@bors-servo
Copy link
Contributor

✌️ @izgzhen can now approve this pull request

@izgzhen
Implement file-type functionalities in htmlinputelement and related
dd590d0 
Changes include:
- Implement file selection and other DOM behaviours in htmlinputelement
- Integrate IpcSender<FileManagerThreadMsg> into ResourceThreads
- Improve filemanager_thread, including adding type_string field to SelectedFile
- Improve interfaces in FileList/File/Blob to accommodate the above changes
@izgzhen
Copy link
Contributor Author

izgzhen commented May 23, 2016 •
edited
Loading

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit dd590d0 has been approved by Manishearth

@highfive highfive 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-rebase There are merge conflict errors. labels May 23, 2016
@bors-servo
Copy link
Contributor

⌛ Testing commit dd590d0 with merge 7cea4eb...

bors-servo pushed a commit that referenced this pull request May 23, 2016
Auto merge of #11225 - izgzhen:patch-input-element-file, r=Manishearth
7cea4eb 
Implement file related functionalities in htmlinputelement and related

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy --faster` does not report any errors
- [x] These changes is related to #11131
- [x] These changes do not require tests because it is a partial implementation

1. Improve the `filemanager_thread` by adding type string and create `SelectedFile`
2. Fill several gaps in `htmlinputelement` implementation related to file type
3. Improve the `File` interface to accommodate the above changes
4. Integrate changes introduced by PR #11189

<!-- 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/11225)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ 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 dd590d0 into servo:master May 23, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 23, 2016
This was referenced May 23, 2016
Closed
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@izgzhen @highfive @bors-servo @Manishearth @nox @pcwalton