Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563103002:
Switch variations http headers to only be reported over https. (Closed)
DescriptionSwitch variations http headers to only be reported over https.
BUG=672950
Committed: https://crrev.com/a949b56566eb6519e7f78cd29a62705914ea82a8
Cr-Commit-Position: refs/heads/master@{#438200}
Patch Set 1 #
Total comments: 12
Patch Set 2 : refactoring code a bit #
Total comments: 6
Patch Set 3 : comment fixes #
Total comments: 2
Patch Set 4 : fixing drive by #
Messages
Total messages: 27 (14 generated)
jwd@chromium.org changed reviewers: + rkaplow@chromium.org
The CQ bit was checked by jwd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:54: // of |url|, and if it's an http/https spec. this method doesn't care about the ssl-ness https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:56: // IsGoogleDomainUrl checks if the url is valid and is http/https. I don't quite get the comment - this doesn't fully check if it's valid since it's not checking if its HTTPS. Isn't this jut checking if it is a google domain? maybe rephase? https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:62: if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS()) i realize this is just refactored, but it seems odd to me that this occurs after isgoogeldomainURL. However I checked and isGoogleDomain checks this already, with: https://codesearch.chromium.org/chromium/src/components/google/core/browser/g... (not sure if it's worth changing to that method, up to you). Another nit on this - seems more intuitive to check before the Google check, but doesn't make a difference https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:78: return google_util::IsYoutubeDomainUrl(url, google_util::ALLOW_SUBDOMAIN, this is a minor nit, but seems strange that this is logically away from the isGoogleDomainURL. I'd prefer this check live up there, and then just a return false here https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:124: "Variations.Headers.ShouldAppendResult"; I don't love the name of the histogram - it seems a bit indirect. I'd prefer something that says this is checking the URL. Not sure... V.H.URLValidationResult? https://codereview.chromium.org/2563103002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563103002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:105430: + <int value="0" label="Rejected: Not a Google domain."/> nit, maybe not a valid Google domain. ? Since this case will also include invalid domains, I guess it is implied though. Might be worth making it explicit.
https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:54: // of |url|, and if it's an http/https spec. On 2016/12/12 18:25:05, rkaplow wrote: > this method doesn't care about the ssl-ness Changed code. https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:56: // IsGoogleDomainUrl checks if the url is valid and is http/https. On 2016/12/12 18:25:05, rkaplow wrote: > I don't quite get the comment - this doesn't fully check if it's valid since > it's not checking if its HTTPS. Isn't this jut checking if it is a google > domain? maybe rephase? Changed code. https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:62: if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS()) On 2016/12/12 18:25:05, rkaplow wrote: > i realize this is just refactored, but it seems odd to me that this occurs after > isgoogeldomainURL. However I checked and isGoogleDomain checks this already, > with: > https://codesearch.chromium.org/chromium/src/components/google/core/browser/g... > > (not sure if it's worth changing to that method, up to you). > > Another nit on this - seems more intuitive to check before the Google check, but > doesn't make a difference Pulled check out to calling function. https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:78: return google_util::IsYoutubeDomainUrl(url, google_util::ALLOW_SUBDOMAIN, On 2016/12/12 18:25:05, rkaplow wrote: > this is a minor nit, but seems strange that this is logically away from the > isGoogleDomainURL. I'd prefer this check live up there, and then just a return > false here Done. https://codereview.chromium.org/2563103002/diff/1/components/variations/net/v... components/variations/net/variations_http_headers.cc:124: "Variations.Headers.ShouldAppendResult"; On 2016/12/12 18:25:05, rkaplow wrote: > I don't love the name of the histogram - it seems a bit indirect. I'd prefer > something that says this is checking the URL. Not sure... > V.H.URLValidationResult? Done. https://codereview.chromium.org/2563103002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2563103002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:105430: + <int value="0" label="Rejected: Not a Google domain."/> On 2016/12/12 18:25:06, rkaplow wrote: > nit, maybe not a valid Google domain. ? Since this case will also include > invalid domains, I guess it is implied though. Might be worth making it > explicit. Changed code, keeping this label as is.
https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:56: bool ShouldAppendForDomain(const GURL& url) { actually don't super like this name, kind of prefer IsGoogleDomain or something. up to you https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:123: const char kShouldAppendResultHistogram[] = nit - could rename this https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:129: if (url.SchemeIs("https")) { !
https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:56: bool ShouldAppendForDomain(const GURL& url) { On 2016/12/12 19:31:17, rkaplow wrote: > actually don't super like this name, kind of prefer IsGoogleDomain or something. > up to you Done. https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:123: const char kShouldAppendResultHistogram[] = On 2016/12/12 19:31:18, rkaplow wrote: > nit - could rename this Done. https://codereview.chromium.org/2563103002/diff/20001/components/variations/n... components/variations/net/variations_http_headers.cc:129: if (url.SchemeIs("https")) { On 2016/12/12 19:31:18, rkaplow wrote: > ! Done.
Patchset #3 (id:40001) has been deleted
lgtm
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
drive-by lgtm with a comment! https://codereview.chromium.org/2563103002/diff/60001/components/variations/n... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/60001/components/variations/n... components/variations/net/variations_http_headers.cc:142: URL_VALIDATION_RESULT_SIZE); Nit: Make a helper function for logging this - since each macro expands to a lot of code. Once you do that, you can inline the string constant into that helper.
https://codereview.chromium.org/2563103002/diff/60001/components/variations/n... File components/variations/net/variations_http_headers.cc (right): https://codereview.chromium.org/2563103002/diff/60001/components/variations/n... components/variations/net/variations_http_headers.cc:142: URL_VALIDATION_RESULT_SIZE); On 2016/12/12 20:41:16, Alexei Svitkine (OO from 15th) wrote: > Nit: Make a helper function for logging this - since each macro expands to a lot > of code. Once you do that, you can inline the string constant into that helper. Done.
The CQ bit was checked by jwd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2563103002/#ps80001 (title: "fixing drive by")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jwd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481646502026810,
"parent_rev": "e5ad50d8d4926a1cd95e23578aa745e463781eae", "commit_rev":
"ed81b996ce8973543cb14ab940fae18670fecaa5"}
Message was sent while issue was closed.
Description was changed from ========== Switch variations http headers to only be reported over https. BUG=672950 ========== to ========== Switch variations http headers to only be reported over https. BUG=672950 Review-Url: https://codereview.chromium.org/2563103002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Switch variations http headers to only be reported over https. BUG=672950 Review-Url: https://codereview.chromium.org/2563103002 ========== to ========== Switch variations http headers to only be reported over https. BUG=672950 Committed: https://crrev.com/a949b56566eb6519e7f78cd29a62705914ea82a8 Cr-Commit-Position: refs/heads/master@{#438200} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a949b56566eb6519e7f78cd29a62705914ea82a8 Cr-Commit-Position: refs/heads/master@{#438200} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
