Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843403002:
Network traffic annotation added to ResourceDisaptcherHostImpl. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays), msramek, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to ResourceDisaptcherHostImpl.
Network traffic annotation is added to network request of:
content/browser/loader/resource_dispatcher_host_impl.cc.
BUG=656607
Review-Url: https://codereview.chromium.org/2843403002
Cr-Commit-Position: refs/heads/master@{#468979}
Committed: https://chromium.googlesource.com/chromium/src/+/d662ca6659b624722ec413933ab8295a63cd0013
Patch Set 1 #
Total comments: 25
Patch Set 2 : Annotation updated. #Patch Set 3 : Annotation updated. #Messages
Total messages: 15 (7 generated)
rhalavati@chromium.org changed reviewers: + mmenke@chromium.org
Hi Matt, Please suggest network traffic annotations for this file. Here are the resources: 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.
https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1210: sender: "..." Renderer Process, and occasionally the Browser Process. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1211: description: "..." Navigation-initiated request or renderer process initiated request, which includes all resources for normal page loads, chrome URLs, resources for installed extensions, as well as downloads. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1212: trigger: "..." Navigating to a URL or downloading a file. A webpage, ServiceWorker, chrome:// page, or extension may also initiate requests in the background. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1213: data: "..." Anything the initiator wants to send. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1214: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL WEBSITE, GOOGLE_OWNED_SERVICE (For things like the NTP), LOCAL (Chrome URLs), and OTHER (for extensions) https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1217: cookies_allowed: false/true true https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1218: cookies_store: "..." Both the main cookie store and per-app cookie stores, for installed Apps (Which are deprecated everywhere but ChromeOS) https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1219: setting: "..." None. If you disable these requests, Chrome will be unable to load any webpage. There are a number of ways to disable particular types of requests, or blacklist individual URLs or websites, but no way to disable all requests. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1223: [POLICY_NAME]: ... //(value to disable it) None, per above. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1226: policy_exception_justification: "..." Without these requests, Chrome will be unable to load any webpage.
Thank you very much, annotation updated, please review. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1210: sender: "..." On 2017/04/27 12:24:09, mmenke wrote: > Renderer Process, and occasionally the Browser Process. Can we call it "Resource Dispatcher Host"? The name should specify the current module/component. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1211: description: "..." On 2017/04/27 12:24:10, mmenke wrote: > Navigation-initiated request or renderer process initiated request, which > includes all resources for normal page loads, chrome URLs, resources for > installed extensions, as well as downloads. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1212: trigger: "..." On 2017/04/27 12:24:09, mmenke wrote: > Navigating to a URL or downloading a file. A webpage, ServiceWorker, chrome:// > page, or extension may also initiate requests in the background. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1213: data: "..." On 2017/04/27 12:24:09, mmenke wrote: > Anything the initiator wants to send. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1214: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/04/27 12:24:09, mmenke wrote: > WEBSITE, GOOGLE_OWNED_SERVICE (For things like the NTP), LOCAL (Chrome URLs), > and OTHER (for extensions) I will set it to OTHER as it supersedes the rest. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1217: cookies_allowed: false/true On 2017/04/27 12:24:09, mmenke wrote: > true Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1218: cookies_store: "..." On 2017/04/27 12:24:09, mmenke wrote: > Both the main cookie store and per-app cookie stores, for installed Apps (Which > are deprecated everywhere but ChromeOS) Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1219: setting: "..." On 2017/04/27 12:24:10, mmenke wrote: > None. If you disable these requests, Chrome will be unable to load any webpage. > There are a number of ways to disable particular types of requests, or > blacklist individual URLs or websites, but no way to disable all requests. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1223: [POLICY_NAME]: ... //(value to disable it) On 2017/04/27 12:24:09, mmenke wrote: > None, per above. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1226: policy_exception_justification: "..." On 2017/04/27 12:24:09, mmenke wrote: > Without these requests, Chrome will be unable to load any webpage. Done. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1996: net::NetworkTrafficAnnotationTag traffic_annotation = Should this annotation be the same as the other one?
LGTM https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1210: sender: "..." On 2017/04/27 12:38:16, Ramin Halavati wrote: > On 2017/04/27 12:24:09, mmenke wrote: > > Renderer Process, and occasionally the Browser Process. > > Can we call it "Resource Dispatcher Host"? The name should specify the current > module/component. Sure, if it's not supposed to have semantic meaning to people who don't work on the code, that sounds reasonable. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1996: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/04/27 12:38:16, Ramin Halavati wrote: > Should this annotation be the same as the other one? Yes. We could be more specific with these, but I see no reason to - this path is also hopefully going away within 6 months (Probably sooner, but we'll see).
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you very much Matt. Comments addressed. Dominic, Martin, Any comments? https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1210: sender: "..." On 2017/04/27 15:20:05, mmenke wrote: > On 2017/04/27 12:38:16, Ramin Halavati wrote: > > On 2017/04/27 12:24:09, mmenke wrote: > > > Renderer Process, and occasionally the Browser Process. > > > > Can we call it "Resource Dispatcher Host"? The name should specify the current > > module/component. > > Sure, if it's not supposed to have semantic meaning to people who don't work on > the code, that sounds reasonable. I think that is more covered in |description| field and this field is somewhat more general. https://codereview.chromium.org/2843403002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1996: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/04/27 15:20:05, mmenke wrote: > On 2017/04/27 12:38:16, Ramin Halavati wrote: > > Should this annotation be the same as the other one? > > Yes. We could be more specific with these, but I see no reason to - this path > is also hopefully going away within 6 months (Probably sooner, but we'll see). Acknowledged.
lgtm
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2843403002/#ps40001 (title: "Annotation updated.")
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": 1493821765439050,
"parent_rev": "d60ed8e2a67ef19c3cef5596938364dea1342b14", "commit_rev":
"d036fd9d0888b9ea96a1bb427dceff68330f7f28"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493821765439050,
"parent_rev": "5d438a6522871655c50fe0caf266887a777db20e", "commit_rev":
"d662ca6659b624722ec413933ab8295a63cd0013"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to ResourceDisaptcherHostImpl. Network traffic annotation is added to network request of: content/browser/loader/resource_dispatcher_host_impl.cc. BUG=656607 ========== to ========== Network traffic annotation added to ResourceDisaptcherHostImpl. Network traffic annotation is added to network request of: content/browser/loader/resource_dispatcher_host_impl.cc. BUG=656607 Review-Url: https://codereview.chromium.org/2843403002 Cr-Commit-Position: refs/heads/master@{#468979} Committed: https://chromium.googlesource.com/chromium/src/+/d662ca6659b624722ec413933ab8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d662ca6659b624722ec413933ab8... |
