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

[fetch] Implement Headers.prototype.getSetCookie. #9490

Merged
merged 1 commit into from Feb 20, 2023

Conversation

andreubotella
Copy link
Contributor

@andreubotella andreubotella commented Feb 1, 2023

77fd8de

[fetch] Implement `Headers.prototype.getSetCookie`.
https://bugs.webkit.org/show_bug.cgi?id=251194

Reviewed by Youenn Fablet.

This change implements the `getSetCookie` method of the fetch spec's
`Headers` interface. This change allows storing `Set-Cookie` headers as
a list of values, and retrieving the separate values, rather than their
combined representation, which is ambiguous. This change also makes it
so iterating through `Headers` objects will yield `Set-Cookie` headers
separately rather than combined, which is part of the same change in the
fetch spec.

Given that `Set-Cookie` is a forbidden header name in both requests and
responses, this will not be useful for any uses of `Headers` related to
fetch. It does allows some non-fetch use cases, however, and enables
further compatibility with server-side runtimes that do allow this
header.

Rather than refactoring `HTTPHeaderMap` to not combine (all) headers,
this implementation makes use of the fact that `Set-Cookie` is forbidden
for both requests and responses, to store the `Set-Cookie` values in a
vector of strings directly in `FetchHeaders` instead. This needed such
header name to be special-cased in almost every operation of this class.
And in order to have `Set-Cookie` headers properly appear in their right
order in the pair iterator, `m_keys` has been changed to be a pair where
the second element is an index into the `Set-Cookie` values vector,
which is ignored for other header names.

Fetch spec PR: whatwg/fetch#1346

This commit also imports the corresponding WPT tests from
web-platform-tests/wpt#31442, and adds some
additional tests.

* Source/WebCore/Modules/fetch/FetchHeaders.cpp:
(WebCore::appendToHeaderMap):
(WebCore::fillHeaderMap):
(WebCore::FetchHeaders::create):
(WebCore::FetchHeaders::fill):
(WebCore::FetchHeaders::append):
(WebCore::FetchHeaders::remove):
(WebCore::FetchHeaders::get const):
(WebCore::FetchHeaders::getSetCookie const):
(WebCore::FetchHeaders::has const):
(WebCore::FetchHeaders::set):
(WebCore::FetchHeaders::Iterator::next):
* Source/WebCore/Modules/fetch/FetchHeaders.h:
(WebCore::FetchHeaders::create):
(WebCore::FetchHeaders::fastGet const):
(WebCore::FetchHeaders::fastHas const):
(WebCore::FetchHeaders::fastSet):
(WebCore::FetchHeaders::FetchHeaders):
* Source/WebCore/Modules/fetch/FetchHeaders.idl:
* Source/WebCore/platform/network/ResourceResponseBase.cpp:
(WebCore::ResourceResponseBase::httpHeaderField const):

Canonical link: https://commits.webkit.org/260533@main

c6b5177

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac ❌ πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim   πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@andreubotella
Copy link
Contributor Author

andreubotella commented Feb 1, 2023

I didn't add tests because there is a WPT PR (web-platform-tests/wpt#31442) that hasn't been merged yet. Or should I add that test to this PR for now, to run it under CI?

I would appreciate comments on the iterator changes and on the fast methods.

I also need to document this to some degree.

Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

Some suggestions below.
This patch needs tests, ideally WPT tests.

Source/WebCore/Modules/fetch/FetchHeaders.cpp Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.h Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
@andreubotella
Copy link
Contributor Author

Some suggestions below. This patch needs tests, ideally WPT tests.

About WPT, there's a PR at web-platform-tests/wpt#31442. I just pushed a few additional tests there, including one for iteration invalidation.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 3, 2023
@andreubotella andreubotella changed the title [fetch] Implement Headers.prototype.getSetCookie. Add WPT tests Feb 8, 2023
@andreubotella andreubotella changed the title Add WPT tests [fetch] Implement Headers.prototype.getSetCookie Feb 8, 2023
@andreubotella andreubotella changed the title [fetch] Implement Headers.prototype.getSetCookie [fetch] Implement Headers.prototype.getSetCookie. Feb 8, 2023
@andreubotella
Copy link
Contributor Author

Sorry for the PR rename churn, this is my first time working with Webkit's Github workflow, and I didn't realize git-webkit pr treated multiple commits as a stack of changes.

