Issue 7092050: code review 7092050: go.net/spdy: update SPDY/2 to SPDY/3
Descriptiongo.net/spdy: update SPDY/2 to SPDY/3
Update to SPDY/3
http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3
Patch Set 1 #Patch Set 2 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #Patch Set 3 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 6
Patch Set 4 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 62
Patch Set 5 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 94
Patch Set 6 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 22
Patch Set 7 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 32
Patch Set 8 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #Patch Set 9 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #Patch Set 10 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
Total comments: 11
Patch Set 11 : diff -r e1c4b7341527 https://code.google.com/p/go.net/ #
MessagesTotal messages: 49
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.net/
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/5001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/5001/spdy/spdy_test.go#newcode616 spdy/spdy_test.go:616: // func TestNoZeroStreamId(t *testing.T) { rather than commenting them out, just add this to the top: t.Log("skipping") // TODO: update to work with SPDY3 return
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
fix commenting out to skipping tests. https://codereview.appspot.com/7092050/diff/5001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/5001/spdy/spdy_test.go#newcode616 spdy/spdy_test.go:616: // func TestNoZeroStreamId(t *testing.T) { On 2013/01/14 00:40:39, adg wrote: > rather than commenting them out, just add this to the top: > > t.Log("skipping") // TODO: update to work with SPDY3 > return Done.
Sign in to reply to this message.
I'm not familiar with spdy, so I can only point out some minor (or stylistic) issues. https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode1 spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights reserved. update the years to 2013. https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode10 spdy/dictionary.go:10: const HeaderDictionary = "" + do you want to export this? also, if you ultimately want it to be used as []byte, i'd suggest you do this: var HeaderDictionary = []byte { /* ... */ } to save the runtime conversion (and copy).
Sign in to reply to this message.
Hi, On Mon, Jan 14, 2013 at 5:19 AM, <block.rxckin.beats@gmail.com> wrote: > Update to SPDY/3 (actually following RFC of HTTP2.0 same of spdy/3) > http://tools.ietf.org/html/draft-ietf-httpbis-http2-00 It's still I-D and charters say that the due date will be the end of CY14. Well, I have no objection to just following standardized one only and drop pre-mature, in-devel or old version support such as spdy/4, spdy/2. But would like to hear others' opinion.
Sign in to reply to this message.
I think we should try to stay updated, especially if somebody has time. No point retaining spdy/2 code at this point. On Mon, Jan 14, 2013 at 5:57 PM, Mikio Hara <mikioh.mikioh@gmail.com> wrote: > Hi, > > On Mon, Jan 14, 2013 at 5:19 AM, <block.rxckin.beats@gmail.com> wrote: > > > Update to SPDY/3 (actually following RFC of HTTP2.0 same of spdy/3) > > http://tools.ietf.org/html/draft-ietf-httpbis-http2-00 > > It's still I-D and charters say that the due date will be the end of CY14. > > Well, I have no objection to just following standardized one only and > drop pre-mature, in-devel or old version support such as spdy/4, spdy/2. > > But would like to hear others' opinion. >
Sign in to reply to this message.
Hi, all. > It's still I-D and charters say that the due date will be the end of CY14. yes, HTTP2 is draft now. But this is not go.net/http2 but go.net/spdy. so I found out I needed to say "following SPDY/3 by google http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3" (even if same spec). Some services (i.e. Google, Twitter etc) using SPDY/3 now, and Google starts discussing SPDY/4. so I think it's so natural to update spdy/2 to spdy/3. thanks Jxck On Tuesday, January 15, 2013 11:01:59 AM UTC+9, Brad Fitzpatrick wrote: > > I think we should try to stay updated, especially if somebody has time. > No point retaining spdy/2 code at this point. > > On Mon, Jan 14, 2013 at 5:57 PM, Mikio Hara <mikioh...@gmail.com<javascript:> > > wrote: > >> Hi, >> >> On Mon, Jan 14, 2013 at 5:19 AM, <block.rxc...@gmail.com <javascript:>> >> wrote: >> >> > Update to SPDY/3 (actually following RFC of HTTP2.0 same of spdy/3) >> > http://tools.ietf.org/html/draft-ietf-httpbis-http2-00 >> >> It's still I-D and charters say that the due date will be the end of CY14. >> >> Well, I have no objection to just following standardized one only and >> drop pre-mature, in-devel or old version support such as spdy/4, spdy/2. >> >> But would like to hear others' opinion. >> > >
Sign in to reply to this message.
On 15 January 2013 04:46, <minux.ma@gmail.com> wrote: > I'm not familiar with spdy, so I can only point out > some minor (or stylistic) issues. > Sorry, I just added you to this CL because you had authored the most changes in go.net/spdy.
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/1002/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode46 spdy/read.go:46: // Read a frame to SettingFrame I would drop all these comments like this one (above and below too). Only keep the ones that add clarity to the code. These are just implementing an interface, so it's obvious what they do. It just looks noisy and confusing as is, and they're not even in the conventional Go full-sentence style. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode5 spdy/types.go:5: // Package spdy implements HTTP2.0 (SPDY/3) protocol which is described in HTTP/2.0 would be the correct punctuation, but it's not HTTP/2.0 yet, so I'd just say: // Package spdy implements the SPDY protocol (currently SPDY/3), described in ..... https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode53 spdy/types.go:53: // Separator for multiple Header Values Correct Go style is to use a complete sentence here, starting with the subject, so it stands alone. // HeaderValueSepator separates multiple header values. But really: why is this exported? Who is expected to need this? https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode164 spdy/types.go:164: STREAM_IN_USE = 8 Wrong case. See the lines above for Go style.
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode1 spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights reserved. s/2011/2013/ https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode8 spdy/dictionary.go:8: // This dictionary defined in The dictionary is defined in... But I'd like to drop this line once we pointed out the reference in the package description. https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode10 spdy/dictionary.go:10: const HeaderDictionary = "" + Please have a look at minux's comments carefully. I agree with minux, no need to expose HeaderDictionary as a part of API and it would be nice to avoid runtime conversion in NewFramer. var headerDictionary = []byte { /* ... */ } https://codereview.appspot.com/7092050/diff/1002/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode59 spdy/read.go:59: // | value(32) | no need to keep protocol wire format fragments here. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode79 spdy/read.go:79: // frame.CFHeader.Flags should be 0 please drop https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode92 spdy/read.go:92: // frame.CFHeader.Flags should be 0 ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode96 spdy/read.go:96: // frame.CFHeader.length should be 8 ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode118 spdy/read.go:118: // frame.CFHeader.Flags should be 0 ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode122 spdy/read.go:122: // frame.CFHeader.length should be 8 ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode176 spdy/read.go:176: // | 1 | Version(15) | Type(16) | ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode190 spdy/read.go:190: // | Flags (8) | Length (24) | ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode198 spdy/read.go:198: if err = cframe.read(header, f); err != nil { // delegating each Frames read() useful comment? https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode264 spdy/read.go:264: remove blank line https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode286 spdy/read.go:286: // check Request Header doesn't includes invalid Header ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode293 spdy/read.go:293: // listed in mustReqHeaders(type.go) if you think it's really needed, you should do. otherwise please drop. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode311 spdy/read.go:311: // set a decompressor using HeaderDictionary to f.headerDecompressor ditto https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode347 spdy/read.go:347: // set a decompressor using HeaderDictionary to f.headerDecompressor ditto https://codereview.appspot.com/7092050/diff/1002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode23 spdy/types.go:23: const ( // Noop (0x0005) removed spdy/3 drop the comment https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode50 spdy/types.go:50: // that can be stored in one frame. why folding? https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode65 spdy/types.go:65: // Control Frame Format i don't think the wire format in the package docs is helpful. please run godoc and see your changes to the docs. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode154 spdy/types.go:154: type RSTStatusCode uint32 perhaps RstStreamStatus might be better because spdy/3 defines both session and stream error handlings and RstStreamStatus is used for handling stream errors. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode156 spdy/types.go:156: const ( // 0 is invalid drop the comment https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157 spdy/types.go:157: ProtocolError RSTStatusCode = 1 perhaps, RstProtocolError RstStreamStatus = iota + 1 RstInvalidStream RstRefusedStream : https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode271 spdy/types.go:271: StatusCode uint32 you can make new type GoAwayStatus or SessionStatus and provide protocol constants to users. const ( GoAwayOK GoAwayStatus = 0 GoAwayProtocolError = 1 GoAwayInternalError = 11 ) https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode384 spdy/types.go:384: // TODO: need this ? do it or drop.
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/1002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157 spdy/types.go:157: ProtocolError RSTStatusCode = 1 or const ( ProtocolError RstStreamStatus = iota + 1 InvalidStream RefusedStream UnsupportedVersion Cancel InternalError FlowControlError StreamInUse StreamAlreadyClosed InvalidCredentials FlameTooLarge )
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
All comments has Done. https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode1 spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights reserved. On 2013/01/14 17:46:54, minux wrote: > update the years to 2013. Done. https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode10 spdy/dictionary.go:10: const HeaderDictionary = "" + On 2013/01/14 17:46:54, minux wrote: > do you want to export this? > > also, if you ultimately want it to be used as []byte, > i'd suggest you do this: > > var HeaderDictionary = []byte { /* ... */ } > > to save the runtime conversion (and copy). Done. https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode1 spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights reserved. On 2013/01/15 22:44:39, mikio wrote: > s/2011/2013/ Done. https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode8 spdy/dictionary.go:8: // This dictionary defined in On 2013/01/15 22:44:39, mikio wrote: > The dictionary is defined in... > But I'd like to drop this line once we pointed out the reference > in the package description. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode10 spdy/dictionary.go:10: const HeaderDictionary = "" + On 2013/01/15 22:44:39, mikio wrote: > Please have a look at minux's comments carefully. > > I agree with minux, no need to expose HeaderDictionary > as a part of API and it would be nice to avoid runtime > conversion in NewFramer. > > var headerDictionary = []byte { /* ... */ } Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode46 spdy/read.go:46: // Read a frame to SettingFrame On 2013/01/15 21:17:22, bradfitz wrote: > I would drop all these comments like this one (above and below too). Only keep > the ones that add clarity to the code. These are just implementing an > interface, so it's obvious what they do. > > It just looks noisy and confusing as is, and they're not even in the > conventional Go full-sentence style. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode59 spdy/read.go:59: // | value(32) | On 2013/01/15 22:44:39, mikio wrote: > no need to keep protocol wire format fragments here. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode79 spdy/read.go:79: // frame.CFHeader.Flags should be 0 On 2013/01/15 22:44:39, mikio wrote: > please drop Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode92 spdy/read.go:92: // frame.CFHeader.Flags should be 0 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode92 spdy/read.go:92: // frame.CFHeader.Flags should be 0 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode96 spdy/read.go:96: // frame.CFHeader.length should be 8 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode96 spdy/read.go:96: // frame.CFHeader.length should be 8 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode118 spdy/read.go:118: // frame.CFHeader.Flags should be 0 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode122 spdy/read.go:122: // frame.CFHeader.length should be 8 On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode176 spdy/read.go:176: // | 1 | Version(15) | Type(16) | On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode190 spdy/read.go:190: // | Flags (8) | Length (24) | On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode198 spdy/read.go:198: if err = cframe.read(header, f); err != nil { // delegating each Frames read() On 2013/01/15 22:44:39, mikio wrote: > useful comment? Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode264 spdy/read.go:264: On 2013/01/15 22:44:39, mikio wrote: > remove blank line Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode286 spdy/read.go:286: // check Request Header doesn't includes invalid Header On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode293 spdy/read.go:293: // listed in mustReqHeaders(type.go) I'll think about this implementation later. remove now for pending. On 2013/01/15 22:44:39, mikio wrote: > if you think it's really needed, you should do. otherwise please drop. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode311 spdy/read.go:311: // set a decompressor using HeaderDictionary to f.headerDecompressor On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode347 spdy/read.go:347: // set a decompressor using HeaderDictionary to f.headerDecompressor On 2013/01/15 22:44:39, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode5 spdy/types.go:5: // Package spdy implements HTTP2.0 (SPDY/3) protocol which is described in On 2013/01/15 21:17:22, bradfitz wrote: > HTTP/2.0 would be the correct punctuation, but it's not HTTP/2.0 yet, so I'd > just say: > > // Package spdy implements the SPDY protocol (currently SPDY/3), described in > ..... Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode23 spdy/types.go:23: const ( // Noop (0x0005) removed spdy/3 On 2013/01/15 22:44:39, mikio wrote: > drop the comment Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode50 spdy/types.go:50: // that can be stored in one frame. On 2013/01/15 22:44:39, mikio wrote: > why folding? Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode53 spdy/types.go:53: // Separator for multiple Header Values On 2013/01/15 21:17:22, bradfitz wrote: > Correct Go style is to use a complete sentence here, starting with the subject, > so it stands alone. > > // HeaderValueSepator separates multiple header values. > > But really: why is this exported? Who is expected to need this? Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode65 spdy/types.go:65: // Control Frame Format On 2013/01/15 22:44:39, mikio wrote: > i don't think the wire format in the package docs is helpful. > please run godoc and see your changes to the docs. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode154 spdy/types.go:154: type RSTStatusCode uint32 On 2013/01/15 22:44:39, mikio wrote: > perhaps RstStreamStatus might be better because > spdy/3 defines both session and stream error > handlings and RstStreamStatus is used for handling > stream errors. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode156 spdy/types.go:156: const ( // 0 is invalid On 2013/01/15 22:44:39, mikio wrote: > drop the comment Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157 spdy/types.go:157: ProtocolError RSTStatusCode = 1 On 2013/01/16 03:02:16, mikio wrote: > or > > const ( > ProtocolError RstStreamStatus = iota + 1 > InvalidStream > RefusedStream > UnsupportedVersion > Cancel > InternalError > FlowControlError > StreamInUse > StreamAlreadyClosed > InvalidCredentials > FlameTooLarge > ) Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157 spdy/types.go:157: ProtocolError RSTStatusCode = 1 On 2013/01/16 03:02:16, mikio wrote: > or > > const ( > ProtocolError RstStreamStatus = iota + 1 > InvalidStream > RefusedStream > UnsupportedVersion > Cancel > InternalError > FlowControlError > StreamInUse > StreamAlreadyClosed > InvalidCredentials > FlameTooLarge > ) Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode164 spdy/types.go:164: STREAM_IN_USE = 8 On 2013/01/15 21:17:22, bradfitz wrote: > Wrong case. See the lines above for Go style. Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode271 spdy/types.go:271: StatusCode uint32 On 2013/01/15 22:44:39, mikio wrote: > you can make new type GoAwayStatus or SessionStatus > and provide protocol constants to users. > > const ( > GoAwayOK GoAwayStatus = 0 > GoAwayProtocolError = 1 > GoAwayInternalError = 11 > ) Done. https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode384 spdy/types.go:384: // TODO: need this ? I'll think about this implementation later. so remove this for pending. On 2013/01/15 22:44:39, mikio wrote: > do it or drop.
Sign in to reply to this message.
Hi all. I'm sorry but I'm not familier with this contributing flow. but I think that I solved all comments for my patch. so please review my new commit. thanks Jxck 2013/1/21 <block.rxckin.beats@gmail.com> > All comments has Done. > > > > https://codereview.appspot.**com/7092050/diff/5001/spdy/**dictionary.go<https... > File spdy/dictionary.go (right): > > https://codereview.appspot.**com/7092050/diff/5001/spdy/** > dictionary.go#newcode1<https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode1> > spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights > reserved. > On 2013/01/14 17:46:54, minux wrote: > >> update the years to 2013. >> > > Done. > > https://codereview.appspot.**com/7092050/diff/5001/spdy/** > dictionary.go#newcode10<https://codereview.appspot.com/7092050/diff/5001/spdy/dictionary.go#newcode10> > > spdy/dictionary.go:10: const HeaderDictionary = "" + > On 2013/01/14 17:46:54, minux wrote: > >> do you want to export this? >> > > also, if you ultimately want it to be used as []byte, >> i'd suggest you do this: >> > > var HeaderDictionary = []byte { /* ... */ } >> > > to save the runtime conversion (and copy). >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/**dictionary.go<https... > File spdy/dictionary.go (right): > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > dictionary.go#newcode1<https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode1> > spdy/dictionary.go:1: // Copyright 2011 The Go Authors. All rights > reserved. > On 2013/01/15 22:44:39, mikio wrote: > >> s/2011/2013/ >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > dictionary.go#newcode8<https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode8> > spdy/dictionary.go:8: // This dictionary defined in > On 2013/01/15 22:44:39, mikio wrote: > >> The dictionary is defined in... >> But I'd like to drop this line once we pointed out the reference >> in the package description. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > dictionary.go#newcode10<https://codereview.appspot.com/7092050/diff/1002/spdy/dictionary.go#newcode10> > spdy/dictionary.go:10: const HeaderDictionary = "" + > On 2013/01/15 22:44:39, mikio wrote: > >> Please have a look at minux's comments carefully. >> > > I agree with minux, no need to expose HeaderDictionary >> as a part of API and it would be nice to avoid runtime >> conversion in NewFramer. >> > > var headerDictionary = []byte { /* ... */ } >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/**read.go<https://cod... > File spdy/read.go (right): > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode46<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode46> > spdy/read.go:46: // Read a frame to SettingFrame > On 2013/01/15 21:17:22, bradfitz wrote: > >> I would drop all these comments like this one (above and below too). >> > Only keep > >> the ones that add clarity to the code. These are just implementing an >> interface, so it's obvious what they do. >> > > It just looks noisy and confusing as is, and they're not even in the >> conventional Go full-sentence style. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode59<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode59> > spdy/read.go:59: // | value(32) | > On 2013/01/15 22:44:39, mikio wrote: > >> no need to keep protocol wire format fragments here. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode79<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode79> > spdy/read.go:79: // frame.CFHeader.Flags should be 0 > On 2013/01/15 22:44:39, mikio wrote: > >> please drop >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode92<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode92> > spdy/read.go:92: // frame.CFHeader.Flags should be 0 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode92<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode92> > spdy/read.go:92: // frame.CFHeader.Flags should be 0 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode96<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode96> > spdy/read.go:96: // frame.CFHeader.length should be 8 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode96<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode96> > spdy/read.go:96: // frame.CFHeader.length should be 8 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode118<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode118> > spdy/read.go:118: // frame.CFHeader.Flags should be 0 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode122<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode122> > spdy/read.go:122: // frame.CFHeader.length should be 8 > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode176<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode176> > spdy/read.go:176: // | 1 | Version(15) | Type(16) | > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode190<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode190> > spdy/read.go:190: // | Flags (8) | Length (24) | > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode198<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode198> > spdy/read.go:198: if err = cframe.read(header, f); err != nil { // > delegating each Frames read() > On 2013/01/15 22:44:39, mikio wrote: > >> useful comment? >> > > Done. > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode264<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode264> > spdy/read.go:264: > On 2013/01/15 22:44:39, mikio wrote: > >> remove blank line >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode286<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode286> > spdy/read.go:286: // check Request Header doesn't includes invalid > Header > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode293<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode293> > spdy/read.go:293: // listed in mustReqHeaders(type.go) > I'll think about this implementation later. > remove now for pending. > > On 2013/01/15 22:44:39, mikio wrote: > >> if you think it's really needed, you should do. otherwise please drop. >> > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode311<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode311> > spdy/read.go:311: // set a decompressor using HeaderDictionary to > f.headerDecompressor > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > read.go#newcode347<https://codereview.appspot.com/7092050/diff/1002/spdy/read.go#newcode347> > spdy/read.go:347: // set a decompressor using HeaderDictionary to > f.headerDecompressor > On 2013/01/15 22:44:39, mikio wrote: > >> ditto >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/**types.go<https://co... > File spdy/types.go (right): > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode5<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode5> > spdy/types.go:5: // Package spdy implements HTTP2.0 (SPDY/3) protocol > which is described in > On 2013/01/15 21:17:22, bradfitz wrote: > >> HTTP/2.0 would be the correct punctuation, but it's not HTTP/2.0 yet, >> > so I'd > >> just say: >> > > // Package spdy implements the SPDY protocol (currently SPDY/3), >> > described in > >> ..... >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode23<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode23> > spdy/types.go:23: const ( // Noop (0x0005) removed spdy/3 > On 2013/01/15 22:44:39, mikio wrote: > >> drop the comment >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode50<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode50> > spdy/types.go:50: // that can be stored in one frame. > On 2013/01/15 22:44:39, mikio wrote: > >> why folding? >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode53<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode53> > spdy/types.go:53: // Separator for multiple Header Values > On 2013/01/15 21:17:22, bradfitz wrote: > >> Correct Go style is to use a complete sentence here, starting with the >> > subject, > >> so it stands alone. >> > > // HeaderValueSepator separates multiple header values. >> > > But really: why is this exported? Who is expected to need this? >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode65<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode65> > spdy/types.go:65: // Control Frame Format > On 2013/01/15 22:44:39, mikio wrote: > >> i don't think the wire format in the package docs is helpful. >> please run godoc and see your changes to the docs. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode154<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode154> > spdy/types.go:154: type RSTStatusCode uint32 > On 2013/01/15 22:44:39, mikio wrote: > >> perhaps RstStreamStatus might be better because >> spdy/3 defines both session and stream error >> handlings and RstStreamStatus is used for handling >> stream errors. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode156<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode156> > spdy/types.go:156: const ( // 0 is invalid > On 2013/01/15 22:44:39, mikio wrote: > >> drop the comment >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode157<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157> > spdy/types.go:157: ProtocolError RSTStatusCode = 1 > On 2013/01/16 03:02:16, mikio wrote: > >> or >> > > const ( >> ProtocolError RstStreamStatus = iota + 1 >> InvalidStream >> RefusedStream >> UnsupportedVersion >> Cancel >> InternalError >> FlowControlError >> StreamInUse >> StreamAlreadyClosed >> InvalidCredentials >> FlameTooLarge >> ) >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode157<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode157> > spdy/types.go:157: ProtocolError RSTStatusCode = 1 > On 2013/01/16 03:02:16, mikio wrote: > >> or >> > > const ( >> ProtocolError RstStreamStatus = iota + 1 >> InvalidStream >> RefusedStream >> UnsupportedVersion >> Cancel >> InternalError >> FlowControlError >> StreamInUse >> StreamAlreadyClosed >> InvalidCredentials >> FlameTooLarge >> ) >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode164<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode164> > spdy/types.go:164: STREAM_IN_USE = 8 > On 2013/01/15 21:17:22, bradfitz wrote: > >> Wrong case. See the lines above for Go style. >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode271<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode271> > spdy/types.go:271: StatusCode uint32 > On 2013/01/15 22:44:39, mikio wrote: > >> you can make new type GoAwayStatus or SessionStatus >> and provide protocol constants to users. >> > > const ( >> GoAwayOK GoAwayStatus = 0 >> GoAwayProtocolError = 1 >> GoAwayInternalError = 11 >> ) >> > > Done. > > > https://codereview.appspot.**com/7092050/diff/1002/spdy/** > types.go#newcode384<https://codereview.appspot.com/7092050/diff/1002/spdy/types.go#newcode384> > spdy/types.go:384: // TODO: need this ? > I'll think about this implementation later. > so remove this for pending. > > On 2013/01/15 22:44:39, mikio wrote: > >> do it or drop. >> > > https://codereview.appspot.**com/7092050/<https://codereview.appspot.com/7092... >
Sign in to reply to this message.
sorry, still minor, stylistic comments. I think it would be nice to keep your changes small, concise and consistent because it makes us easy to focus on the subject of this CL. thanks for tackling this. https://codereview.appspot.com/7092050/diff/17001/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/dictionary.go#newcode7 spdy/dictionary.go:7: // http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3 I think this line is redundant because you already specified protocol version in the package description, so pls drop. https://codereview.appspot.com/7092050/diff/17001/spdy/read.go File spdy/read.go (left): https://codereview.appspot.com/7092050/diff/17001/spdy/read.go#oldcode119 spdy/read.go:119: // ReadFrame reads SPDY encoded data and returns a decompressed Frame. please revert this line https://codereview.appspot.com/7092050/diff/17001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/read.go#newcode1 spdy/read.go:1: // Copyright 2013 The Go Authors. All rights reserved. apology if we didn't mention that no need to edit copyright lines, no need to update the year. just add it to new files. https://codereview.appspot.com/7092050/diff/17001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/spdy_test.go#newcode1 spdy/spdy_test.go:1: // Copyright 2013 The Go Authors. All rights reserved. pls revert https://codereview.appspot.com/7092050/diff/17001/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode1 spdy/types.go:1: // Copyright 2013 The Go Authors. All rights reserved. pls revert https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode6 spdy/types.go:6: // http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3 pls add a full stop https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode56 spdy/types.go:56: // Use Framer to read and write it. i think original codes keep around 80 chars long and consistency. why break? https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode77 spdy/types.go:77: // in-memory representation of a SYN_STREAM frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode90 spdy/types.go:90: // in-memory representation of a SYN_REPLY frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode97 spdy/types.go:97: // RstStreamStatus represents the status that led to a RST_STREAM full stop? https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode102 spdy/types.go:102: ProtocolError RstStreamStatus = iota drop iota for ProtocolError, you've already started w/ blank id. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode116 spdy/types.go:116: // in-memory representation of a RST_STREAM ditto (revert+full stop) https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode136 spdy/types.go:136: SettingsUploadBandwidth SettingsId = 1 iota? https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode148 spdy/types.go:148: // flag/id/value for a setting in a SETTINGS frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode156 spdy/types.go:156: // in-memory representation of a SPDY SETTINGS frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode170 spdy/types.go:170: // in-memory representation of a GOAWAY frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode182 spdy/types.go:182: GoAwayStatus uint32 s/uint32/GoAwayStatus/ https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode186 spdy/types.go:186: // in-memory representation of a HEADERS frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode194 spdy/types.go:194: // in-memory representation of a WINDOW_UPDATE frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode201 spdy/types.go:201: // TODO: Unused, so not implemented maybe; TODO: Implement credential frame and related methods https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode202 spdy/types.go:202: // Control Frame: CREDENTIAL no need to keep the wire format here https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode220 spdy/types.go:220: // in-memory representation of a DATA frame. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode223 spdy/types.go:223: // Should be 0 for data frames. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode270 spdy/types.go:270: // including compressing/decompressing payloads. ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode1 spdy/write.go:1: // Copyright 2013 The Go Authors. All rights reserved. pls revert https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode15 spdy/write.go:15: // delegating framer.writeSynStreamFrame pls drop. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode21 spdy/write.go:21: // delegating framer.writeSynReplayFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode26 spdy/write.go:26: // Writes a frame to RstStreamFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode44 spdy/write.go:44: // RST_STREAM Status should not be 0 ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode53 spdy/write.go:53: // Writes a frame to SettingsFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode78 spdy/write.go:78: // Writes a frame to PingFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode98 spdy/write.go:98: // Writes a frame to GoAwayFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode118 spdy/write.go:118: // Writes a frame to HeadersFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode123 spdy/write.go:123: // Writes a frame to WindowUpdateFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode143 spdy/write.go:143: // Writes a frame to DataFrame ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode149 spdy/write.go:149: // Delegates each frames write() ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode154 spdy/write.go:154: // Write Control bit 1, Version, Type, Flags, Length to buffer ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode156 spdy/write.go:156: controlBit := uint16(0x8000) pls revert https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode172 spdy/write.go:172: // repeats length of name & name, length of value & value ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode204 spdy/write.go:204: // writes a name/value using zlib.NewWriterLevelDict ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode212 spdy/write.go:212: writer = f.headerCompressor // zlib.NewWriterLevelDict drop the comment https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode251 spdy/write.go:251: // writes a name/value using zlib.NewWriterLevelDict ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode259 spdy/write.go:259: writer = f.headerCompressor // zlib.NewWriterLevelDict drop the comment https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode289 spdy/write.go:289: // writes a name/value using zlib.NewWriterLevelDict ditto https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode297 spdy/write.go:297: writer = f.headerCompressor // zlib.NewWriterLevelDict drop the comment https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode325 spdy/write.go:325: // Writes a frame to DataFrame ditto
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/17001/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode182 spdy/types.go:182: GoAwayStatus uint32 and s/^\tGoAwayStatus/^\tStatus/
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
all done https://codereview.appspot.com/7092050/diff/17001/spdy/dictionary.go File spdy/dictionary.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/dictionary.go#newcode7 spdy/dictionary.go:7: // http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3 On 2013/01/21 12:04:24, mikio wrote: > I think this line is redundant because you already specified > protocol version in the package description, so pls drop. Done. https://codereview.appspot.com/7092050/diff/17001/spdy/read.go File spdy/read.go (left): https://codereview.appspot.com/7092050/diff/17001/spdy/read.go#oldcode119 spdy/read.go:119: // ReadFrame reads SPDY encoded data and returns a decompressed Frame. On 2013/01/21 12:04:24, mikio wrote: > please revert this line Done. https://codereview.appspot.com/7092050/diff/17001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/read.go#newcode1 spdy/read.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2013/01/21 12:04:24, mikio wrote: > apology if we didn't mention that no need to edit copyright lines, > no need to update the year. just add it to new files. Done. https://codereview.appspot.com/7092050/diff/17001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/spdy_test.go#newcode1 spdy/spdy_test.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2013/01/21 12:04:24, mikio wrote: > pls revert Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode1 spdy/types.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2013/01/21 12:04:24, mikio wrote: > pls revert Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode6 spdy/types.go:6: // http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3 On 2013/01/21 12:04:24, mikio wrote: > pls add a full stop Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode56 spdy/types.go:56: // Use Framer to read and write it. On 2013/01/21 12:04:24, mikio wrote: > i think original codes keep around 80 chars long > and consistency. why break? Sorry, It's all my fault. I'll fix all. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode77 spdy/types.go:77: // in-memory representation of a SYN_STREAM frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode90 spdy/types.go:90: // in-memory representation of a SYN_REPLY frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode97 spdy/types.go:97: // RstStreamStatus represents the status that led to a RST_STREAM On 2013/01/21 12:04:24, mikio wrote: > full stop? Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode102 spdy/types.go:102: ProtocolError RstStreamStatus = iota On 2013/01/21 12:04:24, mikio wrote: > drop iota for ProtocolError, you've already started w/ blank id. Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode116 spdy/types.go:116: // in-memory representation of a RST_STREAM On 2013/01/21 12:04:24, mikio wrote: > ditto (revert+full stop) Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode136 spdy/types.go:136: SettingsUploadBandwidth SettingsId = 1 On 2013/01/21 12:04:24, mikio wrote: > iota? Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode136 spdy/types.go:136: SettingsUploadBandwidth SettingsId = 1 On 2013/01/21 12:04:24, mikio wrote: > iota? Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode148 spdy/types.go:148: // flag/id/value for a setting in a SETTINGS frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode156 spdy/types.go:156: // in-memory representation of a SPDY SETTINGS frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode170 spdy/types.go:170: // in-memory representation of a GOAWAY frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode182 spdy/types.go:182: GoAwayStatus uint32 On 2013/01/21 12:04:24, mikio wrote: > s/uint32/GoAwayStatus/ Oh I miss understood sorry. :( https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode186 spdy/types.go:186: // in-memory representation of a HEADERS frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode194 spdy/types.go:194: // in-memory representation of a WINDOW_UPDATE frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode201 spdy/types.go:201: // TODO: Unused, so not implemented On 2013/01/21 12:04:24, mikio wrote: > maybe; > > TODO: Implement credential frame and related methods Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode202 spdy/types.go:202: // Control Frame: CREDENTIAL On 2013/01/21 12:04:24, mikio wrote: > no need to keep the wire format here Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode220 spdy/types.go:220: // in-memory representation of a DATA frame. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode223 spdy/types.go:223: // Should be 0 for data frames. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/types.go#newcode270 spdy/types.go:270: // including compressing/decompressing payloads. On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode1 spdy/write.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2013/01/21 12:04:24, mikio wrote: > pls revert Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode15 spdy/write.go:15: // delegating framer.writeSynStreamFrame On 2013/01/21 12:04:24, mikio wrote: > pls drop. Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode21 spdy/write.go:21: // delegating framer.writeSynReplayFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode26 spdy/write.go:26: // Writes a frame to RstStreamFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode44 spdy/write.go:44: // RST_STREAM Status should not be 0 On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode53 spdy/write.go:53: // Writes a frame to SettingsFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode78 spdy/write.go:78: // Writes a frame to PingFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode98 spdy/write.go:98: // Writes a frame to GoAwayFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode118 spdy/write.go:118: // Writes a frame to HeadersFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode123 spdy/write.go:123: // Writes a frame to WindowUpdateFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode143 spdy/write.go:143: // Writes a frame to DataFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode149 spdy/write.go:149: // Delegates each frames write() On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode154 spdy/write.go:154: // Write Control bit 1, Version, Type, Flags, Length to buffer On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode156 spdy/write.go:156: controlBit := uint16(0x8000) On 2013/01/21 12:04:24, mikio wrote: > pls revert Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode172 spdy/write.go:172: // repeats length of name & name, length of value & value On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode204 spdy/write.go:204: // writes a name/value using zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode212 spdy/write.go:212: writer = f.headerCompressor // zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > drop the comment Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode251 spdy/write.go:251: // writes a name/value using zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode259 spdy/write.go:259: writer = f.headerCompressor // zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > drop the comment Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode289 spdy/write.go:289: // writes a name/value using zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode297 spdy/write.go:297: writer = f.headerCompressor // zlib.NewWriterLevelDict On 2013/01/21 12:04:24, mikio wrote: > drop the comment Done. https://codereview.appspot.com/7092050/diff/17001/spdy/write.go#newcode325 spdy/write.go:325: // Writes a frame to DataFrame On 2013/01/21 12:04:24, mikio wrote: > ditto Done.
Sign in to reply to this message.
thanks. just realized there's no client/server test cases so far. do you plan to implement those tests in following CLs? e.g., hg mv spdy_test.go framer_test.go hg add client_test.go server_test.go https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go#newcode11 spdy/spdy_test.go:11: "fmt" not used https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go#newcode615 spdy/spdy_test.go:615: t.Log("skipping") // TODO: update to work with SPDY3 you can use t.Skipf("skipping: TODO: update to work with SPDY/3") https://codereview.appspot.com/7092050/diff/24002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode38 spdy/types.go:38: ControlFlagFin ControlFlags = 0x01 other flags? https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode82 spdy/types.go:82: // Note, only 3 highest bits currently used looks like this comment is wrong as an API description. or should we, I mean API users, set the Priority field of SynStreamFrame to pri<<5 before WriteFrame? https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode161 spdy/types.go:161: // in-memory representation of a PING frame. revert https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode167 spdy/types.go:167: // GoAwayFrame is the unpacked, in-memory representation of a GOAWAY frame. move to just before type GoAwayFrame line. also pls document not only GoAwayFrame but GoAwayStatus. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode173 spdy/types.go:173: GoAwayInternalError = 11 you can also use iota here, the value 11 is just an errata and spdy/3 already fixed it. https://codereview.appspot.com/7092050/diff/24002/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/write.go#newcode302 spdy/write.go:302: // Validate DataFrame. drop
Sign in to reply to this message.
still in progress but some confirm. https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go#newcode615 spdy/spdy_test.go:615: t.Log("skipping") // TODO: update to work with SPDY3 On 2013/01/23 02:24:23, mikio wrote: > you can use t.Skipf("skipping: TODO: update to work with SPDY/3") Is that implemented ? http://golang.org/pkg/testing/ https://codereview.appspot.com/7092050/diff/24002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode82 spdy/types.go:82: // Note, only 3 highest bits currently used No, users dosen't need to care about that. this notes says about implementation. so, Should I drop this Notes ? On 2013/01/23 02:24:23, mikio wrote: > looks like this comment is wrong as an API description. > or should we, I mean API users, set the Priority field of > SynStreamFrame to pri<<5 before WriteFrame? >
Sign in to reply to this message.
On Fri, Jan 25, 2013 at 12:57 PM, <block.rxckin.beats@gmail.com> wrote: >> you can use t.Skipf("skipping: TODO: update to work with SPDY/3") > > Is that implemented ? submitted to tip yesterday. but I think you can fix that test for spdy/3, so please do. > spdy/types.go:82: // Note, only 3 highest bits currently used > No, users dosen't need to care about that. > this notes says about implementation. > so, Should I drop this Notes ? yup. also I'd recommend to add a simple comment to each exposed field which seems vague. e.g., // SynStreamFrame is the unpacked, in-memory representation of a SYN_STREAM // frame. type SynStreamFrame struct { CFHeader ControlFrameHeader StreamId uint32 // stream identifier AssociatedToStreamId uint32 // stream identifer for a stream which this stream is associated to Priority uint8 // 3-bit priority Slot uint8 // index in the server's credential vector of the client certificate Headers http.Header }
Sign in to reply to this message.
> just realized there's no client/server test cases so far. > do you plan to implement those tests in following CLs? never mind, someone will add it someday. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode100 spdy/types.go:100: _ RstStreamStatus = iota ProtocolError RstStreamStatus = iota +1 godoc doesn't display unexposed things. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode134 spdy/types.go:134: _ SettingsId = iota ditto
Sign in to reply to this message.
all done. >> just realized there's no client/server test cases so far. >> do you plan to implement those tests in following CLs? > never mind, someone will add it someday. I want to focus this CL for parser. I'll do that in another CLs. https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go#newcode11 spdy/spdy_test.go:11: "fmt" On 2013/01/23 02:24:23, mikio wrote: > not used Done. https://codereview.appspot.com/7092050/diff/24002/spdy/spdy_test.go#newcode615 spdy/spdy_test.go:615: t.Log("skipping") // TODO: update to work with SPDY3 On 2013/01/25 03:57:46, Jxck wrote: > On 2013/01/23 02:24:23, mikio wrote: > > you can use t.Skipf("skipping: TODO: update to work with SPDY/3") > > Is that implemented ? > http://golang.org/pkg/testing/ Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode38 spdy/types.go:38: ControlFlagFin ControlFlags = 0x01 On 2013/01/23 02:24:23, mikio wrote: > other flags? Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode82 spdy/types.go:82: // Note, only 3 highest bits currently used On 2013/01/25 03:57:46, Jxck wrote: > No, users dosen't need to care about that. > this notes says about implementation. > so, Should I drop this Notes ? > > On 2013/01/23 02:24:23, mikio wrote: > > looks like this comment is wrong as an API description. > > or should we, I mean API users, set the Priority field of > > SynStreamFrame to pri<<5 before WriteFrame? > > > Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode100 spdy/types.go:100: _ RstStreamStatus = iota On 2013/01/25 04:28:49, mikio wrote: > ProtocolError RstStreamStatus = iota +1 > > godoc doesn't display unexposed things. Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode134 spdy/types.go:134: _ SettingsId = iota On 2013/01/25 04:28:49, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode161 spdy/types.go:161: // in-memory representation of a PING frame. On 2013/01/23 02:24:23, mikio wrote: > revert Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode167 spdy/types.go:167: // GoAwayFrame is the unpacked, in-memory representation of a GOAWAY frame. On 2013/01/23 02:24:23, mikio wrote: > move to just before type GoAwayFrame line. > also pls document not only GoAwayFrame but GoAwayStatus. Done. https://codereview.appspot.com/7092050/diff/24002/spdy/types.go#newcode173 spdy/types.go:173: GoAwayInternalError = 11 On 2013/01/23 02:24:23, mikio wrote: > you can also use iota here, > the value 11 is just an errata and spdy/3 already fixed it. Done. https://codereview.appspot.com/7092050/diff/24002/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/24002/spdy/write.go#newcode302 spdy/write.go:302: // Validate DataFrame. On 2013/01/23 02:24:23, mikio wrote: > drop Done.
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
looks close. have you already signed the CLA per http://golang.org/doc/contribute.html#copyright ? https://codereview.appspot.com/7092050/diff/34001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode151 spdy/read.go:151: if (firstWord & 0x80000000) != 0 { unnecessary parens https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode153 spdy/read.go:153: version := uint16(0x7fff & (firstWord >> 16)) version := uint16(firstWord >> 16 & 0x7fff) https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode244 spdy/read.go:244: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { unnecessary parens https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode276 spdy/read.go:276: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { ditto https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode308 spdy/read.go:308: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { ditto https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go#newcode614 spdy/spdy_test.go:614: t.Skipf("skipping: TODO: update to work with SPDY/3") sorry, I changed my mind; opting not to apply t.Skip to to the subrepos makes sense for both Go 1 and 1.1. so pls revert testing.Skip here. thanks. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode81 spdy/types.go:81: StreamId uint32 wouldn't StreamId be the better type here? https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode116 spdy/types.go:116: StreamId uint32 ditto https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode182 spdy/types.go:182: StreamId uint32 ditto https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode199 spdy/types.go:199: StreamId uint32 ditto https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode222 spdy/types.go:222: StreamId uint32 ditto https://codereview.appspot.com/7092050/diff/34001/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode60 spdy/write.go:60: flagId := (uint32(flagIdValue.Flag) << 24) | uint32(flagIdValue.Id) unnecessary parens https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode148 spdy/write.go:148: flagsAndLength := (uint32(h.Flags) << 24) | h.length ditto https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode310 spdy/write.go:310: flagsAndLength := (uint32(frame.Flags) << 24) | uint32(len(frame.Data)) ditto https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode317 spdy/write.go:317: drop blank line
Sign in to reply to this message.
all done. https://codereview.appspot.com/7092050/diff/34001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode151 spdy/read.go:151: if (firstWord & 0x80000000) != 0 { On 2013/01/31 13:12:24, mikio wrote: > unnecessary parens Done. https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode153 spdy/read.go:153: version := uint16(0x7fff & (firstWord >> 16)) On 2013/01/31 13:12:24, mikio wrote: > version := uint16(firstWord >> 16 & 0x7fff) > Done. https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode244 spdy/read.go:244: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { On 2013/01/31 13:12:24, mikio wrote: > unnecessary parens Done. https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode276 spdy/read.go:276: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode308 spdy/read.go:308: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go File spdy/spdy_test.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go#newcode614 spdy/spdy_test.go:614: t.Skipf("skipping: TODO: update to work with SPDY/3") On 2013/01/31 13:12:24, mikio wrote: > sorry, I changed my mind; opting not to apply t.Skip to to the subrepos makes > sense for both Go 1 and 1.1. so pls revert testing.Skip here. thanks. Done. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode81 spdy/types.go:81: StreamId uint32 On 2013/01/31 13:12:24, mikio wrote: > wouldn't StreamId be the better type here? Done. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode116 spdy/types.go:116: StreamId uint32 On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode182 spdy/types.go:182: StreamId uint32 On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode199 spdy/types.go:199: StreamId uint32 On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode222 spdy/types.go:222: StreamId uint32 On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/write.go File spdy/write.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode60 spdy/write.go:60: flagId := (uint32(flagIdValue.Flag) << 24) | uint32(flagIdValue.Id) On 2013/01/31 13:12:24, mikio wrote: > unnecessary parens Done. https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode148 spdy/write.go:148: flagsAndLength := (uint32(h.Flags) << 24) | h.length On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode310 spdy/write.go:310: flagsAndLength := (uint32(frame.Flags) << 24) | uint32(len(frame.Data)) On 2013/01/31 13:12:24, mikio wrote: > ditto Done. https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode317 spdy/write.go:317: On 2013/01/31 13:12:24, mikio wrote: > drop blank line Done.
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
> looks close. have you already signed the CLA per Yes. now processing. On 2013/01/31 13:12:24, mikio wrote: > looks close. have you already signed the CLA per > http://golang.org/doc/contribute.html#copyright ? > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go > File spdy/read.go (right): > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode151 > spdy/read.go:151: if (firstWord & 0x80000000) != 0 { > unnecessary parens > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode153 > spdy/read.go:153: version := uint16(0x7fff & (firstWord >> 16)) > version := uint16(firstWord >> 16 & 0x7fff) > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode244 > spdy/read.go:244: if !f.headerCompressionDisabled && ((err == io.EOF && > f.headerReader.N == 0) || f.headerReader.N != 0) { > unnecessary parens > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode276 > spdy/read.go:276: if !f.headerCompressionDisabled && ((err == io.EOF && > f.headerReader.N == 0) || f.headerReader.N != 0) { > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode308 > spdy/read.go:308: if !f.headerCompressionDisabled && ((err == io.EOF && > f.headerReader.N == 0) || f.headerReader.N != 0) { > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go > File spdy/spdy_test.go (right): > > https://codereview.appspot.com/7092050/diff/34001/spdy/spdy_test.go#newcode614 > spdy/spdy_test.go:614: t.Skipf("skipping: TODO: update to work with SPDY/3") > sorry, I changed my mind; opting not to apply t.Skip to to the subrepos makes > sense for both Go 1 and 1.1. so pls revert testing.Skip here. thanks. > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go > File spdy/types.go (right): > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode81 > spdy/types.go:81: StreamId uint32 > wouldn't StreamId be the better type here? > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode116 > spdy/types.go:116: StreamId uint32 > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode182 > spdy/types.go:182: StreamId uint32 > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode199 > spdy/types.go:199: StreamId uint32 > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/types.go#newcode222 > spdy/types.go:222: StreamId uint32 > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/write.go > File spdy/write.go (right): > > https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode60 > spdy/write.go:60: flagId := (uint32(flagIdValue.Flag) << 24) | > uint32(flagIdValue.Id) > unnecessary parens > > https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode148 > spdy/write.go:148: flagsAndLength := (uint32(h.Flags) << 24) | h.length > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode310 > spdy/write.go:310: flagsAndLength := (uint32(frame.Flags) << 24) | > uint32(len(frame.Data)) > ditto > > https://codereview.appspot.com/7092050/diff/34001/spdy/write.go#newcode317 > spdy/write.go:317: > drop blank line
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/34001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode244 spdy/read.go:244: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { On 2013/02/02 14:05:44, Jxck wrote: > On 2013/01/31 13:12:24, mikio wrote: > > unnecessary parens > > Done. are these the same? if A && B || C and if A && (B || C)
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
done https://codereview.appspot.com/7092050/diff/34001/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/34001/spdy/read.go#newcode244 spdy/read.go:244: if !f.headerCompressionDisabled && ((err == io.EOF && f.headerReader.N == 0) || f.headerReader.N != 0) { ooops ! sorry :( On 2013/02/02 16:04:41, mikio wrote: > On 2013/02/02 14:05:44, Jxck wrote: > > On 2013/01/31 13:12:24, mikio wrote: > > > unnecessary parens > > > > Done. > > are these the same? > > if A && B || C > and > if A && (B || C)
Sign in to reply to this message.
> done not all.
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
I think this would be final tweaks. Please change the CL description to refer the correct one, not IETF I-D. Also please fix a minor typo in the CL description: >But CREDENTIAL frame is not implemented, because this frame >is under discussion and SPEC is unclear. >And I commented out some test because they are too fragile >(havily depends on frame definition). s/havily/heavily/ Wait, hm, I'd like to drop those lines from the CL description because it looks not accurate. Thanks. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode47 spdy/types.go:47: DataFlagCompressed = 0x02 as per chap. 4.5 in the spec, this flag is obsoleted in spdy/3. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode66 spdy/types.go:66: version uint16 // spdy version number (15-bit) // spdy version number https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode69 spdy/types.go:69: length uint32 // length of data field (24-bit) // length of data field https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode77 spdy/types.go:77: type StreamId uint32 pls add a comment. // StreamId represents a 31-bit value identifying the stream. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode83 spdy/types.go:83: StreamId StreamId we now have a concrete type StreamId, so it looks a bit redundant. you can do; s/StreamId/Id/, s/AssociatedToStreamId/AssociatedToId/, s/LastGoodStreamId/LastGoodId/, if you'd like to do.
Sign in to reply to this message.
https://codereview.appspot.com/7092050/diff/46002/spdy/read.go File spdy/read.go (right): https://codereview.appspot.com/7092050/diff/46002/spdy/read.go#newcode276 spdy/read.go:276: if !f.headerCompressionDisabled && (err == io.EOF && f.headerReader.N == 0 || f.headerReader.N != 0) { err == EOF && N == 0 || N != 0 is equivalent to err == EOF || N != 0 Is that what you mean?
Sign in to reply to this message.
Hi Rémy, > Is that what you mean? I guess it just wants to caputure the wrong compressed payload size error from the results of io.LimitedReader. io.LimitedReader returns 0, io.EOF when N <= 0.
Sign in to reply to this message.
done https://codereview.appspot.com/7092050/diff/46002/spdy/types.go File spdy/types.go (right): https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode47 spdy/types.go:47: DataFlagCompressed = 0x02 On 2013/02/04 06:58:17, mikio wrote: > as per chap. 4.5 in the spec, this flag is obsoleted in spdy/3. Done. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode66 spdy/types.go:66: version uint16 // spdy version number (15-bit) On 2013/02/04 06:58:17, mikio wrote: > // spdy version number Done. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode69 spdy/types.go:69: length uint32 // length of data field (24-bit) On 2013/02/04 06:58:17, mikio wrote: > // length of data field Done. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode77 spdy/types.go:77: type StreamId uint32 On 2013/02/04 06:58:17, mikio wrote: > pls add a comment. > // StreamId represents a 31-bit value identifying the stream. Done. https://codereview.appspot.com/7092050/diff/46002/spdy/types.go#newcode83 spdy/types.go:83: StreamId StreamId This name is compatible with frame spec. so won't fix. On 2013/02/04 06:58:17, mikio wrote: > we now have a concrete type StreamId, so it looks a bit redundant. > you can do; > s/StreamId/Id/, > s/AssociatedToStreamId/AssociatedToId/, > s/LastGoodStreamId/LastGoodId/, > if you'd like to do.
Sign in to reply to this message.
Hello adg@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com, bradfitz@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Rémy and Mikio > Is that what you mean? I think it's not equivalent because this case need to find whether payload size is correct or not.
Sign in to reply to this message.
LGTM, thanks. Wait a half day for others.
Sign in to reply to this message.
Mikioh, feel free to submit when you're happy. On Tue, Feb 5, 2013 at 3:56 PM, <mikioh.mikioh@gmail.com> wrote: > LGTM, thanks. > > Wait a half day for others. > > https://codereview.appspot.**com/7092050/<https://codereview.appspot.com/7092... >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=71409a1c89f0&repo=net *** go.net/spdy: update SPDY/2 to SPDY/3 Update to SPDY/3 http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3 R=adg, mikioh.mikioh, minux.ma, bradfitz, remyoudompheng CC=golang-dev https://codereview.appspot.com/7092050 Committer: Mikio Hara <mikioh.mikioh@gmail.com>
Sign in to reply to this message.
Thanks so much !! 2013/2/6 <mikioh.mikioh@gmail.com> > *** Submitted as > https://code.google.com/p/go/**source/detail?r=71409a1c89f0&**repo=net<https:... > > > go.net/spdy: update SPDY/2 to SPDY/3 > > Update to SPDY/3 > http://www.chromium.org/spdy/**spdy-protocol/spdy-protocol-**draft3<http://ww... > > R=adg, mikioh.mikioh, minux.ma, bradfitz, remyoudompheng > CC=golang-dev > https://codereview.appspot.**com/7092050<https://codereview.appspot.com/7092050> > > Committer: Mikio Hara <mikioh.mikioh@gmail.com> > > > https://codereview.appspot.**com/7092050/<https://codereview.appspot.com/7092... >
Sign in to reply to this message.
Next step: want to write the server implementation? There's a hook now. Then test with Chrome. Once working, so the http.RoundTripper implementation. On Feb 6, 2013 5:48 AM, "yusuke" <block.rxckin.beats@gmail.com> wrote: > Thanks so much !! > > 2013/2/6 <mikioh.mikioh@gmail.com> > >> *** Submitted as >> https://code.google.com/p/go/**source/detail?r=71409a1c89f0&**repo=net<https:... >> >> >> go.net/spdy: update SPDY/2 to SPDY/3 >> >> Update to SPDY/3 >> http://www.chromium.org/spdy/**spdy-protocol/spdy-protocol-**draft3<http://ww... >> >> R=adg, mikioh.mikioh, minux.ma, bradfitz, remyoudompheng >> CC=golang-dev >> https://codereview.appspot.**com/7092050<https://codereview.appspot.com/7092050> >> >> Committer: Mikio Hara <mikioh.mikioh@gmail.com> >> >> >> https://codereview.appspot.**com/7092050/<https://codereview.appspot.com/7092... >> > >
Sign in to reply to this message.
I'm just trying to write server implementation. but I think it takes a bit of time for me. If I finish implementation of that, I'll post here again. thanks 2013/2/7 Brad Fitzpatrick <bradfitz@golang.org> > Next step: want to write the server implementation? There's a hook now. > > Then test with Chrome. Once working, so the http.RoundTripper > implementation. > On Feb 6, 2013 5:48 AM, "yusuke" <block.rxckin.beats@gmail.com> wrote: > >> Thanks so much !! >> >> 2013/2/6 <mikioh.mikioh@gmail.com> >> >>> *** Submitted as >>> https://code.google.com/p/go/**source/detail?r=71409a1c89f0&**repo=net<https:... >>> >>> >>> go.net/spdy: update SPDY/2 to SPDY/3 >>> >>> Update to SPDY/3 >>> http://www.chromium.org/spdy/**spdy-protocol/spdy-protocol-**draft3<http://ww... >>> >>> R=adg, mikioh.mikioh, minux.ma, bradfitz, remyoudompheng >>> CC=golang-dev >>> https://codereview.appspot.**com/7092050<https://codereview.appspot.com/7092050> >>> >>> Committer: Mikio Hara <mikioh.mikioh@gmail.com> >>> >>> >>> https://codereview.appspot.**com/7092050/<https://codereview.appspot.com/7092... >>> >> >>
Sign in to reply to this message.
On 2013/02/05 14:59:41, Jxck wrote: > Rémy and Mikio > > > Is that what you mean? > > I think it's not equivalent because this case need to > find whether payload size is correct or not. A && B || !B is always equivalent to A || !B Why do you think it's different?
Sign in to reply to this message.
> A && B || !B is always equivalent to A || !B yup, addressed in https://codereview.appspot.com/7325046/. thanks.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
