[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 #47 from Joseph Pecoraro <joepeck at webkit.org>  2011-01-26 23:13:31 PST ---
(In reply to comment #45)
> (From update of attachment 79986 [details])
> 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/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.

I used CFHTTPMessages in obsoleted patches. I guess I could make yet another platform facade for
handling HTTP Messages, but I don't know how I would be able to share that in WebCore. Would you
be happier with this if the files were renamed to be less generic, and maybe all kept inside of
WebCore/websockets?


> > 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?

In this patch the class WebSocketHandshakeRequest became a subclass of RefCounted HTTPRequest,
and so I found it easier to pass a pointer to the request (refCountedRequest.get()). Is there a convenient
way I could stick with the old prototype?


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

Good catch.


> > 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.

This is unchanged from the existing patch. But yes, it's worth a new bug.
<http://webkit.org/b/53224> Web Socket Console Messages Are Not Localized

> 
> > 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?

Yes, I should probably go with LOG(Network, ...) or removed it entirely.


> > 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.

Sounds good for each of these.

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