Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
387353003:
Modify data_reduction_proxy_header to support tamper detection logic. (Closed)
Description1. Adds action-parsing functionality for data reduction proxy header.
2. Adds a couple of functions to return different fingerprint value for tamper detection.
3. Adds a function to remove Chrome-Proxy header's fingerprint from its header values, and return the rest of header values.
4. Changes HasDataReductionProxyViaHeader to also tell
whether data reduction proxy Via header occurs at the last
or not.
BUG=381907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287561
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287855
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288272
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Modify HasDataReductionProxyViaHeader to tell whether data reduction proxy occurs at last or not. #
Total comments: 17
Patch Set 4 : Adding constants to avoid change the same file in two CLs . #Patch Set 5 : #Patch Set 6 : after sync #
Total comments: 9
Patch Set 7 : #
Total comments: 24
Patch Set 8 : #Patch Set 9 : merged with latest version #
Total comments: 8
Patch Set 10 : #Patch Set 11 : #
Total comments: 4
Patch Set 12 : #
Total comments: 2
Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : Fix timeout issue in Win7 test. #Patch Set 19 : Fix timeout issue on Win7Test, add unittests. #Patch Set 20 : after sync #
Messages
Total messages: 60 (0 generated)
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:39: *action_value = value.substr(action_prefix.size()); Before the loop, either DCHECK that action_value is not NULL, or, if you allow NULL, only write to action_value if not NULL. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:56: // the rest if multiple actions matches |action_prefix|. match https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:92: "=", This shouldn't be allowed by any of the header processing code and is a good reason to have the action_prefix that is passed into the functions not include the '='. Can you change that everywhere? The implementation of the functions should add the '=' to the action before searching.
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; inline this in the func call. (Don't we have constant for this yet?) https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:35: if (value.size() > action_prefix.size()) { >=? add a test case for that. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:56: // the rest if multiple actions matches |action_prefix|. Document return value.
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; On 2014/07/14 19:23:58, bolian wrote: > inline this in the func call. (Don't we have constant for this yet?) Acknowledged. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:35: if (value.size() > action_prefix.size()) { On 2014/07/14 19:23:59, bolian wrote: > >=? add a test case for that. Done. It should be ">=" to get the empty value, but right now, at least action_prefix + "=", so it needs to be ">" https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:39: *action_value = value.substr(action_prefix.size()); On 2014/07/14 18:27:13, bengr1 wrote: > Before the loop, either DCHECK that action_value is not NULL, or, if you allow > NULL, only write to action_value if not NULL. Done. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:56: // the rest if multiple actions matches |action_prefix|. On 2014/07/14 18:27:14, bengr1 wrote: > match Done. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:56: // the rest if multiple actions matches |action_prefix|. On 2014/07/14 19:23:59, bolian wrote: > Document return value. Acknowledged. https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:92: "=", On 2014/07/14 18:27:14, bengr1 wrote: > This shouldn't be allowed by any of the header processing code and is a good > reason to have the action_prefix that is passed into the functions not include > the '='. Can you change that everywhere? The implementation of the functions > should add the '=' to the action before searching. Done.
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; ? https://codereview.chromium.org/387353003/diff/40001/components/data_reductio... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/40001/components/data_reductio... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:50: bool GetDataReductionProxyBypassDuration( why doesn't this one call GetDataReductionProxyActionValue?
https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:32: std::string name = "chrome-proxy"; On 2014/07/14 22:42:43, bolian wrote: > ? Copied code from below. https://codereview.chromium.org/387353003/diff/40001/components/data_reductio... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/40001/components/data_reductio... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:50: bool GetDataReductionProxyBypassDuration( On 2014/07/14 22:42:43, bolian wrote: > why doesn't this one call GetDataReductionProxyActionValue? Need to discuss with Ben: my function (above) returns the first matched string; his function (below) returns the first matched, integer-convertable string.
Changes HasDataReductionProxyViaHeader to also tell whether data reduction proxy Via header occurs at the last or not.
https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: DCHECK(action_prefix.size()); DCHECK(!action_prefix.empty()); https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: std::string name = "chrome-proxy"; Add an anonymous namespace above this namespace and add this as const char [], e.g.: namespace { const char kChromeProxyHeader[] = "chrome-proxy"; } Use this constant directly in EnumerateHeader here and below. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: value[action_prefix.size()] == '=') { I think it's better to add '=' to the prefix before the loop. You should enforce that the last character of the prefix is not an '=' before doing so. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: value[action_prefix.size()] == '=') { See above comment. The loop logic shouldn't change. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:107: bool* is_the_last) { change to has_intermediary. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:40: // value. If data reduction proxy Via header value exists, set |is_the_last| I can't parse this comment. How about: // Returns true if the response contains the data reduction proxy Via header // value. If non-NULL, sets |has_intermediary| to true if another server added a // Via header after the data reduction proxy, and to false otherwise. Also, change is_the_last to has_intermediary https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:112: bool has_action_key = GetDataReductionProxyActionValue(parsed, indentation is strange. Parameters should all be on one line or aligned with each other. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:118: action_value); Move up a line.
https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:27: DCHECK(action_prefix.size()); On 2014/07/16 19:23:56, bengr1 wrote: > DCHECK(!action_prefix.empty()); Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: std::string name = "chrome-proxy"; On 2014/07/16 19:23:56, bengr1 wrote: > Add an anonymous namespace above this namespace and add this as const char [], > e.g.: > > namespace { > const char kChromeProxyHeader[] = "chrome-proxy"; > } > > Use this constant directly in EnumerateHeader here and below. Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: value[action_prefix.size()] == '=') { On 2014/07/16 19:23:56, bengr1 wrote: > I think it's better to add '=' to the prefix before the loop. You should enforce > that the last character of the prefix is not an '=' before doing so. Done. "enforce the last character is not '='", by DCHECK? If so, Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: value[action_prefix.size()] == '=') { On 2014/07/16 19:23:56, bengr1 wrote: > See above comment. The loop logic shouldn't change. Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: value[action_prefix.size()] == '=') { On 2014/07/16 19:23:56, bengr1 wrote: > See above comment. The loop logic shouldn't change. Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:107: bool* is_the_last) { On 2014/07/16 19:23:56, bengr1 wrote: > change to has_intermediary. Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:40: // value. If data reduction proxy Via header value exists, set |is_the_last| On 2014/07/16 19:23:56, bengr1 wrote: > I can't parse this comment. How about: > > // Returns true if the response contains the data reduction proxy Via header > // value. If non-NULL, sets |has_intermediary| to true if another server added a > > // Via header after the data reduction proxy, and to false otherwise. > > Also, change is_the_last to has_intermediary Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:112: bool has_action_key = GetDataReductionProxyActionValue(parsed, On 2014/07/16 19:23:56, bengr1 wrote: > indentation is strange. Parameters should all be on one line or aligned with > each other. Done. https://codereview.chromium.org/387353003/diff/110001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:118: action_value); On 2014/07/16 19:23:56, bengr1 wrote: > Move up a line. Done.
Is this dead? Can you remove us from reviewers or delete this cl?
Add kChromeProxyActionFingerprint* strings.
https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:180: return data_reduction_proxy::HasDataReductionProxyViaHeader(response_headers, new line before the first argument and put the two args on the same line. https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: DCHECK(action_prefix[action_prefix.size() - 1] != '='); I don't think you need this DCHECK. Unnecessary restriction. https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:130: *has_intermediary = !(headers->EnumerateHeader(&iter, "via", &value)); Are you sure it is ok to call EnumerateHeader inside EnumerateHeader loop? https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:62: // value as a string in |action_value|. Only keeps the first one and ignores s/keeps/returns/
Addressed Bolian's comments. https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/387353003/diff/520001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:180: return data_reduction_proxy::HasDataReductionProxyViaHeader(response_headers, On 2014/07/28 22:59:18, bolian wrote: > new line before the first argument and put the two args on the same line. Done. https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/28 22:59:18, bolian wrote: > I don't think you need this DCHECK. Unnecessary restriction. Done. https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:130: *has_intermediary = !(headers->EnumerateHeader(&iter, "via", &value)); On 2014/07/28 22:59:18, bolian wrote: > Are you sure it is ok to call EnumerateHeader inside EnumerateHeader loop? Yes, it should be fine, |iter| will be updated. https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:62: // value as a string in |action_value|. Only keeps the first one and ignores On 2014/07/28 22:59:18, bolian wrote: > s/keeps/returns/ Done.
lgtm. Wait for bengr.
https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/520001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:37: DCHECK(action_prefix[action_prefix.size() - 1] != '='); I don't see this and other comments are addressed. Assume you changed locally and forgot to upload?
https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: DCHECK(action_prefix[action_prefix.size() - 1] != '='); same here, you don't need this DCHECK. Why not just allow '=' in the prefix.
https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: const char kChromeProxyActionFingerprintContentLength[] = "fcl"; I really don't like that you expose these names. Is there really no way to provide methods that access what you need to know about them? E.g., bool HasChromeProxyFingerprint(...) or bool GetChromeProxyFingerprint(std::string* fingerprint) https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:36: DCHECK(!action_prefix.empty()); You should also check that header is not NULL. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:39: std::string prefix = action_prefix + "="; Define "=" as kActionValueDelimiter in the anonymous namespace and use here and below. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:42: // ">=" to allow empty action value. I don't think an empty value should be legal. Look at RFC2616, etc., to see which pattern is used for other headers, e.g., cache-control. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/29 17:03:50, bolian wrote: > same here, you don't need this DCHECK. Why not just allow '=' in the prefix. I asked for the '=' to be separate from the prefix. This looks a little hacky though. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:101: headers, "block", &proxy_info->bypass_duration)) { Add to the anonymous namespace: const char kChromeProxyActionBlock[] = "block"; const char kChromeProxyActionBypass[] = "bypass"; https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:128: if (has_intermediary) Add a comment here saying that we presume an intermediary if there is another via header after the data reduction proxy's. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:46: // Returns true if the response contain the data reduction proxy Via header contains https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:50: // other middleboxes between data reduction proxy and chrome. data --> the data chrome --> the client https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:383: scoped_refptr<net::HttpResponseHeaders> parsed; Why did you move this outside the loop? https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:396: has = HasDataReductionProxyViaHeader(parsed, NULL); Why are you adding a case outside the loop of cases?
https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:30: const char kChromeProxyActionFingerprintContentLength[] = "fcl"; On 2014/07/29 17:50:28, bengr1 wrote: > I really don't like that you expose these names. Is there really no way to > provide methods that access what you need to know about them? E.g., > bool HasChromeProxyFingerprint(...) or > bool GetChromeProxyFingerprint(std::string* fingerprint) Adding additional functions so that these constants would not be exposed to namespace. Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:36: DCHECK(!action_prefix.empty()); On 2014/07/29 17:50:28, bengr1 wrote: > You should also check that header is not NULL. Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:39: std::string prefix = action_prefix + "="; On 2014/07/29 17:50:27, bengr1 wrote: > Define "=" as kActionValueDelimiter in the anonymous namespace and use here and > below. Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:42: // ">=" to allow empty action value. On 2014/07/29 17:50:27, bengr1 wrote: > I don't think an empty value should be legal. Look at RFC2616, etc., to see > which pattern is used for other headers, e.g., cache-control. Done. Previously I'm allowing empty action value to sort of send a command from Flywheel without any parameter, say "via=" means "please check whether there is intermediary on Via header". https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/29 17:50:27, bengr1 wrote: > On 2014/07/29 17:03:50, bolian wrote: > > same here, you don't need this DCHECK. Why not just allow '=' in the prefix. > > I asked for the '=' to be separate from the prefix. This looks a little hacky > though. I coded DCHECK there for now. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:61: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/29 17:03:50, bolian wrote: > same here, you don't need this DCHECK. Why not just allow '=' in the prefix. Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:101: headers, "block", &proxy_info->bypass_duration)) { On 2014/07/29 17:50:28, bengr1 wrote: > Add to the anonymous namespace: > const char kChromeProxyActionBlock[] = "block"; > const char kChromeProxyActionBypass[] = "bypass"; Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:128: if (has_intermediary) On 2014/07/29 17:50:27, bengr1 wrote: > Add a comment here saying that we presume an intermediary if there is another > via header after the data reduction proxy's. Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.h (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:46: // Returns true if the response contain the data reduction proxy Via header On 2014/07/29 17:50:28, bengr1 wrote: > contains Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.h:50: // other middleboxes between data reduction proxy and chrome. On 2014/07/29 17:50:28, bengr1 wrote: > data --> the data > chrome --> the client Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:383: scoped_refptr<net::HttpResponseHeaders> parsed; On 2014/07/29 17:50:28, bengr1 wrote: > Why did you move this outside the loop? Add extra field of "NULL or not". Done. https://codereview.chromium.org/387353003/diff/580001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:396: has = HasDataReductionProxyViaHeader(parsed, NULL); On 2014/07/29 17:50:28, bengr1 wrote: > Why are you adding a case outside the loop of cases? Done.
Just merged with latest version. Changes I made to other codes (same to before): 1) not including "=" into prefix when you call ParseHeadersAndSetBypassDuration (call "block" instead of "block="; 2) add "=", "block", "bypass" to constants.
https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:25: const char kActionValueDelimiter[] = "="; Add a blank line after this line. https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:79: DCHECK(action_prefix[action_prefix.size() - 1] != '='); Add a comment to explain why this DCHECK is here. E.g.: // A valid action does not include a trailing '='. https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:221: bool GetDataReductionProxyActionFingerprintChromeProxy( These four trivial functions need tests. They can be simple. E.g., each one can test the behavior when 0, 1, or 2 of the desired action are present in a Chrome-ProxyHeader that has a bunch of other stuff too.
Added trybots. https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:25: const char kActionValueDelimiter[] = "="; On 2014/07/31 16:38:18, bengr1 wrote: > Add a blank line after this line. Done. https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:79: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/31 16:38:18, bengr1 wrote: > Add a comment to explain why this DCHECK is here. E.g.: > > // A valid action does not include a trailing '='. Done. https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:221: bool GetDataReductionProxyActionFingerprintChromeProxy( On 2014/07/31 16:38:17, bengr1 wrote: > These four trivial functions need tests. They can be simple. E.g., each one can > test the behavior when 0, 1, or 2 of the desired action are present in a > Chrome-ProxyHeader that has a bunch of other stuff too. Done.
https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55: DCHECK(action_prefix[action_prefix.size() - 1] != '='); I think you want to use kActionValueDelimiter here and below DCHECK.
https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/387353003/diff/670001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:55: DCHECK(action_prefix[action_prefix.size() - 1] != '='); On 2014/07/31 18:44:09, bolian wrote: > I think you want to use kActionValueDelimiter here and below DCHECK. Done.
lgtm with nits. Please fix the formatting of the description and add a BUG number before submitting. https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:294: bool is_has_intermediary_null; is_has_intermediary_null --> ignore_intermediary https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:410: bool has, has_intermediary; has -> has_chrome_proxy_via_header
This one is ready. https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:294: bool is_has_intermediary_null; On 2014/08/04 16:32:11, bengr1 wrote: > is_has_intermediary_null --> ignore_intermediary Done. https://codereview.chromium.org/387353003/diff/710001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:410: bool has, has_intermediary; On 2014/08/04 16:32:11, bengr1 wrote: > has -> has_chrome_proxy_via_header Done.
The CQ bit was checked by xingx@google.com
The CQ bit was unchecked by xingx@google.com
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/750001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi, Tony, Yaron, Please review a simple change of files: chrome/renderer/page_load_histograms.cc (for Tony) chrome/browser/android/intercept_download_resource_throttle.cc (for Yaron) We changed the interface of HasDataReductionProxyViaHeader to accept one more parameter, which are omitted as NULL if not necessary (so for your files, it's not needed and thus using NULL).
lgtm
lgtm for page_load_histograms.cc. I didn't look at the data_reduction_proxy/* code closely.
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/750001
Message was sent while issue was closed.
Change committed as 287561
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/444723003/ by robliao@chromium.org. The reason for reverting is: Causes DataReductionProxyHeadersTest.GetDataReductionProxyHeaderWithFingerprintRemoved to timeout on the bots..
Message was sent while issue was closed.
On 2014/08/05 21:49:25, robliao wrote: > A revert of this CL has been created in > https://codereview.chromium.org/444723003/ by mailto:robliao@chromium.org. > > The reason for reverting is: Causes > DataReductionProxyHeadersTest.GetDataReductionProxyHeaderWithFingerprintRemoved > to timeout on the bots.. Here's the first failing bot http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28...
Message was sent while issue was closed.
https://codereview.chromium.org/387353003/diff/750001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/750001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:713: struct { Make this const.
Message was sent while issue was closed.
https://codereview.chromium.org/387353003/diff/750001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc (right): https://codereview.chromium.org/387353003/diff/750001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers_unittest.cc:713: struct { On 2014/08/06 17:26:19, bengr1 wrote: > Make this const. Done.
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/770001
Message was sent while issue was closed.
Change committed as 287855
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/447863004/ by xingx@google.com. The reason for reverting is: One unittest causes timeout on Win7 Tests (dbg)(1).
The CQ bit was checked by xingx@google.com
The CQ bit was unchecked by xingx@google.com
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/930001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
Hunk #2 FAILED at 268.
1 out of 2 hunks FAILED -- saving rejects to file
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc.rej
Patch:
components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
Index: components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
diff --git
a/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
b/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
index
ebd48c4fdabf358b3c3936c0568d371583b9871f..64faa95c2b780b74352eba8c3c0504fa754c8b1a
100644
--- a/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
+++ b/components/data_reduction_proxy/common/data_reduction_proxy_headers.cc
@@ -5,6 +5,7 @@
#include
"components/data_reduction_proxy/common/data_reduction_proxy_headers.h"
#include <string>
+#include <vector>
#include "base/rand_util.h"
#include "base/strings/string_number_conversions.h"
@@ -267,7 +268,8 @@ void GetDataReductionProxyHeaderWithFingerprintRemoved(
std::string value;
void* iter = NULL;
while (headers->EnumerateHeader(&iter, kChromeProxyHeader, &value)) {
- if (!LowerCaseEqualsASCII(
+ if (value.size() <= chrome_proxy_fingerprint_prefix.size() ||
+ !LowerCaseEqualsASCII(
value.begin(),
value.begin() + chrome_proxy_fingerprint_prefix.size(),
chrome_proxy_fingerprint_prefix.c_str())) {
The CQ bit was checked by xingx@google.com
The CQ bit was unchecked by xingx@google.com
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/1030001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by xingx@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xingx@chromium.org/387353003/1030001
Message was sent while issue was closed.
Change committed as 288272 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
