Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
125303002:
Change the detection of preview request to via header. (Closed)
|
Created:
6 years, 11 months ago by ksimbili Modified:
6 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionChange the detection of preview request to via header.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247726
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed review comments. #
Total comments: 16
Patch Set 3 : Addressed all comments. #Patch Set 4 : Modified the comment. #
Total comments: 4
Patch Set 5 : Addressed comments. #
Total comments: 4
Patch Set 6 : Addressed Alexei comments. #
Total comments: 11
Patch Set 7 : Addressed bengr comments. #Patch Set 8 : Addressed bengr comments. #Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:183: bool ViaHeaderContains(WebFrame* frame, const std::string& viaValue) { via_value
https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:193: return via_header.find(viaValue) != std::string::npos; This is overly inclusive. E.g., if some other proxy adds "foo bar promise-preview" to the via header, ViaHeaderContains("promise-preview") will return true. Please do something like the following instead and fix for the data reduction proxy: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:251: ViaHeaderContains(frame, "promise-preview") ? PREVIEW : I would use annotations that are less likely to appear in natural browsing. E.g., "Google promise-preview" or "Chrome promise-preview". Same for "promise-original" Also, this is a bit of an abuse of the Via header, which should say simply that the request went through the promise proxy. Perhaps you can differentiate preview from original some other way?
https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:183: bool ViaHeaderContains(WebFrame* frame, const std::string& viaValue) { On 2014/01/07 10:22:23, jochen wrote: > via_value Done. https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:193: return via_header.find(viaValue) != std::string::npos; On 2014/01/07 17:16:09, bengr1 wrote: > This is overly inclusive. E.g., if some other proxy adds "foo bar > promise-preview" to the via header, ViaHeaderContains("promise-preview") will > return true. Please do something like the following instead and fix for the data > reduction proxy: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net... Done. https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:251: ViaHeaderContains(frame, "promise-preview") ? PREVIEW : On 2014/01/07 17:16:09, bengr1 wrote: > I would use annotations that are less likely to appear in natural browsing. > E.g., "Google promise-preview" or "Chrome promise-preview". Same for > "promise-original" > > Also, this is a bit of an abuse of the Via header, which should say simply that > the request went through the promise proxy. Perhaps you can differentiate > preview from original some other way? Via header was the way we(flywheel and promise team) agreed upon instead of query based checking.
lgtm https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:193: base::SplitString(base::UTF16ToUTF8( Add a comment explaining why we do this
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:183: // Helper function to check for string in 'via' header. Expand comment to mention that this is only done for pages fetched via the proxy. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) Will this find any part separate by commas that contains the string "Via"? If so, please document in the comment. Though, it also seems the perhaps this check should be stricter than that. Also, if you're just doing it this way, why split the string at all - wouldn't just doing a find() on the pre-split value be equivalent? https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:258: ViaHeaderContains(frame, "Google Promise Preview") ? PREVIEW : Nit: Incorrect indent.
https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/1/chrome/renderer/page_load_hi... chrome/renderer/page_load_histograms.cc:251: ViaHeaderContains(frame, "promise-preview") ? PREVIEW : On 2014/01/08 01:11:38, ksimbili wrote: > On 2014/01/07 17:16:09, bengr1 wrote: > > I would use annotations that are less likely to appear in natural browsing. > > E.g., "Google promise-preview" or "Chrome promise-preview". Same for > > "promise-original" > > > > Also, this is a bit of an abuse of the Via header, which should say simply > that > > the request went through the promise proxy. Perhaps you can differentiate > > preview from original some other way? > > Via header was the way we(flywheel and promise team) agreed upon instead of > query based checking. I know. I just don't think you should use a header value that could appear in natural traffic. Adding the "Google" helps. Also, please don't refer to team names, code names, etc. that are internal to Google. Chromium is an open source project. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:195: ',', &via_header_values); This will work most of the time, but please follow a pattern from the network stack implementation (e.g., See HttpResponseHeaders, or the code I linked). There are lots of subtle ways to get header parsing wrong, so you shouldn't be rolling your own. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 16:39:29, Alexei Svitkine wrote: > Will this find any part separate by commas that contains the string "Via"? If > so, please document in the comment. Though, it also seems the perhaps this check > should be stricter than that. > > Also, if you're just doing it this way, why split the string at all - wouldn't > just doing a find() on the pre-split value be equivalent? Via values shouldn't have commas in them. Why not use Enumerate header the way I did in the code I linked?
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:183: // Helper function to check for string in 'via' header. On 2014/01/09 16:39:29, Alexei Svitkine wrote: > Expand comment to mention that this is only done for pages fetched via the > proxy. Done. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:193: base::SplitString(base::UTF16ToUTF8( On 2014/01/08 22:25:29, nikhilmadan wrote: > Add a comment explaining why we do this Done. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:195: ',', &via_header_values); On 2014/01/09 17:32:46, bengr1 wrote: > This will work most of the time, but please follow a pattern from the network > stack implementation (e.g., See HttpResponseHeaders, or the code I linked). > There are lots of subtle ways to get header parsing wrong, so you shouldn't be > rolling your own. Like I said, we don't have HttpResponseHeaders here. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 17:32:46, bengr1 wrote: > On 2014/01/09 16:39:29, Alexei Svitkine wrote: > > Will this find any part separate by commas that contains the string "Via"? If > > so, please document in the comment. Though, it also seems the perhaps this > check > > should be stricter than that. > > > > Also, if you're just doing it this way, why split the string at all - wouldn't > > just doing a find() on the pre-split value be equivalent? > > Via values shouldn't have commas in them. Why not use Enumerate header the way I > did in the code I linked? We don't have network stack HTTPResponseHeaders here. All we have a parsed header values into WebResponse. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 16:39:29, Alexei Svitkine wrote: > Will this find any part separate by commas that contains the string "Via"? If > so, please document in the comment. Though, it also seems the perhaps this check > should be stricter than that. > > Also, if you're just doing it this way, why split the string at all - wouldn't > just doing a find() on the pre-split value be equivalent? we are not using find() directly to make sure that we don't match others proxy which also has this string in it. https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:258: ViaHeaderContains(frame, "Google Promise Preview") ? PREVIEW : On 2014/01/09 16:39:29, Alexei Svitkine wrote: > Nit: Incorrect indent. Done.
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 18:54:50, ksimbili wrote: > On 2014/01/09 16:39:29, Alexei Svitkine wrote: > > Will this find any part separate by commas that contains the string "Via"? If > > so, please document in the comment. Though, it also seems the perhaps this > check > > should be stricter than that. > > > > Also, if you're just doing it this way, why split the string at all - wouldn't > > just doing a find() on the pre-split value be equivalent? > > we are not using find() directly to make sure that we don't match others proxy > which also has this string in it. If |via_value| never contains a comma (which is the case in this CL for all callers of this function), in what case would this logic be different than the following? const string16 header_value = frame->dataSource()->response().httpHeaderField(kViaHeaderName)); return header_value.find(via_value) != std::string::npos; Separately, if you're trying to do a more strict check, should it actually be more strict (e.g. instead of using .find(), use ==)?
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 19:01:42, Alexei Svitkine wrote: > On 2014/01/09 18:54:50, ksimbili wrote: > > On 2014/01/09 16:39:29, Alexei Svitkine wrote: > > > Will this find any part separate by commas that contains the string "Via"? > If > > > so, please document in the comment. Though, it also seems the perhaps this > > check > > > should be stricter than that. > > > > > > Also, if you're just doing it this way, why split the string at all - > wouldn't > > > just doing a find() on the pre-split value be equivalent? > > > > we are not using find() directly to make sure that we don't match others proxy > > which also has this string in it. > > If |via_value| never contains a comma (which is the case in this CL for all > callers of this function), in what case would this logic be different than the > following? > > const string16 header_value = > frame->dataSource()->response().httpHeaderField(kViaHeaderName)); > return header_value.find(via_value) != std::string::npos; > > Separately, if you're trying to do a more strict check, should it actually be > more strict (e.g. instead of using .find(), use ==)? No. Sorry for the confusion. There can be multiple Via headers present, and all these via_values are coalesced into one value using ',' as the seperator. Modified the comment to mean it. eg; Via: 1.0 Compression proxy, 1.1 Google promise preview
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 19:13:49, ksimbili wrote: > On 2014/01/09 19:01:42, Alexei Svitkine wrote: > > On 2014/01/09 18:54:50, ksimbili wrote: > > > On 2014/01/09 16:39:29, Alexei Svitkine wrote: > > > > Will this find any part separate by commas that contains the string "Via"? > > If > > > > so, please document in the comment. Though, it also seems the perhaps this > > > check > > > > should be stricter than that. > > > > > > > > Also, if you're just doing it this way, why split the string at all - > > wouldn't > > > > just doing a find() on the pre-split value be equivalent? > > > > > > we are not using find() directly to make sure that we don't match others > proxy > > > which also has this string in it. > > > > If |via_value| never contains a comma (which is the case in this CL for all > > callers of this function), in what case would this logic be different than the > > following? > > > > const string16 header_value = > > frame->dataSource()->response().httpHeaderField(kViaHeaderName)); > > return header_value.find(via_value) != std::string::npos; > > > > Separately, if you're trying to do a more strict check, should it actually be > > more strict (e.g. instead of using .find(), use ==)? > > No. Sorry for the confusion. > There can be multiple Via headers present, and all these via_values are > coalesced into one value using ',' as the seperator. > Modified the comment to mean it. > eg; Via: 1.0 Compression proxy, 1.1 Google promise preview Ok, thanks for explaining the format of that header. I would actually recommend putting the above example in the comment itself to make things clear. However, I still don't think this answers my question: in what case would iterating over them produce a different result than just doing a .find() on the unsplit value? It seems that it would still be equivalent - so then why split?
https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:195: // seperated by ',' correspond to a proxy. Value added by proxy is not seperated by ',' correspond --> that is separated by a comma corresponds Value addded by --> The value added by a any ',' --> any commas https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:201: if (via_header_values[i].find(via_value) != std::string::npos) I think you should test for equality here. I.e., as written, you'll return true if you're searching for foo and you have this header: Via: bar, foobar, goo
https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/60001/chrome/renderer/page_loa... chrome/renderer/page_load_histograms.cc:197: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 19:34:38, Alexei Svitkine wrote: > On 2014/01/09 19:13:49, ksimbili wrote: > > On 2014/01/09 19:01:42, Alexei Svitkine wrote: > > > On 2014/01/09 18:54:50, ksimbili wrote: > > > > On 2014/01/09 16:39:29, Alexei Svitkine wrote: > > > > > Will this find any part separate by commas that contains the string > "Via"? > > > If > > > > > so, please document in the comment. Though, it also seems the perhaps > this > > > > check > > > > > should be stricter than that. > > > > > > > > > > Also, if you're just doing it this way, why split the string at all - > > > wouldn't > > > > > just doing a find() on the pre-split value be equivalent? > > > > > > > > we are not using find() directly to make sure that we don't match others > > proxy > > > > which also has this string in it. > > > > > > If |via_value| never contains a comma (which is the case in this CL for all > > > callers of this function), in what case would this logic be different than > the > > > following? > > > > > > const string16 header_value = > > > frame->dataSource()->response().httpHeaderField(kViaHeaderName)); > > > return header_value.find(via_value) != std::string::npos; > > > > > > Separately, if you're trying to do a more strict check, should it actually > be > > > more strict (e.g. instead of using .find(), use ==)? > > > > No. Sorry for the confusion. > > There can be multiple Via headers present, and all these via_values are > > coalesced into one value using ',' as the seperator. > > Modified the comment to mean it. > > eg; Via: 1.0 Compression proxy, 1.1 Google promise preview > > Ok, thanks for explaining the format of that header. I would actually recommend > putting the above example in the comment itself to make things clear. > > However, I still don't think this answers my question: in what case would > iterating over them produce a different result than just doing a .find() on the > unsplit value? It seems that it would still be equivalent - so then why split? Sorry, corrected the code. I should have checked for equality. We were trying not to match for 'foo' in Via: bar, foobar, goo https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:195: // seperated by ',' correspond to a proxy. Value added by proxy is not On 2014/01/09 19:39:45, bengr1 wrote: > seperated by ',' correspond --> that is separated by a comma corresponds > > Value addded by --> The value added by a > > any ',' --> any commas Done. https://codereview.chromium.org/125303002/diff/170001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:201: if (via_header_values[i].find(via_value) != std::string::npos) On 2014/01/09 19:39:45, bengr1 wrote: > I think you should test for equality here. > > I.e., as written, you'll return true if you're searching for foo and you have > this header: > Via: bar, foobar, goo > Done.
https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:199: frame->dataSource()->response().httpHeaderField(kViaHeaderName)), I think .utf8() might be cheaper than UTF16ToUTF8 here. https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:201: for (unsigned int i = 0; i < via_header_values.size(); ++i) { This loop is now equivalent to: return std::find(v.begin(), v.end(), via_value) != v.end(); (With v being via_header_values). So just use that. You can probably rename via_header_values to something shorter so the above doesn't wrap. (e.g. values).
https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:199: frame->dataSource()->response().httpHeaderField(kViaHeaderName)), On 2014/01/09 20:32:57, Alexei Svitkine wrote: > I think .utf8() might be cheaper than UTF16ToUTF8 here. Done. https://codereview.chromium.org/125303002/diff/240001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:201: for (unsigned int i = 0; i < via_header_values.size(); ++i) { On 2014/01/09 20:32:57, Alexei Svitkine wrote: > This loop is now equivalent to: > > return std::find(v.begin(), v.end(), via_value) != v.end(); > > (With v being via_header_values). > > So just use that. You can probably rename via_header_values to something shorter > so the above doesn't wrap. (e.g. values). Done.
lgtm
rslgtm
Sorry, I have a few more comments. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:184: // requests fetched via the proxy. "This is used only for requests fetched via the proxy" --> "Returns true if |via_value| is one of the values listed in the Via header and the response was fetched via a proxy." https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:185: bool ViaHeaderContains(WebFrame* frame, const std::string& via_value) { optional: you should check that |via_value| does not contain a comma. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:195: // seperated by a comma corresponds to a proxy. The value added by proxy is seperated --> separated by proxy --> by a proxy https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:253: // 2. URL is loaded from web search. 2. is misleading because you set state even when came_from_websearch is false. Fix. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:254: // Based on via header PREVIEW/PREVIEW_WAS_SHOWN/PREVIEW_NOT_USED is // Determine the preview state (PREVIEW, PREVIEW_WAS_SHOWN, PREVIEW_NOT_USED) by inspecting the Via header. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:260: PREVIEW_NOT_USED; I'd move "frame,..." down to the next line and indent 4, though I'd prefer: if (ViaHeaderContains(frame, "1.1 Google Promise Preview") *preview_state = PREVIEW; else *preview_state = PREVIEW_NOT_USED;
https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... File chrome/renderer/page_load_histograms.cc (right): https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:184: // requests fetched via the proxy. On 2014/01/10 17:40:04, bengr1 wrote: > "This is used only for requests fetched via the proxy" --> "Returns true if > |via_value| is one of the values listed in the Via header and the response was > fetched via a proxy." Done. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:195: // seperated by a comma corresponds to a proxy. The value added by proxy is On 2014/01/10 17:40:04, bengr1 wrote: > seperated --> separated > > by proxy --> by a proxy Done. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:253: // 2. URL is loaded from web search. On 2014/01/10 17:40:04, bengr1 wrote: > 2. is misleading because you set state even when came_from_websearch is false. > > Fix. Done. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:254: // Based on via header PREVIEW/PREVIEW_WAS_SHOWN/PREVIEW_NOT_USED is On 2014/01/10 17:40:04, bengr1 wrote: > // Determine the preview state (PREVIEW, PREVIEW_WAS_SHOWN, PREVIEW_NOT_USED) by > inspecting the Via header. Done. https://codereview.chromium.org/125303002/diff/280001/chrome/renderer/page_lo... chrome/renderer/page_load_histograms.cc:260: PREVIEW_NOT_USED; On 2014/01/10 17:40:04, bengr1 wrote: > I'd move "frame,..." down to the next line and indent 4, though I'd prefer: > > if (ViaHeaderContains(frame, "1.1 Google Promise Preview") > *preview_state = PREVIEW; > else > *preview_state = PREVIEW_NOT_USED; Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ksimbili@google.com/125303002/360001
On 2014/01/28 18:37:49, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/ksimbili%40google.com/125303002/360001 lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ksimbili@google.com/125303002/360001
Message was sent while issue was closed.
Change committed as 247726
Message was sent while issue was closed.
On 2014/01/29 18:52:37, I haz the power (commit-bot) wrote: > Change committed as 247726 I believe this CL has an issue with declaration order. ViaHeaderContains is declared and defiend on line 186 in the anonymous namespace, but used already at line 178. This gives the following build error: ../../../chrome/renderer/page_load_histograms.cc:178:10: error: use of undeclared identifier 'ViaHeaderContains' return ViaHeaderContains(frame, kDatReductionProxyViaValue); |
