Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703283004:
Network traffic annotation added to distiller_url_fetcher. (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, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to distiller_url_fetcher.
Network traffic annotation is added to network request of
distiller_url_fetcher.
BUG=656607
Review-Url: https://codereview.chromium.org/2703283004
Cr-Commit-Position: refs/heads/master@{#455080}
Committed: https://chromium.googlesource.com/chromium/src/+/ff0155c8b55abcc8ce9fa54cd7af315f3e357968
Patch Set 1 #
Total comments: 25
Patch Set 2 : Annotation updated. #Patch Set 3 : Minor update. #
Total comments: 4
Patch Set 4 : Comment addressd. #
Total comments: 4
Patch Set 5 : Annotation updated. #
Total comments: 2
Patch Set 6 : Comments addressed. #Messages
Total messages: 28 (8 generated)
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, nyquist@chromium.org
asanka@: Please review DEPS file as owner of /net. nyquist@: 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 1 file related to distiller url fetcher and its DEPS file and added annotation template to it. Please review them 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.
nyquist@chromium.org changed reviewers: + wychen@chromium.org
wychen: Like we discussed offline, do you think you could help Ramin fill in the details here?
On 2017/02/22 10:47:12, nyquist wrote: > wychen: Like we discussed offline, do you think you could help Ramin fill in the > details here? I'm on it. Thanks for letting me know.
DEPS lgtm
wychen@chromium.org changed reviewers: + noyau@chromium.org, olivierrobin@chromium.org
Add Bling team for comments about Reader Mode and Reading List. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:52: net::DefineNetworkTrafficAnnotation("...", R"( Nit: raw strings should not be indented with the rest of the code, or you'll get extra spaces in front of each lines. Probably fine functional-wise since it's still valid JSON, but this slightly increases the binary size. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:52: net::DefineNetworkTrafficAnnotation("...", R"( unique id: "dom_distiller" https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:54: sender: "..." sender: "DOM Distiller" https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:55: description: "..." description: "Google Chrome provides Mobile-friendly view on Android phones when the web page contains an article, and is not mobile-friendly. If the user enters Mobile-friendly view, the main content would be extracted and reflowed for better readability. DOM distiller is the backend service for Mobile-friendly view, Reader Mode, and Reading List." https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:56: trigger: "..." trigger: "User enters Mobile-friendly view, Reader Mode, or Reading List." https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:57: data: "..." data: "URL of the required website resources to fetch. No user information is sent." https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:58: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER destination: WEBSITE https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:61: cookies_allowed: false/true cookies_allowed: true https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:62: cookies_store: "..." cookies_store: "user" https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:63: setting: "..." setting: "You can enable or disable Mobile-friendly view by toggling chrome://flags#reader-mode-heuristics in Chrome on Android. Reader Mode and Reading List can be enabled or disabled in Chrome iOS settings." https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:64: policy { No policies. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:70: policy_exception_justification: "..." policy_exception_justification: "Not implemented, considered not useful as no content is being uploaded; this request merely downloads the resources on the web."
What will be the consequences of this? Will that add request headers to the requests? Or is it just for logging? https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:63: setting: "..." On 2017/02/23 21:31:24, wychen wrote: > setting: "You can enable or disable Mobile-friendly view by toggling > chrome://flags#reader-mode-heuristics in Chrome on Android. Reader Mode and > Reading List can be enabled or disabled in Chrome iOS settings." Reader mode is not publicly shipped on iOS. Only public visible feature is Reading List (which use distilled pages).
Comments addressed, please review. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:52: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/23 21:31:24, wychen wrote: > Nit: raw strings should not be indented with the rest of the code, or you'll get > extra spaces in front of each lines. Probably fine functional-wise since it's > still valid JSON, but this slightly increases the binary size. Annotation raw strings are optimized out of binary code, and only the unique id will stay in the binary. Raw strings are extracted using a clang tool and while converting to protobufs, the extra spaces are removed. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:54: sender: "..." On 2017/02/23 21:31:24, wychen wrote: > sender: "DOM Distiller" Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:55: description: "..." On 2017/02/23 21:31:24, wychen wrote: > description: "Google Chrome provides Mobile-friendly view on Android phones when > the web page contains an article, and is not mobile-friendly. If the user enters > Mobile-friendly view, the main content would be extracted and reflowed for > better readability. DOM distiller is the backend service for Mobile-friendly > view, Reader Mode, and Reading List." Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:56: trigger: "..." On 2017/02/23 21:31:24, wychen wrote: > trigger: "User enters Mobile-friendly view, Reader Mode, or Reading List." Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:57: data: "..." On 2017/02/23 21:31:24, wychen wrote: > data: "URL of the required website resources to fetch. No user information is > sent." Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:58: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/23 21:31:25, wychen wrote: > destination: WEBSITE Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:61: cookies_allowed: false/true On 2017/02/23 21:31:25, wychen wrote: > cookies_allowed: true Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:62: cookies_store: "..." On 2017/02/23 21:31:24, wychen wrote: > cookies_store: "user" Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:63: setting: "..." On 2017/02/24 08:36:16, Olivier Robin wrote: > On 2017/02/23 21:31:24, wychen wrote: > > setting: "You can enable or disable Mobile-friendly view by toggling > > chrome://flags#reader-mode-heuristics in Chrome on Android. Reader Mode and > > Reading List can be enabled or disabled in Chrome iOS settings." > > Reader mode is not publicly shipped on iOS. Only public visible feature is > Reading List (which use distilled pages). Please advise on the settings comment for Reading List and Reader mode. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:64: policy { On 2017/02/23 21:31:24, wychen wrote: > No policies. Done. https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:70: policy_exception_justification: "..." On 2017/02/23 21:31:25, wychen wrote: > policy_exception_justification: "Not implemented, considered not useful as no > content is being uploaded; this request merely downloads the resources on the > web." Done.
I'm assuming there is a doc somewhere describing what is the purpose of those calls and what data they collect and send. Can I have a link to it? Thanks. https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:56: "Chromium provides Mobile-friendly view on Android phones when the " Please amend this comment. by either removing android or adding iOS. This code is used on both platforms. https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:63: "User enters Mobile-friendly view, Reader Mode, or Reading List." This trigger is incorrect on iOS. On this platform this is called on the background as soon as a user has an entry on the reading list. And the entry can be added from another app.
Comments addressed, please review. You can read about annotations and how they are used in the following link, but TLDR, these annotations are extracted during compile time and are not added to binary. They are now designed to be used for policy enforcement and auditing and can help in network layer debugging. https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:56: "Chromium provides Mobile-friendly view on Android phones when the " On 2017/02/24 09:37:28, noyau wrote: > Please amend this comment. by either removing android or adding iOS. This code > is used on both platforms. Done. https://codereview.chromium.org/2703283004/diff/40001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:63: "User enters Mobile-friendly view, Reader Mode, or Reading List." On 2017/02/24 09:37:28, noyau wrote: > This trigger is incorrect on iOS. On this platform this is called on the > background as soon as a user has an entry on the reading list. And the entry can > be added from another app. Done.
https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/1/components/dom_distiller/co... components/dom_distiller/core/distiller_url_fetcher.cc:63: setting: "..." On 2017/02/24 09:03:49, Ramin Halavati wrote: > On 2017/02/24 08:36:16, Olivier Robin wrote: > > On 2017/02/23 21:31:24, wychen wrote: > > > setting: "You can enable or disable Mobile-friendly view by toggling > > > chrome://flags#reader-mode-heuristics in Chrome on Android. Reader Mode and > > > Reading List can be enabled or disabled in Chrome iOS settings." > > > > Reader mode is not publicly shipped on iOS. Only public visible feature is > > Reading List (which use distilled pages). > > Please advise on the settings comment for Reading List and Reader mode. Then I guess we don't mention Reader Mode on iOS at all. We won't be able to turn off Reading List when we launch, right? https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:56: "Chromium provides Mobile-friendly view on Android and iOS phones " "Chromium provides Mobile-friendly view on Android phones when the web page contains an article, and is not mobile-friendly. If the user enters Mobile-friendly view, the main content would be extracted and reflowed in a simple layout for better readability. On iOS, apps can add URLs to the Reading List in Chromium. When opening the entries in the Reading List with no or limited network, the simple layout would be shown. DOM distiller is the backend service for Mobile-friendly view and Reading List." https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:63: "On Android phones, when the user enters Mobile-friendly view, " I guess the lack of feature parity between Android and iOS can be confusing. :p "When the user enters Mobile-friendly view on Android phones, or adds entries to the Reading List on iOS. Note that Reading List entries can be added from other apps."
Annotation updated, please review. https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:56: "Chromium provides Mobile-friendly view on Android and iOS phones " On 2017/02/24 18:22:10, wychen wrote: > "Chromium provides Mobile-friendly view on Android phones when the web page > contains an article, and is not mobile-friendly. If the user enters > Mobile-friendly view, the main content would be extracted and reflowed in a > simple layout for better readability. On iOS, apps can add URLs to the Reading > List in Chromium. When opening the entries in the Reading List with no or > limited network, the simple layout would be shown. DOM distiller is the backend > service for Mobile-friendly view and Reading List." Done. https://codereview.chromium.org/2703283004/diff/60001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:63: "On Android phones, when the user enters Mobile-friendly view, " On 2017/02/24 18:22:10, wychen wrote: > I guess the lack of feature parity between Android and iOS can be confusing. :p > > "When the user enters Mobile-friendly view on Android phones, or adds entries to > the Reading List on iOS. Note that Reading List entries can be added from other > apps." Done.
olivierrobin@, noyau@, How does it look now?
lgtm
On 2017/03/01 16:55:55, noyau wrote: > lgtm rubber stamp if noyau@ said so. lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you.
https://codereview.chromium.org/2703283004/diff/80001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/80001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:69: "URL of the required website resources to fetch. No user " nit: URLs (since "resources" in this sentence is plural) Can we say that no user information is sent if cookies are allowed?
Comments addressed, please review. https://codereview.chromium.org/2703283004/diff/80001/components/dom_distille... File components/dom_distiller/core/distiller_url_fetcher.cc (right): https://codereview.chromium.org/2703283004/diff/80001/components/dom_distille... components/dom_distiller/core/distiller_url_fetcher.cc:69: "URL of the required website resources to fetch. No user " On 2017/03/07 10:57:27, msramek wrote: > nit: URLs (since "resources" in this sentence is plural) > > Can we say that no user information is sent if cookies are allowed? Done.
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, wychen@chromium.org, noyau@chromium.org Link to the patchset: https://codereview.chromium.org/2703283004/#ps100001 (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": 100001, "attempt_start_ts": 1488894490669140,
"parent_rev": "848f244822a7db6a71ceae2b4698b74362d4d5cb", "commit_rev":
"ff0155c8b55abcc8ce9fa54cd7af315f3e357968"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to distiller_url_fetcher. Network traffic annotation is added to network request of distiller_url_fetcher. BUG=656607 ========== to ========== Network traffic annotation added to distiller_url_fetcher. Network traffic annotation is added to network request of distiller_url_fetcher. BUG=656607 Review-Url: https://codereview.chromium.org/2703283004 Cr-Commit-Position: refs/heads/master@{#455080} Committed: https://chromium.googlesource.com/chromium/src/+/ff0155c8b55abcc8ce9fa54cd7af... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ff0155c8b55abcc8ce9fa54cd7af... |
