Closed Bug 1097320 Opened 6 years ago Closed 6 years ago

Enable advertising h2

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
relnote-firefox --- 36+

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [spdy])

Attachments

(1 file, 1 obsolete file)

1.84 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
Rumor has it everything will be binary compatible from this point on, so let's start advertising the final version (along with all the other versions we're advertising).
Attached patch patch (obsolete) — Splinter Review
This enables advertising h2 unconditionally (still preffable). I went this route, because it's much easier to disable a feature in beta timeframe (in case we shouldn't push h2 out the door) than it is to enable it (in the case we're confident the internet at large is ready to see "h2" in ALPN advertisements).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1ad3838df523

http://ietfmemes.tumblr.com/post/102384795594/disaster-girl-supports-the-standards-process
Attachment #8520991 - Flags: review?(mcmanus)
Depends on: 1101157
we can drop the blocking dep - it would be nice to see that interop, but we've got an internal test and confirmation that the (at least the first part of) the issue is on the other side
No longer depends on: 1101157
Comment on attachment 8520991 [details] [diff] [review]
patch

Review of attachment 8520991 [details] [diff] [review]:
-----------------------------------------------------------------

good to go with compat; lets get this on 36. {h2, h2-14, h2-15} right?
Attachment #8520991 - Flags: review?(mcmanus) → review+
Try run seems to have hit the test_http2.js timeout twice. Retriggering a bunch just to make sure this didn't somehow make the problem worse. Will land once I'm confident.
Attached patch patch v1.1Splinter Review
So as discussed on irc, the current patch causes the test to timeout because of some altsvc wonkiness (seems like a bug in the altsvc code, maybe?)

This patch modifies the test to not tickle the likely altsvc bug, so we can get this landed. I'll also try to dig on on what's going on with the altsvc code, though it seems likely that it's only really a bug when the server says altsvc for 2 different protocols, only one of which it's actually advertising (which is also a server bug at that point, if you ask me).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bfe486ddeea6
Attachment #8520991 - Attachment is obsolete: true
Attachment #8528534 - Flags: review?(mcmanus)
Comment on attachment 8528534 [details] [diff] [review]
patch v1.1

Review of attachment 8528534 [details] [diff] [review]:
-----------------------------------------------------------------

oh - I suspect the issue here is that the test is not robust to alt-svc failure because it is a alt-svc positive test :)

I think gecko itself is robust to using things other than the failing alt-svc (which is what we want!) the test refuses to accept that.

So adjusting the test to reflect the server is the right thing.
Attachment #8528534 - Flags: review?(mcmanus) → review+
Ah yeah, sounds about right! Just waiting on try to validate me... I mean my changes. I need no validation from a computer!
And of course inbound is closed, as well... :sigh: Try has validated me, though, so whenever inbound is open, this can be checked in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b6fe53f601f
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Release Note Request (optional, but appreciated)
[Suggested wording]: see below.
[Why is this notable]: Firefox 36 launches support for the full HTTP/2 protocol. HTTP/2 enables a faster, more scalable, and more responsive web.

[Links (documentation, blog post, etc)]:
I'm going to make a blog post but it does not exist yet.
relnote-firefox: --- → ?
Keywords: relnote
Adding to the release notes with "Support for the full HTTP/2 protocol. HTTP/2 enables a faster, more scalable, and more responsive web." as wording. FYI, it is not necessary to write "Firefox" in the release notes because it is the Firefox release notes (ditto for the version).

By the way, we stopped using the keyword relnote. We are using the tracking flag.
Thanks!
Keywords: relnote
Depends on: 1381016
Depends on: 1500743
Regressions: 1542989
You need to log in before you can comment on or make changes to this bug.