[Webkit-unassigned] [Bug 51364] Web Inspector: Remote Web Inspector - Cross Platform InspectorServer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51364





--- Comment #45 from Alexey Proskuryakov <ap at webkit.org>  2011-01-26 17:21:48 PST ---
(From update of attachment 79986)
View in context: https://bugs.webkit.org/attachment.cgi?id=79986&action=review

In general, I'm not thrilled to see generic HTTP parsing code in WebCore. Can you use CFHTTPMessage instead?

WebSockets got a pass because it's not quite HTTP, and we have some parsing methods where we don't care about precision too much, like in XMLHttpRequest. But using custom HTTP message parsing code for something as subtle as a server (and having it in classes with such generic names) is scary.

> Source/WebCore/ChangeLog:31
> +        (WebCore::parseHTTPHeaders): taken from previous WebSocket parsing. Algorithm unchanged.

This isn't good. WebSocket has all sorts of limitations on how headers can look like - for example, there is no support for continuations.

> Source/WebCore/inspector/InspectorInstrumentation.h:264
> -    static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, const WebSocketHandshakeRequest&);
> +    static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, WebSocketHandshakeRequest*);

I don't understand these changes. Can the argument be null?

> Source/WebCore/platform/network/HTTPParseError.h:43
> +    HTTPParseError(const String& description)
> +        : description(description)
> +    {
> +    }

This should initialize wasLikelyIncompleteInput.

> Source/WebCore/platform/network/HTTPParsers.cpp:414
> +        error.description = "Incomplete Request Line";

WebCore calls out to WebKit for error descriptions, because they generally need to be localized.

> Source/WebCore/platform/network/HTTPParsers.cpp:513
> +        AtomicString nameStr(String::fromUTF8(name.data(), name.size()));
> +        String valueStr = String::fromUTF8(value.data(), value.size());

HTTP headers are not required to be UTF-8, and frequently aren't.

> Source/WebCore/platform/network/HTTPParsers.cpp:524
> +        // FIXME: should not be log error, should be some other type of logging.
> +        LOG_ERROR("name=%s value=%s", nameStr.string().utf8().data(), valueStr.utf8().data());

Did you intend to address this before submitting?

> Source/WebCore/platform/network/HTTPRequest.cpp:92
> +    : m_url(KURL())

This isn't needed.

> Source/WebCore/platform/network/HTTPRequest.cpp:94
> +    , m_requestMethod(String())

Neither is this.

> Source/WebCore/websockets/WebSocketHandshake.cpp:500
> +    HTTPParseError error = HTTPParseError();

HTTPParseError has a default constructor, no need to initialize it like a struct.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.