Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2709483006:
Network traffic annotation added to ntp_snippets. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to ntp_snippets.
Network traffic annotation is added to network request of
ntp_snippets/remote/json_request.
BUG=656607
Review-Url: https://codereview.chromium.org/2709483006
Cr-Commit-Position: refs/heads/master@{#453224}
Committed: https://chromium.googlesource.com/chromium/src/+/c0ea6f04810daed3562ddf3ab89c8c5dd5b988f4
Patch Set 1 #
Total comments: 24
Patch Set 2 : Annotation udpated. #Patch Set 3 : DEPS corrected. #
Messages
Total messages: 26 (11 generated)
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, juliatuttle@chromium.org
asanka@: Please review DEPS as owner of /net. juliatuttle@: 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 json_request.cc in ntp_snippets and added annotation template to it. Please review it and suggest the required answers for the empty parts. 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/cl... 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.
On 2017/02/21 14:38:47, Ramin Halavati wrote: > asanka@: > Please review DEPS as owner of /net. > > juliatuttle@: > 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 json_request.cc in ntp_snippets and added annotation template to > it. Please review it and suggest the required answers for the empty parts. > 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/cl... > > 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. Did you mean to assign this to an ntp_snippets owner?
On 2017/02/23 11:31:23, Marc Treib wrote: > On 2017/02/21 14:38:47, Ramin Halavati wrote: > > asanka@: > > Please review DEPS as owner of /net. > > > > juliatuttle@: > > 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 json_request.cc in ntp_snippets and added annotation template to > > it. Please review it and suggest the required answers for the empty parts. > > 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/cl... > > > > 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. > > Did you mean to assign this to an ntp_snippets owner? Yes, thank you, I've made a mistake.
rhalavati@chromium.org changed reviewers: + treib@chromium.org - juliatuttle@chromium.org
-juliatuttle@ treib@: Please review.
DEPS lgtm
https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/json_request.cc (right): https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:436: net::DefineNetworkTrafficAnnotation("...", R"( ntp_snippets_fetch https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:438: sender: "..." New Tab page content suggestions fetch https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:439: description: "..." Google Chrome can show content suggestions (e.g. news articles) on the New Tab page. For signed-in users, these may be personalized based on the user's synced browsing history. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:440: trigger: "..." Triggered periodically in the background, or upon explicit user request. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:441: data: "..." The Chrome UI language, as well as a second language the user understands, based on translate::LanguageModel. For signed-in users, the requests is authenticated. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:442: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:445: cookies_allowed: false/true false (I think? Not exactly sure what "cookies/channel IDs/..." covers) https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:446: cookies_store: "..." n/a https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:447: setting: "..." n/a (though we might add one; see crbug.com/695129) https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:449: [POLICY_NAME] { NTPContentSuggestionsEnabled https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:450: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} Not sure what this means? Currently the policy is set to "false" (i.e. disable NTP content suggestions) for all enterprise users, but that will likely change soon. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:451: value: ... ?
Annotation updated, please review. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/json_request.cc (right): https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:436: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/23 14:53:46, Marc Treib wrote: > ntp_snippets_fetch Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:438: sender: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > New Tab page content suggestions fetch Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:439: description: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > Google Chrome can show content suggestions (e.g. news articles) on the New Tab > page. For signed-in users, these may be personalized based on the user's synced > browsing history. Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:440: trigger: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > Triggered periodically in the background, or upon explicit user request. Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:441: data: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > The Chrome UI language, as well as a second language the user understands, based > on translate::LanguageModel. For signed-in users, the requests is authenticated. Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:442: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/23 14:53:46, Marc Treib wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:445: cookies_allowed: false/true On 2017/02/23 14:53:46, Marc Treib wrote: > false (I think? Not exactly sure what "cookies/channel IDs/..." covers) Yes, LoadFlags are set to not send or save cookie. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:446: cookies_store: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > n/a Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:447: setting: "..." On 2017/02/23 14:53:46, Marc Treib wrote: > n/a (though we might add one; see crbug.com/695129) Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:449: [POLICY_NAME] { On 2017/02/23 14:53:46, Marc Treib wrote: > NTPContentSuggestionsEnabled Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:450: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/23 14:53:46, Marc Treib wrote: > Not sure what this means? > > Currently the policy is set to "false" (i.e. disable NTP content suggestions) > for all enterprise users, but that will likely change soon. Done. https://codereview.chromium.org/2709483006/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/json_request.cc:451: value: ... On 2017/02/23 14:53:46, Marc Treib wrote: > ? It should be the value that disables this feature, i.e. false.
treib@chromium.org changed reviewers: + jkrcal@chromium.org
LGTM Also +jkrcal FYI and to double-check that I didn't miss anything. (non-blocking - if I did miss anything, we can fix it in a follow-up) Thanks!
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
+battre@, msramek@: Do you have any privacy comment?
On 2017/02/27 11:57:23, Marc Treib wrote: > LGTM > > Also +jkrcal FYI and to double-check that I didn't miss anything. (non-blocking > - if I did miss anything, we can fix it in a follow-up) > > Thanks! lgtm as well
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2709483006/#ps20001 (title: "Annotation udpated.")
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
Failed to apply patch for components/ntp_snippets/DEPS: While running git apply --index -p1; error: patch failed: components/ntp_snippets/DEPS:13 error: components/ntp_snippets/DEPS: patch does not apply Patch: components/ntp_snippets/DEPS Index: components/ntp_snippets/DEPS diff --git a/components/ntp_snippets/DEPS b/components/ntp_snippets/DEPS index ebaabefa33778fb033908b80cf08592771e75a01..2b0f14a2edb2053ae48191f3e4ec852e418aea3d 100644 --- a/components/ntp_snippets/DEPS +++ b/components/ntp_snippets/DEPS @@ -13,6 +13,7 @@ include_rules = [ "+grit/components_strings.h", "+net/base", "+net/http", + "+net/traffic_annotation", "+net/url_request", "+google_apis", "+ui/base",
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, treib@chromium.org, jkrcal@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2709483006/#ps40001 (title: "DEPS corrected.")
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": 1488208146614690,
"parent_rev": "65fa5e5cce6c1e7dcb4947a26d6d219796da0dd0", "commit_rev":
"c0ea6f04810daed3562ddf3ab89c8c5dd5b988f4"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to ntp_snippets. Network traffic annotation is added to network request of ntp_snippets/remote/json_request. BUG=656607 ========== to ========== Network traffic annotation added to ntp_snippets. Network traffic annotation is added to network request of ntp_snippets/remote/json_request. BUG=656607 Review-Url: https://codereview.chromium.org/2709483006 Cr-Commit-Position: refs/heads/master@{#453224} Committed: https://chromium.googlesource.com/chromium/src/+/c0ea6f04810daed3562ddf3ab89c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c0ea6f04810daed3562ddf3ab89c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
