DevTools: no way to remote debug using ToT build.

DevTools: no way to remote debug using ToT build.
Remote debugging works with Chrome M13 as a client, but not M14.
WebSocket server should be updated.

BUG=90743
TEST=none
R=pfeldman,yurys

[pfeldman, 2011-07-28 09:32:47]

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc
File net/server/http_server.cc (right):

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc#newco...
net/server/http_server.cc:402: if (version.length()) {
compare to "8"?

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc#newco...
net/server/http_server.cc:491: std::string data(sec_web_socket_key);
Use StringPrintf ?

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc#newco...
net/server/http_server.cc:514: struct WebSocketFrame {
Declare this struct in anonymous namespace above.

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc#newco...
net/server/http_server.cc:673: CHECK(!(op_code & ~kOpCodeMask));
DCHECK

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.cc#newco...
net/server/http_server.cc:705: for (size_t i = 0; i < data_length; ++i)
Use  {} since more than one line.

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.h
File net/server/http_server.h (right):

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.h#newcode73
net/server/http_server.h:73: bool use_hybi10_web_socket_;
We should flag legacy protocol use instead (use_hixie76)

http://codereview.chromium.org/7482041/diff/4/net/server/http_server.h#newcode79
net/server/http_server.h:79: bool WebSocketHandshakeHyBi10(Connection*
connection,
Lets keep current version methods with no suffix (WebSocketHandshake in this
case).

[pfeldman, 2011-07-28 14:54:18]

http://codereview.chromium.org/7482041/diff/5001/net/server/http_server.cc
File net/server/http_server.cc (right):

http://codereview.chromium.org/7482041/diff/5001/net/server/http_server.cc#ne...
net/server/http_server.cc:377: if (!ParseHeaders(connection, &request, &pos))
Please add FIXME suggesting that hixie76 frames are parsed outside the
ParseHeaders routine.

http://codereview.chromium.org/7482041/diff/5001/net/server/http_server.cc#ne...
net/server/http_server.cc:498: "HTTP/1.1 101 WebSocket Protocol Handshake\r\n"
wrong indent

http://codereview.chromium.org/7482041/diff/5001/net/server/http_server.cc#ne...
net/server/http_server.cc:513: if (connection->use_hixie76_web_socket_)
Having two web socket implementations with the same interface would simplify
understanding significantly.

http://codereview.chromium.org/7482041/diff/5001/net/server/http_server.cc#ne...
net/server/http_server.cc:528: char* data,
poor indentation.

[loislo, 2011-07-30 18:14:59]

web_socket classes were extracted

[eroman, 2011-08-02 18:46:41]

http://codereview.chromium.org/7482041/diff/8001/net/server/http_server.h
File net/server/http_server.h (right):

http://codereview.chromium.org/7482041/diff/8001/net/server/http_server.h#new...
net/server/http_server.h:30: const std::string& RecvData() const { return
recv_data_; }
nit: recv_data() for accessor.

http://codereview.chromium.org/7482041/diff/8001/net/server/http_server.h#new...
net/server/http_server.h:31: const int Id() const { return id_; }
id()

http://codereview.chromium.org/7482041/diff/8001/net/server/http_server.h#new...
net/server/http_server.h:35: static int lastId_;
last_id_

Also please explain what this is.

http://codereview.chromium.org/7482041/diff/8001/net/server/http_server.h#new...
net/server/http_server.h:43: 
extra space

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc
File net/server/web_socket.cc (right):

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:49: 
Note that virtual dtor is missing here, so this will not clean up properly when
deleted as WebSocket*. See my earlier comment about adding virtual destructor in
base class (interface).

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:169: 
Remove space.

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:172: static WebSocket* create(Connection* connection,
Create

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:218: }
nit: Please mark this with:
  // anonymous

(convention used in other files).

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:275: CHECK(op_code_ == kOpCodeText);
CHECK_EQ ?

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:295: static const uint64 max_payload_ength =
0x7FFFFFFFFFFFFFFFull;
ength --> length

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.cc#new...
net/server/web_socket.cc:321: connection_->RecvData().c_str();
Huh? What is subtracting the c_str() about? That looks fishy, is it correct?

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h
File net/server/web_socket.h (right):

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h#newc...
net/server/web_socket.h:22: static WebSocket* createWebSocket(Connection*
connection,
CreateWebSocket

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h#newc...
net/server/web_socket.h:26: enum ParseResult {
Move this to the top of the public section.

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h#newc...
net/server/web_socket.h:29: FrameError
Chrome is using ALL_UPER_CASE_FOR_ENUMS

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h#newc...
net/server/web_socket.h:34: virtual void Send(const std::string& message) = 0;
Please add a virtual destructor.

I always recommend doing this whenever there are virtual methods, to make sure
we don't accidentally leak in child class by forgetting virtual dtor.

http://codereview.chromium.org/7482041/diff/8001/net/server/web_socket.h#newc...
net/server/web_socket.h:37: WebSocket(Connection* connection);
mark this as explicit.

[eroman, 2011-08-02 18:52:56]

oops, please disregard, wrong review thread :)