Introduce RequestWebSocketStream into HttpStreamFactory

net/http/websocket_stream_base.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef NET_HTTP_WEBSOCKET_STREAM_BASE_H_
+#define NET_HTTP_WEBSOCKET_STREAM_BASE_H_
+
+// This file is placed on net/http because net/http classes depends it.
+// Since net/http can be built without linking net/websockets code,
+// you should not introduce dependency to net/websockets in this file.

[mmenke, 2013/05/28 21:22:37]

My suggestion is just to put this in net/websockets.  Since it's a header,
excluding it won't break the build.

[yhirano, 2013/05/30 04:44:32]

Currently, files in net/websockets are excluded in net/net.gyp if
enable_websockets != 1. I think it is difficult to re-include individual files. 
And, now we have net/http/websocket_stream_base.cc in response to your comment.
So I would like to place websocket_stream_base.{h, cc} in net/http.

[mmenke, 2013/05/30 19:27:12]

Header files excluded from the build don't break compilation.  Just cc files. 
Make WebSocketStreamBase pure virtual, and the problem is solved.

I'm also fine with just disabling the WebSocket tests in this CL if WebSockets
are not being built.

[yhirano, 2013/05/31 08:36:45]

Done.

+
+#include <base/basictypes.h>
+
+namespace net {
+
+class ClientSocketHandle;
+class SpdySession;
+
+// WebSocketStreamBase is the base class of WebSocketStream.
+// net/http code can hold WebSocketStream objects via this interface
+// without linking net/websockets code.

[mmenke, 2013/05/28 21:22:37]

Not really following the second sentence here.

[yhirano, 2013/05/30 04:44:32]

Done?

+class WebSocketStreamBase {
+ public:
+  // The enum describing the stream type.
+  // Each type corresponds to a WebSocketStream derived class.
+  enum StreamType {
+    // correspond to WebSocketBasicStream
+    kStreamTypeBasic,
+    // correspond to WebSockeSpdyStream

[mmenke, 2013/05/28 21:22:37]

nit:  Comments should generally start with a capital and end with a period. 
Also, should be "Corresponds" (There's an implied "this" at the start of the
sentence, so it is "[This] corresponds to ..."

[yhirano, 2013/05/30 04:44:32]

Done.

+    kStreamTypeSpdy,
+    // for testing purpose
+    kStreamTypeTest

[mmenke, 2013/05/28 21:22:37]

Prefer not to add this unless / until we need it.

[yhirano, 2013/05/30 04:44:32]

Done.

+  };
+
+  // You can derive this class and implement each factory method.

[mmenke, 2013/05/28 21:22:37]

Comment doesn't add anything useful.

[yhirano, 2013/05/30 04:44:32]

Done.

+  class Factory {

[mmenke, 2013/05/28 21:22:37]

Long term, I'd like to get rid of this class, and just use if-defs on platforms
without WebSockets enabled.  Passing around the factory for every call seems a
bit weird.  Could also just pass on construction, but think it's better to just
get rid of it.

[yhirano, 2013/05/30 04:44:32]

Likely enough. Or, perhaps we may be able to decouple WebSocketStreamFactory and
move it into net/websockets someday.

+   public:
+    // Create a WebSocketBasicStream.
+    // This function (or the returned object) takes the ownership
+    // of |connection|.
+    virtual WebSocketStreamBase* CreateBasicStream(
+        ClientSocketHandle* connection,
+        bool using_proxy) = 0;
+
+    // Create a WebSocketSpdyStream.
+    virtual WebSocketStreamBase* CreateSpdyStream(
+        SpdySession* session,
+        bool use_relative_url) = 0;
+
+    virtual ~Factory() {}

[mmenke, 2013/05/28 21:22:37]

The destructor should go first (It could also be protected, but think we may be
destroying the virtual class directly).

[yhirano, 2013/05/30 04:44:32]

Done.

+  };
+
+  StreamType type() const {
+    return type_;
+  }
+
+  virtual ~WebSocketStreamBase() { }

[mmenke, 2013/05/28 21:22:37]

nit:  Google style guide says no horizontal whitespace in empty function braces.

[yhirano, 2013/05/30 04:44:32]

Done.

+
+ protected:
+  explicit WebSocketStreamBase(StreamType type): type_(type) {}

[mmenke, 2013/05/28 21:22:37]

Mind de-inlining this?

[mmenke, 2013/05/28 21:22:37]

nit:  Space before the colon.

[yhirano, 2013/05/30 04:44:32]

Done.

[yhirano, 2013/05/30 04:44:32]

Done.

+
+ private:
+  const StreamType type_;
+  DISALLOW_COPY_AND_ASSIGN(WebSocketStreamBase);
+};
+
+}  // namespace net
+
+#endif  // NET_HTTP_WEBSOCKET_STREAM_BASE_H_