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

Add support for Content-DPR response header #5112

Closed
wants to merge 13 commits into from
Closed

Conversation

eeeps
Copy link
Contributor

@eeeps eeeps commented Nov 26, 2019

This WIP PR adds support for the Content-DPR response header on image responses (see explainer here).

Bonus: here's Chrome's usage tracking.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 15, 2021, 8:00 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

<html>
<head><title>504 Gateway Time-out</title></head>
<body bgcolor="white">
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>nginx/1.10.3</center>
</body>
</html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@rniwa
Copy link

rniwa commented Nov 26, 2019

Note that having this PR does not constitute an implementation interest in WebKit. It’s simply a prerequisite.

source Outdated
<p>Wait until the result of fetching is either a <span data-x="concept-response">response</span>
or an error. If it is a <span data-x="concept-response">response</span>, and the
<span data-x="concept-response">response</span> has an
<span data-x="concept-response-image-density">image density</span> that is not null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not parse for the response header here? Are you trying to avoid having service workers set it or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just wasn't aware that parsing for headers before a Response exists was an option. Any pointers to another section of the spec that does this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before a response exists? You're holding a response here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's my confusion. Is a Response object available as soon as step 21 completes, before we get to the networking task source stuff? How do I wait for the image density property I tried to define here whatwg/fetch#971 to be populated, and is it guaranteed to be populated before the first chunks of image data are ready to paint?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be better to make this part of the next step yeah, once the first task is queued. You'll then have a response (and potentially some initial bytes as well, but definitely all the headers).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it, per this and our conversation on IRC

@annevk
Copy link
Member

annevk commented Jan 24, 2020

I guess another problem here is that image fetching isn't properly abstracted yet and used by all things that fetch images. So technically this would only work for img.

…loads

...and point to an issue about unifying image loading, generally.
@annevk
Copy link
Member

annevk commented Feb 7, 2020

@eeeps I thought I'd state it again for clarity, I don't think we need any changes to Fetch here. HTML can be in charge of parsing the header and forwarding the information to the image decoder.

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: img labels Feb 7, 2020
- add reference to Fetch's `get a structured header` algorithm
- add reference to the Structured Headers spec, and its definitions of Integers and Decimals
- get the Content-DPR header and check that it's valid ourselves, right here in HTML, before setting the image density
@eeeps
Copy link
Contributor Author

eeeps commented Feb 22, 2020

@annevk Ok, closed the Fetch PR, called out to Fetch's get a strucructured header, and am validating the result against the Structured Headers definitions of Integer/Decimal. That seemed like the cleanest way to parse, but am I off base?

@sideshowbarker
Copy link
Contributor

I don't know what's causing the PR Preview error. Maybe it's some intermittent failure. I'll try to figure out if there's a way to trigger it to rebuild.

@annevk
Copy link
Member

annevk commented Feb 25, 2020

  1. You should add an IANA template similar to what we have for the Refresh and Ping-To headers and such.
  2. I think we should state that it's expected to apply to all image fetches (and have tests for that). That way the only cleanup that remains to be done is abstracting the algorithm and getting it appropriately invoked. That also avoids potential forward compatibility issues with sites not expecting Content-DPR to work for non-img sources.

@eeeps
Copy link
Contributor Author

eeeps commented Feb 25, 2020

@annevk great. I agree on №2; we have tests for CSS background-images but not fetches whose destination is image. Other contexts that come to mind:

  • SVG <image>
  • <video poster>
  • Canvas
  • <button type=image>

Not sure if Content-DPR should have effects within all of these, but I'll think through them and flesh out + toughen up the note (and work on tests). Any other contexts, that I haven't thought of?


Update:

  • I don't think the intrinsic dimensions of a fetch destination=image Response are inspectable in any way before that image is used somewhere; perhaps that path only makes sense to test in conjunction with some or all of the possible rendering contexts.
  • In the same vein, we should test <link rel=preload>-fetched images, I think...
  • <object> is another context...

@eeeps
Copy link
Contributor Author

eeeps commented Feb 26, 2020

@annevk added the IANA section. The other headers had their <dfn>s down there; I left mine in the prose? If you have any opinions on where the <dfn> should live, let me know.

@noamr also added a bunch of tests web-platform-tests/wpt#21962

@eeeps
Copy link
Contributor Author

eeeps commented Feb 26, 2020

Ok, added the expectation that Content-DPR applies everywhere, too. (Maybe a bit wordy?)

@eeeps
Copy link
Contributor Author

eeeps commented Feb 26, 2020

Hm, I think I need to specify that Content-DPR values must be >0 (and what to do when they're not).

@noamr
Copy link
Contributor

noamr commented Feb 26, 2020

Hm, I think I need to specify that Content-DPR values must be >0 (and what to do when they're not).

I believe Content-DPR should be ignored for <= 0

@annevk
Copy link
Member

annevk commented Mar 2, 2020

CSS has a large number of properties that take images in one form or another. And yeah, this can only be tested on a per-endpoint basis unfortunately. Looks pretty good otherwise. I guess the main things remaining are implementer interest and someone verifying the test coverage. (Happy to help with the latter once the former is sorted.)

Base automatically changed from master to main January 15, 2021 07:57
@eeeps
Copy link
Contributor Author

eeeps commented Mar 3, 2021

Abandoned this approach; switched to using EXIF information for these use cases, rather than an HTTP header. #5574

@eeeps eeeps closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: img
Development

Successfully merging this pull request may close these issues.

5 participants