Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728313002:
Network traffic annotation added to content_hash_fetcher.cc. (Closed)
|
Created:
3 years, 9 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, msramek Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to content_hash_fetcher.cc.
Network traffic annotation is added to network request of
content_hash_fetcher.cc.
BUG=656607
Review-Url: https://codereview.chromium.org/2728313002
Cr-Commit-Position: refs/heads/master@{#457426}
Committed: https://chromium.googlesource.com/chromium/src/+/780f5eecdbd16d97ff77304d7ccbaff2cebd3aa2
Patch Set 1 #Patch Set 2 : Annotation assigned. #
Total comments: 14
Patch Set 3 : Annotation updated. #
Total comments: 3
Patch Set 4 : Annotation updated. #Messages
Total messages: 15 (5 generated)
rhalavati@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Could you please review this or point it to someone you know familiar with the 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 content_hash_fetcher 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/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.
https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:245: net::DefineNetworkTrafficAnnotation("...", R"( ContentHashVerificationJob https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:247: sender: "..." ContentHashFetcherJob https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:248: description: "..." The request sent to retrieve the verified_contents.json file for an extension id. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:249: trigger: "..." An extension from the webstore is missing the verified_contents.json file required for extension content verification. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:250: data: "..." The extension id and extension version. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:255: setting: "..." This feature cannot be directly disabled; it is enabled if any extension from the webstore is installed in the browser. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:262: policy_exception_justification: "..." This policy only takes effect if the user has extensions from the webstore; if the user has extensions from the webstore, this feature is required to ensure the extensions match what is distributed by the store.
Annotation updated, please review. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:245: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/11 03:17:13, Devlin wrote: > ContentHashVerificationJob Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:247: sender: "..." On 2017/03/11 03:17:13, Devlin wrote: > ContentHashFetcherJob Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:248: description: "..." On 2017/03/11 03:17:13, Devlin wrote: > The request sent to retrieve the verified_contents.json file for an extension > id. Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:249: trigger: "..." On 2017/03/11 03:17:13, Devlin wrote: > An extension from the webstore is missing the verified_contents.json file > required for extension content verification. Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:250: data: "..." On 2017/03/11 03:17:13, Devlin wrote: > The extension id and extension version. Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:255: setting: "..." On 2017/03/11 03:17:13, Devlin wrote: > This feature cannot be directly disabled; it is enabled if any extension from > the webstore is installed in the browser. Done. https://codereview.chromium.org/2728313002/diff/20001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:262: policy_exception_justification: "..." On 2017/03/11 03:17:13, Devlin wrote: > This policy only takes effect if the user has extensions from the webstore; if > the user has extensions from the webstore, this feature is required to ensure > the extensions match what is distributed by the store. Done. https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:247: sender: "Content Hash Verification Job" Can this be 'Web Store' or 'Web Store Content Verification'?
https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:247: sender: "Content Hash Verification Job" On 2017/03/11 16:05:00, Ramin Halavati wrote: > Can this be 'Web Store' or 'Web Store Content Verification'? Web Store is a bit misleading - it's not the webstore sending the ping, it's chrome. "Web Store Content Verification" could potentially work. Is there guidance on what, conceptually, "sender" should be? e.g., should it be unique? Or should it intentionally try to group together similar/related requests?
Comment addressed, annotation updated, please review. https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... File extensions/browser/content_hash_fetcher.cc (right): https://codereview.chromium.org/2728313002/diff/40001/extensions/browser/cont... extensions/browser/content_hash_fetcher.cc:247: sender: "Content Hash Verification Job" On 2017/03/13 20:46:25, Devlin wrote: > On 2017/03/11 16:05:00, Ramin Halavati wrote: > > Can this be 'Web Store' or 'Web Store Content Verification'? > > Web Store is a bit misleading - it's not the webstore sending the ping, it's > chrome. > > "Web Store Content Verification" could potentially work. Is there guidance on > what, conceptually, "sender" should be? e.g., should it be unique? Or should > it intentionally try to group together similar/related requests? It doesn't need to be unique and if different annotations are from similar unit, we use a common name.
lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you. battre@, msramek@: Do you have any comments?
LGTM
The CQ bit was checked by rhalavati@chromium.org
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": 60001, "attempt_start_ts": 1489670261416490,
"parent_rev": "e5b7696cdcdf1942312eb593167e502493d59e9c", "commit_rev":
"780f5eecdbd16d97ff77304d7ccbaff2cebd3aa2"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to content_hash_fetcher.cc. Network traffic annotation is added to network request of content_hash_fetcher.cc. BUG=656607 ========== to ========== Network traffic annotation added to content_hash_fetcher.cc. Network traffic annotation is added to network request of content_hash_fetcher.cc. BUG=656607 Review-Url: https://codereview.chromium.org/2728313002 Cr-Commit-Position: refs/heads/master@{#457426} Committed: https://chromium.googlesource.com/chromium/src/+/780f5eecdbd16d97ff77304d7ccb... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/780f5eecdbd16d97ff77304d7ccb... |
