Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889033002:
Better header value parsing for Server-Timing. (Closed)
|
Created:
3 years, 7 months ago by cvazac-akam Modified:
3 years, 6 months ago CC:
blink-reviews, chromium-reviews, e_hakkinen, kinuko+watch Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBetter header value parsing for Server-Timing.
The parsing code now leverages the same helper methods that we use to
parse the content-type header. I also added some tests to make sure
double-quotes and other delimiters are properly handled in a quoted
description.
BUG=702760
Review-Url: https://codereview.chromium.org/2889033002
Cr-Commit-Position: refs/heads/master@{#479443}
Committed: https://chromium.googlesource.com/chromium/src/+/a8342f08488f15c766e3882ec3ce84c952a8a0de
Patch Set 1 #Patch Set 2 : add TODOs for skipping all whitespace #
Total comments: 2
Patch Set 3 : add comment, fix Consume() #Patch Set 4 : test escaped double-quotes #
Total comments: 3
Patch Set 5 : use HeaderFieldTokenizer instead of redundant methods #
Total comments: 8
Patch Set 6 : handle htabs in SkipSpaces, add test coverage, unused code removal #Patch Set 7 : only convert duation token to string and then to double if it was parsed #
Total comments: 1
Patch Set 8 : init description as empty string for readability #
Total comments: 1
Patch Set 9 : remove htab parsing, added TODO #
Total comments: 1
Patch Set 10 : init duration as 0.0 #
Messages
Total messages: 53 (34 generated)
Description was changed from ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG= ========== to ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG= ==========
cvazac@akamai.com changed reviewers: + yoav@yoav.ws
yoav@yoav.ws changed reviewers: + mkwst@chromium.org
LGTM https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:919: // TODO: skip all whitespace, not just spaces The format should be `// TODO(cvazac): ...`. Also can you include a link to https://tools.ietf.org/html/rfc7231#section-3.1.1 in at least the first one. https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp (right): https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp:1002: headerValue.push_back('"'); Why are you pushing back the chars here? Did string initialization fail otherwise for some reason?
The CQ bit was checked by yoav@yoav.ws 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.
On 2017/05/18 21:45:05, Yoav Weiss wrote: > LGTM > > https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): > > https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/network/HTTPParsers.cpp:919: // TODO: skip > all whitespace, not just spaces > The format should be `// TODO(cvazac): ...`. Also can you include a link to > https://tools.ietf.org/html/rfc7231#section-3.1.1 in at least the first one. > > https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp (right): > > https://codereview.chromium.org/2889033002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp:1002: > headerValue.push_back('"'); > Why are you pushing back the chars here? Did string initialization fail > otherwise for some reason? Also, can you add a bug number? (702760 is fine, I don't think this needs its own issue)
Description was changed from ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG= ========== to ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG=702760 ==========
LGTM
The CQ bit was checked by yoav@yoav.ws 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...
https://codereview.chromium.org/2889033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:921: // TODO: skip all whitespace, not just spaces Can you `TODO(cvazac):` this? (and maybe open an issue and link to it here)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eero.hakkinen@intel.com changed reviewers: + eero.hakkinen@intel.com
https://codereview.chromium.org/2889033002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:860: unsigned index = 0; Please rebase, create a HeaderFieldTokenizer object here and use its Consume* member functions instead of using non-member functions which I removed. Sorry for the inconvenience. https://codereview.chromium.org/2889033002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:921: // TODO: skip all whitespace, not just spaces Feel free to move these comments to HeaderFieldTokenizer::SkipSpaces after rebase.
eero.hakkinen@intel.com changed reviewers: - eero.hakkinen@intel.com
This should be good to go. FYI the Consume* methods in HeaderFieldTokenizer are fine as-is, from what I can tell they handle h-tabs correctly, so I didn't move over any TODOs
Thanks for continuing to work on this, despite my nit-picking :) Got a few more though... https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:877: HeaderFieldTokenizer tokenizer(headerValue); Should we call SkipSpaces() here before consuming our first token? (as ConsumeToken() seems to skip spaces after a token but not before it) https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:887: tokenizer.ConsumeToken(Mode::kNormal, duration); Can you call ToDouble here only if there is a duration? (so if ConsumeToken did not fail?) That way we're not allocating a String a calling a long chain of functions in the potentially-common case where duration is not specified. https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:894: metric.ToString(), duration.ToString().ToDouble(), RE "duration.ToString().ToDouble()", we could have avoided the string allocation here but it will result in significantly less readable code. Let's leave it as is https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:905: bool IsTokenCharacter(Mode mode, UChar c) { As the same function defined in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... and this is not used here, I'm assuming this is a leftover from a previous patch https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp (right): https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsersTest.cpp:971: testServerTimingHeader("metric;\"\\\\\\\"\"", {{"metric", "0", "\\\""}}); Can you add a test where there's a leading space here (as well as more tests with OWS in general?)
On 2017/06/08 03:15:30, cvazac-akam wrote: > This should be good to go. FYI the Consume* methods in HeaderFieldTokenizer are > fine as-is, from what I can tell they handle h-tabs correctly, so I didn't move > over any TODOs https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... doesn't seem to handle h-tabs. Is there tab handling that's happening elsewhere?
https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:877: HeaderFieldTokenizer tokenizer(headerValue); On 2017/06/08 06:07:55, Yoav Weiss wrote: > Should we call SkipSpaces() here before consuming our first token? No, the HeaderFieldTokenizer ctor does that. Also, SkipSpaces is private and cannot thus be called here. > (as ConsumeToken() seems to skip spaces after a token but not before it) It does not have to skip spaces before a token, because HeaderFieldTokenizer ensures that it is always in a state where all preceding spaces are already been skipped. https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:878: while (index < headerValue.length()) { This should be while (!tokenizer.IsConsumed()) { (the code works with a wrong while condition because the if block on lines 880-882 will eventually stop the loop).
https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:877: HeaderFieldTokenizer tokenizer(headerValue); On 2017/06/08 07:35:24, e_hakkinen wrote: > On 2017/06/08 06:07:55, Yoav Weiss wrote: > > Should we call SkipSpaces() here before consuming our first token? > > No, the HeaderFieldTokenizer ctor does that. Also, SkipSpaces is private and > cannot thus be called here. > > > (as ConsumeToken() seems to skip spaces after a token but not before it) > > It does not have to skip spaces before a token, because HeaderFieldTokenizer > ensures that it is always in a state where all preceding spaces are already been > skipped. OK, cool. missed the ctor bit
LGTM % nit Mike - care to take another look? A lot have changed since you last LGTMed it. The main point which might be contentious is that whitespace in HeaderFieldTokenizer now also covers horizontal tabs, which impacts both Server Timing as well as Content Type. https://codereview.chromium.org/2889033002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:896: metric.ToString(), duration, description ? description : "")); RE `description ? description : ""` can you initialize description as "" and avoid this condition?
The CQ bit was checked by yoav@yoav.ws 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 checked by yoav@yoav.ws 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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yoav@yoav.ws 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.
https://codereview.chromium.org/2889033002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HeaderFieldTokenizer.cpp (right): https://codereview.chromium.org/2889033002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HeaderFieldTokenizer.cpp:121: while (!IsConsumed() && (input_[index_] == ' ' || input_[index_] == '\t')) Hrm. On the one hand, sure, let's skip tabs. On the other, why aren't we skipping other control characters? https://infra.spec.whatwg.org/#ascii-whitespace defines ASCII whitespace to include some other characters. Can you point to docs that describe the rule this is implementing, and perhaps add a comment to that effect? Is this what's defined in https://tools.ietf.org/html/rfc7230#section-3.2.6 (because that section seems to have more limitations on the character set than we have here.
The CQ bit was checked by yoav@yoav.ws 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...
LGTM % nit https://codereview.chromium.org/2889033002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/HTTPParsers.cpp (right): https://codereview.chromium.org/2889033002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/HTTPParsers.cpp:883: double duration = 0; Nit: An implicit cast... Can you turn that to "0.0"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by yoav@yoav.ws
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2889033002/#ps180001 (title: "init duration as 0.0")
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by yoav@yoav.ws
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": 180001, "attempt_start_ts": 1497460132142810,
"parent_rev": "9e1d8f00a274c6b78edeeb8772cbe62970a6c8a2", "commit_rev":
"a8342f08488f15c766e3882ec3ce84c952a8a0de"}
Message was sent while issue was closed.
Description was changed from ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG=702760 ========== to ========== Better header value parsing for Server-Timing. The parsing code now leverages the same helper methods that we use to parse the content-type header. I also added some tests to make sure double-quotes and other delimiters are properly handled in a quoted description. BUG=702760 Review-Url: https://codereview.chromium.org/2889033002 Cr-Commit-Position: refs/heads/master@{#479443} Committed: https://chromium.googlesource.com/chromium/src/+/a8342f08488f15c766e3882ec3ce... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/a8342f08488f15c766e3882ec3ce... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
