Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement "update the image data" #16238

Merged
merged 1 commit into from May 30, 2017

Conversation

Projects
None yet
6 participants
@gterzian
Copy link
Collaborator

commented Apr 3, 2017

Spec compliant implementation of the update the image data algorithm.

Currently still a work in progress, the 'async src complete test` is still passing as it was before, even though I switched to the new code, so I guess that's something.

@jdm I will be picking this up next weekend, I left a bunch of TODO and NOTES in the code, if you or someone else have time this week I would appreciate an initial scan and feedback.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11517 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 3, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/htmlimageelement.rs
  • @KiChjang: components/script/dom/htmlimageelement.rs
@highfive

This comment has been minimized.

Copy link

commented Apr 3, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm

This comment has been minimized.

Copy link
Member

commented Apr 3, 2017

Wonderful! Thanks for tackling this!

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 3, 2017

Note: there's no need to include any implementation related to srcset/sizes/other responsive image things, as that's going to be a student project starting this summer.

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2017

@jdm great, glad to hear that! That would include concepts like selecting and image source and updating the source set, correct? Currently I have left those at the bare minimum, essentially just grabbing the src and ignoring the actual spec of those concepts...

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 4, 2017

Yep, that sounds great.

@emilio

This comment has been minimized.

Copy link
Member

commented Apr 5, 2017

r? @jdm

@highfive highfive assigned jdm and unassigned emilio Apr 5, 2017

@gterzian gterzian force-pushed the gterzian:implement_update_the_image_data branch 3 times, most recently from a71c9bc to f25ce3d Apr 8, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2017

@jdm here is this weekend's harvest:

  1. All img.complete tests are now passing.
  2. I had to edit one test, because from what I could tell the while loop that was in there was preventing the page from reaching ths stable state, while the specific case that is being tested requires the fetch part of the code to have started to run, therefore the test was always bound to fail.
  3. I tried to get the fetch part of the code to be a bit more spec compliant, meaning following the comcept of the image request, as well as adding some things related to step 14 of the spec.
  4. I made Complete more spec compliant.

Re 3, the previous code would always just use self.current_request, however to follow the spec we must first pick the image_request from either the pending or current one, and then associate the fetch code with this image_request. Due to the fact that ImageRequest isn't thread safe in it's current form, I couldn't pass the actual request along the algorithm, so instead I pass around the url and grab the 'right' request based on the url when needed. I "think" that's pretty good for now...

Re 4, I had to decide how to have Complete return true in the case that "The final task that is queued by the networking task source once the resource has been fetched has been queued". I didn't want to add more state to ImageRequest, so instead I made the choice that if current_request has a parsed_url, but isn't yet in either Broken or CompletelyAvailable state, we can assume the final task has been queued but not processed yet... Once again, I "think" that's good enough for now...

If you have time this week, I think the PR is good for a more comprehensive review at this stage...

@gterzian gterzian force-pushed the gterzian:implement_update_the_image_data branch 2 times, most recently from 80d3bd9 to f3ff232 Apr 9, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2017

@jdm I'm reading through the spec again and I see I missed some key concepts regarding current and pending requests in step 12 and 14 of the specs, so that's certainly something I will still need to implement, feedback in the meantime welcome off course...

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 9, 2017

Re: "The final task queued that is queued", see whatwg/html#1055.

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 9, 2017

@jdm thanks, so that means we can keep that test as it was before, and with a FAIL? Glad to read that because it wasn't entirely clear to me.

Re pending/current request, here is the work I intend to still do, next weekend:

  1. make pending_requent an Option<DOMRefCell<ImageRequest>>, and initially set it to None.
  2. Make the fetch and 'choose a request' code comply with step 12 and 14 of the spec, for example the whole abort current and upgrade pending to current. Also currently I just return in some cases, while in fact the spec speaks of aborting these steps, which I guess means just aborting step 12, not returning form the whole algorithm(This relates to picking the right request to fetch the image).

