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.

Sign up for GitHub

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

Support partial-URI (relative URL) in Web Bundle format #15

Merged
merged 3 commits into from Sep 22, 2021
Merged

Support partial-URI (relative URL) in Web Bundle format #15

merged 3 commits into from Sep 22, 2021

Conversation

horo-t
Copy link
Collaborator

@horo-t horo-t commented Sep 14, 2021

Closes #14

@horo-t horo-t mentioned this pull request Sep 14, 2021
Closed
jyasskin
jyasskin reviewed Sep 14, 2021
View reviewed changes
draft-ietf-wpack-bundled-responses.md Outdated
as the representation's URL.
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI. In the
latter case, the referenced URL is relative to the bundle's URL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
latter case, the referenced URL is relative to the bundle's URL.
latter case, the referenced URL is relative to the bundle's URL unless a section in the bundle specifies a different base URL.

To allow a separate spec to override this. e.g. the primary-url section should do so.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good.
Done

draft-ietf-wpack-bundled-responses.md Outdated
8.7 of {{!I-D.ietf-httpbis-semantics}}), which holds a URL. This is also known
as the representation's URL.
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI. In the
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not certain whether to cite absolute-URI or a partial-URI ({{Section 2.7 of RFC7230}}) or valid url string without a fragment ({{URL}}) here, but do include the link to the specification that defines the term.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

horo-t
horo-t commented Sep 15, 2021
View reviewed changes
Copy link
Collaborator Author

@horo-t horo-t left a comment

Choose a reason for hiding this comment

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

Thank you very much for reviewing.

draft-ietf-wpack-bundled-responses.md Outdated
8.7 of {{!I-D.ietf-httpbis-semantics}}), which holds a URL. This is also known
as the representation's URL.
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI. In the
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

draft-ietf-wpack-bundled-responses.md Outdated
as the representation's URL.
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI. In the
latter case, the referenced URL is relative to the bundle's URL.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good.
Done

jyasskin
jyasskin approved these changes Sep 15, 2021
View reviewed changes
Copy link
Collaborator

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Looks good, but please do run this by wpack@ietf.org before merging.

@horo-t
Copy link
Collaborator Author

horo-t commented Sep 15, 2021

Thanks. Sent https://mailarchive.ietf.org/arch/msg/wpack/myHwuSoKVdqRxHC6YKCSvPvfpTs/

felipeerias
felipeerias reviewed Sep 16, 2021
View reviewed changes
Copy link
Collaborator

@felipeerias felipeerias left a comment

Choose a reason for hiding this comment

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

Thank you, this change looks good to me. I have a small suggestion below.

draft-ietf-wpack-bundled-responses.md Outdated
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI (Section
2.7 of {{!RFC7230}}). In the latter case, the referenced URL is relative to the
bundle's URL unless a section in the bundle specifies a different base URL.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line would be clearer if it specified that it is not talking about any of the default sections defined in the spec, but about an additional one defined by an extension.

I am not sure about the best way to express this idea, so take this as a suggestion:

Suggested change
bundle's URL unless a section in the bundle specifies a different base URL.
bundle's URL unless an additional section in the bundle specifies a different
base URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

horo-t
horo-t commented Sep 16, 2021
View reviewed changes
Copy link
Collaborator Author

@horo-t horo-t left a comment

Choose a reason for hiding this comment

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

Thanks!

draft-ietf-wpack-bundled-responses.md Outdated
8.7 of {{!I-D.ietf-httpbis-semantics}}). This is also known as the
representation's URL. It is either an absolute-URI or a partial-URI (Section
2.7 of {{!RFC7230}}). In the latter case, the referenced URL is relative to the
bundle's URL unless a section in the bundle specifies a different base URL.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@horo-t horo-t merged commit 24040f8 into wpack-wg:main Sep 22, 2021
@horo-t horo-t mentioned this pull request Sep 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipeerias felipeerias felipeerias left review comments

@jyasskin jyasskin jyasskin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Needs Clarification: A relative URL is supposed to work in index section?
3 participants
@horo-t @jyasskin @felipeerias