|
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 10 months ago CC:
chromium-reviews, 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 popular_sites_impl.
Network traffic annotation is added to network request of
ntp_tiles/popular_sites_impl.
BUG=656607
Review-Url: https://codereview.chromium.org/2710713002
Cr-Commit-Position: refs/heads/master@{#452021}
Committed: https://chromium.googlesource.com/chromium/src/+/db28325f75a0ea04c612f93849861d01997d3aeb
Patch Set 1 #
Total comments: 25
Patch Set 2 : Annotation updated. #
Total comments: 4
Patch Set 3 : Comments addressed. #Messages
Total messages: 17 (7 generated)
rhalavati@chromium.org changed reviewers: + treib@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 popular_sites_impl.cc in ntp_tiles 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.
treib@chromium.org changed reviewers: + mastiz@chromium.org, sfiera@chromium.org
I added suggested data below. Also +more popular_sites owners FYI. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:311: net::DefineNetworkTrafficAnnotation("...", R"( popular_sites or popular_sites_fetch? https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:313: sender: "..." popular sites https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:314: description: "..." Google Chrome may display a list of regionally-popular web sites on the New Tab Page. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:315: trigger: "..." Triggered automatically once per day, unless no popular web sites are required (because the New Tab Page is filled with suggestions based on the user's browsing history). https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:316: data: "..." A two-letter country code based on the user's location. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:317: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:320: cookies_allowed: false/true false https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:321: cookies_store: "..." n/a https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:322: setting: "..." NA https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:329: policy_exception_justification: "..." Not sure - since we don't send any PII, I think a policy is generally not required?
https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:311: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/22 09:49:37, Marc Treib wrote: > popular_sites or popular_sites_fetch? Should we prefix "popular sites" with "new tab page" where it appears here and below? If these identifiers are going to be user-visible, then we should make sure not to imply that this is the bandwidth that popular sites are using. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:314: description: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > Google Chrome may display a list of regionally-popular web sites on the New Tab > Page. Do we not disable it for enterprise users, as we do articles for you? https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:333: data_use_measurement::DataUseUserData::AttachToFetcher( Will this new feature eventually supersede data_use_measurement?
rhalavati@chromium.org changed reviewers: + battre@chromium.org
Annotation updated, please review. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:311: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/22 10:06:12, sfiera wrote: > On 2017/02/22 09:49:37, Marc Treib wrote: > > popular_sites or popular_sites_fetch? > > Should we prefix "popular sites" with "new tab page" where it appears here and > below? If these identifiers are going to be user-visible, then we should make > sure not to imply that this is the bandwidth that popular sites are using. The first identifier is mostly a unique (meaningful) id, and the sender would be used in reports and needs to be human readable and https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:313: sender: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > popular sites Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:315: trigger: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > Triggered automatically once per day, unless no popular web sites are required > (because the New Tab Page is filled with suggestions based on the user's > browsing history). Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:316: data: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > A two-letter country code based on the user's location. Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:317: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/22 09:49:37, Marc Treib wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:320: cookies_allowed: false/true On 2017/02/22 09:49:36, Marc Treib wrote: > false Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:321: cookies_store: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > n/a Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:322: setting: "..." On 2017/02/22 09:49:36, Marc Treib wrote: > NA Done. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:329: policy_exception_justification: "..." On 2017/02/22 09:49:37, Marc Treib wrote: > Not sure - since we don't send any PII, I think a policy is generally not > required? But as you are sending user's country, it might cause a privacy concern. battre@: Any comments? https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:333: data_use_measurement::DataUseUserData::AttachToFetcher( On 2017/02/22 10:06:12, sfiera wrote: > Will this new feature eventually supersede data_use_measurement? I don't think so. battre@: Any comments?
lgtm https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:319: "the New Tab Page is filled with suggestions based on the user's" nit: space missing before " https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:328: "Not implemented, considered not useful?" remove the ? - we can still evaluate that later.
LGTM once battre's nits are adressed https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:314: description: "..." On 2017/02/22 10:06:12, sfiera wrote: > On 2017/02/22 09:49:37, Marc Treib wrote: > > Google Chrome may display a list of regionally-popular web sites on the New > Tab > > Page. > > Do we not disable it for enterprise users, as we do articles for you? No, I don't think so. Shouldn't be too hard to set up, but AFAIK nobody has asked for it. https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... components/ntp_tiles/popular_sites_impl.cc:333: data_use_measurement::DataUseUserData::AttachToFetcher( On 2017/02/22 10:35:09, Ramin Halavati wrote: > On 2017/02/22 10:06:12, sfiera wrote: > > Will this new feature eventually supersede data_use_measurement? > > I don't think so. > > battre@: > Any comments? I wondered the same thing: We now have two things that should be attached to every URLFetcher. But since they have completely different purposes, I'm not sure if it makes sense to merge them.
On 2017/02/22 12:16:37, Marc Treib wrote: > LGTM once battre's nits are adressed > > https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... > File components/ntp_tiles/popular_sites_impl.cc (right): > > https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... > components/ntp_tiles/popular_sites_impl.cc:314: description: "..." > On 2017/02/22 10:06:12, sfiera wrote: > > On 2017/02/22 09:49:37, Marc Treib wrote: > > > Google Chrome may display a list of regionally-popular web sites on the New > > Tab > > > Page. > > > > Do we not disable it for enterprise users, as we do articles for you? > > No, I don't think so. > Shouldn't be too hard to set up, but AFAIK nobody has asked for it. > > https://codereview.chromium.org/2710713002/diff/1/components/ntp_tiles/popula... > components/ntp_tiles/popular_sites_impl.cc:333: > data_use_measurement::DataUseUserData::AttachToFetcher( > On 2017/02/22 10:35:09, Ramin Halavati wrote: > > On 2017/02/22 10:06:12, sfiera wrote: > > > Will this new feature eventually supersede data_use_measurement? > > > > I don't think so. > > > > battre@: > > Any comments? > > I wondered the same thing: We now have two things that should be attached to > every URLFetcher. But since they have completely different purposes, I'm not > sure if it makes sense to merge them. This is the first time I have heard about data_use_measurement and the usecase is covered by our work as well. We discussed this with the network stack team and they explicitly asked us not to take the path that was taken for data_use_measurement (using base::SupportsUserData). I will start a thread.
Comments addressed, landing. https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites_impl.cc (right): https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:319: "the New Tab Page is filled with suggestions based on the user's" On 2017/02/22 10:44:05, battre wrote: > nit: space missing before " Done. https://codereview.chromium.org/2710713002/diff/20001/components/ntp_tiles/po... components/ntp_tiles/popular_sites_impl.cc:328: "Not implemented, considered not useful?" On 2017/02/22 10:44:05, battre wrote: > remove the ? - we can still evaluate that later. Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2710713002/#ps40001 (title: "Comments addressed.")
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": 1487766413524400, "parent_rev": "0bf44aba77e1f6a9dc6b0c5c7e3ebd41f251e2d1", "commit_rev": "db28325f75a0ea04c612f93849861d01997d3aeb"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to popular_sites_impl. Network traffic annotation is added to network request of ntp_tiles/popular_sites_impl. BUG=656607 ========== to ========== Network traffic annotation added to popular_sites_impl. Network traffic annotation is added to network request of ntp_tiles/popular_sites_impl. BUG=656607 Review-Url: https://codereview.chromium.org/2710713002 Cr-Commit-Position: refs/heads/master@{#452021} Committed: https://chromium.googlesource.com/chromium/src/+/db28325f75a0ea04c612f9384986... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/db28325f75a0ea04c612f9384986... |