I guess some of the stuff I currently do with url might become obsolete if pending/current is better implemented...

So it's mainly looking at step 12 and 14 in more details....

I'll also undo the changes I made to the tests, and keep that "final task" one on FAIL...

@jdm
Copy link
Member

left a comment

Great work so far! I found these changes to be quite readable in general, although it might be nice to follow the spec text about aborting a particular request or creating a new request, rather than what is (presumably) equivalent.

@@ -113,7 +116,8 @@ impl Runnable for ImageResponseHandlerRunnable {
let element = self.element.root();
// Ignore any image response for a previous request that has been discarded.
if element.generation.get() == self.generation {
element.process_image_response(self.image);
let url = self.url.clone();
element.process_image_response(self.image, &url);

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

Wouldn't &self.url avoid the need to clone?

This comment has been minimized.

Copy link
@gterzian

gterzian Apr 15, 2017

Author Collaborator

yes indeed, and I also had to use &self.image to prevent use of moved value: self

let _ = task_source.queue_with_wrapper(box runnable, &wrapper);
});

image_cache.add_listener(id, ImageResponder::new(responder_sender, id));
}

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

Let's restore this newline.

if trigger_image_load {
self.upcast::<EventTarget>().fire_event(atom!("load"));
self.upcast::<EventTarget>().fire_event(atom!("loadend"));
}

