Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794063002:
Add a SpdyFramer extension API implementation. (Closed)
DescriptionAdd a SpdyFramer extension API implementation.
Add a SpdyFramer extension API implementation that can be used
to parse and emit METADATA frames.
This CL lands server change 150400088 by birenroy
and server change 152256907 by bnc.
BUG=488484
Review-Url: https://codereview.chromium.org/2794063002
Cr-Commit-Position: refs/heads/master@{#462089}
Committed: https://chromium.googlesource.com/chromium/src/+/3cda020e6fa6fe0c9fbdb9a3589520e786425826
Patch Set 1 #
Total comments: 5
Patch Set 2 : Re: #12. #
Total comments: 2
Patch Set 3 : Re: #14. #Patch Set 4 : Add BeginNewExtensionFrame. #
Total comments: 1
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bnc@chromium.org changed reviewers: + diannahu@chromium.org
diannahu: PTAL. Thank you.
lgtm
bnc@chromium.org changed reviewers: + xunjieli@chromium.org
xunjieli: please rubber stamp. diannahu: thank you for the review.
https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder.h File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder... net/spdy/spdy_frame_builder.h:68: uint8_t type, Isn't a SpdyFrameType a uint8_t? What's the diff between this method and the one above?
https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder.h File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder... net/spdy/spdy_frame_builder.h:68: uint8_t type, On 2017/04/04 14:36:36, xunjieli wrote: > Isn't a SpdyFrameType a uint8_t? > What's the diff between this method and the one above? SpdyFrameType is an enum class, and implicit cast is not allowed for enum classes. For extension frame types that are not included in the SpdyFrameType enum it feels wrong to cast them into a SpdyFrameType (an enum with an invalid value?), and for frame types that are included, it feels wrong having to cast them into the underlying uint8_t type at the call site. That's why there are two methods.
https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder.h File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder... net/spdy/spdy_frame_builder.h:68: uint8_t type, On 2017/04/04 15:19:20, Bence wrote: > On 2017/04/04 14:36:36, xunjieli wrote: > > Isn't a SpdyFrameType a uint8_t? > > What's the diff between this method and the one above? > > SpdyFrameType is an enum class, and implicit cast is not allowed for enum > classes. For extension frame types that are not included in the SpdyFrameType > enum it feels wrong to cast them into a SpdyFrameType (an enum with an invalid > value?), and for frame types that are included, it feels wrong having to cast > them into the underlying uint8_t type at the call site. That's why there are > two methods. I see. Function overloads is discouraged per https://google.github.io/styleguide/cppguide.html#Function_Overloading "If a function is overloaded by the argument types alone, a reader may have to understand C++'s complex matching rules in order to tell what's going on. Also many people are confused by the semantics of inheritance if a derived class overrides only some of the variants of a function. If you want to overload a function, consider qualifying the name with some information about the arguments, e.g., AppendString(), AppendInt() rather than just Append(). If you are overloading a function to support variable number of arguments of the same type, consider making it take a std::vector so that the user can use an initializer list to specify the arguments. " We could have a different name to this variant, something like BeginNewFrameWithRawType(). Even with a different name, two uint8_t arguments are next to each other. It's likely that a user can switch |flags| and |type| without knowing. I prefer explicit cast at call places in this case.
xunjieli: PTAL. Thank you. https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder.h File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder... net/spdy/spdy_frame_builder.h:68: uint8_t type, Sorry, I was not aware of this recommendation from the style guide. I'm removing one of the two method. This also means that the DCHECK(IsDefinedFrameType) must be removed, because this method can be called with an undefined (extension) frame type. For the same reason, I decided to keep the method with the uint8_t signature, and remove the one with SpdyFrameType, because SpdyFrameType implies that the frame type is defined. Also, for extension frames, calling a method that takes a SpdyFrameType would involve casting an uint8_t to SpdyFrameType only so that it would be cast back the first thing in BeginNewFrame, which is silly. Also, ParseFrameType() explicitly is undefined for undefined frames. As you said, the disadvantage of this is that |type| and |flags| are side-by-side and of identical types, so a caller might pass them in the wrong order. I still think this is the best option.
lgtm. one minor nit below. https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder.h File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/1/net/spdy/spdy_frame_builder... net/spdy/spdy_frame_builder.h:68: uint8_t type, On 2017/04/04 16:58:47, Bence wrote: > Sorry, I was not aware of this recommendation from the style guide. > > I'm removing one of the two method. This also means that the > DCHECK(IsDefinedFrameType) must be removed, because this method can be called > with an undefined (extension) frame type. > > For the same reason, I decided to keep the method with the uint8_t signature, > and remove the one with SpdyFrameType, because SpdyFrameType implies that the > frame type is defined. Also, for extension frames, calling a method that takes > a SpdyFrameType would involve casting an uint8_t to SpdyFrameType only so that > it would be cast back the first thing in BeginNewFrame, which is silly. Also, > ParseFrameType() explicitly is undefined for undefined frames. > > As you said, the disadvantage of this is that |type| and |flags| are > side-by-side and of identical types, so a caller might pass them in the wrong > order. I still think this is the best option. Acknowledged. That makes sense. https://codereview.chromium.org/2794063002/diff/20001/net/spdy/spdy_frame_bui... File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/20001/net/spdy/spdy_frame_bui... net/spdy/spdy_frame_builder.h:60: uint8_t type, minor nit: could you s/type/raw_frame_type to match the one in the cc file? "raw_frame_type" is slightly more informative. And add a comment in the doc mentioning that we use raw uint8_t here because we accept extension frame types that aren't explicitly defined in the SpdyFrameType enum.
Thank you for the review.
https://codereview.chromium.org/2794063002/diff/20001/net/spdy/spdy_frame_bui... File net/spdy/spdy_frame_builder.h (right): https://codereview.chromium.org/2794063002/diff/20001/net/spdy/spdy_frame_bui... net/spdy/spdy_frame_builder.h:60: uint8_t type, On 2017/04/04 17:07:36, xunjieli wrote: > minor nit: could you s/type/raw_frame_type to match the one in the cc file? > "raw_frame_type" is slightly more informative. > And add a comment in the doc mentioning that we use raw uint8_t here because we > accept extension frame types that aren't explicitly defined in the SpdyFrameType > enum. > > Done.
Description was changed from ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy. BUG=488484 ========== to ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy and server change 152159214 by bnc. BUG=488484 ==========
lgtm! Thanks.
xunjieli: PTAL at Patch Set 4. Thank you. https://codereview.chromium.org/2794063002/diff/60001/net/spdy/spdy_frame_bui... File net/spdy/spdy_frame_builder.h (left): https://codereview.chromium.org/2794063002/diff/60001/net/spdy/spdy_frame_bui... net/spdy/spdy_frame_builder.h:58: // The given type must be a control frame type. This is incorrect: "control frame" means defined frame type other than HEADERS, but in fact this method is also used for HEADER frames.
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/05 13:43:09, Bence wrote: > xunjieli: PTAL at Patch Set 4. Thank you. > > https://codereview.chromium.org/2794063002/diff/60001/net/spdy/spdy_frame_bui... > File net/spdy/spdy_frame_builder.h (left): > > https://codereview.chromium.org/2794063002/diff/60001/net/spdy/spdy_frame_bui... > net/spdy/spdy_frame_builder.h:58: // The given type must be a control frame > type. > This is incorrect: "control frame" means defined frame type other than HEADERS, > but in fact this method is also used for HEADER frames. lgtm!
lgtm
Description was changed from ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy and server change 152159214 by bnc. BUG=488484 ========== to ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy and server change 152256907 by bnc. BUG=488484 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bnc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491407611016080,
"parent_rev": "44a5f5abd385a7bbaddb88b6f743c2864c1c1ec7", "commit_rev":
"3cda020e6fa6fe0c9fbdb9a3589520e786425826"}
Message was sent while issue was closed.
Description was changed from ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy and server change 152256907 by bnc. BUG=488484 ========== to ========== Add a SpdyFramer extension API implementation. Add a SpdyFramer extension API implementation that can be used to parse and emit METADATA frames. This CL lands server change 150400088 by birenroy and server change 152256907 by bnc. BUG=488484 Review-Url: https://codereview.chromium.org/2794063002 Cr-Commit-Position: refs/heads/master@{#462089} Committed: https://chromium.googlesource.com/chromium/src/+/3cda020e6fa6fe0c9fbdb9a35895... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3cda020e6fa6fe0c9fbdb9a35895... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
