New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 791054 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 776415



Sign in to add a comment

Remove SpdySession::UnclaimedStreamContainer, manage all unclaimed pushed streams in Http2PushPromiseIndex.

Project Member Reported by b...@chromium.org, Dec 1 2017

Issue description

Draft CL https://crrev.com/c/734223 is blocked on this, because the way SpdySession::UnclaimedStreamContainer and Http2PushPromiseIndex are currently entangled would make some very messy calls back and forth between the two classes that would make class invariants difficult to argue about.



 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3b60901cd16927a20269cfc7177997f2584912c6

commit 3b60901cd16927a20269cfc7177997f2584912c6
Author: Bence Béky <bnc@chromium.org>
Date: Mon Dec 04 15:19:35 2017

Clean up UnclaimedPushedStreamContainer API.

In preparation for moving UnclaimedPushedStreamContainer functionality
into Http2PushPromiseIndex, rename
UnclaimedPushedStreamContainer::find() to FindStream() and rename
Http2PushPromiseIndex::Find() to FindSession(), so that when both
methods live in Http2PushPromiseIndex, there will be no name collision.

Also, remove publicly exposed UnclaimedPushedStreamContainer::iterator,
instead introduce new constant kNoPushedStreamFound to be returned by
FindStream() if no stream is found.  (This will also be used by other
Http2PushPromiseIndex methods, see https://crrev.com/c/734223 for proof
of concept.)

Also, remove empty(), size(), begin(), end(), count(), lower_bound(),
and erase by iterator methods from UnclaimedPushedStreamContainer, and
rename size to CountStreamsForSession().

New method names will make sense when they all are moved to
Http2PushPromiseIndex in the next CL.

Bug:  791054 
Change-Id: I8a3e34b0af78480b1bb3cd22cba2fccf4c6e3e39
Reviewed-on: https://chromium-review.googlesource.com/805074
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521340}
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/3b60901cd16927a20269cfc7177997f2584912c6/net/spdy/chromium/spdy_session_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a12c770e918a69cc0bbba2d2579c22ff6ae76936

commit a12c770e918a69cc0bbba2d2579c22ff6ae76936
Author: Bence Béky <bnc@chromium.org>
Date: Wed Dec 06 21:07:28 2017

Record stream IDs in Http2PushPromiseIndex.

Stop using SpdySession instances in Http2PushPromiseIndexTests.  This
makes tests much simpler.

Rename SpdySessionKey::Equals() to SpdySessionKey::operator==() so that
it can be tested by gMock.  This conforms to the style guide "prefer to
define == [...] rather than Equals()", see
https://google.github.io/styleguide/cppguide.html#Operator_Overloading

Define struct UnclaimedPushedStream instead of using std::pair or
std::tuple.  Named members are much nicer than first, second, or get<>.

Define CompareByUrl in conformance with the same section of the style
guide "if your type doesn't have a natural ordering [...] use a custom
comparator rather than overloading <".

The next CL will merge SpdySession::UnclaimedPushedStreamContainer into
Http2PushPromiseIndex.  For this, two sets of UnclaimedPushedStream
entries will need to be maintained with identical
contents but different ordering.  The other will be done by
CompareByDelegate and will allow efficient lookup of entries for a given
Delegate.

Bug:  776415 ,  791054 
Change-Id: I0c1912e9932b102e17528f9319163232860a8813
Reviewed-on: https://chromium-review.googlesource.com/809187
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522192}
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_key.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_key.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_pool.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd2c1fd79bb9dc4eaee85af917525cb72520726a

commit fd2c1fd79bb9dc4eaee85af917525cb72520726a
Author: Bence Béky <bnc@chromium.org>
Date: Fri Dec 15 02:32:03 2017

Remove SpdySession::UnclaimedPushedStreamContainer.

Add methods to Http2PushPromiseIndex so that it could take over the
functionality of SpdySession::UnclaimedPushedStreamContainer.

Bug:  791054 
Change-Id: Ia0dfc13faea2b3d261d3bd1827b796222130ffbf
Reviewed-on: https://chromium-review.googlesource.com/820195
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524298}
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/spdy_session_pool.cc
[modify] https://crrev.com/fd2c1fd79bb9dc4eaee85af917525cb72520726a/net/spdy/chromium/spdy_session_unittest.cc

Comment 4 by b...@chromium.org, Dec 15 2017

Status: Fixed (was: Started)

Sign in to add a comment