HttpStreamFactoryImpl::Job cleanup.

HttpStreamFactoryImpl::Job cleanup.

Introduce const member |expect_spdy_| instead of IsSpdyAlternative().
Make |using_quic_| member const and use instead of IsQuicAlternative().
Const members make reasoning about Job easier.

Remove |alternative_service_| private member from Job.  Process
alternative_protocol argument in Job constructor to initialize
|expect_spdy_| and |using_quic_|.  Save alternative service in a
JobController private member for the purposes of marking it broken on
failure.

Consolidate HttpStreamFactoryImpl::Job constructors.  Since there are
factory methods for creating this class, it makes more sense to only
have one constructor, and fill in the default NextProto and ProxyServer
arguments in the factory methods.  Same with
MockHttpStreamFactoryImplJob constructors.

Make ShouldForceQuic() and GetSpdySessionKey() static, call
ShouldForceQuic() in initializer list, allowing  |using_quic_| to be a
const member.  Calling a non-static method from the initializer list
makes me nervous, since it is not clear at the call site which members
have to be already initialized for the method to work correctly.
Passing them explicitly to a static method makes such dependencies
transparent.

BUG=475060
Review-Url: https://codereview.chromium.org/2935293002
Cr-Commit-Position: refs/heads/master@{#479506}
Committed: https://chromium.googlesource.com/chromium/src/+/582da3db32c75943097dabe889b0fe9c680cecaa

[Bence, 2017-06-14 15:27:27]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:27:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 17:17:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 17:17:03]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-06-14 17:29:09]

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

[Bence, 2017-06-14 17:29:10]

xunjieli: PTAL.  Thank you.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (left):

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:809: using_spdy_ = false;
This is redundant with the default value in the initializer list in the
constructor.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:811: if (ShouldForceQuic())
This logic has been moved to the initializer list.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:814: DCHECK(!using_quic_ ||
session_->IsQuicEnabled());
This DCHECK is in the constructor body.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:816: if (proxy_info_.is_quic()) {
This logic has been moved to ShouldForceQuic().

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1034: using_quic_ = false;
I believe that at this point the Job fails and will not do any more work, so
this assignment is unnecessary.

[xunjieli, 2017-06-14 18:28:01]

wow This is a great change. Can't LGTM enough!

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:161: // Note that this can be
overwritten by using a QUIC proxy, or by setting
Just to be explicit. Could you specify that the QUIC proxy is done through
|alternative_proxy_server| ?

by using a QUIC proxy -> by using a QUIC proxy through
|alternative_proxy_server|.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:342: GURL origin_url,
nit: const GURL&

[Bence, 2017-06-14 19:33:19]

The CQ bit was checked by bnc@chromium.org to run a CQ dry run

[Bence, 2017-06-14 19:33:30]

PTAL.  Thanks.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:161: // Note that this can be
overwritten by using a QUIC proxy, or by setting
On 2017/06/14 18:28:01, xunjieli wrote:
> Just to be explicit. Could you specify that the QUIC proxy is done through
> |alternative_proxy_server| ?
> 
> by using a QUIC proxy -> by using a QUIC proxy through
> |alternative_proxy_server|.

It's good that you bring it up: in fact it is proxy_info_.is_quic() that forces
QUIC to be used.  In the old code that was in line 817, in the new code it's at
line 385, in the .cc file.

For an alternative proxy job, http_stream_factory_impl_job_controller.cc:831
fills |alternative_proxy_info| with |alternative_proxy_server|, and that
|alternative_proxy_info| gets passed to the factory as its |proxy_info|
argument.  So you are right that |alternative_proxy_server| can indirectly force
QUIC, though this seems to happen in JobController, not in Job.

https://codereview.chromium.org/2935293002/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:342: GURL origin_url,
On 2017/06/14 18:28:01, xunjieli wrote:
> nit: const GURL& 

Done.

[commit-bot: I haz the power, 2017-06-14 19:33:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2017-06-14 19:35:29]

lgtm. Thanks for explaining the force quic bits.

[Bence, 2017-06-14 19:36:02]

The CQ bit was unchecked by bnc@chromium.org

[Bence, 2017-06-14 19:36:23]

The CQ bit was checked by bnc@chromium.org

[Bence, 2017-06-14 19:36:23]

The CQ bit was unchecked by bnc@chromium.org

[Bence, 2017-06-14 19:43:03]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2017-06-14 19:43:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 21:11:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497469383426960,
"parent_rev": "11259e450d6002293271d60949681a1d99d7422b", "commit_rev":
"582da3db32c75943097dabe889b0fe9c680cecaa"}

[commit-bot: I haz the power, 2017-06-14 21:12:04]

Description was changed from

==========
HttpStreamFactoryImpl::Job cleanup.

Introduce const member |expect_spdy_| instead of IsSpdyAlternative().
Make |using_quic_| member const and use instead of IsQuicAlternative().
Const members make reasoning about Job easier.

Remove |alternative_service_| private member from Job.  Process
alternative_protocol argument in Job constructor to initialize
|expect_spdy_| and |using_quic_|.  Save alternative service in a
JobController private member for the purposes of marking it broken on
failure.

Consolidate HttpStreamFactoryImpl::Job constructors.  Since there are
factory methods for creating this class, it makes more sense to only
have one constructor, and fill in the default NextProto and ProxyServer
arguments in the factory methods.  Same with
MockHttpStreamFactoryImplJob constructors.

Make ShouldForceQuic() and GetSpdySessionKey() static, call
ShouldForceQuic() in initializer list, allowing  |using_quic_| to be a
const member.  Calling a non-static method from the initializer list
makes me nervous, since it is not clear at the call site which members
have to be already initialized for the method to work correctly.
Passing them explicitly to a static method makes such dependencies
transparent.

BUG=475060
==========

to

==========
HttpStreamFactoryImpl::Job cleanup.

Introduce const member |expect_spdy_| instead of IsSpdyAlternative().
Make |using_quic_| member const and use instead of IsQuicAlternative().
Const members make reasoning about Job easier.

Remove |alternative_service_| private member from Job.  Process
alternative_protocol argument in Job constructor to initialize
|expect_spdy_| and |using_quic_|.  Save alternative service in a
JobController private member for the purposes of marking it broken on
failure.

Consolidate HttpStreamFactoryImpl::Job constructors.  Since there are
factory methods for creating this class, it makes more sense to only
have one constructor, and fill in the default NextProto and ProxyServer
arguments in the factory methods.  Same with
MockHttpStreamFactoryImplJob constructors.

Make ShouldForceQuic() and GetSpdySessionKey() static, call
ShouldForceQuic() in initializer list, allowing  |using_quic_| to be a
const member.  Calling a non-static method from the initializer list
makes me nervous, since it is not clear at the call site which members
have to be already initialized for the method to work correctly.
Passing them explicitly to a static method makes such dependencies
transparent.

BUG=475060

Review-Url: https://codereview.chromium.org/2935293002
Cr-Commit-Position: refs/heads/master@{#479506}
Committed:
https://chromium.googlesource.com/chromium/src/+/582da3db32c75943097dabe889b0...
==========

[commit-bot: I haz the power, 2017-06-14 21:12:06]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/582da3db32c75943097dabe889b0...