servo: Merge #12897 - Improve File API related comments (from izgzhen:improve-file-api-comments); r=Manishearth
authorZhen Zhang <izgzhen@gmail.com>
Wed, 17 Aug 2016 06:38:34 -0500
changeset 339510 27e7e5cc53f38a4b9724f422f7db4ac64e52f1ec
parent 339509 a95e7913c1fcfc17faa0ed79b027f812a369141c
child 339511 0c19f00167c2bbee9696e75137985709dead7872
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersManishearth
servo: Merge #12897 - Improve File API related comments (from izgzhen:improve-file-api-comments); r=Manishearth r? @Manishearth --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors Source-Repo: https://github.com/servo/servo Source-Revision: f4f212fd18290eab51eaac972335cec60d2a34e0
servo/components/net/filemanager_thread.rs
servo/components/net_traits/filemanager_thread.rs
servo/components/script/dom/blob.rs
--- a/servo/components/net/filemanager_thread.rs
+++ b/servo/components/net/filemanager_thread.rs
@@ -18,17 +18,17 @@ use std::sync::atomic::{self, AtomicUsiz
 use std::sync::{Arc, RwLock};
 #[cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))]
 use tinyfiledialogs;
 use url::Url;
 use util::prefs::PREFS;
 use util::thread::spawn_named;
 use uuid::Uuid;
 
-/// Trait that provider of file-dialog UI should implement.
+/// The provider of file-dialog UI should implement this trait.
 /// It will be used to initialize a generic FileManager.
 /// For example, we can choose a dummy UI for testing purpose.
 pub trait UIProvider where Self: Sync {
     fn open_file_dialog(&self, path: &str, patterns: Vec<FilterPattern>) -> Option<String>;
 
     fn open_file_dialog_multi(&self, path: &str, patterns: Vec<FilterPattern>) -> Option<Vec<String>>;
 }
 
@@ -77,18 +77,20 @@ impl UIProvider for TFDProvider {
 }
 
 /// FileManagerStore's entry
 struct FileStoreEntry {
     /// Origin of the entry's "creator"
     origin: FileOrigin,
     /// Backend implementation
     file_impl: FileImpl,
-    /// Number of `FileImpl::Sliced` entries in `FileManagerStore`
-    /// that has a reference (FileID) to this entry
+    /// Number of FileID holders that the ID is used to
+    /// index this entry in `FileManagerStore`.
+    /// Reference holders include a FileStoreEntry or
+    /// a script-side File-based Blob
     refs: AtomicUsize,
     /// UUIDs only become valid blob URIs when explicitly requested
     /// by the user with createObjectURL. Validity can be revoked as well.
     /// (The UUID is the one that maps to this entry in `FileManagerStore`)
     is_valid_url: AtomicBool
 }
 
 #[derive(Clone)]
@@ -152,28 +154,25 @@ impl<UI: 'static + UIProvider> FileManag
             FileManagerThreadMsg::AddSlicedURLEntry(id, rel_pos, sender, origin) =>{
                 spawn_named("add sliced URL entry".to_owned(), move || {
                     store.add_sliced_url_entry(id, rel_pos, sender, origin);
                 })
             }
             FileManagerThreadMsg::DecRef(id, origin, sender) => {
                 if let Ok(id) = Uuid::parse_str(&id.0) {
                     spawn_named("dec ref".to_owned(), move || {
-                        // Since it is simple DecRef (possibly caused by close/drop),
-                        // unset_url_validity is false
                         let _ = sender.send(store.dec_ref(&id, &origin));
                     })
                 } else {
                     let _ = sender.send(Err(BlobURLStoreError::InvalidFileID));
                 }
             }
             FileManagerThreadMsg::RevokeBlobURL(id, origin, sender) => {
                 if let Ok(id) = Uuid::parse_str(&id.0) {
                     spawn_named("revoke blob url".to_owned(), move || {
-                        // Since it is revocation, unset_url_validity is true
                         let _ = sender.send(store.set_blob_url_validity(false, &id, &origin));
                     })
                 } else {
                     let _ = sender.send(Err(BlobURLStoreError::InvalidFileID));
                 }
             }
             FileManagerThreadMsg::ActivateBlobURL(id, sender, origin) => {
                 if let Ok(id) = Uuid::parse_str(&id.0) {
@@ -482,35 +481,32 @@ impl <UI: 'static + UIProvider> FileMana
 
         // Trigger removing if its last reference is gone and it is
         // not a part of a valid Blob URL
         if do_remove {
             atomic::fence(Ordering::Acquire);
             self.remove(id);
 
             if let Some(parent_id) = opt_parent_id {
-                // unset_url_validity for parent is false since we only need
-                // to unset the initial requesting URL
                 return self.dec_ref(&parent_id, origin_in);
             }
         }
 
         Ok(())
     }
 
     fn promote_memory(&self, blob_buf: BlobBuf, set_valid: bool,
                        sender: IpcSender<Result<SelectedFileId, BlobURLStoreError>>, origin: FileOrigin) {
         match Url::parse(&origin) { // parse to check sanity
             Ok(_) => {
                 let id = Uuid::new_v4();
                 self.insert(id, FileStoreEntry {
                     origin: origin.clone(),
                     file_impl: FileImpl::Memory(blob_buf),
                     refs: AtomicUsize::new(1),
-                    // Valid here since PromoteMemory implies URL creation
                     is_valid_url: AtomicBool::new(set_valid),
                 });
 
                 let _ = sender.send(Ok(SelectedFileId(id.simple().to_string())));
             }
             Err(_) => {
                 let _ = sender.send(Err(BlobURLStoreError::InvalidOrigin));
             }
--- a/servo/components/net_traits/filemanager_thread.rs
+++ b/servo/components/net_traits/filemanager_thread.rs
@@ -116,27 +116,27 @@ pub struct SelectedFile {
 
 /// Filter for file selection;
 /// the `String` content is expected to be extension (e.g, "doc", without the prefixing ".")
 #[derive(Clone, Debug, Deserialize, Serialize)]
 pub struct FilterPattern(pub String);
 
 #[derive(Deserialize, Serialize)]
 pub enum FileManagerThreadMsg {
-    /// Select a single file, return triple (FileID, FileName, lastModified)
+    /// Select a single file. Last field is pre-selected file path for testing
     SelectFile(Vec<FilterPattern>, IpcSender<FileManagerResult<SelectedFile>>, FileOrigin, Option<String>),
 
-    /// Select multiple files, return a vector of triples
+    /// Select multiple files. Last field is pre-selected file paths for testing
     SelectFiles(Vec<FilterPattern>, IpcSender<FileManagerResult<Vec<SelectedFile>>>, FileOrigin, Option<Vec<String>>),
 
-    /// Read file in chunks by FileID, optionally check URL validity based on fourth flag
+    /// Read FileID-indexed file in chunks, optionally check URL validity based on boolean flag
     ReadFile(IpcSender<FileManagerResult<ReadFileProgress>>, SelectedFileId, bool, FileOrigin),
 
     /// Add an entry as promoted memory-based blob and send back the associated FileID
-    /// as part of a valid/invalid Blob URL depending on the second bool flag
+    /// as part of a valid/invalid Blob URL depending on the boolean flag
     PromoteMemory(BlobBuf, bool, IpcSender<Result<SelectedFileId, BlobURLStoreError>>, FileOrigin),
 
     /// Add a sliced entry pointing to the parent FileID, and send back the associated FileID
     /// as part of a valid Blob URL
     AddSlicedURLEntry(SelectedFileId, RelativePos, IpcSender<Result<SelectedFileId, BlobURLStoreError>>, FileOrigin),
 
     /// Decrease reference count and send back the acknowledgement
     DecRef(SelectedFileId, FileOrigin, IpcSender<Result<(), BlobURLStoreError>>),
--- a/servo/components/script/dom/blob.rs
+++ b/servo/components/script/dom/blob.rs
@@ -28,27 +28,28 @@ use uuid::Uuid;
 pub struct FileBlob {
     id: SelectedFileId,
     name: Option<PathBuf>,
     cache: DOMRefCell<Option<Vec<u8>>>,
     size: u64,
 }
 
 
-/// Blob backend implementation
+/// Different backends of Blob
 #[must_root]
 #[derive(JSTraceable)]
 pub enum BlobImpl {
-    /// File-based blob
+    /// File-based blob, whose content lives in the net process
     File(FileBlob),
-    /// Memory-based blob
+    /// Memory-based blob, whose content lives in the script process
     Memory(Vec<u8>),
-    /// Sliced blob, including parent blob and
-    /// relative positions representing current slicing range,
-    /// it is leaf of a two-layer fat tree
+    /// Sliced blob, including parent blob reference and
+    /// relative positions of current slicing range,
+    /// IMPORTANT: The depth of tree is only two, i.e. the parent Blob must be
+    /// either File-based or Memory-based
     Sliced(JS<Blob>, RelativePos),
 }
 
 impl BlobImpl {
     /// Construct memory-backed BlobImpl
     #[allow(unrooted_must_root)]
     pub fn new_from_bytes(bytes: Vec<u8>) -> BlobImpl {
         BlobImpl::Memory(bytes)
@@ -66,16 +67,17 @@ impl BlobImpl {
 }
 
 // https://w3c.github.io/FileAPI/#blob
 #[dom_struct]
 pub struct Blob {
     reflector_: Reflector,
     #[ignore_heap_size_of = "No clear owner"]
     blob_impl: DOMRefCell<BlobImpl>,
+    /// content-type string
     typeString: String,
     isClosed_: Cell<bool>,
 }
 
 impl Blob {
     #[allow(unrooted_must_root)]
     pub fn new(global: GlobalRef, blob_impl: BlobImpl, typeString: String) -> Root<Blob> {
         let boxed_blob = box Blob::new_inherited(blob_impl, typeString);
@@ -176,17 +178,17 @@ impl Blob {
         match opt_sliced_parent {
             Some((parent_id, rel_pos, size)) => self.create_sliced_url_id(&parent_id, &rel_pos, size),
             None => self.promote(/* set_valid is */ true),
         }
     }
 
     /// Promote non-Slice blob:
     /// 1. Memory-based: The bytes in data slice will be transferred to file manager thread.
-    /// 2. File-based: Activation
+    /// 2. File-based: If set_valid, then activate the FileID so it can serve as URL
     /// Depending on set_valid, the returned FileID can be part of
     /// valid or invalid Blob URL.
     fn promote(&self, set_valid: bool) -> SelectedFileId {
         let mut bytes = vec![];
 
         match *self.blob_impl.borrow_mut() {
             BlobImpl::Sliced(_, _) => {
                 debug!("Sliced can't have a sliced parent");