Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
390533003:
Bypassed Bytes UMAs (Closed)
|
Created:
6 years, 5 months ago by megjablon Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionBypassed Bytes UMAs
Adding UMAs to count the number of bytes that did not go through Flywheel as the result of a bypass event.
DataReductionProxy.BypassedBytes.NotBypassed
DataReductionProxy.BypassedBytes.SSL
DataReductionProxy.BypassedBytes.LocalBypassRules
DataReductionProxy.BypassedBytes.ShortAll
DataReductionProxy.BypassedBytes.ShortTriggeringRequest
DataReductionProxy.BypassedBytes.ShortAudioVideo
DataReductionProxy.BypassedBytes.MediumAll
DataReductionProxy.BypassedBytes.MediumTriggeringRequest
DataReductionProxy.BypassedBytes.LongAll
DataReductionProxy.BypassedBytes.LongTriggeringRequest
DataReductionProxy.BypassedBytes.MissingViaHeader4xx
DataReductionProxy.BypassedBytes.MissingViaHeaderOther
DataReductionProxy.BypassedBytes.Malformed407
DataReductionProxy.BypassedBytes.Status500HttpInternalServerError
DataReductionProxy.BypassedBytes.Status502HttpBadGateway
DataReductionProxy.BypassedBytes.ServiceUnavailable
DataReductionProxy.BypassedBytes.NetworkErrorOther
BUG=381416, 384369
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285439
Patch Set 1 #
Total comments: 30
Patch Set 2 : "Addressed bengr comments" #
Total comments: 7
Patch Set 3 : Fixed RecordingBypassedBytes #
Total comments: 21
Patch Set 4 : Addressed comments and fixed network error logic #Patch Set 5 : Removed logging and fixed MaybeBypassProxyAndPrepareToRetry #
Total comments: 28
Patch Set 6 : Addressed bengr comments #
Total comments: 2
Patch Set 7 : Fixed logic of WereDataReductionProxiesBypassed #Patch Set 8 : Fixing chrome_network_delegate_unittest.cc #Patch Set 9 : Fixed WereDataReductionProxiesBypassed #
Total comments: 4
Patch Set 10 : Added WereProxiesBypassed test and addressed mef comments #
Total comments: 48
Patch Set 11 : Addressed bengr comments #
Total comments: 44
Patch Set 12 : Addressed bengr comments #
Total comments: 26
Patch Set 13 : Addressed bengr and asvitkine comments #
Total comments: 16
Patch Set 14 : Addressed Comments #
Total comments: 2
Patch Set 15 : Addressed Alexei Comment #Patch Set 16 : Addressing trybot errors #Patch Set 17 : Moving #if defined(SPDY_PROXY_AUTH_ORIGIN) #
Total comments: 5
Messages
Total messages: 51 (0 generated)
On 2014/07/11 22:22:22, megjablon wrote: I'll take a careful look on Monday. This seems pretty good in general.
https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:368: spdy_proxy_auth_enabled->Init(data_reduction_proxy::prefs:: Move data_reduction_proxy::prefs to the next line, indent 4, and have pref_service line up with it. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:612: if (spdy_proxy_auth_enabled_ && spdy_proxy_auth_enabled_->GetValue()) { If you guard this with spdy_proxy_auth_enabled_, then there's another case: the proxy wasn't used because it was overridden by a managed configuration (e.g., by enterprise). We should have a separate UMA bin if you do it this way, and we'd need to check that the effective configuration is that of the data reduction proxy. The alternative is to only check spdy_proxy_auth_enabled_ when deciding if an https response was received while the proxy was enabled. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:615: request); |request| can move up a line. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.h:116: void set_spdy_proxy_auth_enabled( call this set_data_reduction_proxy_enabled(). In this CL or a separate one, rename the BooleanPrefMember to match. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:63: usage_stats->SetBypassType(bypass_type); I would move this outside of MaybeBypassProxyAndPrepareToRetry: bool foo = MaybeBypassProxyAndPrepareToRetry(...) usage->states(foo); https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:56: TestDataReductionProxyUsageStats() : DataReductionProxyUsageStats(NULL, Can all the params fit on the same line as the first? If so, move. If not, move all params to the second line. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:122: LOG(WARNING) << "Data Reduction Eligible: " Remove this logging. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:137: string* mime_type = new string(); std::string and don't new(). std::string mime_type; request->GetMimeType(&mime_type); https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:139: if((*mime_type).find("audio/") != string::npos || Add comment that describes how you know that audio/ and video/ are all you need to search for. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:40: /** Use // style comments. Fix the other methods' comments. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:45: net::ProxyService::DataReductionProxyBypassType type); Move up a line if it fits in 80 chars.
https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:60: net::ProxyService::DataReductionProxyBypassType type) OVERRIDE {} I'd add a variable, e.g., set_bypass_type_called_count_, that counts the number of times this method is called. Then in your tests you can do EXPECT_EQ(1, set_bypass_type_called_count_) https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:125: UMA_HISTOGRAM_COUNTS( Can you move all of your UMA into RecordBypassedBytes? https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:141: UMA_HISTOGRAM_COUNTS("DataReductionProxy.BypassedBytes.ShortAudioVideo", Can you move all of your UMA into RecordBypassedBytes? https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:149: void DataReductionProxyUsageStats::RecordTriggeringRequestBypassedBytes( Can you move all of your UMA into RecordBypassedBytes?
https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:368: spdy_proxy_auth_enabled->Init(data_reduction_proxy::prefs:: On 2014/07/12 00:11:59, bengr1 wrote: > Move data_reduction_proxy::prefs to the next line, indent 4, and have > pref_service line up with it. Done. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:612: if (spdy_proxy_auth_enabled_ && spdy_proxy_auth_enabled_->GetValue()) { On 2014/07/12 00:11:59, bengr1 wrote: > If you guard this with spdy_proxy_auth_enabled_, then there's another case: the > proxy wasn't used because it was overridden by a managed configuration (e.g., by > enterprise). We should have a separate UMA bin if you do it this way, and we'd > need to check that the effective configuration is that of the data reduction > proxy. > > The alternative is to only check spdy_proxy_auth_enabled_ when deciding if an > https response was received while the proxy was enabled. Would it be helpful for us to know of these bypasses due to managed configuration? If not, I'll do it the second way. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:615: request); On 2014/07/12 00:11:59, bengr1 wrote: > |request| can move up a line. Done. https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.h:116: void set_spdy_proxy_auth_enabled( On 2014/07/12 00:11:59, bengr1 wrote: > call this set_data_reduction_proxy_enabled(). In this CL or a separate one, > rename the BooleanPrefMember to match. Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:63: usage_stats->SetBypassType(bypass_type); On 2014/07/12 00:11:59, bengr1 wrote: > I would move this outside of MaybeBypassProxyAndPrepareToRetry: > > bool foo = MaybeBypassProxyAndPrepareToRetry(...) > usage->states(foo); Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:60: net::ProxyService::DataReductionProxyBypassType type) OVERRIDE {} On 2014/07/14 17:44:02, bengr1 wrote: > I'd add a variable, e.g., set_bypass_type_called_count_, that counts the number > of times this method is called. Then in your tests you can do EXPECT_EQ(1, > set_bypass_type_called_count_) With moving the SetBypassType outside of MaybeBypassProxyAndPrepareToRetry, we don't need this class to test anymore. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:122: LOG(WARNING) << "Data Reduction Eligible: " On 2014/07/12 00:11:59, bengr1 wrote: > Remove this logging. Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:125: UMA_HISTOGRAM_COUNTS( On 2014/07/14 17:44:03, bengr1 wrote: > Can you move all of your UMA into RecordBypassedBytes? Should I do this by adding parameter bool is_local_bypass_rule to RecordBypassedBytes? Or to move everything there should I use a struct with all the other recording types we might want? LOCAL_BYPASS_RULES AUDIO_VIDEO TRIGGERING_REQUEST https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:137: string* mime_type = new string(); On 2014/07/12 00:11:59, bengr1 wrote: > std::string and don't new(). > > std::string mime_type; > request->GetMimeType(&mime_type); Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:139: if((*mime_type).find("audio/") != string::npos || On 2014/07/12 00:11:59, bengr1 wrote: > Add comment that describes how you know that audio/ and video/ are all you need > to search for. Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:40: /** On 2014/07/12 00:11:59, bengr1 wrote: > Use // style comments. Fix the other methods' comments. Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:45: net::ProxyService::DataReductionProxyBypassType type); On 2014/07/12 00:11:59, bengr1 wrote: > Move up a line if it fits in 80 chars. Done.
https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/1/chrome/browser/net/chrome_ne... chrome/browser/net/chrome_network_delegate.cc:612: if (spdy_proxy_auth_enabled_ && spdy_proxy_auth_enabled_->GetValue()) { On 2014/07/14 19:06:41, megjablon wrote: > On 2014/07/12 00:11:59, bengr1 wrote: > > If you guard this with spdy_proxy_auth_enabled_, then there's another case: > the > > proxy wasn't used because it was overridden by a managed configuration (e.g., > by > > enterprise). We should have a separate UMA bin if you do it this way, and we'd > > need to check that the effective configuration is that of the data reduction > > proxy. > > > > The alternative is to only check spdy_proxy_auth_enabled_ when deciding if an > > https response was received while the proxy was enabled. > > Would it be helpful for us to know of these bypasses due to managed > configuration? If not, I'll do it the second way It would be helpful, but would require extra work. But I'd recommend doing it the second way and leaving the first way for a followup CL. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:56: TestDataReductionProxyUsageStats() : DataReductionProxyUsageStats(NULL, On 2014/07/12 00:11:59, bengr1 wrote: > Can all the params fit on the same line as the first? If so, move. If not, move > all params to the second line. Done. https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/1/components/data_reduction_pr... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:125: UMA_HISTOGRAM_COUNTS( On 2014/07/14 19:06:41, megjablon wrote: > On 2014/07/14 17:44:03, bengr1 wrote: > > Can you move all of your UMA into RecordBypassedBytes? > Should I do this by adding parameter bool is_local_bypass_rule to > RecordBypassedBytes? > > Or to move everything there should I use a struct with all the other recording > types we might want? > LOCAL_BYPASS_RULES > AUDIO_VIDEO > TRIGGERING_REQUEST I was thinking the former, I think. The RecordBypassedBytes function should only check the type of UMA it is supposed to report and report that UMA. https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:63: if(proxy_bypass_type) if (proxy_bypass_type) https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:64: proxy_bypass_type = bypass_type; I don't understand this. What is it doing? https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:180: UMA_HISTOGRAM_COUNTS("DataReductionProxy.BypassedBytes.Current", Why did you shift all of these? https://codereview.chromium.org/390533003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/390533003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:48055: +<histogram_suffixes name="DataReductionProxyBypassedBytes" separator="."> Why do we not just want an enumerated histogram?
https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:64: proxy_bypass_type = bypass_type; On 2014/07/14 22:04:29, bengr1 wrote: > I don't understand this. What is it doing? Whoops meant to take that out but missed it. https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:180: UMA_HISTOGRAM_COUNTS("DataReductionProxy.BypassedBytes.Current", On 2014/07/14 22:04:30, bengr1 wrote: > Why did you shift all of these? That was not intentional. I have no idea how that happened. Fixed. https://codereview.chromium.org/390533003/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/390533003/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:48055: +<histogram_suffixes name="DataReductionProxyBypassedBytes" separator="."> On 2014/07/14 22:04:30, bengr1 wrote: > Why do we not just want an enumerated histogram? With an enumerated histogram we can't store the number of bytes, only how many times an enumerated constant was used. This is the histograms_suffix construct that Alexei told me about that creates a histogram for each type, and then we can store the bytes in there.
net/*: mef histograms: asvitkine
https://codereview.chromium.org/390533003/diff/40001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/40001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:525: data_reduction_proxy_usage_stats_->SetBypassType(bypass_type); Is it expected to always be net::ProxyService::BYPASS_EVENT_TYPE_MAX? https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:38: net::ProxyService::DataReductionProxyBypassType proxy_bypass_type, Should this be passed by pointer? https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:63: proxy_bypass_type = bypass_type; Umm, what's the point of passing in |proxy_bypass_type| if it is always overridden with |bypass_type|?
https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:16: using net::ProxyService; Nit: You fully qualify this in some places below, like net::ProxyService::DataReductionProxyBypassType Make consistent. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:120: request->url().SchemeIs("https") && Nit: url::kHttpsScheme instead of "https" https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:121: data_reduction_proxy_enabled->GetValue()) { This condition is repeated. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:148: if((mime_type).find("audio/") != string::npos || Why the extra parens around mime_type? Also, add a space after if. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:163: bypassed_bytes_type, Indent 4 more. Or find a way to not wrap this. For example, why does DataReductionProxyBypassedBytesType have such a long name? If it's inside DataReductionProxyUsageStats then its name could be simplified - e.g. to: DataReductionProxyUsageStats::BypassedBytesType https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:165: switch(bypassed_bytes_type) { Nit: Space after switch. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:43: const int64 content_length, Nit: No need for const on primitives. Also, is this param necessary or can this function simply get its value via request->received_response_content_length()? https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:44: const net::URLRequest* request, If |request| can never be NULL, which seems to be the case here, pass by const ref instead. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:100: void RecordNetworkErrorBypassedBytes(const net::URLRequest* request, Nit: Pass by const ref?
https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:38: net::ProxyService::DataReductionProxyBypassType proxy_bypass_type, On 2014/07/15 18:20:04, mef wrote: > Should this be passed by pointer? Whoops, yes fixed. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:16: using net::ProxyService; On 2014/07/15 18:20:07, Alexei Svitkine wrote: > Nit: You fully qualify this in some places below, like > net::ProxyService::DataReductionProxyBypassType > > Make consistent. Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:120: request->url().SchemeIs("https") && On 2014/07/15 18:20:07, Alexei Svitkine wrote: > Nit: url::kHttpsScheme instead of "https" Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:121: data_reduction_proxy_enabled->GetValue()) { On 2014/07/15 18:20:07, Alexei Svitkine wrote: > This condition is repeated. Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:148: if((mime_type).find("audio/") != string::npos || On 2014/07/15 18:20:08, Alexei Svitkine wrote: > Why the extra parens around mime_type? Also, add a space after if. Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:163: bypassed_bytes_type, On 2014/07/15 18:20:08, Alexei Svitkine wrote: > Indent 4 more. Or find a way to not wrap this. > > For example, why does DataReductionProxyBypassedBytesType have such a long name? > If it's inside DataReductionProxyUsageStats then its name could be simplified - > e.g. to: > > DataReductionProxyUsageStats::BypassedBytesType Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:165: switch(bypassed_bytes_type) { On 2014/07/15 18:20:07, Alexei Svitkine wrote: > Nit: Space after switch. Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:44: const net::URLRequest* request, On 2014/07/15 18:20:08, Alexei Svitkine wrote: > If |request| can never be NULL, which seems to be the case here, pass by const > ref instead. Done. https://codereview.chromium.org/390533003/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:100: void RecordNetworkErrorBypassedBytes(const net::URLRequest* request, On 2014/07/15 18:20:08, Alexei Svitkine wrote: > Nit: Pass by const ref? We moved this logic into RecordBypassedBytes, deleted.
https://codereview.chromium.org/390533003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:352: BooleanPrefMember* data_reduction_proxy_enabled, How did you resolve the problem of this being initialized twice? https://codereview.chromium.org/390533003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:623: data_reduction_proxy_usage_stats_->RecordBypassedBytesHistograms( Please file a bug to add tests to verify that the actual ChromeNetworkDelegate (not a test one) calls your functions. Xing is writing a similar test. https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.h:536: mutable BooleanPrefMember spdy_proxy_auth_enabled_; You shouldn't need both a data_reduction_proxy_enabled_ and a spdy_proxy_auth_enabled_. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:339: if (request == NULL || request->context() == NULL || Are any of these NULL in practice? I'd prefer: DCHECK(request); DCHECK(request->context()); DCHECK(request->context()->proxy_service()); https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:350: for (size_t i = 0; i < proxies.size(); ++i) { This isn't right if alt proxies are involved. If they are, the data reduction proxy is bypassed if: 1) both origin() and fallback_origin() appear in the retry map, or 2) both alt_origin() and alt_fallback_origin() appear in the map and the request scheme is http, or 3) ssl_origin() is in the map and the scheme is https. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:95: // Returns true if the request is bypassed by all configured data reduction Before more specific. Does this return true if and only if all configured data reduction proxies are on the ProxyRetryInfoMap? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:40: net::ProxyService::DataReductionProxyBypassType* proxy_bypass_type, While you're here, reorder the parameters. Parameters that get modified go at the end. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:33: net::ProxyService::DataReductionProxyBypassType* proxy_bypass_type, Explain in the comment what this new paramater is and that it is allowed to be NULL. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:71: NULL, You should also test that if you pass in non-NULL, the correct type is returned. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:110: const BooleanPrefMember* data_reduction_proxy_enabled) { Why isn't this a const &? If you need to leave it the way it is, check for NULL. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:146: // media type is audio or video. Please check how the data reduction proxy decides if a resource is audio or video and do the same thing. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:44: const net::URLRequest& request, #include "net/url_request/url_request.h" https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:53: SSL, Give these each an explicit number, e.g., SSL = 1 and move the comment to the same line.
https://codereview.chromium.org/390533003/diff/80001/chrome/browser/net/chrom... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/80001/chrome/browser/net/chrom... chrome/browser/net/chrome_network_delegate.cc:623: data_reduction_proxy_usage_stats_->RecordBypassedBytesHistograms( On 2014/07/16 01:40:09, bengr1 wrote: > Please file a bug to add tests to verify that the actual ChromeNetworkDelegate > (not a test one) calls your functions. Xing is writing a similar test. Done. https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.h:536: mutable BooleanPrefMember spdy_proxy_auth_enabled_; On 2014/07/16 01:40:09, bengr1 wrote: > You shouldn't need both a data_reduction_proxy_enabled_ and a > spdy_proxy_auth_enabled_. This is how we fixed the problem of it being initialized twice. Will data_reduction_proxy_enabled_ be safe to use? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:350: for (size_t i = 0; i < proxies.size(); ++i) { On 2014/07/16 01:40:10, bengr1 wrote: > This isn't right if alt proxies are involved. If they are, the data reduction > proxy is bypassed if: > > 1) both origin() and fallback_origin() appear in the retry map, or > 2) both alt_origin() and alt_fallback_origin() appear in the map and the request > scheme is http, or > 3) ssl_origin() is in the map and the scheme is https. This is based on: https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... Does that need to be fixed too? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:95: // Returns true if the request is bypassed by all configured data reduction On 2014/07/16 01:40:10, bengr1 wrote: > Before more specific. Does this return true if and only if all configured data > reduction proxies are on the ProxyRetryInfoMap? Done. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:40: net::ProxyService::DataReductionProxyBypassType* proxy_bypass_type, On 2014/07/16 01:40:10, bengr1 wrote: > While you're here, reorder the parameters. Parameters that get modified go at > the end. Done. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.h:33: net::ProxyService::DataReductionProxyBypassType* proxy_bypass_type, On 2014/07/16 01:40:10, bengr1 wrote: > Explain in the comment what this new paramater is and that it is allowed to be > NULL. Done. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:110: const BooleanPrefMember* data_reduction_proxy_enabled) { On 2014/07/16 01:40:10, bengr1 wrote: > Why isn't this a const &? If you need to leave it the way it is, check for NULL. I do check to make sure it's not null whenever I use it. See below. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:146: // media type is audio or video. On 2014/07/16 01:40:10, bengr1 wrote: > Please check how the data reduction proxy decides if a resource is audio or > video and do the same thing. Where can I find this? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:44: const net::URLRequest& request, On 2014/07/16 01:40:10, bengr1 wrote: > #include "net/url_request/url_request.h" Done. https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:53: SSL, On 2014/07/16 01:40:10, bengr1 wrote: > Give these each an explicit number, e.g., SSL = 1 and move the comment to the > same line. Done.
https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_io_data.h (right): https://codereview.chromium.org/390533003/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_io_data.h:536: mutable BooleanPrefMember spdy_proxy_auth_enabled_; On 2014/07/16 23:07:14, megjablon wrote: > On 2014/07/16 01:40:09, bengr1 wrote: > > You shouldn't need both a data_reduction_proxy_enabled_ and a > > spdy_proxy_auth_enabled_. Figure out the initialization order and the order of use. If one is initialized and used before the other, use that one only. If not, pick a single initialization site that guarantees initial use is post initialization. > > This is how we fixed the problem of it being initialized twice. Will > data_reduction_proxy_enabled_ be safe to use? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:350: for (size_t i = 0; i < proxies.size(); ++i) { On 2014/07/16 23:07:14, megjablon wrote: > On 2014/07/16 01:40:10, bengr1 wrote: > > This isn't right if alt proxies are involved. If they are, the data reduction > > proxy is bypassed if: > > > > 1) both origin() and fallback_origin() appear in the retry map, or > > 2) both alt_origin() and alt_fallback_origin() appear in the map and the > request > > scheme is http, or > > 3) ssl_origin() is in the map and the scheme is https. > > This is based on: > > https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... > > Does that need to be fixed too? > Yes https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:110: const BooleanPrefMember* data_reduction_proxy_enabled) { On 2014/07/16 23:07:14, megjablon wrote: > On 2014/07/16 01:40:10, bengr1 wrote: > > Why isn't this a const &? If you need to leave it the way it is, check for > NULL. > > I do check to make sure it's not null whenever I use it. See below. OK. Is it always non-NULL? Can you pass a const &? https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:146: // media type is audio or video. On 2014/07/16 23:07:14, megjablon wrote: > On 2014/07/16 01:40:10, bengr1 wrote: > > Please check how the data reduction proxy decides if a resource is audio or > > video and do the same thing. > > Where can I find this? ask piatek@ https://codereview.chromium.org/390533003/diff/100001/components/data_reducti... File components/data_reduction_proxy/common/data_reduction_proxy_headers.cc (right): https://codereview.chromium.org/390533003/diff/100001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:117: //bypass=0 means bypass for a random duration between 1 to 5 minutes add space after // https://codereview.chromium.org/390533003/diff/100001/components/data_reducti... components/data_reduction_proxy/common/data_reduction_proxy_headers.cc:118: if(duration == TimeDelta()) if (duration...
https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/80001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:110: const BooleanPrefMember* data_reduction_proxy_enabled) { On 2014/07/16 23:28:14, bengr1 wrote: > On 2014/07/16 23:07:14, megjablon wrote: > > On 2014/07/16 01:40:10, bengr1 wrote: > > > Why isn't this a const &? If you need to leave it the way it is, check for > > NULL. > > > > I do check to make sure it's not null whenever I use it. See below. > > OK. Is it always non-NULL? Can you pass a const & No it is not always non-NULL. I can check in network delegate and only call RecordBypassedBytes once its initialized.
https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:525: data_reduction_proxy_usage_stats_->SetBypassType(bypass_type); |data_reduction_proxy_usage_stats_| could be NULL. https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:616: data_reduction_proxy_usage_stats_->RecordBypassedBytesHistograms( |data_reduction_proxy_usage_stats_| could be NULL.
https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:525: data_reduction_proxy_usage_stats_->SetBypassType(bypass_type); On 2014/07/18 16:15:20, mef wrote: > |data_reduction_proxy_usage_stats_| could be NULL. Done. https://codereview.chromium.org/390533003/diff/160001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:616: data_reduction_proxy_usage_stats_->RecordBypassedBytesHistograms( On 2014/07/18 16:15:20, mef wrote: > |data_reduction_proxy_usage_stats_| could be NULL. Done.
https://codereview.chromium.org/390533003/diff/180001/android_webview/browser... File android_webview/browser/net/aw_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/180001/android_webview/browser... android_webview/browser/net/aw_network_delegate.cc:73: NULL); add a comment so we know what's null: NULL /* returned bypass type */ ); https://codereview.chromium.org/390533003/diff/180001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/180001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:125: void set_data_reduction_proxy_enabled( Unfortunately, this function makes it sound like you're enabling the proxy. Maybe "set_data_reduction_proxy_enabled_pref"? https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:22: using namespace net; Do you need this? If so, specify each class you are using, e.g., using net::URLRequest; I typically do this if I'm referring to the class more than one or two times. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:335: const net::URLRequest& request, int64* delay_seconds) const { why is this an int64 and not a base::TimeDelta? Also, why an int64? That's a lot of seconds. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:337: DCHECK(request.context()->proxy_service()); So this should never be called if the request does not have a valid proxy service? https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:344: return WereProxiesBypassed(retry_map, ssl, delay_seconds); Why not just: return WereProxiesBypassed(request.context()->proxy_service()->proxy_retry_info(), request.url().SchemeIs(url::kHttpsScheme), delay_seconds); https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:349: bool ssl, rename ssl -> is_https https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:379: return false; indentation here and below. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: GURL primary, const GURL& https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:392: // The retry list has the scheme prefix for https but not for http. Maybe a better way to do this would be to create a ProxyServer from the GURL and then search for ProxyServer::ToURI() in the retry_map. See: https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_se... Something like: retry_map.find(ProxyServer(primary.SchemeIs("https") ? SCHEME_HTTPS : SCHEME_HTTP, HostPortPair::FromURL(primary))).ToURI() https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:99: // It returns the bypass delay in delay_seconds (if not NULL). If It returns --> and returns https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:101: // shortest delay. the shortest https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:108: // It returns the bypass delay in delay_seconds (if not NULL). If It -> and https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:110: // shortest delay. the shortest https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:111: bool WereProxiesBypassed(const net::ProxyRetryInfoMap& retry_map, AreProxies...? I'm not sure if "bypassed" is the present state or a former one. I think it's the present state, right? https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:224: bool WerePrimaryAndFallbackBypassed(const net::ProxyRetryInfoMap& retry_map, Are? https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:225: GURL primary, These can be const GURL&. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:578: //expected result no need for this comment. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:627: //proxy rules Always add a space after // https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:884: if (origin.find("https")==std::string::npos) { eek. this is busted for http://https.com https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:924: if (tests[i].origin) { Here and below can lose the curly braces. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:67: if (bypass_type == net::ProxyService::BYPASS_EVENT_TYPE_MAX) { no need for curly braces. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:62: : net::NetworkDelegate(), test_data_reduction_proxy_params_(test_params), move test_data... to the next line https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:221: remove blank line
https://codereview.chromium.org/390533003/diff/180001/android_webview/browser... File android_webview/browser/net/aw_network_delegate.cc (right): https://codereview.chromium.org/390533003/diff/180001/android_webview/browser... android_webview/browser/net/aw_network_delegate.cc:73: NULL); On 2014/07/19 00:13:00, bengr1 wrote: > add a comment so we know what's null: > > NULL /* returned bypass type */ ); Done. https://codereview.chromium.org/390533003/diff/180001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/180001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:125: void set_data_reduction_proxy_enabled( On 2014/07/19 00:13:00, bengr1 wrote: > Unfortunately, this function makes it sound like you're enabling the proxy. > Maybe "set_data_reduction_proxy_enabled_pref"? Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:22: using namespace net; On 2014/07/19 00:13:00, bengr1 wrote: > Do you need this? If so, specify each class you are using, e.g., > using net::URLRequest; > > I typically do this if I'm referring to the class more than one or two times. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:335: const net::URLRequest& request, int64* delay_seconds) const { On 2014/07/19 00:13:00, bengr1 wrote: > why is this an int64 and not a base::TimeDelta? > Also, why an int64? That's a lot of seconds. We were using an int64 before in proxy metrics so I just kept it the same. Changed. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:337: DCHECK(request.context()->proxy_service()); On 2014/07/19 00:13:00, bengr1 wrote: > So this should never be called if the request does not have a valid proxy > service? Switching back to how we checked previously. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:344: return WereProxiesBypassed(retry_map, ssl, delay_seconds); On 2014/07/19 00:13:00, bengr1 wrote: > Why not just: > > return > WereProxiesBypassed(request.context()->proxy_service()->proxy_retry_info(), > request.url().SchemeIs(url::kHttpsScheme), > delay_seconds); Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:349: bool ssl, On 2014/07/19 00:13:00, bengr1 wrote: > rename ssl -> is_https Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:379: return false; On 2014/07/19 00:13:00, bengr1 wrote: > indentation here and below. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: GURL primary, On 2014/07/19 00:13:00, bengr1 wrote: > const GURL& Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:392: // The retry list has the scheme prefix for https but not for http. On 2014/07/19 00:13:01, bengr1 wrote: > Maybe a better way to do this would be to create a ProxyServer from the GURL and > then search for ProxyServer::ToURI() in the retry_map. > > See: > https://code.google.com/p/chromium/codesearch#chromium/src/net/proxy/proxy_se... > > Something like: > > retry_map.find(ProxyServer(primary.SchemeIs("https") ? SCHEME_HTTPS : > SCHEME_HTTP, > HostPortPair::FromURL(primary))).ToURI() Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:99: // It returns the bypass delay in delay_seconds (if not NULL). If On 2014/07/19 00:13:01, bengr1 wrote: > It returns --> and returns Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:101: // shortest delay. On 2014/07/19 00:13:01, bengr1 wrote: > the shortest Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:108: // It returns the bypass delay in delay_seconds (if not NULL). If On 2014/07/19 00:13:01, bengr1 wrote: > It -> and Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:110: // shortest delay. On 2014/07/19 00:13:01, bengr1 wrote: > the shortest Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:111: bool WereProxiesBypassed(const net::ProxyRetryInfoMap& retry_map, On 2014/07/19 00:13:01, bengr1 wrote: > AreProxies...? > > I'm not sure if "bypassed" is the present state or a former one. I think it's > the present state, right? Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:224: bool WerePrimaryAndFallbackBypassed(const net::ProxyRetryInfoMap& retry_map, On 2014/07/19 00:13:01, bengr1 wrote: > Are? Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:225: GURL primary, On 2014/07/19 00:13:01, bengr1 wrote: > These can be const GURL&. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:578: //expected result On 2014/07/19 00:13:01, bengr1 wrote: > no need for this comment. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:627: //proxy rules On 2014/07/19 00:13:01, bengr1 wrote: > Always add a space after // Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:884: if (origin.find("https")==std::string::npos) { On 2014/07/19 00:13:01, bengr1 wrote: > eek. this is busted for http://https.com Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:924: if (tests[i].origin) { On 2014/07/19 00:13:01, bengr1 wrote: > Here and below can lose the curly braces. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol.cc:67: if (bypass_type == net::ProxyService::BYPASS_EVENT_TYPE_MAX) { On 2014/07/19 00:13:01, bengr1 wrote: > no need for curly braces. Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:62: : net::NetworkDelegate(), test_data_reduction_proxy_params_(test_params), On 2014/07/19 00:13:01, bengr1 wrote: > move test_data... to the next line Done. https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/180001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:221: On 2014/07/19 00:13:01, bengr1 wrote: > remove blank line Done.
https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:308: if (params.AreDataReductionProxiesBypassed(*request, &bypass_delay)) { Thanks for doing this! https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:8: #include "base/metrics/field_trial.h" #include "base/time/time.h" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:360: if (ArePrimaryAndFallbackBypassed(retry_map, Here and below you can probably fit all parameters on one line. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: base::TimeDelta* delay_seconds) const { This is no longer in seconds and it otherwise vague. It should be called minimum_retry_delay, or something like that. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:391: net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP, #include "net/proxy/proxy_server.h" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:394: shortest_delay = found->second.current_delay; There are only two possible delays, so this should be "shorter_delay" right? (Sorry, I've been influenced by Weird Al's Word Crimes.) https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:397: net::ProxyServer(fallback.SchemeIs(url::kHttpsScheme) ? #include "url/url_constants.h" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:399: net::HostPortPair::FromURL(fallback)).ToURI()); #include "net/base/host_port_pair.h" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:14: #include "net/proxy/proxy_retry_info.h" You should be able to forward declare ProxyRetryInfoMap instead of including it. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:103: base::TimeDelta* delay_seconds) const; add: namespace base { class TimeDelta; } https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:224: bool ArePrimaryAndFallbackBypassed(const net::ProxyRetryInfoMap& retry_map, Move this up to before the member variables. Add a comment describing what it does. All functions that are not completely obvious should have comments describing what they do. (I know Chromium has lots of examples of functions without comments.) https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:578: bool expected_result; Add a blank line above this so we know it does not fall under "// proxies in retry map" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:580: { See http_response_headers_unittest.cc for formatting. I like: } tests[] = { { // proxy rules false, ..., }, { ... https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:885: origin = origin.substr(7); Add a helper function called GetRetryMapKeyFromOrigin(); Then do something like: std::string origin = GetRetryMapKeyFromOrigin(TestDataReductionProxyParams::DefaultOrigin()); https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:100: // Sets up the |TestURLRequestContext| with the provided |ProxyService|. Mention the bypass_type https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:5: #include "base/metrics/histogram.h" This should go above line 7 with a blank line above it. The header for this cc should come first and should be separated. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:145: if (mime_type.find("audio/") != string::npos || using string::compare is probably more efficient (and correct) because you only care if the mime type *begins* with "audio/" or "video/" https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:186: if (bypass_type_ == ProxyService::SHORT_BYPASS) Add curly braces https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:38: // |triggering_request_| to true. Say what it means to be the triggering request. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:42: // respective UMAs. Describe the parameters. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:51: NOT_BYPASSED = 0, Can you condense? E.g.: enum BypassedBytesType { NOT_BYPASSED = 0, /* Not bypassed. */ SSL = 1, /* Bypass due to SSL. */ ... You can line up the comments if you'd like. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:73: net::ProxyService::DataReductionProxyBypassType bypass_type_; is the the last_bypass_type_? If so, rename. Add a comment. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:74: bool triggering_request_; Comment.
adding sgurun for android_webview/* https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:8: #include "base/metrics/field_trial.h" On 2014/07/21 22:23:56, bengr1 wrote: > #include "base/time/time.h" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:360: if (ArePrimaryAndFallbackBypassed(retry_map, On 2014/07/21 22:23:57, bengr1 wrote: > Here and below you can probably fit all parameters on one line. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: base::TimeDelta* delay_seconds) const { On 2014/07/21 22:23:57, bengr1 wrote: > This is no longer in seconds and it otherwise vague. It should be called > minimum_retry_delay, or something like that. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:391: net::ProxyServer::SCHEME_HTTPS : net::ProxyServer::SCHEME_HTTP, On 2014/07/21 22:23:56, bengr1 wrote: > #include "net/proxy/proxy_server.h" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:394: shortest_delay = found->second.current_delay; On 2014/07/21 22:23:57, bengr1 wrote: > There are only two possible delays, so this should be "shorter_delay" right? > (Sorry, I've been influenced by Weird Al's Word Crimes.) Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:397: net::ProxyServer(fallback.SchemeIs(url::kHttpsScheme) ? On 2014/07/21 22:23:57, bengr1 wrote: > #include "url/url_constants.h" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:399: net::HostPortPair::FromURL(fallback)).ToURI()); On 2014/07/21 22:23:56, bengr1 wrote: > #include "net/base/host_port_pair.h" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.h (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:103: base::TimeDelta* delay_seconds) const; On 2014/07/21 22:23:57, bengr1 wrote: > add: > > namespace base { > class TimeDelta; > } Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.h:224: bool ArePrimaryAndFallbackBypassed(const net::ProxyRetryInfoMap& retry_map, On 2014/07/21 22:23:57, bengr1 wrote: > Move this up to before the member variables. Add a comment describing what it > does. All functions that are not completely obvious should have comments > describing what they do. (I know Chromium has lots of examples of functions > without comments.) Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:578: bool expected_result; On 2014/07/21 22:23:57, bengr1 wrote: > Add a blank line above this so we know it does not fall under "// proxies in > retry map" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:580: { On 2014/07/21 22:23:57, bengr1 wrote: > See http_response_headers_unittest.cc for formatting. I like: > > } tests[] = { > { // proxy rules > false, > ..., > }, > { ... Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:885: origin = origin.substr(7); On 2014/07/21 22:23:57, bengr1 wrote: > Add a helper function called GetRetryMapKeyFromOrigin(); > > Then do something like: > std::string origin = > GetRetryMapKeyFromOrigin(TestDataReductionProxyParams::DefaultOrigin()); Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_protocol_unittest.cc:100: // Sets up the |TestURLRequestContext| with the provided |ProxyService|. On 2014/07/21 22:23:57, bengr1 wrote: > Mention the bypass_type Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:5: #include "base/metrics/histogram.h" On 2014/07/21 22:23:58, bengr1 wrote: > This should go above line 7 with a blank line above it. The header for this cc > should come first and should be separated. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:145: if (mime_type.find("audio/") != string::npos || On 2014/07/21 22:23:58, bengr1 wrote: > using string::compare is probably more efficient (and correct) because you only > care if the mime type *begins* with "audio/" or "video/" Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:186: if (bypass_type_ == ProxyService::SHORT_BYPASS) On 2014/07/21 22:23:57, bengr1 wrote: > Add curly braces Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:38: // |triggering_request_| to true. On 2014/07/21 22:23:58, bengr1 wrote: > Say what it means to be the triggering request. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:42: // respective UMAs. On 2014/07/21 22:23:58, bengr1 wrote: > Describe the parameters. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:51: NOT_BYPASSED = 0, On 2014/07/21 22:23:58, bengr1 wrote: > Can you condense? E.g.: > > enum BypassedBytesType { > NOT_BYPASSED = 0, /* Not bypassed. */ > SSL = 1, /* Bypass due to SSL. */ > ... > > You can line up the comments if you'd like. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:73: net::ProxyService::DataReductionProxyBypassType bypass_type_; On 2014/07/21 22:23:58, bengr1 wrote: > is the the last_bypass_type_? If so, rename. Add a comment. Done. https://codereview.chromium.org/390533003/diff/200001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:74: bool triggering_request_; On 2014/07/21 22:23:58, bengr1 wrote: > Comment. Done.
https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:356: if (is_https && alt_allowed_) Nit: Add {}'s https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:361: if (ArePrimaryAndFallbackBypassed( Nit: Merge with if above. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:368: if (ArePrimaryAndFallbackBypassed( Nit: Merge with if above. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:389: if (!(found == retry_map.end())) { Nit: Change to != https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:397: if(min_delay > found->second.current_delay) Nit: Add space after if https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:403: } else { Nit: Reverse the cond of the if (make it check for ==) and make this the only block with an early return. Then, the code in the block above can follow it with being nested (due to there being an early return). https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:172: switch (bypassed_bytes_type) { Add a comment explaining why separate UMA macros are needed. (So someone doesn't try to refactor it in the future). https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:52: enum BypassedBytesType { Mention that this is used in a histogram, so entries shouldn't be renumbered. https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3069: + Counts bytes of various events that trigger Chrome to bypass the data What's bytes of an event? Do you mean bytes of the server response data? Please clarify this.
On 2014/07/22 13:57:21, Alexei Svitkine wrote: > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc > (right): > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:356: if > (is_https && alt_allowed_) > Nit: Add {}'s > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:361: if > (ArePrimaryAndFallbackBypassed( > Nit: Merge with if above. > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:368: if > (ArePrimaryAndFallbackBypassed( > Nit: Merge with if above. > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:389: if > (!(found == retry_map.end())) { > Nit: Change to != > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:397: > if(min_delay > found->second.current_delay) > Nit: Add space after if > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:403: } > else { > Nit: Reverse the cond of the if (make it check for ==) and make this the only > block with an early return. > > Then, the code in the block above can follow it with being nested (due to there > being an early return). > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc > (right): > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:172: > switch (bypassed_bytes_type) { > Add a comment explaining why separate UMA macros are needed. (So someone doesn't > try to refactor it in the future). > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h > (right): > > https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... > components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:52: > enum BypassedBytesType { > Mention that this is used in a histogram, so entries shouldn't be renumbered. > > https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:3069: + Counts bytes of various > events that trigger Chrome to bypass the data > What's bytes of an event? Do you mean bytes of the server response data? Please > clarify this. aw lgtm
https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:310: base::TimeDelta::FromSeconds(kLongBypassDelayInSeconds)) ? I don't think the ternary buys anything. Rewrite as: if (bypass_delay > base::TimeDelta::FromSeconds(kLongBypassDelayInSeconds)) return LONG_BYPASS; return SHORT_BYPASS; https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:588: } tests[] = { Indentation should be only two, and you should probably put the comment above each case. E.g.: } tests[] = { // proxy rules { false, false, }, { ... https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:59: BYPASSED_BYTES_TYPE_MAX = 6 /*This must always be last.*/ Add a space in the comment.
https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_metrics.cc:310: base::TimeDelta::FromSeconds(kLongBypassDelayInSeconds)) ? On 2014/07/22 16:59:44, bengr1 wrote: > I don't think the ternary buys anything. Rewrite as: > > if (bypass_delay > base::TimeDelta::FromSeconds(kLongBypassDelayInSeconds)) > return LONG_BYPASS; > return SHORT_BYPASS; Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:356: if (is_https && alt_allowed_) On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Add {}'s Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:361: if (ArePrimaryAndFallbackBypassed( On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Merge with if above. Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:368: if (ArePrimaryAndFallbackBypassed( On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Merge with if above. Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:389: if (!(found == retry_map.end())) { On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Change to != Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:397: if(min_delay > found->second.current_delay) On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Add space after if Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:403: } else { On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Nit: Reverse the cond of the if (make it check for ==) and make this the only > block with an early return. > > Then, the code in the block above can follow it with being nested (due to there > being an early return). Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:588: } tests[] = { On 2014/07/22 16:59:44, bengr1 wrote: > Indentation should be only two, and you should probably put the comment above > each case. E.g.: > > } tests[] = { > // proxy rules > { false, > false, > }, > { ... I think the comment makes more sense in the current location because it's for the group of flags below it, not for the test struct as a whole. Changing wording to make more sense. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:172: switch (bypassed_bytes_type) { On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Add a comment explaining why separate UMA macros are needed. (So someone doesn't > try to refactor it in the future). Done. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:52: enum BypassedBytesType { On 2014/07/22 13:57:21, Alexei Svitkine wrote: > Mention that this is used in a histogram, so entries shouldn't be renumbered. This isn't used in the histograms, it's used to make the decision of which histogram to record the bytes in. We're using histogram counts rather than enumerated histograms to collect the data we want. https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:59: BYPASSED_BYTES_TYPE_MAX = 6 /*This must always be last.*/ On 2014/07/22 16:59:44, bengr1 wrote: > Add a space in the comment. Done. https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/390533003/diff/220001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3069: + Counts bytes of various events that trigger Chrome to bypass the data On 2014/07/22 13:57:21, Alexei Svitkine wrote: > What's bytes of an event? Do you mean bytes of the server response data? Please > clarify this. Done.
https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:52: enum BypassedBytesType { On 2014/07/22 18:40:46, megjablon wrote: > On 2014/07/22 13:57:21, Alexei Svitkine wrote: > > Mention that this is used in a histogram, so entries shouldn't be renumbered. > This isn't used in the histograms, it's used to make the decision of which > histogram to record the bytes in. We're using histogram counts rather than > enumerated histograms to collect the data we want. Okay, in that case remove the explicit values of entries, since they don't need to be stable between versions. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:359: } else { No else after early return. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:361: if (ArePrimaryAndFallbackBypassed( I think you misunderstood my previous comment. My point is you can have a single if here and the line above, i.e.: if (allowed_ && ArePrimary...()) { return true; } if (alt_allowed_ && ArePrimary...()) { return true; } https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: found = retry_map.find( nit: just put = on the previous line and wrap after it, instead of having both a decl and assign statement. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:391: return false; Nit: No need for {}'s since body is one line. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:394: min_delay = found->second.current_delay; Just declare min_delay and assign to it here, instead of declaring at the top of the function. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:407: return false; Nit: No need for {}'s https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:908: if(tests[i].alt_allowed) Nit: space after if https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:109: int64 content_length, Nit: Remove this param and make it a local variable instead, initializing it via request.received_response_content_length();
https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h (right): https://codereview.chromium.org/390533003/diff/220001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h:52: enum BypassedBytesType { On 2014/07/22 18:50:18, Alexei Svitkine wrote: > On 2014/07/22 18:40:46, megjablon wrote: > > On 2014/07/22 13:57:21, Alexei Svitkine wrote: > > > Mention that this is used in a histogram, so entries shouldn't be > renumbered. > > This isn't used in the histograms, it's used to make the decision of which > > histogram to record the bytes in. We're using histogram counts rather than > > enumerated histograms to collect the data we want. > > Okay, in that case remove the explicit values of entries, since they don't need > to be stable between versions. Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:359: } else { On 2014/07/22 18:50:18, Alexei Svitkine wrote: > No else after early return. Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:361: if (ArePrimaryAndFallbackBypassed( On 2014/07/22 18:50:18, Alexei Svitkine wrote: > I think you misunderstood my previous comment. My point is you can have a single > if here and the line above, i.e.: > > if (allowed_ && ArePrimary...()) { > return true; > } > > if (alt_allowed_ && ArePrimary...()) { > return true; > } Ahh that makes much more sense. Fixed. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:385: found = retry_map.find( On 2014/07/22 18:50:18, Alexei Svitkine wrote: > nit: just put = on the previous line and wrap after it, instead of having both a > decl and assign statement. Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:391: return false; On 2014/07/22 18:50:18, Alexei Svitkine wrote: > Nit: No need for {}'s since body is one line. Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:394: min_delay = found->second.current_delay; On 2014/07/22 18:50:18, Alexei Svitkine wrote: > Just declare min_delay and assign to it here, instead of declaring at the top of > the function. Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params.cc:407: return false; On 2014/07/22 18:50:18, Alexei Svitkine wrote: > Nit: No need for {}'s Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:908: if(tests[i].alt_allowed) On 2014/07/22 18:50:18, Alexei Svitkine wrote: > Nit: space after if Done. https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/390533003/diff/240001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:109: int64 content_length, On 2014/07/22 18:50:18, Alexei Svitkine wrote: > Nit: Remove this param and make it a local variable instead, initializing it via > request.received_response_content_length(); Done.
lgtm https://codereview.chromium.org/390533003/diff/260001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/260001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:914: ~TestDataReductionProxyParams::HAS_DEV_ORIGIN); Nit: I'd extract the 2nd param into a local variable to make this more readable.
lgtm
https://codereview.chromium.org/390533003/diff/260001/components/data_reducti... File components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc (right): https://codereview.chromium.org/390533003/diff/260001/components/data_reducti... components/data_reduction_proxy/browser/data_reduction_proxy_params_unittest.cc:914: ~TestDataReductionProxyParams::HAS_DEV_ORIGIN); On 2014/07/22 23:14:19, Alexei Svitkine wrote: > Nit: I'd extract the 2nd param into a local variable to make this more readable. Done.
lgtm
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/megjablon@chromium.org/390533003/280001
The CQ bit was unchecked by megjablon@chromium.org
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/megjablon@chromium.org/390533003/400001
The CQ bit was unchecked by megjablon@chromium.org
mmenke for chrome/browser/profiles/profile_io_data.cc
https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:128: } nit: Setters for all DRP settings should be together, in the same order they're declared at the bottom of the file (And should probably be together and initialized in the same order in profil_io_data, unless there's a reason not to do that). https://codereview.chromium.org/390533003/diff/400001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/390533003/diff/400001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1034: #endif The other DRP variables are set unconditionally, looks like. Any reason this is different?
https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:128: } On 2014/07/24 19:32:05, mmenke wrote: > nit: Setters for all DRP settings should be together, in the same order they're > declared at the bottom of the file (And should probably be together and > initialized in the same order in profil_io_data, unless there's a reason not to > do that). There's no reason not to do it that way, but right now we have the setters for all BooleanPrefMembers grouped together. I can change the ordering in profile_io_data if you like to the same order as the setters here. https://codereview.chromium.org/390533003/diff/400001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/390533003/diff/400001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1034: #endif On 2014/07/24 19:32:05, mmenke wrote: > The other DRP variables are set unconditionally, looks like. Any reason this is > different? The BooleanPrefMember only exists on Android currently. kundaji is currently working on a CL that will be removing these platform conditionals, but for now we are fine only reporting to the UMA on Android.
LGTM https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.h (right): https://codereview.chromium.org/390533003/diff/400001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.h:128: } On 2014/07/24 20:09:37, megjablon wrote: > On 2014/07/24 19:32:05, mmenke wrote: > > nit: Setters for all DRP settings should be together, in the same order > they're > > declared at the bottom of the file (And should probably be together and > > initialized in the same order in profil_io_data, unless there's a reason not > to > > do that). > > There's no reason not to do it that way, but right now we have the setters for > all BooleanPrefMembers grouped together. I can change the ordering in > profile_io_data if you like to the same order as the setters here. Makes sense. For some reason, I was thinking that it was declared near the other DRP settings, but the setter was in a different order (Which is not actually true).
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/megjablon@chromium.org/390533003/400001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/megjablon@chromium.org/390533003/400001
Message was sent while issue was closed.
Change committed as 285439 |