// Fire image.onerror
if trigger_image_error {
self.upcast::<EventTarget>().fire_event(atom!("error"));

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

We should be firing loadend after error as well according to the steps in https://html.spec.whatwg.org/multipage/embedded-content.html#updating-the-image-data .

This comment has been minimized.

Copy link
@gterzian

gterzian Apr 16, 2017

Author Collaborator

done below

fn has_parent_image(&self) -> bool {
let node = self.upcast::<Node>();
if let Some(ref parent) = node.GetParentNode() {
if parent.is::<HTMLImageElement>() {

This comment has been minimized.

Copy link
@jdm
current_request.state = State::CompletelyAvailable;
current_request.parsed_url = Some(img_url);
current_request.source_url = Some(src);
self.upcast::<EventTarget>().fire_event(atom!("load"));

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

5.3.7 should happen as part of a queued task.

let mut image_request = self.request_for_url(url);
image_request.state = State::Broken;
image_request.image = None;
image_request.metadata = None;

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

This looks like it's missing some steps from the relevant "Otherwise" branches.

};
self.current_request.borrow_mut().image = image;
self.current_request.borrow_mut().metadata = metadata;

// Mark the node dirty
self.upcast::<Node>().dirty(NodeDamage::OtherNodeDamage);

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

We should only do this if the current request is the one being serviced.

document.getElementById("imgTestTag3").src = '3.jpg?nocache=' + Math.random();
document.getElementById("imgTestTag3").onload = t.step_func(function(){
assert_true(document.getElementById("imgTestTag3").complete);
assert_equals(document.getElementById("imgTestTag3").src.split("/").pop().split('?')[0], "3.jpg");

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

What if we use currentSrc here instead of src?

if (imageInstance.complete === true) break;
}
t.done();
setTimeout(function(){

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

t.step_timeout(function {

// NOTE: this is an attempt to comply with:
// "(return true if)The final task that is queued by the networking task source,
// once the resource has been fetched, has been queued"
return request.parsed_url.is_some()

This comment has been minimized.

Copy link
@jdm

jdm Apr 11, 2017

Member

Let's forget about this.

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 11, 2017

@jdm thanks for the review, I'll look at it in more details over the weekend...

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 16, 2017

@jdm OK I have addressed most of your comments, I just had a question re "We should store the selected source and pass it as an argument here instead" and "Don't we need to initialize the image request's state here?", and I have also made further attempt to implement the concepts of pending and current requests in a more spec compliant way. Please take a look when you have a chance...

@gterzian gterzian force-pushed the gterzian:implement_update_the_image_data branch from f5f94b6 to 0f630f5 Apr 20, 2017

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented May 25, 2017

@jdm OK, fixed the double borrowing crash through scoping...

@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented May 28, 2017

@jdm This one might be ready for another test run. The previous run I think confirmed that the timeouts where simply due to my machine being too slow, and the only code change that would require additional review is where the current request is set to Unavailable at the beginning of the algorithm, this allows an additional test to pass. The crashes from the previous run where due to the fact that I hadn't scoped that part, so the current_request was doubly borrowed(this wasn't apparent from running the img specific tests, since the images are not cached, and the double borrowing occured in the "we found a cached image" part).

@jdm jdm changed the title [WIP] Implement "update the image data" Implement "update the image data" May 29, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

⌛️ Trying commit 71b0c10 with merge f4333fc...

bors-servo added a commit that referenced this pull request May 29, 2017

Auto merge of #16238 - gterzian:implement_update_the_image_data, r=<try>
Implement "update the image data"

<!-- Please describe your changes on the following line: -->
Spec compliant implementation of the [update the image data algorithm](https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data).

Currently still a work in progress, the ['async src complete test`](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/img.complete.html#L33) is still passing as it was before, even though I switched to the new code, so I guess that's something.

@jdm I will be picking this up next weekend, I left a bunch of TODO and NOTES in the code, if you or someone else have time this week I would appreciate an initial scan and feedback.

---
<!-- 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 #11517 (github issue number if applicable).

<!-- Either: -->
- [x] 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. -->

<!-- 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/16238)
<!-- Reviewable:end -->
@jdm

jdm approved these changes May 29, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 29, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented May 30, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

📌 Commit 71b0c10 has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

⌛️ Testing commit 71b0c10 with merge 7f5bc73...

bors-servo added a commit that referenced this pull request May 30, 2017

Auto merge of #16238 - gterzian:implement_update_the_image_data, r=jdm
Implement "update the image data"

<!-- Please describe your changes on the following line: -->
Spec compliant implementation of the [update the image data algorithm](https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data).

Currently still a work in progress, the ['async src complete test`](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/img.complete.html#L33) is still passing as it was before, even though I switched to the new code, so I guess that's something.

@jdm I will be picking this up next weekend, I left a bunch of TODO and NOTES in the code, if you or someone else have time this week I would appreciate an initial scan and feedback.

---
<!-- 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 #11517 (github issue number if applicable).

<!-- Either: -->
- [x] 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. -->

<!-- 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/16238)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

💔 Test failed - mac-rel-wpt2

@jdm

This comment has been minimized.

Copy link
Member

commented May 30, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

⌛️ Testing commit 71b0c10 with merge fe7d039...

bors-servo added a commit that referenced this pull request May 30, 2017

Auto merge of #16238 - gterzian:implement_update_the_image_data, r=jdm
Implement "update the image data"

<!-- Please describe your changes on the following line: -->
Spec compliant implementation of the [update the image data algorithm](https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-image-data).

Currently still a work in progress, the ['async src complete test`](https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/html/semantics/embedded-content/the-img-element/img.complete.html#L33) is still passing as it was before, even though I switched to the new code, so I guess that's something.

@jdm I will be picking this up next weekend, I left a bunch of TODO and NOTES in the code, if you or someone else have time this week I would appreciate an initial scan and feedback.

---
<!-- 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 #11517 (github issue number if applicable).

<!-- Either: -->
- [x] 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. -->

<!-- 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/16238)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 30, 2017

@bors-servo bors-servo merged commit 71b0c10 into servo:master May 30, 2017

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
@gterzian

This comment has been minimized.

Copy link
Collaborator Author

commented May 31, 2017

@jdm that was a big one, thanks for the efforts you put into reviewing this!

@gterzian gterzian deleted the gterzian:implement_update_the_image_data branch May 31, 2017

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.