Remove unused SocketDataProvider argument from SpdyNetworkTransactionTest::ReadResult

Remove unused SocketDataProvider argument from SpdyNetworkTransactionTest::ReadResult

Also clarify the split between Deterministic and non-Deterministic
socket data providers.

Committed: https://crrev.com/0aecfd86a6ec86a12224eac8e8f02ce6be5f434f
Cr-Commit-Position: refs/heads/master@{#330546}

[Ryan Hamilton, 2015-05-19 00:50:39]

rch@chromium.org changed reviewers:
+ bnc@chromium.org

[Ryan Hamilton, 2015-05-19 00:50:43]

Noticed this in an unrelated CL I'm working on...

[Bence, 2015-05-19 12:08:52]

I am not actually convinced that separating |deterministic_data_vector_| from
|data_vector_| is an improvement for maintainability: it is very easy to forget
to use the right one depending on |deterministic_|.  What is your long term plan
here?

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_unittest.cc:271: for (const
SocketDataProvider* provider : data_vector_) {
This is not what you want for deterministic tests.  Instead, use 

deterministic_ ? deterministic_data_vector_ : data_vector_

or

if(deterministic_) {...}

or add VerifyDeterministicDataConsumed() and add DCHECK(deterministic_) to that
and DCHECK(!deterministic_) to this.

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_unittest.cc:281: for (const
SocketDataProvider* provider : data_vector_) {
Ditto.  Since this member is only called from tests not using deterministic
data, a |DCHECK(deterministic_)| would suffice here.

[Ryan Hamilton, 2015-05-19 14:31:22]

I hear your point about readability. The problem is that the current
SocketDataProvider class hierarchy is error prone. DeterministicSocketData does
not work without a DeterministicSocket and a DeterministicFactory. However,
because it extends SocketDataProvider, it's easy to screw up and instantiate a
DeterministicSocketData and then try to use it with a normal
MockClientSocketFactory. This is really frustrating when it happens. To fix
this, I'm landing the following CL:

https://codereview.chromium.org/1135553003/

This makes DeterministicSocketData no longer extend SocketDataProvider which
prevents people shooting themselves in the foot.  But in order to land this, I
have to fix the one test which treats DeterministicSocketData as a
SocketDataProvider, hence this CL.

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_unittest.cc:271: for (const
SocketDataProvider* provider : data_vector_) {
On 2015/05/19 12:08:52, Bence wrote:
> This is not what you want for deterministic tests.  Instead, use 
> 
> deterministic_ ? deterministic_data_vector_ : data_vector_
> 
> or
> 
> if(deterministic_) {...}
> 
> or add VerifyDeterministicDataConsumed() and add DCHECK(deterministic_) to
that
> and DCHECK(!deterministic_) to this.

Good point. I'm now verifying both vectors.

https://codereview.chromium.org/1142893003/diff/1/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_unittest.cc:281: for (const
SocketDataProvider* provider : data_vector_) {
On 2015/05/19 12:08:52, Bence wrote:
> Ditto.  Since this member is only called from tests not using deterministic
> data, a |DCHECK(deterministic_)| would suffice here.

In for a penny, in for a pound. I'm no verifying both vectors.

[Bence, 2015-05-19 14:39:06]

Thanks for the detailed explanation.  LGTM.

[Ryan Hamilton, 2015-05-19 14:50:48]

On 2015/05/19 14:39:06, Bence wrote:
> Thanks for the detailed explanation.  LGTM.

Thanks! I should have included that in my initial email.

[Ryan Hamilton, 2015-05-19 14:50:52]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2015-05-19 14:51:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142893003/20001

[commit-bot: I haz the power, 2015-05-19 17:22:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-19 17:24:15]

Patchset 2 (id:??) landed as
https://crrev.com/0aecfd86a6ec86a12224eac8e8f02ce6be5f434f
Cr-Commit-Position: refs/heads/master@{#330546}