Server push cancellation: add PushPromiseHelper which reflects information on the push promise.

net/spdy/spdy_session.cc

Index: net/spdy/spdy_session.cc
diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index b79c5e62e3d17f2745c369428c8ac35d9d663349..be1b0d0be6b51af1cdb8f5e860b2bf2a55662751 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -295,6 +295,25 @@ class RequestEquals {
 // the server permits more, we will never exceed this limit.
 const size_t kMaxConcurrentStreamLimit = 256;
 
+class SpdyPushPromiseHelper : public PushPromiseHelper {
+ public:
+  explicit SpdyPushPromiseHelper(base::WeakPtr<SpdySession> session,
+                                 const GURL& url)
+      : session_(session), request_url_(url) {}
+
+  void Cancel() override {
+    if (session_) {
+      session_->CancelPush(request_url_);
+    }

[Ryan Hamilton, 2016/10/28 00:32:22]

nit: no {}s on single line ifs in net code.

[Zhongyi Shi, 2016/11/07 22:07:03]

Done.

+  }
+
+  const GURL& GetURL() override { return request_url_; }
+
+ private:
+  base::WeakPtr<SpdySession> session_;
+  const GURL request_url_;
+};
+
 }  // namespace
 
 SpdyProtocolErrorDetails MapFramerErrorToProtocolError(
@@ -644,6 +663,7 @@ SpdySession::SpdySession(const SpdySessionKey& spdy_session_key,
       stream_hi_water_mark_(kFirstStreamId),
       last_accepted_push_stream_id_(0),
       unclaimed_pushed_streams_(this),
+      push_delegate_(nullptr),
       num_pushed_streams_(0u),
       num_active_pushed_streams_(0u),
       bytes_pushed_count_(0u),
@@ -782,6 +802,21 @@ int SpdySession::GetPushStream(const GURL& url,
   return OK;
 }
 
+void SpdySession::CancelPush(const GURL& url) {

[Ryan Hamilton, 2016/10/28 00:32:22]

Instead of canceling by URL, if we passed the promised stream ID in to the
helper, then the helper could cancel by stream ID which seems like it might be
cleaner. What do you think?

[Zhongyi Shi, 2016/11/07 22:07:04]

We could definitely do that, but considering we will check whether the push has
been claimed before cancelling from the UnclaimedPushedStreamContainer which is
key'd by url. I think we still need to pass in the url in your proposed method,
which makes me feel passing the url is enough. 

+  UnclaimedPushedStreamContainer::const_iterator unclaimed_it =
+      unclaimed_pushed_streams_.find(url);
+  if (unclaimed_it == unclaimed_pushed_streams_.end())
+    return;
+
+  SpdyStreamId stream_id = unclaimed_it->second.stream_id;
+
+  if (active_streams_.find(stream_id) == active_streams_.end()) {
+    ResetStream(stream_id, RST_STREAM_CANCEL,
+                "Cancelled push stream with url: " + url.spec());
+  }
+  unclaimed_pushed_streams_.erase(unclaimed_it);
+}
+
 // {,Try}CreateStream() can be called with |in_io_loop_| set if a stream is
 // being created in response to another being closed due to received data.
 
@@ -2633,27 +2668,18 @@ bool SpdySession::TryCreatePushStream(SpdyStreamId stream_id,
     return false;
   }
 
+  // Notify the push_delegate that a push promise has been received.
+  if (push_delegate_) {
+    push_delegate_->OnPush(base::MakeUnique<SpdyPushPromiseHelper>(
+        weak_factory_.GetWeakPtr(), gurl));
+  }
+
   active_it->second.stream->OnPushPromiseHeadersReceived(std::move(headers));
   DCHECK(active_it->second.stream->IsReservedRemote());
   num_pushed_streams_++;
   return true;
 }
 
-void SpdySession::CancelPush(const GURL& url) {
-  UnclaimedPushedStreamContainer::const_iterator unclaimed_it =
-      unclaimed_pushed_streams_.find(url);
-  if (unclaimed_it == unclaimed_pushed_streams_.end())
-    return;
-
-  SpdyStreamId stream_id = unclaimed_it->second.stream_id;
-
-  if (active_streams_.find(stream_id) == active_streams_.end()) {
-    ResetStream(stream_id, RST_STREAM_CANCEL,
-                "Cancelled push stream with url: " + url.spec());
-  }
-  unclaimed_pushed_streams_.erase(unclaimed_it);
-}
-
 void SpdySession::OnPushPromise(SpdyStreamId stream_id,
                                 SpdyStreamId promised_stream_id,
                                 SpdyHeaderBlock headers) {