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

Address client-hints-04 feedback #361

Merged
merged 13 commits into from
Jul 21, 2017
Merged

Address client-hints-04 feedback #361

merged 13 commits into from
Jul 21, 2017

Conversation

igrigorik
Copy link
Member

See #359, #360.

@igrigorik igrigorik requested review from mnot and reschke June 26, 2017 20:57
This was referenced Jun 26, 2017
Copy link
Member

@mnot mnot left a comment

Choose a reason for hiding this comment

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

LGTM. I suspect you might have people bring up further nits about the boilerplate, etc. in IETF LC, but let's let them raise them (gives people something to do :)

Go ahead and ship a new draft and we'll kick it down the road.

@mnot
Copy link
Member

mnot commented Jun 27, 2017

Oh, one thing -- you removed Downlink. It'd be good to mention that in the change notes, and on list, so people are aware.

@@ -89,7 +89,7 @@ Client Hints does not supersede or replace the User-Agent header field. Existing

## Notational Conventions

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in {{RFC2119}}.
The key words "MUST", "MUST NOT", "REQUIRED", "SHOULD", "SHOULD NOT", "MAY", and "OPTIONAL" in this document are to be interpreted as described in BCP 14 {{RFC2119}} when, and only when, they appear capitalized, as shown.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be consistent with https://tools.ietf.org/html/rfc8174#section-2

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, looks like I took the text from outdated draft.

@igrigorik
Copy link
Member Author

Thanks guys, pushed a few more updates.

@reschke everything else looks good? If so I'll squash+push a new draft.

@igrigorik
Copy link
Member Author

@reschke can you take another pass, please? Want to make sure we caught all of your feedback here.

@igrigorik igrigorik merged commit 110d0cb into master Jul 21, 2017
@igrigorik
Copy link
Member Author

@reschke merging. If you have any followup feedback, we can open a new issue.

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

Successfully merging this pull request may close these issues.

3 participants