Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on significant internal refactoring of the HTTP and WebSocket layers to enhance code organization and readability. It centralizes callback definitions into dedicated structs, breaks down a complex data handling method into more manageable private functions, and consolidates duplicated WebSocket close notification logic. These changes are purely structural, ensuring no alterations to existing behavior or introduction of runtime overhead, thereby improving the maintainability and clarity of the codebase. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request refactors callback management for HTTP and WebSocket connections by introducing a new http_callbacks.h header. This centralizes callback type definitions into HttpConnCallbacks and WsCallbacks structs, replacing individual callback members in HttpConnectionHandler and WebSocketConnection for improved organization. The HttpConnectionHandler::OnRawData method was also refactored into smaller, more readable helper methods, and a SafeNotifyWsClose helper was added to HttpServer for robust WebSocket close notifications. A review comment suggests shortening the HTTP_CALLBACKS_NAMESPACE name for better readability.
| struct HttpRequest; | ||
| class HttpResponse; | ||
|
|
||
| namespace HTTP_CALLBACKS_NAMESPACE { |
Summary
Align the HTTP/WS layer's callback organization with the base reactor's established pattern, and improve readability of the largest method in the codebase.
Changes
Callback Registry Extraction
include/http/http_callbacks.hwithHTTP_CALLBACKS_NAMESPACEcontaining:HttpConnCallbacksWsCallbacksinclude/callbacks.horganizes core reactor callbacks.HttpConnectionHandler:usingdeclarations and 4 individual member fieldscallbacks_structWebSocketConnection:callbacks_structOnRawData Split
Split
HttpConnectionHandler::OnRawData()(~500 lines) into 4 private member functions:HandleUpgradedData()HandleParseError()HandleCompleteRequest()HandleIncompleteRequest()Expectheader handling, and body-size checksNotes:
std::functioncallbacks were introducedHelper Extraction
HttpServer::SafeNotifyWsClose()try/catchWebSocket close-notify blocks across:HandleNewConnectionHandleCloseConnectionHandleErrorConnectionHandleMessageTest Plan
make clean && make -j4compiles cleanly51/51tests pass via./runNotes