Move SpdySessionRequestMap to SpdySessionPool

net/spdy/chromium/spdy_session_pool.cc

Index: net/spdy/chromium/spdy_session_pool.cc
diff --git a/net/spdy/chromium/spdy_session_pool.cc b/net/spdy/chromium/spdy_session_pool.cc
index 135138f4c269bf915c1ca615777dddb678985229..7266e20e04f54ebcdf57ca9bc474f34b2172a117 100644
--- a/net/spdy/chromium/spdy_session_pool.cc
+++ b/net/spdy/chromium/spdy_session_pool.cc
@@ -22,6 +22,8 @@
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source.h"
 #include "net/log/net_log_with_source.h"
+#include "net/spdy/chromium/bidirectional_stream_spdy_impl.h"
+#include "net/spdy/chromium/spdy_http_stream.h"
 #include "net/spdy/chromium/spdy_session.h"
 #include "net/spdy/core/hpack/hpack_constants.h"
 #include "net/spdy/core/hpack/hpack_huffman_table.h"
@@ -72,6 +74,7 @@ SpdySessionPool::SpdySessionPool(
 }
 
 SpdySessionPool::~SpdySessionPool() {
+  DCHECK(spdy_session_request_map_.empty());
   CloseAllSessions();
 
   while (!sessions_.empty()) {
@@ -395,6 +398,71 @@ void SpdySessionPool::OnCertDBChanged() {
   CloseCurrentSessions(ERR_CERT_DATABASE_CHANGED);
 }
 
+void SpdySessionPool::OnNewSpdySessionReady(
+    const base::WeakPtr<SpdySession>& spdy_session,
+    bool direct,
+    const SSLConfig& used_ssl_config,
+    const ProxyInfo& used_proxy_info,
+    bool was_alpn_negotiated,
+    NextProto negotiated_protocol,
+    bool using_spdy,
+    NetLogSource source_dependency) {
+  while (true) {

[Bence, 2017/06/07 17:41:43]

Optional: while(spdy_session) {

[xunjieli, 2017/06/07 21:55:56]

Done.

+    if (!spdy_session)
+      break;
+    const SpdySessionKey& spdy_session_key = spdy_session->spdy_session_key();
+    // Each iteration may empty out the RequestSet for |spdy_session_key| in
+    // |spdy_session_request_map_|. So each time, check for RequestSet and use
+    // the first one.
+    //
+    // TODO(willchan): If it's important, switch RequestSet out for a FIFO
+    // queue (Order by priority first, then FIFO within same priority). Unclear
+    // that it matters here.
+    if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key))

[Bence, 2017/06/07 17:41:43]

ContainsKey and operator[] below are two lookups, you could cut down on one of
them by:
iterator it = find();
if (it == end)
  return;

[xunjieli, 2017/06/07 21:55:56]

Done.

+      break;

[Bence, 2017/06/07 17:41:43]

Optional: return instead of break might reflect our intention better.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    HttpStreamFactoryImpl::Request* request =
+        *spdy_session_request_map_[spdy_session_key].begin();
+    request->Complete(was_alpn_negotiated, negotiated_protocol, using_spdy);
+    if (request->stream_type() == HttpStreamRequest::BIDIRECTIONAL_STREAM) {
+      request->OnBidirectionalStreamImplReady(
+          used_ssl_config, used_proxy_info,
+          new BidirectionalStreamSpdyImpl(spdy_session, source_dependency));
+    } else {
+      bool use_relative_url =
+          direct || request->url().SchemeIs(url::kHttpsScheme);
+      request->OnStreamReady(used_ssl_config, used_proxy_info,
+                             new SpdyHttpStream(spdy_session, use_relative_url,
+                                                source_dependency));
+    }
+  }
+  // TODO(mbelshe): Alert other valid requests.
+}
+
+void SpdySessionPool::RemoveRequestFromSpdySessionRequestMap(
+    HttpStreamFactoryImpl::Request* request) {
+  if (request->HasSpdySessionKey()) {

[Bence, 2017/06/07 17:41:43]

Optional: early return instead of wrapping entire function body in if.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    const SpdySessionKey& spdy_session_key = request->GetSpdySessionKey();
+    DCHECK(base::ContainsKey(spdy_session_request_map_, spdy_session_key));

[Bence, 2017/06/07 17:41:43]

Optional: do only one lookup instead of two in debug build with find(). 
Optional because debug build performance is not critical.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    RequestSet& request_set = spdy_session_request_map_[spdy_session_key];
+    DCHECK(base::ContainsKey(request_set, request));

[Bence, 2017/06/07 17:41:43]

Optional: do only one lookup instead of two in debug build with find(). 
Optional because debug build performance is not critical.

[xunjieli, 2017/06/07 21:55:56]

Acknowledged.
One lookup is in |request_set| and the other is in |spdy_session_request_map_|.

+    request_set.erase(request);
+    if (request_set.empty())
+      spdy_session_request_map_.erase(spdy_session_key);
+    request->ResetSpdySessionKey();
+  }
+}
+
+void SpdySessionPool::AddRequestToSpdySessionRequestMap(
+    const SpdySessionKey& spdy_session_key,
+    HttpStreamFactoryImpl::Request* request) {
+  if (!request->HasSpdySessionKey()) {

[Bence, 2017/06/07 17:41:43]

Optional: early return instead of wrapping entire function body in if.

[xunjieli, 2017/06/07 21:55:56]

Done.

+    RequestSet& request_set = spdy_session_request_map_[spdy_session_key];
+    DCHECK(!base::ContainsKey(request_set, request));

[Bence, 2017/06/07 17:41:42]

Optional: cut down on lookups by
auto result = insert();
DCHECK(result.second);

[xunjieli, 2017/06/07 21:55:56]

Acknowledged.
One lookup is in |request_set| and the other is in |spdy_session_request_map_|.

+    request_set.insert(request);
+    request->SetSpdySessionKey(spdy_session_key);
+  }
+}
+
 void SpdySessionPool::DumpMemoryStats(
     base::trace_event::ProcessMemoryDump* pmd,
     const SpdyString& parent_dump_absolute_name) const {