Add FileManagerThread #11029

izgzhen · 2016-05-05T12:17:07Z

This is intended to support the File API implementation. Basically an event loop with three kinds of messages:

Select a file
Read a file with ID
Delete the ID from manager-owned map

The design decision in this PR is not the final (or best I think) version, welcome reviews :)

TODOs:

Add multiple file selection

This change is

highfive · 2016-05-05T12:17:11Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/filemanager_thread.rs, components/net/lib.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml, components/net_traits/filemanager_thread.rs, components/net_traits/filemanager_thread.rs

highfive · 2016-05-05T12:17:12Z

Warning

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

izgzhen · 2016-05-05T12:23:36Z

@Manishearth.
Related: #10851

Manishearth · 2016-05-05T14:28:41Z

components/net/filemanager_thread.rs

+                                              .and_then(|systime| systime.elapsed().map_err(|_| ()))
+                                              .and_then(|elapsed| Ok(elapsed.as_secs()));
+
+                let filename = path.file_name().and_then(|filename| filename.to_str())


We should probably store this as a path, filenames aren't necessarily utf8.

The problem is that both Path and OsStr don't implement the Serialize trait.

PathBuf has an impl. Path/OsStr are slices.

If that doesn't work, just use a byte vector and work on getting an impl upstream in serde.

OK, I will see if I can make OsString and PathBuf serializable in upstream.

Manishearth · 2016-05-05T14:35:59Z

components/net/filemanager_thread.rs

+        };
+    }
+
+    fn read_file(&mut self, sender: IpcSender<FileManagerResult<Vec<u8>>>, id: Uuid) {


This should probably send chunks. Not necessary right now, but eventually.

Manishearth · 2016-05-05T15:15:21Z

LGTM with the path change.

highfive · 2016-05-05T16:07:28Z

New code was committed to pull request.

highfive · 2016-05-06T00:58:55Z

New code was committed to pull request.

highfive · 2016-05-06T03:08:57Z

New code was committed to pull request.

highfive · 2016-05-06T04:38:46Z

New code was committed to pull request.

highfive · 2016-05-08T05:52:25Z

New code was committed to pull request.

Manishearth · 2016-05-11T06:15:38Z

components/net/filemanager_thread.rs

+
+pub struct FileManager {
+    receiver: IpcReceiver<FileManagerThreadMsg>,
+    idmap: RefCell<HashMap<Uuid, String>>,


String -> PathBuf?

Currently, the libnfd returns a String in this way: CString::from_raw(*out_path).into_string(). Maybe there is encoding checking stuff. I will take a look at this.

I suspect we should convert from CString to OSString to Path directly.

Looks like a vital piece of conversion is missing: https://doc.servo.org/src/std/up/src/libstd/ffi/os_str.rs.html#59

Manishearth · 2016-05-11T06:16:56Z

components/net/filemanager_thread.rs

+impl FileManager {
+    fn select(&mut self, sender: SelectedPort) {
+        // TODO: Pull the dialog UI in and get selected
+        match sender {


I sort of feel that this would be better as two functions with no SelectedPort, and have the dialog code take an argument specifying single or multiple.

Do you mean split FileManager::select into two?

Manishearth · 2016-05-11T06:23:24Z

components/net/filemanager_thread.rs

+                self.idmap.borrow_mut().insert(id, file_path.to_string());
+
+                // Unix Epoch: https://doc.servo.org/std/time/constant.UNIX_EPOCH.html
+                let epoch = handler.metadata().and_then(|metadata| metadata.modified()).map_err(|_| ())


probably should leave a note about mtime vs ctime, iirc we haven't made that decision yet

Manishearth · 2016-05-11T06:27:03Z

components/net/filemanager_thread.rs

+        }
+    }
+
+    fn create_entry(&mut self, file_path: &str) -> Option<(Uuid, PathBuf, u64)> {


file_path should be Path or perhaps PathBuf

Manishearth · 2016-05-11T06:28:10Z

components/net/filemanager_thread.rs

+        };
+    }
+
+    fn delete_fileid(&mut self, id: Uuid) {


delete_file

I thought about this name before as well, and I doubt if this name is kind of misleading?

Nah, just consistency. This isn't really important, so feel free to keep it as is if you like this name.


        add filemanager_thread

highfive · 2016-05-11T07:14:58Z

New code was committed to pull request.

Manishearth · 2016-05-11T07:16:43Z

@bors r+

KiChjang · 2016-05-11T11:23:23Z

@bors-servo r=Manishearth

bors-servo · 2016-05-11T11:23:23Z

📌 Commit c618ee2 has been approved by Manishearth

bors-servo · 2016-05-11T11:29:00Z

⌛ Testing commit c618ee2 with merge b61ad41...


        Auto merge of #11029 - izgzhen:filemanager_thread, r=Manishearth

Add FileManagerThread This is intended to support the File API implementation. Basically an event loop with three kinds of messages: + Select a file + Read a file with ID + Delete the ID from manager-owned map The design decision in this PR is not the final (or best I think) version, welcome reviews :) TODOs: - [x] Add multiple file selection  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11029)

bors-servo · 2016-05-11T12:34:05Z

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

izgzhen · 2016-05-11T15:15:45Z

Ping issue #11131

highfive assigned pcwalton May 5, 2016

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

Manishearth assigned Manishearth and unassigned pcwalton May 5, 2016

Manishearth reviewed May 5, 2016
View changes

components/net/filemanager_thread.rs Outdated

};

}

