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

LibRequests+LibWeb+RequestServer: Propagate HTTP reason phrase #1876

Merged
merged 2 commits into from
Nov 2, 2024

Conversation

rmg-x
Copy link
Contributor

@rmg-x rmg-x commented Oct 20, 2024

Fixes 54 test cases across the following WPT tests :^)

@rmg-x rmg-x changed the title LibRequests+LibWeb+RequestServer: Add and assign optional reason phrase LibRequests+LibWeb+RequestServer: Propagate HTTP reason phrase Oct 22, 2024
@rmg-x rmg-x force-pushed the http-reason-phrase branch 4 times, most recently from 2371877 to 8fba95b Compare October 23, 2024 21:46
@rmg-x rmg-x marked this pull request as ready for review October 23, 2024 22:41
@rmg-x rmg-x requested a review from alimpfard as a code owner October 23, 2024 22:41
@rmg-x
Copy link
Contributor Author

rmg-x commented Oct 23, 2024

Marking this ready for review -- I don't see a good way of adding in-tree tests for this without something like an echo server proposed in #1801

@rmg-x rmg-x force-pushed the http-reason-phrase branch from 8fba95b to 843aba5 Compare October 25, 2024 23:01
@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@rmg-x rmg-x force-pushed the http-reason-phrase branch 2 times, most recently from fb80fec to c43dc9a Compare October 25, 2024 23:55
@rmg-x rmg-x force-pushed the http-reason-phrase branch 4 times, most recently from 7b5c90c to 8e9ab9e Compare October 30, 2024 22:56
@@ -27,7 +27,7 @@ target_link_libraries(RequestServer PRIVATE requestserverservice)

target_include_directories(requestserverservice PRIVATE ${LADYBIRD_SOURCE_DIR}/Userland/Services/)
target_include_directories(requestserverservice PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/..)
target_link_libraries(requestserverservice PUBLIC LibCore LibMain LibCrypto LibFileSystem LibIPC LibMain LibTLS LibWebView LibWebSocket LibURL LibThreading CURL::libcurl)
target_link_libraries(requestserverservice PUBLIC LibCore LibMain LibCrypto LibFileSystem LibIPC LibMain LibTLS LibWeb LibWebView LibWebSocket LibURL LibThreading CURL::libcurl)
Copy link
Contributor

Choose a reason for hiding this comment

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

LibWeb LibWebView

Not exactly your fault but...why does RS need to link against anything libweb?

Copy link
Contributor

Choose a reason for hiding this comment

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

This really goes way back: 58e3b8c (though it makes sense in a weird way)
Also cc @trflynn89...that seems like a hack at best? why not depend directly on generate_RequestClientEndpoint.h and generate_RequestServerEndpoint.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a really hacky way to get IPC-generated files into the ladybird build, back when we had to deal with both lagom and serenity sources:

if (ENABLE_LAGOM_LIBWEB)
list(APPEND lagom_standard_libraries Web)
# WebView
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/AccessibilityTreeModel.cpp")
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/DOMTreeModel.cpp")
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/StylePropertiesModel.cpp")
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/ViewImplementation.cpp")
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/WebContentClient.cpp")
list(APPEND LIBWEBVIEW_SOURCES "../../Userland/Libraries/LibWebView/RequestServerAdapter.cpp")
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/WebContent/WebContentServer.ipc WebContent/WebContentServerEndpoint.h)
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/WebContent/WebContentClient.ipc WebContent/WebContentClientEndpoint.h)
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/WebContent/WebDriverClient.ipc WebContent/WebDriverClientEndpoint.h)
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/WebContent/WebDriverServer.ipc WebContent/WebDriverServerEndpoint.h)
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/RequestServer/RequestClient.ipc RequestServer/RequestClientEndpoint.h)
compile_ipc(${SERENITY_PROJECT_ROOT}/Userland/Services/RequestServer/RequestServer.ipc RequestServer/RequestServerEndpoint.h)

Don't know if we're still doing this, but I can't imagine there's a reason we need to continue that hack

Copy link
Contributor

Choose a reason for hiding this comment

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

We're still doing that (roughly, those are listed in the GENERATED_SOURCES for WebView), but even then there's no real reason to actually link that thing in afaict

Copy link
Member

Choose a reason for hiding this comment

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

In the gn build we generate the header in the client library build script. I would prefer that here in CMake as well.

auto const reason_phrase_string_view = header_line.substring_view(second_space_offset + 1).trim_whitespace();

if (!reason_phrase_string_view.is_empty()) {
request->reason_phrase = Web::Infra::isomorphic_decode(reason_phrase_string_view.bytes());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to link against libweb just to get a stringbuilder + append_code_point in a loop?
Also cc @trflynn89 (iirc you merged that), maybe we could put that function in the header or just inline here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could also be moved to LibTextCodec

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 PR was previously linking against LibTextCodec and just using a Latin1 decoder directly. Should I just revert to using that again?

@rmg-x rmg-x force-pushed the http-reason-phrase branch from 8e9ab9e to e89e084 Compare November 1, 2024 23:41
@rmg-x rmg-x requested a review from alimpfard November 2, 2024 15:16
@alimpfard
Copy link
Contributor

Alright, let's get this in the tree, we can fix the dependency problem later.

@alimpfard alimpfard merged commit c042971 into LadybirdBrowser:master Nov 2, 2024
6 checks passed
@rmg-x rmg-x deleted the http-reason-phrase branch November 2, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants