Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2711833003:
Network traffic annotation added to template_url_fetcher. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, battre, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to template_url_fetcher.
Network traffic annotation is added to network request of
search_engines/template_url_fetcher.
BUG=656607
Review-Url: https://codereview.chromium.org/2711833003
Cr-Commit-Position: refs/heads/master@{#453544}
Committed: https://chromium.googlesource.com/chromium/src/+/6943c89daf869bcde4f02d98e8268fa253cb47b1
Patch Set 1 #
Total comments: 20
Patch Set 2 : Annotation updated. #
Total comments: 13
Patch Set 3 : Annotation updated. #Patch Set 4 : Rebased on disabled cookies. #
Total comments: 4
Patch Set 5 : nits #Messages
Total messages: 20 (6 generated)
rhalavati@chromium.org changed reviewers: + vasilii@chromium.org
Vasilii, Another file for annotation, please review. Here are the references: 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.
vasilii@chromium.org changed reviewers: + pkasting@chromium.org
+pkasting I'm baffled by two things - the request isn't cookieless; - there is no policy or setting to turn the feature off; Peter, do you have some context? https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:25: net::DefineNetworkTrafficAnnotation("...", R"( open_search https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:27: sender: "..." open_search_fetcher https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:28: description: "..." The web pages can include an OpenSearch description doc in their HTML. In this case Chrome downloads and parses the file. The corresponding search engine is added to the list in the browser (chrome://settings/searchEngines). https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:29: trigger: "..." User visits a web page containing <link rel="search"> tag. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:30: data: "..." Nothing is uploaded. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:31: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER WEBSITE https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:34: cookies_allowed: false/true true https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:35: cookies_store: "..." user https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:36: setting: "..." N/A https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:43: policy_exception_justification: "..." It's a web feature?
Annotation updated. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:25: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/24 14:42:38, vasilii wrote: > open_search Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:27: sender: "..." On 2017/02/24 14:42:38, vasilii wrote: > open_search_fetcher Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:28: description: "..." On 2017/02/24 14:42:38, vasilii wrote: > The web pages can include an OpenSearch description doc in their HTML. In this > case Chrome downloads and parses the file. The corresponding search engine is > added to the list in the browser (chrome://settings/searchEngines). Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:29: trigger: "..." On 2017/02/24 14:42:38, vasilii wrote: > User visits a web page containing <link rel="search"> tag. Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:30: data: "..." On 2017/02/24 14:42:38, vasilii wrote: > Nothing is uploaded. Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:31: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/24 14:42:38, vasilii wrote: > WEBSITE Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:34: cookies_allowed: false/true On 2017/02/24 14:42:38, vasilii wrote: > true Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:35: cookies_store: "..." On 2017/02/24 14:42:38, vasilii wrote: > user Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:36: setting: "..." On 2017/02/24 14:42:38, vasilii wrote: > N/A Done. https://codereview.chromium.org/2711833003/diff/1/components/search_engines/t... components/search_engines/template_url_fetcher.cc:43: policy_exception_justification: "..." On 2017/02/24 14:42:37, vasilii wrote: > It's a web feature? Done.
https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:39: cookies_allowed: false true
https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:27: sender: "Open Search Fetcher" I think this should be Omnibox, as that's ultimately the larger feature this is part of. https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:29: "The web pages can include an OpenSearch description doc in their " Nit: No "the" https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:30: "HTML. In this case Chromium downloads and parses the file. The " Is merely visiting the page, and not actually using the search engine, enough to trigger this? https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:34: "User visits a web page containing <link rel="search"> tag." Nit: Two spaces after "containing" should be " a " instead. https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:43: "upload any data." Since we send cookies, this seems incorrect. Maybe we shouldn't send/save cookies with this request?
Comments addressed, please review. Would you like me to create a CL to disable Cookies, and then base this CL on that? Or they are required and we add a recommendation to implement a policy for that? https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:27: sender: "Open Search Fetcher" On 2017/02/24 23:33:09, Peter Kasting wrote: > I think this should be Omnibox, as that's ultimately the larger feature this is > part of. Done. https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:29: "The web pages can include an OpenSearch description doc in their " On 2017/02/24 23:33:09, Peter Kasting wrote: > Nit: No "the" Done. https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:34: "User visits a web page containing <link rel="search"> tag." On 2017/02/24 23:33:09, Peter Kasting wrote: > Nit: Two spaces after "containing" should be " a " instead. Done.
Ramin, it would be great if you created a separate CL for suppressing the cookies. https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:30: "HTML. In this case Chromium downloads and parses the file. The " On 2017/02/24 23:33:09, Peter Kasting wrote: > Is merely visiting the page, and not actually using the search engine, enough to > trigger this? Yes, you can land on https://yandex.ru/ and observe a new entry in chrome://settings/searchEngines https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:43: "upload any data." On 2017/02/24 23:33:09, Peter Kasting wrote: > Since we send cookies, this seems incorrect. > > Maybe we shouldn't send/save cookies with this request? I didn't find anything related to cookies in the spec (http://www.opensearch.org/Specifications/OpenSearch/1.1#Autodiscovery_in_HTML...). From the privacy perspective it doesn't bring a lot as the link may contain the user id. There is a problem with using the user request context in the browser process (https://crbug.com/675944 and https://codereview.chromium.org/2249213002/). The request may poison the main profile. A short-term quick fix proposed was the combination net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_AUTH_DATA. I suggest we apply it here.
https://codereview.chromium.org/2717323002/ created to disable cookies and AUTH and rebased this CL on that.
https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:27: sender: "Open Search Fetcher" On 2017/02/24 23:33:09, Peter Kasting wrote: > I think this should be Omnibox, as that's ultimately the larger feature this is > part of. Peter, why omnibox? If I search for "yandex" and then click the first link on the Google result page, the search engine is still registered. Despite I didn't do anything with the omnibox.
https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/20001/components/search_engin... components/search_engines/template_url_fetcher.cc:27: sender: "Open Search Fetcher" On 2017/02/27 13:14:01, vasilii wrote: > On 2017/02/24 23:33:09, Peter Kasting wrote: > > I think this should be Omnibox, as that's ultimately the larger feature this > is > > part of. > > Peter, why omnibox? If I search for "yandex" and then click the first link on > the Google result page, the search engine is still registered. Despite I didn't > do anything with the omnibox. Because that's what these search engines are actually used for: tab-to-search in the omnibox. In other words, this is "used by" rather than "caused by".
LGTM https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... components/search_engines/template_url_fetcher.cc:23: // Traffic annotation for ReqeustDelegate Nit: RequestDelegate. Also, add trailing period. Also, I'd put blank lines between this declaration and the surrounding "namespace {"-type lines. https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... components/search_engines/template_url_fetcher.cc:31: "corresponding search engine is added to the list in the browser " Nit: browser -> browser settings (or just "settings")
Comments addressed, landing. https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... File components/search_engines/template_url_fetcher.cc (right): https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... components/search_engines/template_url_fetcher.cc:23: // Traffic annotation for ReqeustDelegate On 2017/02/28 01:00:15, Peter Kasting wrote: > Nit: RequestDelegate. Also, add trailing period. Also, I'd put blank lines > between this declaration and the surrounding "namespace {"-type lines. Done. https://codereview.chromium.org/2711833003/diff/60001/components/search_engin... components/search_engines/template_url_fetcher.cc:31: "corresponding search engine is added to the list in the browser " On 2017/02/28 01:00:15, Peter Kasting wrote: > Nit: browser -> browser settings (or just "settings") Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2711833003/#ps80001 (title: "nits")
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": 1488267207217900,
"parent_rev": "aa2f72e8c561e31ba7c79233ff7ed7c5a4b74dc9", "commit_rev":
"6943c89daf869bcde4f02d98e8268fa253cb47b1"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to template_url_fetcher. Network traffic annotation is added to network request of search_engines/template_url_fetcher. BUG=656607 ========== to ========== Network traffic annotation added to template_url_fetcher. Network traffic annotation is added to network request of search_engines/template_url_fetcher. BUG=656607 Review-Url: https://codereview.chromium.org/2711833003 Cr-Commit-Position: refs/heads/master@{#453544} Committed: https://chromium.googlesource.com/chromium/src/+/6943c89daf869bcde4f02d98e826... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6943c89daf869bcde4f02d98e826... |
