Remove SPDY2 and SPDY/3.0 from tests.

Remove SPDY2 and SPDY/3.0 from tests.

Remove NextProto::kProtoDeprecatedSPDY2, NextProto::KProtoSPDY3, and
SpdyMajorVersion::SPDY2 from tests to ease the burden on trybots.  This has been
made possible by https://crrev.com/708623002, which guarantees that SPDY/3.0 is
never negotiated with a server.  SPDY/2 has been obsolete for a while.

Note that such tests cannot yet be removed from certain files for legacy
reasons.

Note that SpdyMajorVersion::SPDY3 cannot be removed, as it includes SPDY/3.1,
which we still actively support.

BUG=431299
Committed: https://crrev.com/33b8cef49dd924d51b017aa2ab361055ffd4a725
Cr-Commit-Position: refs/heads/master@{#304830}

[Bence, 2014-11-07 19:42:07]

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

[Bence, 2014-11-07 19:42:08]

Asanka: PTAL.  Thank you.

[asanka, 2014-11-10 19:18:37]

Given that I'm not super familiar with SPDY, my review shouldn't be considered a
detailed review. But lgtm % nits.

https://codereview.chromium.org/710713002/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:10110: "Alternate-Protocol:
443:npn-spdy/3.1\r\n"
Should this be parameterized as well? I.e. compose it using
GetAlternateProtocolHttpHeader()?

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_framer_test.cc
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_framer_test.cc...
net/spdy/spdy_framer_test.cc:1915: if (!IsSpdy3()) {
Nit: IMHO assuming the numerical value of the SpdyMajorVersion constants are
increasing, the original condition is more readable and corresponds to the
comment.

Applies here and elsewhere.

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:173: return static_cast<SpdyPriority>(HIGHEST -
priority);
Why HIGHEST here vs. MAXIMUM_PRIORITY when converting back to a RequestPriority?
They are the same value now, but I think consistency would be good between the
two conversion functions.

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:180: return (priority >= MAXIMUM_PRIORITY) ?
This compares a SpdyPriority to a RequestPriority. It's a bit confusing what's
going on. A comment describing the justification would help.

Also, could you update the corresponding comments in spdy_http_utils.h?

[Bence, 2014-11-10 22:52:09]

Asanka: thanks for your review.  PTAL.

https://codereview.chromium.org/710713002/diff/1/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:10110: "Alternate-Protocol:
443:npn-spdy/3.1\r\n"
On 2014/11/10 19:18:37, asanka wrote:
> Should this be parameterized as well? I.e. compose it using
> GetAlternateProtocolHttpHeader()?

Done.

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_framer_test.cc
File net/spdy/spdy_framer_test.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_framer_test.cc...
net/spdy/spdy_framer_test.cc:1915: if (!IsSpdy3()) {
On 2014/11/10 19:18:37, asanka wrote:
> Nit: IMHO assuming the numerical value of the SpdyMajorVersion constants are
> increasing, the original condition is more readable and corresponds to the
> comment.
> 
> Applies here and elsewhere.

Done.

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:173: return static_cast<SpdyPriority>(HIGHEST -
priority);
On 2014/11/10 19:18:37, asanka wrote:
> Why HIGHEST here vs. MAXIMUM_PRIORITY when converting back to a
RequestPriority?
> They are the same value now, but I think consistency would be good between the
> two conversion functions.

Done.

https://codereview.chromium.org/710713002/diff/1/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:180: return (priority >= MAXIMUM_PRIORITY) ?
On 2014/11/10 19:18:37, asanka wrote:
> This compares a SpdyPriority to a RequestPriority. It's a bit confusing what's
> going on. A comment describing the justification would help.
> 
> Also, could you update the corresponding comments in spdy_http_utils.h?

Good catch, thanks.  Done.

[Bence, 2014-11-10 22:53:18]

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

[Bence, 2014-11-10 22:53:18]

Ryan: PTAL.  Asanka has reviewed already with the provision that he is "not
super familiar with SPDY".  Thank you.

[Ryan Hamilton, 2014-11-10 23:08:27]

I think this is good, but I'm unclear on one point. Some of these changes are
chrome specific and some are in the shared code. Have the shared-code changes
been made internally too? Should they be?

https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
File net/spdy/spdy_framer_test.cc (left):

https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
net/spdy/spdy_framer_test.cc:673: bool IsSpdy2() { return spdy_version_ ==
SPDY2; }
Have these changes been made server side too?

[Bence, 2014-11-10 23:36:35]

On 2014/11/10 23:08:27, Ryan Hamilton wrote:
> I think this is good, but I'm unclear on one point. Some of these changes are
> chrome specific and some are in the shared code. Have the shared-code changes
> been made internally too? Should they be?
> 
>
https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
> File net/spdy/spdy_framer_test.cc (left):
> 
>
https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
> net/spdy/spdy_framer_test.cc:673: bool IsSpdy2() { return spdy_version_ ==
> SPDY2; }
> Have these changes been made server side too?

No, these changes have not been made on server side yet.  We have to have an
internal discussion on SPDY/2 and SPDY/3.0 usage statistics and deprecation
schedule on that side.  I don't mind temporary code divergence.

[Ryan Hamilton, 2014-11-10 23:41:41]

On 2014/11/10 23:36:35, Bence wrote:
> On 2014/11/10 23:08:27, Ryan Hamilton wrote:
> > I think this is good, but I'm unclear on one point. Some of these changes
are
> > chrome specific and some are in the shared code. Have the shared-code
changes
> > been made internally too? Should they be?
> > 
> >
>
https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
> > File net/spdy/spdy_framer_test.cc (left):
> > 
> >
>
https://codereview.chromium.org/710713002/diff/40001/net/spdy/spdy_framer_tes...
> > net/spdy/spdy_framer_test.cc:673: bool IsSpdy2() { return spdy_version_ ==
> > SPDY2; }
> > Have these changes been made server side too?
> 
> No, these changes have not been made on server side yet.  We have to have an
> internal discussion on SPDY/2 and SPDY/3.0 usage statistics and deprecation
> schedule on that side.  I don't mind temporary code divergence.

Hm. I'm happy to land all of the chrome-specific changes but I don't think it's
a good idea to fork the shared code. It looks like there are only a few shared
files, so it should be pretty easy to split that out into a distinct CL which
can be landed server-side.

[Bence, 2014-11-19 15:42:58]

bnc@chromium.org changed reviewers:
- asanka@chromium.org

[Bence, 2014-11-19 15:42:58]

Ryan: PTAL.  Rebased, and removed changes in shared code (which has to support
old SPDY versions for various reasons).

[Ryan Hamilton, 2014-11-19 16:43:08]

lgtm

[Bence, 2014-11-19 16:44:03]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-11-19 16:44:26]

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

[commit-bot: I haz the power, 2014-11-19 17:30:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-19 17:31:39]

Patchset 5 (id:??) landed as
https://crrev.com/33b8cef49dd924d51b017aa2ab361055ffd4a725
Cr-Commit-Position: refs/heads/master@{#304830}