Auto merge of #25227 - gterzian:remove_unnecessary_thread_in_filereader, r=jdm

Remove unnecessary thread in filereader, add stream TODO

<!-- Please describe your changes on the following line: -->

I think there are no benefits to queuing the various `FileReadingTask` from a thread, and reading the actual blob bytes are still done on the main-thread. We could try to move the reading off the main-thread(but note the `Blob` is not something you would want to move to another thread "as-is" since it's a `DomObject`), and I think we should just aim for implementing streams and follow the spec instead.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

components/script/dom/filereader.rs

@@ -23,8 +23,7 @@ use crate::dom::eventtarget::EventTarget;
 use crate::dom::globalscope::GlobalScope;
 use crate::dom::progressevent::ProgressEvent;
 use crate::script_runtime::JSContext;
-use crate::task::TaskCanceller;
+use crate::task_source::file_reading::FileReadingTask;
-use crate::task_source::file_reading::{FileReadingTask, FileReadingTaskSource};
 use crate::task_source::{TaskSource, TaskSourceName};
 use base64;
 use dom_struct::dom_struct;
@@ -37,8 +36,6 @@ use mime::{self, Mime};
 use servo_atoms::Atom;
 use std::cell::Cell;
 use std::ptr;
-use std::sync::Arc;
-use std::thread;
 
 #[derive(Clone, Copy, JSTraceable, MallocSizeOf, PartialEq)]
 pub enum FileReaderFunction {
@@ -236,7 +233,7 @@ impl FileReader {
         filereader: TrustedFileReader,
         gen_id: GenerationId,
         data: ReadMetaData,
-        blob_contents: Arc<Vec<u8>>,
+        blob_contents: Vec<u8>,
     ) {
         let fr = filereader.root();
 
@@ -426,6 +423,7 @@ impl FileReader {
         self.generation_id.set(GenerationId(prev_id + 1));
     }
 
+    /// <https://w3c.github.io/FileAPI/#readOperation>
     fn read(
         &self,
         function: FileReaderFunction,
@@ -443,35 +441,40 @@ impl FileReader {
         // Step 3
         *self.result.borrow_mut() = None;
 
-        let blob_contents = Arc::new(blob.get_bytes().unwrap_or(vec![]));
-
         let type_ = blob.Type();
 
         let load_data = ReadMetaData::new(String::from(type_), label.map(String::from), function);
 
-        let fr = Trusted::new(self);
-
         let GenerationId(prev_id) = self.generation_id.get();
         self.generation_id.set(GenerationId(prev_id + 1));
         let gen_id = self.generation_id.get();
 
+        // Step 10, in parallel, wait on stream promises to resolve and queue tasks.
+
+        // TODO: follow the spec which requires implementing blob `get_stream`,
+        // see https://github.com/servo/servo/issues/25209
+
+        // Currently bytes are first read "sync", and then the appropriate tasks are queued.
+
+        // Read the blob bytes "sync".
+        let blob_contents = blob.get_bytes().unwrap_or_else(|_| vec![]);
+
+        let filereader = Trusted::new(self);
         let global = self.global();
         let canceller = global.task_canceller(TaskSourceName::FileReading);
         let task_source = global.file_reading_task_source();
 
-        thread::Builder::new()
+        // Queue tasks as appropriate.
-            .name("file reader async operation".to_owned())
+        let task = FileReadingTask::ProcessRead(filereader.clone(), gen_id);
-            .spawn(move || {
+        task_source.queue_with_canceller(task, &canceller).unwrap();
-                perform_annotated_read_operation(
+
-                    gen_id,
+        if !blob_contents.is_empty() {
-                    load_data,
+            let task = FileReadingTask::ProcessReadData(filereader.clone(), gen_id);
-                    blob_contents,
+            task_source.queue_with_canceller(task, &canceller).unwrap();
-                    fr,
+        }
-                    task_source,
+
-                    canceller,
+        let task = FileReadingTask::ProcessReadEOF(filereader, gen_id, load_data, blob_contents);
-                )
+        task_source.queue_with_canceller(task, &canceller).unwrap();
-            })
-            .expect("Thread spawning failed");
 
         Ok(())
     }
@@ -480,25 +483,3 @@ impl FileReader {
         self.ready_state.set(state);
     }
 }
-
-// https://w3c.github.io/FileAPI/#thread-read-operation
-fn perform_annotated_read_operation(
-    gen_id: GenerationId,
-    data: ReadMetaData,
-    blob_contents: Arc<Vec<u8>>,
-    filereader: TrustedFileReader,
-    task_source: FileReadingTaskSource,
-    canceller: TaskCanceller,
-) {
-    // Step 4
-    let task = FileReadingTask::ProcessRead(filereader.clone(), gen_id);
-    task_source.queue_with_canceller(task, &canceller).unwrap();
-
-    if !blob_contents.is_empty() {
-        let task = FileReadingTask::ProcessReadData(filereader.clone(), gen_id);
-        task_source.queue_with_canceller(task, &canceller).unwrap();
-    }
-
-    let task = FileReadingTask::ProcessReadEOF(filereader, gen_id, data, blob_contents);
-    task_source.queue_with_canceller(task, &canceller).unwrap();
-}

components/script/task_source/file_reading.rs

@@ -8,7 +8,6 @@ use crate::script_runtime::{CommonScriptMsg, ScriptChan, ScriptThreadEventCatego
 use crate::task::{TaskCanceller, TaskOnce};
 use crate::task_source::{TaskSource, TaskSourceName};
 use msg::constellation_msg::PipelineId;
-use std::sync::Arc;
 
 #[derive(JSTraceable)]
 pub struct FileReadingTaskSource(pub Box<dyn ScriptChan + Send + 'static>, pub PipelineId);
@@ -46,7 +45,7 @@ pub enum FileReadingTask {
     ProcessRead(TrustedFileReader, GenerationId),
     ProcessReadData(TrustedFileReader, GenerationId),
     ProcessReadError(TrustedFileReader, GenerationId, DOMErrorName),
-    ProcessReadEOF(TrustedFileReader, GenerationId, ReadMetaData, Arc<Vec<u8>>),
+    ProcessReadEOF(TrustedFileReader, GenerationId, ReadMetaData, Vec<u8>),
 }
 
 impl FileReadingTask {