Anyway, the WPT PR has now been merged (as well as the spec PR), and I've updated this PR to include those tests.


if (equalIgnoringASCIICase(name, "Set-Cookie"_str)) {
if (m_setCookieValues.isEmpty())
return String();
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably do not need to optimize this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just about the size-0 case, right? The size-1 optimization can stay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I hadn't realized that returning String() on the size-0 case isn't an optimization, since get() must return null. The size-1 case is an optimization, but not apparently one that would achieve anything, since it seems like StringBuilder directly uses the passed string in this case.

I'm therefore keeping the size-0 case and removing the size-1 case.

Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
Source/WebCore/Modules/fetch/FetchHeaders.cpp Show resolved Hide resolved
@youennf
Copy link
Contributor

youennf commented Feb 9, 2023

Looks almost ready to me, some nits below.
If you made some WPT test changes, please put up a WPT PR for it.

@andreubotella
Copy link
Contributor Author

Looks almost ready to me, some nits below. If you made some WPT test changes, please put up a WPT PR for it.

I opened web-platform-tests/wpt#38437

@andreubotella
Copy link
Contributor Author

@youennf PTAL

Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM

Source/WebCore/Modules/fetch/FetchHeaders.cpp Outdated Show resolved Hide resolved
@youennf youennf added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Feb 20, 2023
https://bugs.webkit.org/show_bug.cgi?id=251194

Reviewed by Youenn Fablet.

This change implements the `getSetCookie` method of the fetch spec's
`Headers` interface. This change allows storing `Set-Cookie` headers as
a list of values, and retrieving the separate values, rather than their
combined representation, which is ambiguous. This change also makes it
so iterating through `Headers` objects will yield `Set-Cookie` headers
separately rather than combined, which is part of the same change in the
fetch spec.

Given that `Set-Cookie` is a forbidden header name in both requests and
responses, this will not be useful for any uses of `Headers` related to
fetch. It does allows some non-fetch use cases, however, and enables
further compatibility with server-side runtimes that do allow this
header.

Rather than refactoring `HTTPHeaderMap` to not combine (all) headers,
this implementation makes use of the fact that `Set-Cookie` is forbidden
for both requests and responses, to store the `Set-Cookie` values in a
vector of strings directly in `FetchHeaders` instead. This needed such
header name to be special-cased in almost every operation of this class.
And in order to have `Set-Cookie` headers properly appear in their right
order in the pair iterator, `m_keys` has been changed to be a pair where
the second element is an index into the `Set-Cookie` values vector,
which is ignored for other header names.

Fetch spec PR: whatwg/fetch#1346

This commit also imports the corresponding WPT tests from
web-platform-tests/wpt#31442, and adds some
additional tests.

* Source/WebCore/Modules/fetch/FetchHeaders.cpp:
(WebCore::appendToHeaderMap):
(WebCore::fillHeaderMap):
(WebCore::FetchHeaders::create):
(WebCore::FetchHeaders::fill):
(WebCore::FetchHeaders::append):
(WebCore::FetchHeaders::remove):
(WebCore::FetchHeaders::get const):
(WebCore::FetchHeaders::getSetCookie const):
(WebCore::FetchHeaders::has const):
(WebCore::FetchHeaders::set):
(WebCore::FetchHeaders::Iterator::next):
* Source/WebCore/Modules/fetch/FetchHeaders.h:
(WebCore::FetchHeaders::create):
(WebCore::FetchHeaders::fastGet const):
(WebCore::FetchHeaders::fastHas const):
(WebCore::FetchHeaders::fastSet):
(WebCore::FetchHeaders::FetchHeaders):
* Source/WebCore/Modules/fetch/FetchHeaders.idl:
* Source/WebCore/platform/network/ResourceResponseBase.cpp:
(WebCore::ResourceResponseBase::httpHeaderField const):

Canonical link: https://commits.webkit.org/260533@main
@webkit-commit-queue
Copy link
Collaborator

Committed 260533@main (77fd8de): https://commits.webkit.org/260533@main

Reviewed commits have been landed. Closing PR #9490 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 20, 2023
@webkit-early-warning-system webkit-early-warning-system merged commit 77fd8de into WebKit:main Feb 20, 2023
@andreubotella andreubotella deleted the getSetCookie branch February 20, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants