Implement file related functionalities in htmlinputelement and related #11225

izgzhen · 2016-05-17T10:55:53Z

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

Major changes

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

This change is

highfive · 2016-05-17T10:55:57Z

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 · 2016-05-17T10:55:58Z

Warning

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

izgzhen · 2016-05-17T10:56:31Z

@Manishearth yet another piece of code :P

Manishearth · 2016-05-17T22:20:55Z

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);


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

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.

Manishearth · 2016-05-17T22:21:50Z

components/script/dom/htmlinputelement.rs

-                DOMString::from("")
+                let default = DOMString::from("");
+                match *self.filelist.borrow() {
+                    Some(ref fl) => match fl.Item(0) {


This doesn't use fakepath?

Ahhh it my fault. I thought the comment doesn't mean fakepath as it is .... http://www.w3.org/TR/html5/forms.html#dom-input-value-filename

bors-servo · 2016-05-19T20:33:01Z

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

bors-servo · 2016-05-20T14:20:36Z

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

Manishearth · 2016-05-20T14:21:48Z

components/net_traits/filemanager_thread.rs

+    pub filename: PathBuf,
+    pub modified: u64,
+    // https://w3c.github.io/FileAPI/#dfn-type
+    pub type_string: Option<String>,


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

Manishearth · 2016-05-20T14:49:10Z

components/script/dom/file.rs

+        let slice = DataSlice::empty();
+
+        let global = GlobalRef::Window(window);
+        // XXX: as i64? looks like DOM codegen and std::fs don't agree on this type


It's only one bit of difference in capacity. Though I'm not sure why the DOM has a signed long long there (bug?)

Should we file an issue on this?

On the spec? Sure. https://github.com/whatwg/html. Might be worth investigating what the type is in the kernel first and whether a negative mtime is useful.

Windows: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724284(v=vs.85).aspx
Linux: http://stackoverflow.com/questions/471248/what-is-ultimately-a-time-t-typedef-to

Ok, maybe we should left this as it is :(

Seems like it won't matter since Date breaks down at i64::max_value anyway. If the cast breaks anything, it will be a billion years from now :p

I guess we can remove this xxx?

Manishearth · 2016-05-20T14:51:33Z

components/script/dom/filelist.rs

-    pub fn new(window: &Window, files: Vec<JS<File>>) -> Root<FileList> {
-        reflect_dom_object(box FileList::new_inherited(files), GlobalRef::Window(window), FileListBinding::Wrap)
+    pub fn new(window: &Window, files: Vec<Root<File>>) -> Root<FileList> {
+        reflect_dom_object(box FileList::new_inherited(files.iter().map(|r| JS::from_rooted(&r)).collect()),


The rooting here seems sketchy; but I think it should work. @nox, sanity-check?

Should work, since files still root them.

Manishearth · 2016-05-20T14:55:00Z

components/script/dom/htmlinputelement.rs

+                match *self.filelist.borrow() {
+                    Some(ref fl) => match fl.Item(0) {
+                        Some(ref f) => {
+                            path.push_str("C:\\fakepath\\");


Interestingly, firefox doesn't seem to follow this. But we'll implement the spec and perhaps file a firefox or spec bug later.

Manishearth · 2016-05-20T14:58:23Z

components/script/dom/htmlinputelement.rs

@@ -81,7 +86,7 @@ pub struct HTMLInputElement {
    // https://html.spec.whatwg.org/multipage/#concept-input-value-dirty-flag
    value_dirty: Cell<bool>,

-    // TODO: selected files for file input
+    filelist: DOMRefCell<Option<JS<FileList>>>,


MutNullableHeap?

Not sure if this needs to be Option in the first place, really. Helps avoid unnecessary heap allocation, but there may be more holistic solutions for this.

By "holistic" you mean?

The Option lets us avoid allocating extra JavaScript objects in this specific case. A more holistic/general solution would have some form of reflector lazy loading so that JS isn't hit as much. We have discussed this in the past though there isn't anything concrete yet. But I'd prefer a general solution over a bunch of specific ones.

Having read doc on MutNullableHeap which says that it is safer. I mimicked the other fields in HTMLInputElement so I initially wrote the wrapper as DOMRefCell. MutNullableHeap looks like a primitive interface provided by jsapi.

Oh, I see, it's an option because not all text inputs are file inputs, my bad.

Yeah, just use mutnullableheap here. It's not a primitive interface, but it's basically RefCell<Option<T>> where T is JS<U>. It's using DOMRefCell above because TextInput isn't a JS thing.

Manishearth · 2016-05-20T14:59:09Z

components/script/dom/htmlinputelement.rs

+                    let msg = FileManagerThreadMsg::SelectFiles(chan);
+                    let _ = filemanager.send(msg).unwrap();
+
+                    match recv.recv().unwrap() {


Manishearth · 2016-05-20T14:59:28Z

components/script/dom/htmlinputelement.rs

+                        Err(err) => panic!("Input file select error: {:?}", err)
+                    };
+                } else {
+                    let (chan, recv) = ipc::channel().unwrap();


izgzhen · 2016-05-20T16:58:51Z

components/script/dom/htmlinputelement.rs

+                            }
+                        },
+                        // XXX: I don't think we should panic here, but how to handle better?
+                        Err(err) => panic!("Input file select error: {:?}", err)


@Manishearth don't know how to handle this properly without panicking ... any suggestion?

Treat it as if no files were selected. This will probably happen if file upload was canceled.

Actually, treat it as a cancel. The difference is that as a cancel, the existing file list will not be cleared.

Good. I didn't see spec mentioning error handling so I am wondering if this is the so-called "implementation details"

Yeah, it is, basically

Manishearth · 2016-05-23T07:25:35Z

@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 · 2016-05-23T07:25:37Z

✌️ @izgzhen can now approve this pull request


        Implement file-type functionalities in htmlinputelement and related

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 · 2016-05-23T07:38:00Z

@bors-servo r=Manishearth

bors-servo · 2016-05-23T07:38:09Z

📌 Commit dd590d0 has been approved by Manishearth

bors-servo · 2016-05-23T08:10:48Z

⌛ Testing commit dd590d0 with merge 7cea4eb...


        Auto merge of #11225 - izgzhen:patch-input-element-file, r=Manishearth

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11225)

bors-servo · 2016-05-23T09:14:31Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned pcwalton May 17, 2016

highfive added the S-awaiting-review label May 17, 2016

Manishearth reviewed May 17, 2016
View changes

mbrubeck assigned Manishearth and unassigned pcwalton May 19, 2016

highfive added the S-needs-rebase label May 19, 2016

izgzhen force-pushed the izgzhen:patch-input-element-file branch 2 times, most recently from 24ccc71 to 38832c2 May 20, 2016

highfive added the S-needs-rebase label May 20, 2016

Manishearth reviewed May 20, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

let msg = FileManagerThreadMsg::SelectFiles(chan);

let _ = filemanager.send(msg).unwrap();

match recv.recv().unwrap() {

This comment has been minimized.

Sign in to view

Manishearth May 20, 2016
Member

expect

Manishearth reviewed May 20, 2016
View changes

components/script/dom/htmlinputelement.rs Outdated

Err(err) => panic!("Input file select error: {:?}", err)

};

} else {

let (chan, recv) = ipc::channel().unwrap();

This comment has been minimized.

Sign in to view

Manishearth May 20, 2016
Member

expect

izgzhen force-pushed the izgzhen:patch-input-element-file branch from 38832c2 to bfd1c45 May 20, 2016

izgzhen reviewed May 20, 2016
View changes

izgzhen force-pushed the izgzhen:patch-input-element-file branch from bfd1c45 to 6e74884 May 21, 2016

izgzhen force-pushed the izgzhen:patch-input-element-file branch from 6e74884 to dd590d0 May 23, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels May 23, 2016

bors-servo merged commit dd590d0 into servo:master May 23, 2016
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

highfive removed the S-awaiting-merge label May 23, 2016

This was referenced May 23, 2016

CSSRule, CSSRuleList, CSSStyleSheet partial implementation #10765

Closed

Move DOMString back to script #11326

Merged

implement related service worker interface and register method #11114

Merged

Add file backend support for Blob and related #11221

Merged

servo / servo

Join GitHub today

Implement file related functionalities in htmlinputelement and related #11225

Implement file related functionalities in htmlinputelement and related #11225

Conversation

izgzhen commented May 17, 2016 • edited

Major changes

highfive commented May 17, 2016

highfive commented May 17, 2016

izgzhen commented May 17, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 19, 2016

bors-servo commented May 20, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth May 20, 2016 • edited

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

izgzhen May 21, 2016 • edited

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.

Manishearth commented May 23, 2016

bors-servo commented May 23, 2016

izgzhen commented May 23, 2016 • edited

bors-servo commented May 23, 2016

bors-servo commented May 23, 2016

bors-servo commented May 23, 2016

izgzhen commented May 17, 2016 •

edited

Manishearth May 20, 2016 •

edited

izgzhen May 21, 2016 •

edited

izgzhen commented May 23, 2016 •

edited