Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2900673002:
Network traffic annotation added to one_google_bar. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
battre, chromium-reviews, David Black, donnd+watch_chromium.org, jered+watch_chromium.org, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to one_google_bar.
Network traffic annotation is added to network request of:
chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2900673002
Cr-Commit-Position: refs/heads/master@{#473860}
Committed: https://chromium.googlesource.com/chromium/src/+/7ba9103e3f8c8826dd7794867f789fd21c3cec7c
Patch Set 1 #
Total comments: 15
Patch Set 2 : Annotation updated. #
Total comments: 2
Patch Set 3 : Comment addressed. #Patch Set 4 : Annotation updated. #
Total comments: 2
Patch Set 5 : Comment addressed. #Messages
Total messages: 31 (16 generated)
rhalavati@chromium.org changed reviewers: + jochen@chromium.org
Jochen, Marc Treib is the only owner of this file and he is on leave for two weeks. Do you know who else I can ask for suggesting annotations?
The CQ bit was checked by rhalavati@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.
maybe sfiera@ ?
rhalavati@chromium.org changed reviewers: + sfiera@chromium.org
Thank you Jochen. sfiera@, Are you familiar with this code? 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 one_google_bar_fetcher_impl.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.
This should be basically similar to the annotation in doodle_service.cc, as it's for similar purposes. I believe that this request is only made for signed-in users, and my comments reflect that, but I'm not 100% certain of it. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:273: net::DefineNetworkTrafficAnnotation("...", R"( one_google_bar_service https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:275: sender: "..." One Google Bar Service https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:276: description: "..." Downloads the "One Google" bar if Google is the configured search provider and the user is signed in to a Google account. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:277: trigger: "..." Displaying the new tab page on Desktop https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:278: data: "..." Credentials of signed-in user https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:283: setting: "..." Signing out of Chrome or choosing a non-Google search engine in Chrome settings under 'Search Engine' disables this feature. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:288: } DefaultSearchProviderEnabled { policy_options {mode: MANDATORY} DefaultSearchProviderEnabled: false }
https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:276: description: "..." On 2017/05/22 12:48:16, sfiera wrote: > Downloads the "One Google" bar if Google is the configured search provider and > the user is signed in to a Google account. Ah, with another look, I see we reference an API key in this file, which means that we do it for signed-out users and this part is incorrect.
Patchset #2 (id:20001) has been deleted
Thank you Chris, comments addressed, please review. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:273: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/22 12:48:17, sfiera wrote: > one_google_bar_service Done. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:275: sender: "..." On 2017/05/22 12:48:17, sfiera wrote: > One Google Bar Service Done. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:276: description: "..." On 2017/05/22 12:51:37, sfiera wrote: > On 2017/05/22 12:48:16, sfiera wrote: > > Downloads the "One Google" bar if Google is the configured search provider and > > the user is signed in to a Google account. > > Ah, with another look, I see we reference an API key in this file, which means > that we do it for signed-out users and this part is incorrect. I am confused, could you please rephrase? https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:277: trigger: "..." On 2017/05/22 12:48:17, sfiera wrote: > Displaying the new tab page on Desktop Done. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:278: data: "..." On 2017/05/22 12:48:17, sfiera wrote: > Credentials of signed-in user Done. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:283: setting: "..." On 2017/05/22 12:48:16, sfiera wrote: > Signing out of Chrome or choosing a non-Google search engine in Chrome settings > under 'Search Engine' disables this feature. Done. https://codereview.chromium.org/2900673002/diff/1/chrome/browser/search/one_g... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:288: } On 2017/05/22 12:48:17, sfiera wrote: > DefaultSearchProviderEnabled { > policy_options {mode: MANDATORY} > DefaultSearchProviderEnabled: false > } Done.
https://codereview.chromium.org/2900673002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:279: "Chrome and Google is the configured search provider." My earlier comments were incorrect. The request is made whether the user is signed-in or signed-out and signing out does not disable it.
Thank you, comment addressed, please review. I assumed that data is "None", now that sign-in is no more required. https://codereview.chromium.org/2900673002/diff/40001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/40001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:279: "Chrome and Google is the configured search provider." On 2017/05/22 13:24:13, sfiera wrote: > My earlier comments were incorrect. The request is made whether the user is > signed-in or signed-out and signing out does not disable it. Done.
LGTM (but I don't think I'm an owner) On 2017/05/22 13:28:15, Ramin Halavati wrote: > Thank you, comment addressed, please review. > > I assumed that data is "None", now that sign-in is no more required. If the user is signed in, user credentials will still be sent. If the user is signed-out, nothing is sent (aside, potentially, from variation IDs, which is true of any Google-owned endpoint).
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Chris. Martin, Any comments?
LGTM https://codereview.chromium.org/2900673002/diff/80001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/80001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:280: data: "Credentials if user has signed-in." nit: AFAIU "signed-in" (with the dash) is an adjective. Therefore, this should be "has signed in" or, as we usually write, "is signed in".
Thank you Martin, comment addressed. Jochen, Could you please approve this? https://codereview.chromium.org/2900673002/diff/80001/chrome/browser/search/o... File chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc (right): https://codereview.chromium.org/2900673002/diff/80001/chrome/browser/search/o... chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc:280: data: "Credentials if user has signed-in." On 2017/05/22 16:51:16, msramek wrote: > nit: AFAIU "signed-in" (with the dash) is an adjective. Therefore, this should > be "has signed in" or, as we usually write, "is signed in". Done.
The CQ bit was checked by rhalavati@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.
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2900673002/#ps100001 (title: "Comment 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": 100001, "attempt_start_ts": 1495539312791380,
"parent_rev": "17d6248cb822cf06ae06fccd65fbeebfa7b4af2a", "commit_rev":
"7ba9103e3f8c8826dd7794867f789fd21c3cec7c"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to one_google_bar. Network traffic annotation is added to network request of: chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc BUG=656607 ========== to ========== Network traffic annotation added to one_google_bar. Network traffic annotation is added to network request of: chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc BUG=656607 Review-Url: https://codereview.chromium.org/2900673002 Cr-Commit-Position: refs/heads/master@{#473860} Committed: https://chromium.googlesource.com/chromium/src/+/7ba9103e3f8c8826dd7794867f78... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7ba9103e3f8c8826dd7794867f78... |
