Skip to content

/ servo

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

Jump to bottom

Implement an image loading model that conforms to the specification #11517

Closed
jdm opened this issue May 30, 2016 · 7 comments
Closed

Implement an image loading model that conforms to the specification #11517

jdm opened this issue May 30, 2016 · 7 comments
Labels
A-content/dom A-content/images B-high-value C-assigned E-hard

Comments

@jdm
Copy link
Member

@jdm jdm commented May 30, 2016 •
edited

The goal of this work is to implement the update the image data algorithm in its entirety. It's a big algorithm that doesn't necessarily break down into substeps very well, except for the bits that delegate to sub-algorithms that deal with responsive images (like selecting an image source).

  1. move all network requests into the DOM (#7708)
  2. make use of the pending and current request data structures in HTMLImageElement
  3. get tests for HTMLImageElement.complete passing
  4. support responsive images (#11416)
@Coder206
Copy link
Contributor

@Coder206 Coder206 commented May 31, 2016

@jdm I am interested in helping out for this issue
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jun 1, 2016 •
edited

@Coder206 This requires a large amount of effort to get it right, and cannot be separated into chunks that can easily be fit into individual PRs. To give some more perspective, this was a student project at NCSU (as noted in #11416) and there were still issues that prevented us from merging their PRs. Are you still certain you are able to work on this?
@Coder206
Copy link
Contributor

@Coder206 Coder206 commented Jun 1, 2016

@KiChjang Thanks for the heads up! It's actually a busy time for me, so that wouldn't have mixed too well. I will work on simpler code for the time being, to get the hang of it, I didn't realize how big of an endeavor it really was. I think it was a little too early for me to start an E-Hard...
@jdm jdm mentioned this issue Feb 28, 2017
Closed
@gterzian
Copy link
Member

@gterzian gterzian commented Mar 15, 2017 •
edited

is anyone working on this at the moment, or planning to? I would be interested otherwise...
@jdm
Copy link
Member Author

@jdm jdm commented Mar 16, 2017 •
edited

@gterzian That would be great! I think the best place to start would be to get the following working:

This means that something like img.src = "foo"; img.src = "bar" should only trigger one request, because the code that actually processes that runs in the stable state that step 6 mentions which happens after the JS stops executing.
@gterzian
Copy link
Member

@gterzian gterzian commented Mar 16, 2017

@jdm thanks for the additional guidance! Ok I think we're almost done with the Blob cloning, so I'll start with this once it's done...
@gterzian
Copy link
Member

@gterzian gterzian commented Mar 23, 2017

I will start looking into this now, perhaps add the 'assigned' label? 😄
@jdm jdm added the C-assigned label Mar 23, 2017
@gterzian gterzian mentioned this issue Apr 3, 2017
Merged
1 of 5 tasks complete
bors-servo added a commit that referenced this issue May 10, 2017
@bors-servo
Auto merge of #16238 - gterzian:implement_update_the_image_data, r=jdm
Loading status checks…
701f27f 
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 added a commit that referenced this issue May 24, 2017
@bors-servo
Auto merge of #16238 - gterzian:implement_update_the_image_data, r=<try>
Loading status checks…
758f41f 
[WIP] 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 added a commit that referenced this issue May 29, 2017
@bors-servo
Auto merge of #16238 - gterzian:implement_update_the_image_data, r=<try>
Loading status checks…
f4333fc 
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 added a commit that referenced this issue May 30, 2017
@bors-servo
Auto merge of #16238 - gterzian:implement_update_the_image_data, r=jdm
Loading status checks…
7f5bc73 
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 added a commit that referenced this issue May 30, 2017
@bors-servo
Auto merge of #16238 - gterzian:implement_update_the_image_data, r=jdm
Loading status checks…
fe7d039 
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/dom A-content/images B-high-value C-assigned E-hard
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Implement "update the image data"
4 participants
@jdm @gterzian @KiChjang @Coder206
You can’t perform that action at this time.