Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement Blob methods (text/arraybuffer) #25524
Conversation
highfive
commented
Jan 14, 2020
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jan 14, 2020
|
|
|
@bors-servo try=wpt |
Implement Blob methods (text/arraybuffer) <!-- Please describe your changes on the following line: --> #24287 (comment) #24287 (comment) r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24287 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
Good start! Next we need to make use of the new async reading coding. |
|
|
|
|
Two tests present in Should I run Complete log: |
|
I suspect that adding a |
highfive
added
S-awaiting-review
and removed
S-tests-failed
S-needs-code-changes
labels
|
tests in |
| let trusted_promise = TrustedPromise::new(promise); | ||
| let canceller = self.task_canceller(TaskSourceName::FileReading); | ||
| let task_source = self.file_reading_task_source(); | ||
| thread::spawn(move || { |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 20, 2020
•
Member
I think you can use an IPC route on recv instead of a thread, see for example
servo/components/script/dom/globalscope.rs
Line 472 in 6d5d350
The route is valid for multiple messages, you wouldn't need the loop on the recv. So instead you could just move a let mut bytes = vec![]; into the route closure, handle each message as now, and queue the task when getting the eof or error message.
This comment has been minimized.
This comment has been minimized.
kunalmohan
Jan 20, 2020
Author
Collaborator
I am facing some problem in this part. I have made the above changes to some extent. I am facing a couple of errors which I could not figure out how to get past.
error[E0507]: cannot move out of `trusted_promise`, a captured variable in an `FnMut` closure
--> components/script/task.rs:27:15
|
27 | $name(move || $body)
| ^^^^^^^ move out of `trusted_promise` occurs here
|
::: components/script/dom/globalscope.rs:1259:13
|
1259 | let trusted_promise = TrustedPromise::new(promise);
| --------------- captured outer variable
...
1279 | / task!(resolve_promise: move || {
1280 | | let promise = trusted_promise.root();
| | ---------------
| | |
| | move occurs because `trusted_promise` has type `dom::bindings::refcounted::TrustedPromise`, which does not implement the `Copy` trait
| | move occurs due to use in closure
1281 | | let _ac = enter_realm(&*promise.global());
1282 | | f(promise, bytes);
1283 | | }),
| |______________________________- in this macro invocation
error[E0507]: cannot move out of `f`, a captured variable in an `FnMut` closure
--> components/script/task.rs:27:15
|
27 | $name(move || $body)
| ^^^^^^^ move out of `f` occurs here
|
::: components/script/dom/globalscope.rs:1256:9
|
1256 | f: Box<dyn Fn(Rc<Promise>, Result<Vec<u8>, Error>) + Send>,
| - captured outer variable
...
1279 | / task!(resolve_promise: move || {
1280 | | let promise = trusted_promise.root();
1281 | | let _ac = enter_realm(&*promise.global());
1282 | | f(promise, bytes);
| | -
| | |
| | move occurs because `f` has type `std::boxed::Box<dyn std::ops::Fn(std::rc::Rc<dom::promise::Promise>, std::result::Result<std::vec::Vec<u8>, dom::bindings::error::Error>) + std::marker::Send>`, which does not implement the `Copy` trait
| | move occurs due to use in closure
1283 | | }),
| |______________________________- in this macro invocation
I would need some help here. I have pushed the changes (for you to review). Hopefully it is close to what you had asked for :)
| id, | ||
| p.clone(), | ||
| Box::new(|promise, bytes| { | ||
| let bytes = bytes.unwrap(); |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 20, 2020
•
Member
I think both here and in the other method, if bytes is an error, you want to reject the promise, instead of panicking.
See step 3(the "error" case) of https://fetch.spec.whatwg.org/#concept-read-all-bytes-from-readablestream (which is not implemented here due to the lack of stream support, and I just think rejecting the promise is more in the spirit of the spec).
The question would still be, what kind of error? I guess a NetworkError could do.
|
Looks like it's going in the right direction! I've looked at the compile error, and there is also one other comment. |
| let bytes = Ok(bytes); | ||
| let _ = task_source.queue_with_canceller( | ||
| task!(resolve_promise: move || { | ||
| let promise = trusted_promise.root(); |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 21, 2020
•
Member
I think you need to clone the promise(outside of the task!), and then move the clone into the closure of the task!.
You might want to implement a struct to give the code some more structure, see the examples of TimerListener and MessageListener.
But actually for this one, since you need to queue only one task, you could avoid the clone as described in the other comment.
| recv.to_opaque(), | ||
| Box::new(move |message| { | ||
| let message = message.to(); | ||
| let mut bytes = vec![]; |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 21, 2020
•
Member
You also want to create the vec outside of the route, and move it into it, otherwise you will create a new one for each message received.
You might also want to put the bytes inside a struct, together with the trusted promise, as mentioned in the other comment.
I would suggest the following, to give the code more structure:
enum FileBytes {
/// The initial value when first creating a FileListener,
Empty(Trusted<Promise>),
/// The value after having received the Meta message.
Receiving(Vec<u8>, Trusted<Promise>),
/// The value after having queued the task resolving the promise.
Done,
}Then you could have a struct like:
struct FileListener {
bytes: FileBytes
task_source: //
task_canceller: //
}which would allow you to express more precisely how the bytes are received and then passed to the promise via the task. In the sense that when receiving ReadFileProgress::Meta, bytes you could assert bytes is FileBytes::Empty, and then transform it into FileBytes::Receiving(bytes), then we you receive the EOF message, you can assert bytes is FileBytes::Receiving(bytes), then take bytes out of FileBytes::Receiving(bytes), move it into the closure of the queued task, and transform FileListener into FileBytes::Done.
And when you receive the Err message, only queue an error if it is either be at FileBytes::Empty or FileBytes::Receiving(bytes), ignore it is it is Done.
This comment has been minimized.
This comment has been minimized.
kunalmohan
Jan 21, 2020
•
Author
Collaborator
I have made some progress here. First of all, I have used bytes: DomRefCell<FileBytes> in FileListener. But still I am some problems here.
- I cannot assert
bytesto beEmptyorReceivingbecauseTrustedPromisedoes not implementDebugorPartialEq. For now I have usedif letto mutably borrowsbytesout ofReceiving(bytes)and in theelseblock usedpanic!. - I cannot figure out how to transform
Empty(trusted_promise)toReceiving(bytes, trusted_promise). I cannot transfertrusted_promisefrom one enum variant to another. So I removedTrustedPromisefromFileBytesin order to get past that. (I am thinking of usingOption<TrustedPromise>insideEmpty. Maybe that would help. Could not try that today; will do tomorrow.) - Now I am facing a lifetime error.
😞 . I guess I'll have to clone bytes before queuing the task.
I have pushed the changes for you to review.
This comment has been minimized.
This comment has been minimized.
kunalmohan
Jan 22, 2020
Author
Collaborator
I have removed the lifetime error with clone, but I cannot pass the closure(f) as parameter to handle() since it does not implement Copy or Clone trait. (same for trusted_promise, if passed as parameter instead of enum)
|
Yes you are right, it is tricky to implement this enum as I suggested. Let me play with the code a bit to try to find a solution... |
|
It seems pretty good to me. I tried using Option, but never considered removing the enum. I'll update my code with this and run the tests. Is there anything that is yet to be implemented? |
|
I just saw the latest changes you made. (I'll be more careful with the commas and spacing next time |
|
All of the related tests (which were passing) are crashing at (line 454, globalscope.rs) |
You can simply use
It might be that the route always gets a last
Ok, let me know what you find. |
I tried that- replacing it with empty block- all the tests fail-with the NetworkError- in that case. |
|
Done! |
|
LGTM, just two nits. |
| &self, | ||
| id: Uuid, | ||
| promise: Rc<Promise>, | ||
| callback: Box<dyn Fn(Rc<Promise>, Result<Vec<u8>, Error>) + Send>, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 27, 2020
Member
Actually, please discard this comment, we already type the callback below when we add it to the file listener, and using FileListenerCallback in the function signature directly will just make its use by the blob more complicated.
So just the space with the doc comment, and it's good to go!
| @@ -231,6 +235,23 @@ struct TimerListener { | |||
| context: Trusted<GlobalScope>, | |||
| } | |||
|
|
|||
| /// A wrapper for the handling of file data received by the ipc router | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 25, 2020
•
Member
nit: Lets consistently use one space following ///(see below where the space is missing, here it looks like there are two). ./mach fmt doesn't catch this I think.
|
Sorry for the late response. I was on a two-day vacation devoid of any network. |
|
Ok, I finally looked to the actual DOM side of things, and I'm sorry for not having mentioned it earlier, but I have a last few suggestions. |
| p.clone(), | ||
| Box::new(|promise, bytes| match bytes { | ||
| Ok(b) => { | ||
| let (text, _, _) = UTF_8.decode(&b); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 27, 2020
Member
Ah no forget about this one, I think what you are doing with UTF_8 fits the spec more, in the light of:
Note: This is different from the behavior of readAsText() to align better with the behavior of Fetch’s text(). Specifically this method will always use UTF-8 as encoding, while FileReader can use a different encoding depending on the blob’s type and passed in encoding name.
https://w3c.github.io/FileAPI/#text-method-algo
I think the other comment is still relevant.
| match bytes { | ||
| Ok(b) => { | ||
| let cx = promise.global().get_cx(); | ||
| unsafe { |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 27, 2020
•
Member
Here I think we can reuse run_array_buffer_data_algorithm
For both suggestions, to see how the results can be resolved into a promise can be seen at
servo/components/script/body.rs
Line 95 in 12693b5
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 27, 2020
Member
Also note that in the case of an error being returned by that function, I think it would be more appropriate to reject the promise with it, versus panicking.
| } | ||
|
|
||
| // https://w3c.github.io/FileAPI/#arraybuffer-method-algo | ||
| #[allow(unsafe_code)] |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 27, 2020
Member
This could also be removed if we re-use run_array_buffer_data_algorithm
|
Looks good, thanks for making that additional change. I have one small comment on the change, after that please squash into one commit and it should be good to go! |
| match result { | ||
| Ok(FetchedData::ArrayBuffer(a)) => promise.resolve_native(&a), | ||
| Err(e) => promise.reject_error(e), | ||
| _ => {}, |
This comment has been minimized.
This comment has been minimized.
gterzian
Jan 28, 2020
Member
nit: Here you can actually panic with an appropriate message, like "Unexpected result from run_array_buffer_data_algorithm".
|
|
||
| fn read_msg( | ||
| object: profile_ipc::IpcReceiver<FileManagerResult<ReadFileProgress>>, |
This comment has been minimized.
This comment has been minimized.
|
@kunalmohan thank you, great work! @bors-servo r=jdm,gterzian |
|
|
Implement Blob methods (text/arraybuffer) <!-- Please describe your changes on the following line: --> #24287 (comment) #24287 (comment) r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24287 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
|
@bors-servo retry #25632 |
Implement Blob methods (text/arraybuffer) <!-- Please describe your changes on the following line: --> #24287 (comment) #24287 (comment) r?@jdm --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24287 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes <!-- 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. -->
|
|
kunalmohan commentedJan 14, 2020
#24287 (comment)
#24287 (comment)
r?@jdm
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors