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

Integrate Client-Hints with Fetch #258

Closed
wants to merge 1 commit into from

Conversation

igrigorik
Copy link
Member

  • Last-Event-ID is defined in the HTML spec as a utf-8 string, hence we
    don't place any restrictions on its value
  • Each Client Hints header has a BNF grammar that should be validated by
    the user agent; the header is defined as simple if the name of the
    header matches one of CH header names, and its value conforms to the
    defined grammar

Closes:

`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-downlink-client-hint>Downlink</a></code>`,
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-save-data-hint>Save-Data</a></code>`,
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-viewport-width-client-hint>Viewport-Width</a></code>`, or
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-width-client-hint>Width</a></code>`, and whose value, once parsed, is not a failure.
Copy link
Member

Choose a reason for hiding this comment

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

value and once parsed need to be cross-referenced.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, updated.

@annevk
Copy link
Member

annevk commented Mar 22, 2016

I have some concerns here:

  1. Where did this get security review?
  2. Note that in [EventSource] Is Last-Event-ID header value allowed to contain any-char? html#689 we might change the value space of Last-Event-ID.
  3. According to dev.platform the Client-Hints headers are still controversial. Are there any user agents other than Chromium shipping these?

@igrigorik
Copy link
Member Author

Where did this get security review?

For Client Hints, we went through a privacy and security review as part of our intent to ship; same for Save-Data.

Note that in whatwg/html#689 we might change the value space of Last-Event-ID.

Ah, that makes sense. I'm happy to drop Last-Event-ID from this pull until that's resolved.

According to dev.platform the Client-Hints headers are still controversial. Are there any user agents other than Chromium shipping these?

Chrome and Opera supports Width, Viewport-Width, DPR, Save-Data. Yandex is shipping Save-Data. Edge has it listed as under consideration.

@annevk
Copy link
Member

annevk commented Mar 23, 2016

Okay, so other than Chromium there's no other engines yet. That's a bit of a concern. It'd be better if one of Mozilla, Apple, or Microsoft expressed support.

Should we expand the scope of this PR to include httpwg/http-extensions#156?

And yeah, doing Last-Event-ID separately seems better.

@igrigorik
Copy link
Member Author

Dropped Last-Event-ID and took a first run at integrating the proposed logic in httpwg/http-extensions#156 (comment) with Fetch. I'm sure it's rough around the edges, let me know if I'm heading in the right direction.

@igrigorik igrigorik changed the title Treat Last-Event-ID and Client-Hints headers as simple headers Integrate Client-Hints with Fetch Mar 30, 2016
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-viewport-width-client-hint>Viewport-Width</a></code>`, or
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#the-width-client-hint>Width</a></code>`, and whose <span title=concept-header-value>value</span>,
<span title=concept-header-parse>once parsed</span>, is not a failure.
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to have <p>or whose name is one of </p> <ul class=brief> ... </ul> <p>and value, once parsed, is not failure.</p> after the Content-Type entry. It seems a little weird to have a list of header names and then have a list of header names inside that list.

@igrigorik
Copy link
Member Author

@annevk thanks for the review! Updated the pull -- ptal.


<p>A <dfn title=concept-client-hints-list>client hints list</dfn> is a list of
<a href=http://httpwg.org/http-extensions/client-hints.html#rfc.section.2.2.1>Client hint tokens</a>,
which is one of `<code title>dpr</code>`, `<code title>save-data</code>`,
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 this should say "each of which". Otherwise it reads as if the list is a byte sequence.

@annevk
Copy link
Member

annevk commented Apr 1, 2016

Also, the URLs you use don't really give information on the value. And don't use HTTPS.

@igrigorik
Copy link
Member Author

Updated to use a table and fixed the links (good catch!). Hopefully we're getting closer :)

/cc @mnot https://httpwg.org is timing out for me. Is that a known issue?

@annevk
Copy link
Member

annevk commented Apr 1, 2016

  1. The URL fragments seem incorrect. You hyphenate the section titles but the fragment is actually shorter.
  2. Those sections, in your draft, should probably say they define the header field value. A header field consists of a name and a value.
  3. Rather than referencing them as "definition" it would be clearer to use a suitable <a><code>Width</code> value</a> or some such.

@annevk
Copy link
Member

annevk commented Apr 1, 2016

If you could also patch https://github.com/whatwg/xref to add the new reference that'd be great. I'll help out a bit with the final formatting once most is in shape. And yeah, I think it's pretty close.

@igrigorik
Copy link
Member Author

The URL fragments seem incorrect. You hyphenate the section titles but the fragment is actually shorter.

Oi.. Not sure what happened there. Pushed a fix.

Those sections, in your draft, should probably say they define the header field value. A header field consists of a name and a value.

Sounds reasonable, I'll take that as a separate AI.

Rather than referencing them as "definition" it would be clearer to use a suitable Width value or some such.

Replaced "definition" with "suitable value", which links to the definition. Does that look better? I didn't want to repeat the header name.

Re, xref: whatwg/xref#10


Do you want me to squash the changes?

@annevk
Copy link
Member

annevk commented Apr 3, 2016

The problem with not repeating the name of the header (though technically you'd be giving the name of the value production, not the header name) is that then you have six or so links that have an identical name. That is terrible UX for people with screen readers.

@annevk
Copy link
Member

annevk commented Apr 3, 2016

I'll take that as a separate AI

Not sure I understood, should I file an issue somewhere?

@igrigorik
Copy link
Member Author

The problem with not repeating the name of the header (though technically you'd be giving the name of the value production, not the header name) is that then you have six or so links that have an identical name. That is terrible UX for people with screen readers.

Ah, that's fair. Updated.

Not sure I understood, should I file an issue somewhere?

I'll tackle it as part of httpwg/http-extensions#156 (comment).

Anything else left?

`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#downlink>Downlink</a></code>`,
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#save-data>Save-Data</a></code>`,
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#viewport-width>Viewport-Width</a></code>`, or
`<code title><a href=http://httpwg.org/http-extensions/client-hints.html#width>Width</a></code>`
Copy link
Member

Choose a reason for hiding this comment

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

These should all have their own <li>.

Having said that, I would be okay with taking your text, modifying it a bit to match existing editorial style, and then committing it (with you still listed as author of course). The only downside of that is that this PR will be red, not purple. If that is a concern, I'll continue nitpicking a bit.

@igrigorik
Copy link
Member Author

@annevk split the long list into separate li's and squashed the commits. In terms of next steps: whichever is easier for you. I'm happy to continue updating, or you can take it and run with it.. the "color" on the PR doesn't bother me :)

update: rebased to resolve conflicts.


<p class="note no-backref">This can be used to override a client hints list associated with
an <span data-anolis-spec=html>environment settings object</span>.
<span data-anolis-ref>CLIENT-HINTS</span>
Copy link
Member

Choose a reason for hiding this comment

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

CLIENT-HINTS doesn't define a client hints list associated with an environment settings object. What exactly is the purpose of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that part hasn't been specced yet. My intent is to mirror similar language as referrer policy.

  • We can either do that within the CH draft.
  • Or, we can integrate that language directly into the HTML spec?

Any preference on one vs other? /cc @mnot

Copy link
Member

Choose a reason for hiding this comment

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

Not really, as long as it's clearly defined. I guess it would be good to state in the note that this is not yet defined. Or say "will be used" or some such. Then we can update it later once things are defined.

@igrigorik
Copy link
Member Author

@annevk ack on formatting, see 1185bc1.

<a href=http://httpwg.org/http-extensions/client-hints.html#accept-ch>Client hint tokens</a>,
each of which is one of `<code title>dpr</code>`,
`<code title>save-data</code>`, `<code title>viewport-width</code>`, or
`<code title>width</code>`.
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to define this concept ("client hints list") directly in CLIENT-HINTS too, especially if it is going to be reused by multiple concepts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems reasonable. Do you want me to drop this from here then? Or, should we remove this and add a reference once we have the other bits specc'ed in CH/HTML spec.

Copy link
Member

Choose a reason for hiding this comment

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

Either way is fine with me. If you want to go with the former approach I'd appreciate you adding <!-- XXX this needs to move to CLIENT-HINTS --> after this <p>.

@igrigorik
Copy link
Member Author

@annevk ack on formatting, ptal.

Also, as an aside: it'd be great to have a section in the readme, or a standalone doc, with formatting conventions.

@annevk
Copy link
Member

annevk commented Apr 8, 2016

Thanks for mentioning that. I created https://gist.github.com/annevk/2f51a1082850a2814204a9a8a07f9654 as a start. What do you think? It's not entirely clear to me yet where to best host it, since these guidelines are shared across many repositories, but maybe here is fine in a FORMATTING.md file and then the other repositories can point to it. (whatwg/html will need a slightly different one that I can also write I suppose.) @domenic @foolip thoughts?

@domenic
Copy link
Member

domenic commented Apr 8, 2016

Seems OK for DOM and partially for HTML. You should probably also mention your preferences on end tag omission and attribute quoting omission.

I don't think any of those guidelines apply to streams, e.g. there we use 120 chars and Markdown-style lists (in both Bikeshed and with the Ecmarkup integration). So probably introduce this on a repository-by-repository basis.

@foolip
Copy link
Member

foolip commented Apr 8, 2016

I suppose we'll need one style for HTML and one for all other WHATWG specs? Media Session uses 80 columns, FWIW.

Also, the 'Do not use newlines inside "inline" elements' rule seems weird, that's not what we do in HTML.

@annevk
Copy link
Member

annevk commented Apr 8, 2016

Yeah, I figure we need multiple for now. Was just wondering about reuse but I suppose specifications that reuse can point at each other their policy just as we do now.

- DPR, Save-Data, Viewport-Width are sent on navigation requests
- Subresource requests are subject to the set client hints policy
- Client Hints headers are treated as simple headers: each Client Hints
  heaer has a BNF grammar that should be validated bythe user agent.

Closes:
- whatwg#52
- httpwg/http-extensions#141
@igrigorik
Copy link
Member Author

@annevk added the XXX/TODO and squashed -- think it should be good to go!

Re, gist: yes, that's exactly what I was looking for. Now, if only we could coerce tidy into automating this for us... that would be super.

This was referenced Apr 10, 2016
@annevk
Copy link
Member

annevk commented Apr 10, 2016

I landed this as 404dc3a. Purple PR was not possible anyway due to Overview.html not being generated.

I changed the XXX comment to a note and moved it to a more appropriate place.

@annevk annevk closed this Apr 10, 2016
@igrigorik
Copy link
Member Author

👍 looks great, thanks Anne!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants