Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730263003:
Network traffic annotation added to rlz/lib. (Closed)
|
Created:
3 years, 9 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to rlz/lib.
Network traffic annotation is added to network request of
rlz/lib.
BUG=656607
Review-Url: https://codereview.chromium.org/2730263003
Cr-Commit-Position: refs/heads/master@{#455077}
Committed: https://chromium.googlesource.com/chromium/src/+/f3dc234c2f85b6262efcf0939c7b0e621a101c91
Patch Set 1 #
Total comments: 1
Patch Set 2 : Annotation updated. #
Total comments: 6
Patch Set 3 : Comments addressed. #Messages
Total messages: 17 (8 generated)
rhalavati@chromium.org changed reviewers: + rogerta@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified rlz/lib/financial_ping.cc and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
lgtm https://codereview.chromium.org/2730263003/diff/1/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2730263003/diff/1/rlz/lib/financial_ping.cc#n... rlz/lib/financial_ping.cc:344: GURL(url), net::URLFetcher::GET, &delegate, traffic_annotation); net::NetworkTrafficAnnotationTag traffic_annotation = net::DefineNetworkTrafficAnnotation("rlz_ping", R"( semantics { sender: "rlz_ping" description: "Used for measuring effectiveness of a promotion. " "See Chrome privacy policy for complete details: " "https://www.google.com/chrome/browser/privacy/whitepaper.html#measurepromotions" trigger: "1/ At chrome first run, " "2/ When Chrome is re-activated by a new promotion, " "3/ Once a week thereafter as long as chrome is used." data: "1/ Non-unique cohort tag of when Chrome was installed, " "2/ Unique machine id on desktop platforms, " "3/ whether Google is the default omnibox search, " "4/ whether google.com is the default home page." destination: GOOGLE_OWNED_SERVICE } policy { cookies_allowed: false setting: "This feature is always enabled." })");
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Annotation updated.
LGTM % comments. https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:327: "Used for measuring effectiveness of a promotion. See Chromium " nit: Not a native speaker, but shouldn't this be "the effectiveness"? https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:328: "privacy policy for complete details: https://www.google.com/chrome" "See the Chrome Privacy Whitepaper" https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:331: "1/ At Chromium first run, " Should we add "\n" so it looks good when strings are extracted?
Comments addressed, landing. https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.cc File rlz/lib/financial_ping.cc (right): https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:327: "Used for measuring effectiveness of a promotion. See Chromium " On 2017/03/07 10:24:49, msramek wrote: > nit: Not a native speaker, but shouldn't this be "the effectiveness"? Done. https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:328: "privacy policy for complete details: https://www.google.com/chrome" On 2017/03/07 10:24:49, msramek wrote: > "See the Chrome Privacy Whitepaper" Done. https://codereview.chromium.org/2730263003/diff/20001/rlz/lib/financial_ping.... rlz/lib/financial_ping.cc:331: "1/ At Chromium first run, " On 2017/03/07 10:24:49, msramek wrote: > Should we add "\n" so it looks good when strings are extracted? Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2730263003/#ps40001 (title: "Comments addressed.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rhalavati@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": 40001, "attempt_start_ts": 1488896386657230,
"parent_rev": "5b3981679d8eee4fdbb16d612b571de4ab71498f", "commit_rev":
"f3dc234c2f85b6262efcf0939c7b0e621a101c91"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to rlz/lib. Network traffic annotation is added to network request of rlz/lib. BUG=656607 ========== to ========== Network traffic annotation added to rlz/lib. Network traffic annotation is added to network request of rlz/lib. BUG=656607 Review-Url: https://codereview.chromium.org/2730263003 Cr-Commit-Position: refs/heads/master@{#455077} Committed: https://chromium.googlesource.com/chromium/src/+/f3dc234c2f85b6262efcf0939c7b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/f3dc234c2f85b6262efcf0939c7b... |
