Minor cleanups in HttpStreamFactoryImpl and member classes.

Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

BUG=475060
Committed: https://crrev.com/91e8446d820f2ba118504c73c60ba96cc1b150ac
Cr-Commit-Position: refs/heads/master@{#420725}

[Bence, 2016-09-22 16:51:16]

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

[commit-bot: I haz the power, 2016-09-22 16:51:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2016-09-22 17:04:38]

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

[commit-bot: I haz the power, 2016-09-22 17:04:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bence, 2016-09-22 17:05:23]

Description was changed from

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

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

to

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

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

[commit-bot: I haz the power, 2016-09-22 18:20:03]

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

[commit-bot: I haz the power, 2016-09-22 18:20:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Bence, 2016-09-22 19:30:31]

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

[Bence, 2016-09-22 19:30:33]

Ryan: PTAL.  Thank you.

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

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:380:
proxy_info_.proxy_server().is_https() || IsSpdyAlternative();
See DCHECK in line 232.

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1094: if (IsSpdyAlternative() &&
origin_url_.SchemeIs("http")) {
See DCHECK in line 232.

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1308: if (IsSpdyAlternative() ||
IsQuicAlternative()) {
See DCHECK in line 232 and the one below.

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

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:989: DCHECK(using_quic_);
See assignment in line 821.

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1306: // We currently only support
Alternate-Protocol where the original scheme
I think we do not support QUIC Alt-Svc either for "http" schemes.  Maybe remove
this if() here, and add DCHECK on scheme to line 235?

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1385: // This can happen in the case of
trying to talk to a proxy using SSL, and
Does this comment and the next one each refer to the case right above or right
below?  I almost have the impression that they both refer to
|ERR_PROXY_CERTIFICATE_INVALID|.  I'm confused.

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

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.h:268: // http connection is typically
faster than npn-spdy, since npn-spdy
This is obsolete: Alt-Svc is only allowed for https origins, so both the main
and the alternative jobs would need a TLS handshake.

[Ryan Hamilton, 2016-09-22 21:38:05]

Everything other than IsSpdyAlternative() removals looks good to me. I'm not
sure I understand why removing them is right.

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

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1306: // We currently only support
Alternate-Protocol where the original scheme
On 2016/09/22 19:30:32, Bence wrote:
> I think we do not support QUIC Alt-Svc either for "http" schemes.  Maybe
remove
> this if() here, and add DCHECK on scheme to line 235?

I confess, I don't understand this comment at all. I think it's just wrong.

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1385: // This can happen in the case of
trying to talk to a proxy using SSL, and
On 2016/09/22 19:30:32, Bence wrote:
> Does this comment and the next one each refer to the case right above or right
> below?  I almost have the impression that they both refer to
> |ERR_PROXY_CERTIFICATE_INVALID|.  I'm confused.

I think the second comment actually applies to:

 case ERR_SSL_PROTOCOL_ERROR:

[Bence, 2016-09-23 13:39:08]

PTAL.

On 2016/09/22 21:38:05, Ryan Hamilton wrote:
> Everything other than IsSpdyAlternative() removals looks good to me. I'm not
> sure I understand why removing them is right.
> 

A := IsSpdyAlternative()
B := origin_url_.SchemeIs(url::kHttpsScheme)

The return value of IsSpdyAlternative() only depends on |alternative_service_|. 
Both |alternative_service_| and |origin_url_| members are const.  Therefore the
truth value of A and B both are constant throughout the lifetime of the class
instance.

DCHECK on line 231 guarantees A => B.  This is equivalent to (A or B) == B.  I
used this to simplify the conditions on lines 379 and 1495.  It is also
equivalent to not (A and not B).  I used this to eliminate old lines 1095--1097.

[Bence, 2016-09-23 13:41:04]

PTAL.

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

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1306: // We currently only support
Alternate-Protocol where the original scheme
On 2016/09/22 21:38:05, Ryan Hamilton wrote:
> On 2016/09/22 19:30:32, Bence wrote:
> > I think we do not support QUIC Alt-Svc either for "http" schemes.  Maybe
> remove
> > this if() here, and add DCHECK on scheme to line 235?
> 
> I confess, I don't understand this comment at all. I think it's just wrong.

I added a TODO here, I'd like to clean this up in a follow-up CL.

https://codereview.chromium.org/2359153003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1385: // This can happen in the case of
trying to talk to a proxy using SSL, and
Thanks.  I moved and refined the comments.

[Bence, 2016-09-23 13:43:47]

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

[commit-bot: I haz the power, 2016-09-23 13:43:56]

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, 2016-09-23 13:55:34]

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

[commit-bot: I haz the power, 2016-09-23 13:55:36]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Ryan Hamilton, 2016-09-23 19:26:03]

lgtm

[Bence, 2016-09-23 20:03:48]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2016-09-23 20:04:15]

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, 2016-09-23 21:03:39]

Description was changed from

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

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

to

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

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

[commit-bot: I haz the power, 2016-09-23 21:03:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-23 21:05:27]

Description was changed from

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

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

to

==========
Minor cleanups in HttpStreamFactoryImpl and member classes.

* Remove all unnecessary IsSpdyAlternative() checks.
  IsSpdyAlternative() implies origin_url_.SchemeIs(url::kHttpsScheme),
  see DCHECK in constructor.
* Inline now const |spdy_certificate_error_|.
* Inline and remove SwitchToSpdyMode().  This method was only used twice out of
  four potential places.  Removal makes sense as the method is trivial, and
  there are no corresponding methods for |using_spdy_ = false|, |using_quic_ =
  true|, and |using_quic_ = false| assignments.
* Use url_constants instead of string literals for schemes (http, https, ftp,
  and wss).  There has already been two uses of such constants.
* Remove unused |destination| local variable.
* Fix some comments.
* git cl lint: Remove unnecessary semicolon, add includes.

BUG=475060

Committed: https://crrev.com/91e8446d820f2ba118504c73c60ba96cc1b150ac
Cr-Commit-Position: refs/heads/master@{#420725}
==========

[commit-bot: I haz the power, 2016-09-23 21:05:28]

Patchset 3 (id:??) landed as
https://crrev.com/91e8446d820f2ba118504c73c60ba96cc1b150ac
Cr-Commit-Position: refs/heads/master@{#420725}