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

LibHTTP+LibWeb+RequestServer: Add HTTP::HeaderMap + simplify Set-Cookie handling #24553

Merged
merged 3 commits into from
Jun 10, 2024
Merged

LibHTTP+LibWeb+RequestServer: Add HTTP::HeaderMap + simplify Set-Cookie handling #24553

merged 3 commits into from
Jun 10, 2024

Conversation

kennethmyhra
Copy link
Member

@kennethmyhra kennethmyhra commented Jun 9, 2024 •
edited
Loading

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 9, 2024
@tcl3 tcl3 added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jun 9, 2024
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed labels Jun 9, 2024
@kennethmyhra
Copy link
Member Author

Missed a spot Fetch::Fetching::log_response(), not sure why that didn't fail locally 🤔

@nico
Copy link
Contributor

nico commented Jun 9, 2024

Hm, how should we handle commit messages for cherry-picks that break the build and need fixups?

  • just sneak it into the cherry-pick?
  • tweak commit message to say "cherry-picked from abcde and then changed XXX to fix compile"?
  • put the fix in a separate commit? (kinda cleanest, but breaks the "every revision must build" invariant, so that's a nonstarter)

In some cases it might be possible to do the fixup in a commit before the breaking upstream change. Is that possible here? If not, then "cherry-pick with amended commit message" is maybe best?

@kennethmyhra
Copy link
Member Author

This what I did: just sneak it into the cherry-pick?. Not possible to sneak it in before or after without breaking the build.

I can add a note to the commits, for this one that would basically be:

Updated various SerenityOS components to make it build.

The Fetching::Fetch::log_response() fix I mentioned above should though have broken the build in Ladybird as well. Seems it does so on our GNU DEBUG_ALL pipeline.

@kennethmyhra kennethmyhra marked this pull request as draft June 9, 2024 17:23
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 9, 2024
@nico
Copy link
Contributor

nico commented Jun 9, 2024

Adding something like "Updated to fix DEBUG_ALL build" to the cherry pick commit message makes sense to me :)

@kennethmyhra
Copy link
Member Author

It breaks on Ladybird too if I build with -DENABLE_ALL_THE_DEBUG_MACROS=ON. I'll make a PR against Ladybird to fix that and cherry pick it back in this PR before we merge.

@kennethmyhra
Copy link
Member Author

Put up the mentioned fix here: LadybirdBrowser/ladybird#119

@nico
Copy link
Contributor

nico commented Jun 9, 2024 •
edited
Loading

It breaks on Ladybird too if I build with -DENABLE_ALL_THE_DEBUG_MACROS=ON. I'll make a PR against Ladybird to fix that and cherry pick it back in this PR before we merge.

Thinking about this again, this doesn't help with the "every commit should build". I suppose we could squash the two cherry-picks and put two "cherry-picked commit XXX" lines in the squashed commit message.

@awesomekling @kennethmyhra
Everywhere: Add HTTP::HeaderMap and use for response headers
7d9a04e 
Instead of using a HashMap<ByteString, ByteString, CaseInsensitive...>
everywhere, we now encapsulate this in a class.

Even better, the new class also allows keeping track of multiple headers
with the same name! This will make it possible for HTTP responses to
actually retain all their headers on the perilous journey from
RequestServer to LibWeb.

(cherry picked from commit e636851481eabdf00953573a5eb459ee52feeacc)

Updated various SerenityOS components to make it build.

Fetch: Make sure we iterate over HeaderMap's headers()

This fixes a build failure when built with CMake option
'-DENABLE_ALL_THE_DEBUG_MACROS=ON'.

(cherry picked from commit c51d01bea712d75f9b2cd700be942935044e49b4)
@awesomekling @kennethmyhra
LibHTTP+LibWeb: Stop bundling "Set-Cookie" headers as JSON
7051974 
Before we had HTTP::HeaderMap (which preserves multiple headers with the
same name), we collected multiple "Set-Cookie" headers and bundled them
together as a JSON array.

This was a huge hack, and now we can stop doing that, since LibWeb gets
access to the full set of headers now.

(cherry picked from commit 5ac093885922246529a467054888e598f8832450)
@awesomekling @kennethmyhra
Everywhere: Use HTTP::HeaderMap for request headers
06a1408 
No longer just for response headers! The same type is obviously useful
and ergonomic when making requests as well.

(cherry picked from commit 260c5c50ad19f19d0d4c30984e512f56c055ecff)

Updated various SerenityOS components to make it build.
@kennethmyhra
Copy link
Member Author

I squashed commit: LadybirdBrowser/ladybird@c51d01b into the first one where the change/breakage occurs.

@kennethmyhra kennethmyhra marked this pull request as ready for review June 10, 2024 05:48
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 10, 2024
@alimpfard alimpfard merged commit 8994dcb into SerenityOS:master Jun 10, 2024
13 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 10, 2024
@kennethmyhra kennethmyhra deleted the http-headers branch June 10, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcl3 tcl3 tcl3 approved these changes

@alimpfard alimpfard Awaiting requested review from alimpfard alimpfard is a code owner

@trflynn89 trflynn89 Awaiting requested review from trflynn89 trflynn89 is a code owner

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@kennethmyhra @nico @tcl3 @alimpfard @awesomekling