Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2009623002:
SpdyUtils support coalescing multi-value headers. (Closed)
|
Created:
4 years, 7 months ago by dahollings Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Buck Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSpdyUtils support coalescing multi-value headers.
Tests the case where a header has more than three values.
This CL lands server change 123109579 by birenroy.
BUG=488484
Committed: https://crrev.com/6c85e25f6c0268ab26f9bd8364e2901e6379eeb4
Cr-Commit-Position: refs/heads/master@{#395893}
Patch Set 1 : SpdyUtils support coalescing multi-value headers. #
Total comments: 9
Patch Set 2 : SpdyUtils support coalescing multi-value headers. #
Total comments: 2
Patch Set 3 : SpdyUtils support coalescing multi-value headers. #
Messages
Total messages: 17 (5 generated)
Patchset #1 (id:1) has been deleted
dahollings@chromium.org changed reviewers: + rch@chromium.org
Thanks for doing this! https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (left): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:127: } Did you mean to remove this? https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); Are you planning to make the internal version of this file match this code? I'm curious what the bug was which makes the test pass internally but fail in the chromium version. Can you clarify?
https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (left): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:127: } On 2016/05/24 21:26:05, Ryan Hamilton wrote: > Did you mean to remove this? No, we should let that happen upon revert of revert (in fact this broke a unit test, hence trybot failures). Thanks for the catch! https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); On 2016/05/24 21:26:05, Ryan Hamilton wrote: > Are you planning to make the internal version of this file match this code? I'm > curious what the bug was which makes the test pass internally but fail in the > chromium version. Can you clarify? Clarification on the bug (sorry if over-detailed): The internal version uses StrCat, which was replaced by StringPrintf in merge to chromium. StringPrintf takes C strings, so doesn't play well with null character delimiting. When we convert iter->second to a char *, it terminates at the first '\0' on read, even if that was intended as a header-value delimiter instead of a null-termination character. Example: StrCat("red\0blue", '\0', "green") -> "red\0blue\0green", yay StringPrintf("%s%c%s", "red\0", '\0', "blue\0") -> "red\0blue\0", yay but StringPrintf("%s%c%s", "red\0blue\0", '\0', "green\0") -> "red\0green\0", since the C string "red\0blue\0" terminates at the first '\0'.
Sweet. Just a couple minor nits. https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:129: std::string s(v.data(), v.length()); nit: you can just use "string" and "StringPiece" since we're doing "using std::string" and "using base::StringPiece at the top of this file. Sorry for missing this. Also down on line 134. https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); On 2016/05/24 22:12:16, dahollings wrote: > On 2016/05/24 21:26:05, Ryan Hamilton wrote: > > Are you planning to make the internal version of this file match this code? > I'm > > curious what the bug was which makes the test pass internally but fail in the > > chromium version. Can you clarify? > > Clarification on the bug (sorry if over-detailed): > > The internal version uses StrCat, which was replaced by StringPrintf in merge to > chromium. StringPrintf takes C strings, so doesn't play well with null character > delimiting. When we convert iter->second to a char *, it terminates at the first > '\0' on read, even if that was intended as a header-value delimiter instead of a > null-termination character. > > Example: > StrCat("red\0blue", '\0', "green") -> "red\0blue\0green", yay > StringPrintf("%s%c%s", "red\0", '\0', "blue\0") -> "red\0blue\0", yay > but StringPrintf("%s%c%s", "red\0blue\0", '\0', "green\0") -> "red\0green\0", > since the C string "red\0blue\0" terminates at the first '\0'. Ah, that makes sense. Should we transition the internal version of this code to use your implementation here for consistency? https://codereview.chromium.org/2009623002/diff/40001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (left): https://codereview.chromium.org/2009623002/diff/40001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:128: Can you restore these lines?
birenroy@chromium.org changed reviewers: + birenroy@chromium.org
https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); On 2016/05/24 22:18:46, Ryan Hamilton wrote: > On 2016/05/24 22:12:16, dahollings wrote: > Ah, that makes sense. Should we transition the internal version of this code to > use your implementation here for consistency? I think StrCat() is great, and the internal version should continue using it.
https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:129: std::string s(v.data(), v.length()); On 2016/05/24 22:18:46, Ryan Hamilton wrote: > nit: you can just use "string" and "StringPiece" since we're doing "using > std::string" and "using base::StringPiece at the top of this file. Sorry for > missing this. Also down on line 134. Done. https://codereview.chromium.org/2009623002/diff/40001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (left): https://codereview.chromium.org/2009623002/diff/40001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:128: On 2016/05/24 22:18:46, Ryan Hamilton wrote: > Can you restore these lines? Done.
lgtm
lgtm https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc File net/quic/spdy_utils.cc (right): https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); On 2016/05/24 22:27:28, Biren Roy wrote: > On 2016/05/24 22:18:46, Ryan Hamilton wrote: > > On 2016/05/24 22:12:16, dahollings wrote: > > Ah, that makes sense. Should we transition the internal version of this code > to > > use your implementation here for consistency? > > I think StrCat() is great, and the internal version should continue using it. In general, we try to minimize the differences between the internal an external versions of the QUIC code so as to simplify the merge process. Is there some reason we should no do that here?
The CQ bit was checked by dahollings@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009623002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009623002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== SpdyUtils support coalescing multi-value headers. Tests the case where a header has more than three values. This CL lands server change 123109579 by birenroy. BUG=488484 ========== to ========== SpdyUtils support coalescing multi-value headers. Tests the case where a header has more than three values. This CL lands server change 123109579 by birenroy. BUG=488484 Committed: https://crrev.com/6c85e25f6c0268ab26f9bd8364e2901e6379eeb4 Cr-Commit-Position: refs/heads/master@{#395893} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6c85e25f6c0268ab26f9bd8364e2901e6379eeb4 Cr-Commit-Position: refs/heads/master@{#395893}
Message was sent while issue was closed.
On 2016/05/24 22:43:56, Ryan Hamilton wrote: > lgtm > > https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc > File net/quic/spdy_utils.cc (right): > > https://codereview.chromium.org/2009623002/diff/20001/net/quic/spdy_utils.cc#... > net/quic/spdy_utils.cc:137: headers->ReplaceOrAppendHeader(name, s); > On 2016/05/24 22:27:28, Biren Roy wrote: > > On 2016/05/24 22:18:46, Ryan Hamilton wrote: > > > On 2016/05/24 22:12:16, dahollings wrote: > > > Ah, that makes sense. Should we transition the internal version of this code > > to > > > use your implementation here for consistency? > > > > I think StrCat() is great, and the internal version should continue using it. > > In general, we try to minimize the differences between the internal an external > versions of the QUIC code so as to simplify the merge process. Is there some > reason we should no do that here? Since it is QUIC code we're talking about, if you prefer the internal and external versions of the code to be the same, I will rubber-stamp your google3 changelist. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