fn read_file(&mut self, sender: IpcSender<FileManagerResult<Vec<u8>>>, id: Uuid) {

This comment has been minimized.

Sign in to view

Manishearth May 5, 2016
Member

This should probably send chunks. Not necessary right now, but eventually.

jdm added S-needs-code-changes and removed S-awaiting-review labels May 5, 2016

izgzhen force-pushed the izgzhen:filemanager_thread branch from c2add7e to de4d077 May 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels May 5, 2016

izgzhen force-pushed the izgzhen:filemanager_thread branch from de4d077 to 5c212bd May 6, 2016

izgzhen force-pushed the izgzhen:filemanager_thread branch from 5c212bd to 4f76350 May 6, 2016

izgzhen force-pushed the izgzhen:filemanager_thread branch from 4f76350 to 06edf6d May 6, 2016

izgzhen force-pushed the izgzhen:filemanager_thread branch from 06edf6d to fcc8be4 May 8, 2016

Manishearth reviewed May 11, 2016
View changes

components/net/filemanager_thread.rs Outdated

}

}

fn create_entry(&mut self, file_path: &str) -> Option<(Uuid, PathBuf, u64)> {

This comment has been minimized.

Sign in to view

Manishearth May 11, 2016
Member

file_path should be Path or perhaps PathBuf

Manishearth reviewed May 11, 2016
View changes

add filemanager_thread

Loading status checks…

c618ee2

izgzhen force-pushed the izgzhen:filemanager_thread branch from fcc8be4 to c618ee2 May 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 11, 2016

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

servo / servo

Add FileManagerThread #11029

Add FileManagerThread #11029

izgzhen commented May 5, 2016 •

edited

highfive commented May 5, 2016

highfive commented May 5, 2016

izgzhen commented May 5, 2016

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 5, 2016

highfive commented May 5, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 8, 2016

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.

highfive commented May 11, 2016

Manishearth commented May 11, 2016

KiChjang commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

izgzhen commented May 11, 2016

servo / servo

Join GitHub today

Add FileManagerThread #11029

Add FileManagerThread #11029

Conversation

izgzhen commented May 5, 2016 • edited

highfive commented May 5, 2016

highfive commented May 5, 2016

izgzhen commented May 5, 2016

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 5, 2016

highfive commented May 5, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 8, 2016

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.

highfive commented May 11, 2016

Manishearth commented May 11, 2016

KiChjang commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 11, 2016

izgzhen commented May 11, 2016

izgzhen commented May 5, 2016 •

edited