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 upReacting to environment changes #21280
Conversation
highfive
commented
Jul 30, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jul 30, 2018
|
r? @jdm |
|
Good start! The general framework is here; now we need to focus on some missing details. |
| return | ||
| } | ||
| } | ||
| let area_elements = self.areas(); |
This comment has been minimized.
This comment has been minimized.
| @@ -871,6 +871,166 @@ impl HTMLImageElement { | |||
| ScriptThread::await_stable_state(Microtask::ImageElement(task)); | |||
| } | |||
|
|
|||
| /// <https://html.spec.whatwg.org/multipage/#img-environment-changes> | |||
| fn react_to_environment_changes(&self) { | |||
This comment has been minimized.
This comment has been minimized.
jdm
Jul 30, 2018
Member
This isn't used anywhere yet, so let's add some integration:
- add a
Vec<Dom<HTMLImageElement>>member toDocument; this will be a list of responsive images that exist in the document - Add methods to document to register and unregister responsive images by manipulating the vector
- Add method to
Documentthat iterates over the vector and invokesreact_to_environment_changes - Use the new registration APIs from HTMLImageElement's
bind_to_treeandunbind_from_treemethods (when binding, if is_in_doc is true then the element should be registered, when unbinding the element should be unregistered) - call the new Document method from Window.evaluate_media_queries_and_report_changes
| ScriptThread::await_stable_state(Microtask::ImageElement(task)); | ||
| } | ||
|
|
||
| /// Step 3-12 of https://html.spec.whatwg.org/multipage/#img-environment-changes |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
paavininanda
Jul 31, 2018
Author
Contributor
Didn't know how to handle multipart/x-mixed-replace, couldn't find it anywhere in the already implemented code.
This comment has been minimized.
This comment has been minimized.
jdm
Jul 31, 2018
Member
Step 2 is a list of condititions that should be checked, and the function should return immediately if any of them are true.
This comment has been minimized.
This comment has been minimized.
nupurbaghel
Jul 31, 2018
Contributor
We have added checks for all conditions except multipart/x-mixed-replace. Multipart is a mime::TopLevel but how do we check for x-mixed-replace ?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| let elem = self.upcast::<Element>(); | ||
| let document = document_from_node(elem); | ||
| let base_url = document.base_url(); | ||
| match self.select_image_source() { |
This comment has been minimized.
This comment has been minimized.
jdm
Jul 30, 2018
Member
We can skip some work here by doing:
// Steps 3-4
let (selected_source, selected_pixel_density) = match self.select_image_source() {
Some(selected) => selected,
None => return,
};| } | ||
|
|
||
| // Step 6 | ||
| if let Ok(img_url) = base_url.join(&String::from(src.clone())) { |
This comment has been minimized.
This comment has been minimized.
jdm
Jul 30, 2018
Member
For cases where we return if an operation didn't succeed, it's more readable to do:
let img_url = match base_url.join(&src) {
Ok(url) => url,
Err(_) => return,
};| let this = this.root(); | ||
| // Step 15.1 | ||
| if relevant_mutation { | ||
| let mut _pending_request = |
This comment has been minimized.
This comment has been minimized.
| // Step 15.2 | ||
| *this.last_selected_source.borrow_mut() = Some(DOMString::from_string(new_selected_source)); | ||
| let mut current_request = this.current_request.borrow_mut(); | ||
| current_request.current_pixel_density = selected_pixel_density; |
This comment has been minimized.
This comment has been minimized.
jdm
Jul 30, 2018
Member
I believe this needs to use the pending request, since it will be upgraded to the current request in 15.5.
| current_request.current_pixel_density = selected_pixel_density; | ||
|
|
||
| // Step 15.3 | ||
| this.current_request.borrow_mut().state = State::CompletelyAvailable; |
This comment has been minimized.
This comment has been minimized.
| // Already a part of the list of available images due to Step 14 | ||
|
|
||
| // Step 15.5 | ||
| current_request = this.pending_request.borrow_mut(); |
This comment has been minimized.
This comment has been minimized.
jdm
Jul 30, 2018
Member
This doesn't actually overwrite the contents of current_request. We need *current_request for that.
| ); | ||
|
|
||
| } | ||
| return; |
This comment has been minimized.
This comment has been minimized.
a65f904
to
11c5ec3
|
Closer! Once the code in document.rs is correctly registering and unregistering image elements, running the tests should produce different results. Until then, none of this new code is being executed. |
| } | ||
|
|
||
| pub fn register_responsive_images(&self) { | ||
| for image in self.responsive_images.iter() { |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
I don't understand what this method is attempting to do - the APIs I suggested for registering and unregistering would take an &HTMLImageElement argument and modify the responsive_images field to either append or remove it, and these new methods would be called from HTMLImageElement's bind_to_tree and unbind_from_tree methods.
|
|
||
| pub fn responsive_images(&self) { | ||
| for image in self.responsive_images.iter() { | ||
| (*image).react_to_environment_changes(); |
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.
jdm
Aug 2, 2018
Member
Right, but it shouldn't be necessary. The Rust compiler auto-dereferences for us.
This comment has been minimized.
This comment has been minimized.
| @@ -2221,6 +2223,30 @@ impl Document { | |||
| let counter = self.throw_on_dynamic_markup_insertion_counter.get(); | |||
| self.throw_on_dynamic_markup_insertion_counter.set(counter - 1); | |||
| } | |||
|
|
|||
| pub fn responsive_images(&self) { | |||
This comment has been minimized.
This comment has been minimized.
| @@ -120,7 +120,7 @@ enum ImageRequestPhase { | |||
| Pending, | |||
| Current | |||
| } | |||
| #[derive(JSTraceable, MallocSizeOf)] | |||
| #[derive(Clone, JSTraceable, MallocSizeOf)] | |||
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
I suspect we can find another solution instead of cloning, which would allow us to remove this and the one for LoadBlocker that was added.
| let src = match selected_source { | ||
| Some(src) => src, | ||
| _ => return, | ||
| let (selected_source, selected_pixel_density) = match self.select_image_source() { |
This comment has been minimized.
This comment has been minimized.
| @@ -2669,7 +2669,7 @@ pub struct UnbindContext<'a> { | |||
|
|
|||
| impl<'a> UnbindContext<'a> { | |||
| /// Create a new `UnbindContext` value. | |||
| fn new(parent: &'a Node, | |||
| pub fn new(parent: &'a Node, | |||
This comment has been minimized.
This comment has been minimized.
| // TODO: Handle multipart/x-mixed-replace | ||
| // Step 2 of https://html.spec.whatwg.org/multipage/#img-environment-changes, | ||
| let element = elem.upcast::<Element>(); | ||
| let has_src = element.has_attribute(&local_name!("srcset")); |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
This check also needs to include the parent element, per https://html.spec.whatwg.org/multipage/images.html#use-srcset-or-picture . It would be nice to create a helper function that we could use in severalother places in the code.
| let window = window_from_node(self); | ||
| let image_cache = window.image_cache(); | ||
| let response = | ||
| image_cache.find_image_or_metadata(img_url.clone().into(), |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
Since we already perform this check in the caller, we should avoid duplicating it here. We can move all of the match arms into the caller, along with add_cache_listener_for_element, and then remove fetch_image_for_environment_change.
| @@ -375,6 +437,22 @@ impl HTMLImageElement { | |||
| window.add_pending_reflow(); | |||
| } | |||
|
|
|||
| fn process_image_response_for_environment_change(&self, image: ImageResponse) { | |||
| let element = self.upcast::<Element>(); | |||
| let is_multipart_x_mixed_replace = HTMLImageElement::check_mutipart_x_mixed_replace(element); | |||
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
This check will need to happen as part of the process_response method, by obtaining the mime type of the Metadata object that is available there.
This comment has been minimized.
This comment has been minimized.
paavininanda
Aug 2, 2018
•
Author
Contributor
We were trying to check for the mime type as follows -
if let Some(metadata) = metadata {
if let Some(mime) = metadata.content_type {
let mime_type = String::from("x-mixed-replace");
match mime.into_inner() {
ContentType::Mime(TopLevel::Multipart, SubLevel::Ext(s), _) => {
if s == mime_type {
self.abort_request(State::Unavailable, ImageRequestPhase::Pending);
}
},
_ => ()
}
}
}
ContentType has to be imported which is not possible because common module is private. Also we are unsure what to do in case of abort since we are inside ImageContext scope?
This comment has been minimized.
This comment has been minimized.
paavininanda
Aug 2, 2018
Author
Contributor
We are able to find the mime type correctly. But still stuck on how to abort here ?
This comment has been minimized.
This comment has been minimized.
jdm
Aug 2, 2018
Member
The best way to abort is to add a field to ImageContext called aborted, set it to true, and return early from all of the handlers if aborted is true.
This comment has been minimized.
This comment has been minimized.
jdm
Aug 2, 2018
Member
You can import ContentType from the hyper::header module: https://doc.servo.org/hyper/header/index.html
| self.abort_request(State::Unavailable, ImageRequestPhase::Pending); | ||
| } | ||
|
|
||
| match (image, self.image_request.get()) { |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 1, 2018
Member
We should just be able to match on image here. We will need to modify the contents of self.pending_request in order to store the image data and metadata in it when we they are available (ImageResponse::Loaded, ImageResponse::PlaceholderLoaded, ImageResponse::MetadataLoaded). When we have a fully loaded image, we should invoke finish_reacting_to_environment_changes.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3098a4c
to
7943397
|
We have updated all the suggested changes and ready for next review :) |
|
We are having 16 Pass, 20 Fail, 8 Timeout now. |
|
It would be worth figuring out which subtests fail or time out and why that occurs. |
|
This is in very good shape! It's much more readable now and easier to follow when comparing it to the specification. Most of my comments are about improving the readability; the implementation looks good! |
| @@ -2221,6 +2223,21 @@ impl Document { | |||
| let counter = self.throw_on_dynamic_markup_insertion_counter.get(); | |||
| self.throw_on_dynamic_markup_insertion_counter.set(counter - 1); | |||
| } | |||
|
|
|||
| pub fn react_to_environment_changes(&self) { | |||
| for image in self.responsive_images.borrow_mut().iter() { | |||
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| pub fn unregister_responsive_image(&self, img: &HTMLImageElement) { | ||
| let index = self.responsive_images.borrow_mut().iter().position(|x| **x == *img).unwrap(); |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
Instead of unwrapping, let's use if let to only call remove() when we find a matching value. We can use borrow instead of borrow_mut here, too.
| @@ -178,6 +181,19 @@ impl FetchResponseListener for ImageContext { | |||
| } | |||
| }); | |||
|
|
|||
| if let Some(metadata) = metadata.as_ref() { | |||
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
Let's add a comment to be clear that this is implementing part of the check in a particular step of the "reacting to the environment changes" algorithm (and which step that is).
| if let Some(ref content_type) = metadata.content_type { | ||
| match content_type.clone().into_inner().0 { | ||
| Mime(TopLevel::Multipart, SubLevel::Ext(s), _) => { | ||
| if s == String::from("x-mixed-replace") { |
This comment has been minimized.
This comment has been minimized.
| @@ -375,6 +396,31 @@ impl HTMLImageElement { | |||
| window.add_pending_reflow(); | |||
| } | |||
|
|
|||
| fn process_image_response_for_environment_change(&self, image: ImageResponse, src: DOMString, generation: u32, | |||
| selected_pixel_density: f64) { | |||
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
Let's format this as:
fn process_image_response_for_environment_change(
&self,
image: ImageResponse,
src: DOMString,
generation: u32,
selected_pixel_density: f64)
) {|
|
||
| // Step 15.1 | ||
| if relevant_mutation { | ||
| this.abort_request(State::Unavailable, ImageRequestPhase::Pending); |
This comment has been minimized.
This comment has been minimized.
| _ => false | ||
| }; | ||
| // TODO Handle image/x-mixed-replace | ||
| if document.is_active() && has_src && !has_pending_request { |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
We should remove these checks now, since the exist inside react_to_environment_changes_sync_steps.
| s.bind_to_tree(tree_in_doc); | ||
| } | ||
| let document = document_from_node(self); | ||
| document.register_responsive_image(self); |
This comment has been minimized.
This comment has been minimized.
| task!(process_image_response_for_environment_change: move || { | ||
| let element = element.root(); | ||
| // Steps 3-4 | ||
| let (selected_source, selected_pixel_density) = match element.select_image_source() { |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
Instead of calling this, we should pass the selected source and density as arguments to add_cache_listener_for_element so we can use them inside this task.
This comment has been minimized.
This comment has been minimized.
|
|
||
| /// Step 2-12 of https://html.spec.whatwg.org/multipage/#img-environment-changes | ||
| fn react_to_environment_changes_sync_steps(&self, generation: u32) { | ||
| fn add_cache_listener_for_element(image_cache: Arc<ImageCache>, |
This comment has been minimized.
This comment has been minimized.
jdm
Aug 3, 2018
Member
Let's add a TODO comment about reducing the duplicating between this code and the code for regular image fetching. We can file a followup issue to address that.
f2daca2
to
bfcee08
bfcee08
to
570cb75
|
|
|
@bors-servo retry |
|
|
|
|
|
More passing tests!
|
|
We'll need to add that test name to the title of the intermittent issue. |
|
@bors-servo retry |
Reacting to environment changes <!-- Please describe your changes on the following line: --> --- <!-- 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 a subset of #11416 <!-- Either: --> - [x] These changes do not require tests because they will be added when the complete responsive images is complete <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21280) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
Reacting to environment changes <!-- Please describe your changes on the following line: --> --- <!-- 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 a subset of #11416 <!-- Either: --> - [x] These changes do not require tests because they will be added when the complete responsive images is complete <!-- 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. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21280) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
|
|
|
paavininanda commentedJul 30, 2018
•
edited
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is