Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421333002:
Protobuf for Traffic Annotation and first use by a URLFetcher. (Closed)
|
Created:
4 years, 2 months ago by Ramin Halavati Modified:
3 years, 10 months ago CC:
cbentzel+watch_chromium.org, chromium-reviews, groby+spellwatch_chromium.org, msramek, rlp+watch_chromium.org, rouslan+spell_chromium.org, timvolodine Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProtobuf for Traffic Annotation and first use by a URLFetcher.
This CL introduces a Traffic Annotation data type that allows annotating
network traffic issued by Chrome. The annotation is expressed in such a way
that neither the network request nor the Chrome binary grow in size. Instead
the annotation is written such that a source code analysis tool can detect and
extract the information. The network request may, in the future, be extended to
store an identifier in the network request, which allows to even do runtime
analysis and auditing.
Once we settle on interfaces, the old Create() method of URLFetcher will be
deprecated and successively migrated to the annotated versio.
BUG=656607
TBR=dpranke@chromium.org
Review-Url: https://codereview.chromium.org/2421333002
Cr-Commit-Position: refs/heads/master@{#449265}
Committed: https://chromium.googlesource.com/chromium/src/+/a9b551dfda8c2231f7ba4733cdee33bc9747979b
Patch Set 1 #
Total comments: 37
Patch Set 2 : Addressed comments. #
Total comments: 30
Patch Set 3 : Comments addressed. #Patch Set 4 : Minor update. #Patch Set 5 : String hasing function simplified. #Patch Set 6 : Minor updates to annotation definition and one more sample. #Patch Set 7 : One more file added due to dependacy. #Patch Set 8 : Windows compatibility. #Patch Set 9 : Traffic annotation sourceset added to net/BUILD.gn #
Total comments: 8
Patch Set 10 : All comments addressed. #
Total comments: 16
Patch Set 11 : Comments addressed. #
Total comments: 10
Patch Set 12 : Network Traffic Annotation tag changed to const char*. #
Total comments: 23
Patch Set 13 : Comments addressed. #
Total comments: 10
Patch Set 14 : All comments addressed. #Patch Set 15 : File downloader removed. #Patch Set 16 : nits #Patch Set 17 : Minor update on BUILD.gn #Patch Set 18 : Direct dependency added to traffic_annotations #Patch Set 19 : Traffic annotation moved to net public deps #
Total comments: 2
Patch Set 20 : test_helper moved to test_support. #Patch Set 21 : More comments added. #Dependent Patchsets: Messages
Total messages: 62 (18 generated)
Description was changed from ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 ========== to ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 ==========
rhalavati@chromium.org changed reviewers: + battre@chromium.org
Please review this https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:1: // Copyright? Separate as two files one just copied things and the other ours? TODO: License must match: .*? Copyright (\(c\) )?(2016|2015|2014|2013|2012|2011|2010|2009|2008|2007|2006|2006-2008|2006-2009|2006-2010) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n
https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... File components/spellcheck/browser/spelling_service_client.cc (right): https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:113: "word, if a user right-clicks on it." Looking at this again, I think that we need to describe what happens but also what the value proposition is. description: Google Chrome can provide smarter spell-checking by sending text you type into the browser to Google's servers, allowing you to use the same spell-checking technology used by Google products, such as Docs. If the feature is enabled, Chrome will send the entire contents of text fields as you type in them to Google along with the browser’s default language. Google returns a list of suggested spellings, which will be displayed in the context menu. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:114: trigger: "User types something or asks to correct a misspelled word." trigger: User types text into a text field or asks to correct a misspelled word. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:122: "spelling errors.' in Chrome's settings under Advanced." You can enable or disable this feature via '...' .... The feature is disabled by default. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:123: policy: "SpellCheckServiceEnabled: " nit: no space after colon https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:3: #ifndef NETWORK_TRAFFIC_ANNOTATION #ifndef NET_TRAFFIC_ANNOTATION_NETWORK_TRAFFIC_ANNOTATION_H https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:6: namespace { namespace net { https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:56: // Recursive function for CRC computation comments in Chrome should end with a period (".") https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:88: // tools/traffic_annotaiton/traffic_annotation.proto typo: annotaition https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:89: constexpr NetworkTrafficAnnotationTag DefineNetworkTrafficAnnotation( // Function to convert a network traffic annotation's unique id and protobuf // text into a NetworkTrafficAnnotationTag. // // This function serves as a tag that can be discovered and extracted via // clang tools. This allows reviewing all network traffic that is generated // and annotated by Chrome. // // |unique_id| should be a string that uniquely identifies this annotation // across all of Chromium source code. // |proto| is a text-encoded NetworkTrafficAnnotation protobuf (see // tools/traffic_annotaiton/traffic_annotation.proto) // // This function should be called with inlined parameters like // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( // "unique_tag_id", // R"(semantics: {...} // policy: {...} // )"); // This allows the compiler to calculate and extract the hashed |unique_id| // at compile time without copying the entire protobuf into the text segment // of the binary or creating any runtime performance impact. // // Please do NOT use the following syntax: // const char* proto = R("..."); // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( // "unique_tag_id", proto); https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:97: DefineNetworkTrafficAnnotation("Undefined", "Nothing here yet.") new line after this. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:98: } } // namespace net https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher.h File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:105: // |d| the object that will receive the callback on fetch completion. // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below instead. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:113: // who is creating the URLFetcher. // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below instead. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:122: // |d| the object that will receive the callback on fetch completion. // |traffic_annotation| metadata about the network traffic send via this URLFetcher. See net::DefineNetworkTrafficAnnotation. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:124: // traffic annotation as a mandatory parameter. Delete this sentence. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:135: // traffic annotation as a mandatory parameter. Delete this sentence. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... net/url_request/url_request_context.h:67: std::unique_ptr<URLRequest> CreateRequest( Add // DEPRECATED. Please use the version with NetworkTrafficAnnotationTag in the future. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... net/url_request/url_request_context.h:73: // traffic annotation parameter. Delete this comment.
On 2016/10/17 14:58:46, battre wrote: > https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... > File components/spellcheck/browser/spelling_service_client.cc (right): > > https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... > components/spellcheck/browser/spelling_service_client.cc:113: "word, if a user > right-clicks on it." > Looking at this again, I think that we need to describe what happens but also > what the value proposition is. > > description: > Google Chrome can provide smarter spell-checking by sending text you type into > the browser to Google's servers, allowing you to use the same spell-checking > technology used by Google products, such as Docs. If the feature is enabled, > Chrome will send the entire contents of text fields as you type in them to > Google along with the browser’s default language. Google returns a list of > suggested spellings, which will be displayed in the context menu. > > https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... > components/spellcheck/browser/spelling_service_client.cc:114: trigger: "User > types something or asks to correct a misspelled word." > trigger: > User types text into a text field or asks to correct a misspelled word. > > https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... > components/spellcheck/browser/spelling_service_client.cc:122: "spelling errors.' > in Chrome's settings under Advanced." > You can enable or disable this feature via '...' .... The feature is disabled by > default. > > https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... > components/spellcheck/browser/spelling_service_client.cc:123: policy: > "SpellCheckServiceEnabled: " > nit: no space after colon > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > File net/traffic_annotation/network_traffic_annotation.h (right): > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:3: #ifndef > NETWORK_TRAFFIC_ANNOTATION > #ifndef NET_TRAFFIC_ANNOTATION_NETWORK_TRAFFIC_ANNOTATION_H > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:6: namespace { > namespace net { > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:56: // Recursive function > for CRC computation > comments in Chrome should end with a period (".") > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:88: // > tools/traffic_annotaiton/traffic_annotation.proto > typo: annotaition > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:89: constexpr > NetworkTrafficAnnotationTag DefineNetworkTrafficAnnotation( > // Function to convert a network traffic annotation's unique id and protobuf > // text into a NetworkTrafficAnnotationTag. > // > // This function serves as a tag that can be discovered and extracted via > // clang tools. This allows reviewing all network traffic that is generated > // and annotated by Chrome. > // > // |unique_id| should be a string that uniquely identifies this annotation > // across all of Chromium source code. > // |proto| is a text-encoded NetworkTrafficAnnotation protobuf (see > // tools/traffic_annotaiton/traffic_annotation.proto) > // > // This function should be called with inlined parameters like > // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( > // "unique_tag_id", > // R"(semantics: {...} > // policy: {...} > // )"); > // This allows the compiler to calculate and extract the hashed |unique_id| > // at compile time without copying the entire protobuf into the text segment > // of the binary or creating any runtime performance impact. > // > // Please do NOT use the following syntax: > // const char* proto = R("..."); > // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( > // "unique_tag_id", proto); > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:97: > DefineNetworkTrafficAnnotation("Undefined", "Nothing here yet.") > new line after this. > > https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... > net/traffic_annotation/network_traffic_annotation.h:98: } > } // namespace net > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher.h > File net/url_request/url_fetcher.h (right): > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... > net/url_request/url_fetcher.h:105: // |d| the object that will receive the > callback on fetch completion. > // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below > instead. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... > net/url_request/url_fetcher.h:113: // who is creating the URLFetcher. > // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below > instead. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... > net/url_request/url_fetcher.h:122: // |d| the object that will receive the > callback on fetch completion. > // |traffic_annotation| metadata about the network traffic send via this > URLFetcher. See net::DefineNetworkTrafficAnnotation. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... > net/url_request/url_fetcher.h:124: // traffic annotation as a mandatory > parameter. > Delete this sentence. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... > net/url_request/url_fetcher.h:135: // traffic annotation as a mandatory > parameter. > Delete this sentence. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... > File net/url_request/url_request_context.h (right): > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... > net/url_request/url_request_context.h:67: std::unique_ptr<URLRequest> > CreateRequest( > Add > // DEPRECATED. Please use the version with NetworkTrafficAnnotationTag in the > future. > > https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... > net/url_request/url_request_context.h:73: // traffic annotation parameter. > Delete this comment. Is there a doc describing this/how to use it/what the benefits are?
Addressed your comments. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... File components/spellcheck/browser/spelling_service_client.cc (right): https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:113: "word, if a user right-clicks on it." On 2016/10/17 14:58:45, battre wrote: > Looking at this again, I think that we need to describe what happens but also > what the value proposition is. > > description: > Google Chrome can provide smarter spell-checking by sending text you type into > the browser to Google's servers, allowing you to use the same spell-checking > technology used by Google products, such as Docs. If the feature is enabled, > Chrome will send the entire contents of text fields as you type in them to > Google along with the browser’s default language. Google returns a list of > suggested spellings, which will be displayed in the context menu. Done. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:114: trigger: "User types something or asks to correct a misspelled word." On 2016/10/17 14:58:45, battre wrote: > trigger: > User types text into a text field or asks to correct a misspelled word. Done. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:122: "spelling errors.' in Chrome's settings under Advanced." On 2016/10/17 14:58:45, battre wrote: > You can enable or disable this feature via '...' .... The feature is disabled by > default. Done. https://codereview.chromium.org/2421333002/diff/1/components/spellcheck/brows... components/spellcheck/browser/spelling_service_client.cc:123: policy: "SpellCheckServiceEnabled: " On 2016/10/17 14:58:45, battre wrote: > nit: no space after colon Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:3: #ifndef NETWORK_TRAFFIC_ANNOTATION On 2016/10/17 14:58:46, battre wrote: > #ifndef NET_TRAFFIC_ANNOTATION_NETWORK_TRAFFIC_ANNOTATION_H Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:6: namespace { On 2016/10/17 14:58:46, battre wrote: > namespace net { CRC computation code is separated from net namespace as it is only used in this file. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:56: // Recursive function for CRC computation On 2016/10/17 14:58:46, battre wrote: > comments in Chrome should end with a period (".") Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:88: // tools/traffic_annotaiton/traffic_annotation.proto On 2016/10/17 14:58:45, battre wrote: > typo: annotaition Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:89: constexpr NetworkTrafficAnnotationTag DefineNetworkTrafficAnnotation( On 2016/10/17 14:58:45, battre wrote: > // Function to convert a network traffic annotation's unique id and protobuf > // text into a NetworkTrafficAnnotationTag. > // > // This function serves as a tag that can be discovered and extracted via > // clang tools. This allows reviewing all network traffic that is generated > // and annotated by Chrome. > // > // |unique_id| should be a string that uniquely identifies this annotation > // across all of Chromium source code. > // |proto| is a text-encoded NetworkTrafficAnnotation protobuf (see > // tools/traffic_annotaiton/traffic_annotation.proto) > // > // This function should be called with inlined parameters like > // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( > // "unique_tag_id", > // R"(semantics: {...} > // policy: {...} > // )"); > // This allows the compiler to calculate and extract the hashed |unique_id| > // at compile time without copying the entire protobuf into the text segment > // of the binary or creating any runtime performance impact. > // > // Please do NOT use the following syntax: > // const char* proto = R("..."); > // NetworkTrafficAnnotationTag tag = DefineNetworkTrafficAnnotation( > // "unique_tag_id", proto); Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:97: DefineNetworkTrafficAnnotation("Undefined", "Nothing here yet.") On 2016/10/17 14:58:46, battre wrote: > new line after this. Done. https://codereview.chromium.org/2421333002/diff/1/net/traffic_annotation/netw... net/traffic_annotation/network_traffic_annotation.h:98: } On 2016/10/17 14:58:46, battre wrote: > } // namespace net Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher.h File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:105: // |d| the object that will receive the callback on fetch completion. On 2016/10/17 14:58:46, battre wrote: > // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below > instead. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:113: // who is creating the URLFetcher. On 2016/10/17 14:58:46, battre wrote: > // DEPRECATED: Please use the version with NetworkTrafficAnnotationTag below > instead. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:122: // |d| the object that will receive the callback on fetch completion. On 2016/10/17 14:58:46, battre wrote: > // |traffic_annotation| metadata about the network traffic send via this > URLFetcher. See net::DefineNetworkTrafficAnnotation. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:124: // traffic annotation as a mandatory parameter. On 2016/10/17 14:58:46, battre wrote: > Delete this sentence. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_fetcher... net/url_request/url_fetcher.h:135: // traffic annotation as a mandatory parameter. On 2016/10/17 14:58:46, battre wrote: > Delete this sentence. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... net/url_request/url_request_context.h:67: std::unique_ptr<URLRequest> CreateRequest( On 2016/10/17 14:58:46, battre wrote: > Add > // DEPRECATED. Please use the version with NetworkTrafficAnnotationTag in the > future. Done. https://codereview.chromium.org/2421333002/diff/1/net/url_request/url_request... net/url_request/url_request_context.h:73: // traffic annotation parameter. On 2016/10/17 14:58:46, battre wrote: > Delete this comment. Done.
More comments. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:11: // Section 1: Location of what is audited. please delete these lines. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:37: // needs to be put into a special whitelist file. Please delete this block. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:43: // audit policies and keep the annotation assigned to the policy even when The purpose of this ID is to give humans a chance to reference NetworkTrafficAnnotations externally even when those change a little bit (...). https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:47: // network requests by unique_id groups those policies that belong to the so that sorting all NetworkTrafficAnnotations by unique_id groups those that belong to the same component together. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:55: // network traffic. please fix line wrapping. Also below. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:73: // For whitelisted network requests in third_party that cannot be properly third_party/ https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:77: // For whitelisted network requests in third_party that cannot be properly third_party/ https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:79: // second, … annotated call. please replace ... with three proper dots. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:88: // Section 2: Meta information about the network request. Replace this just with // Meta information about the network request. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:104: // understandable by admins (ideally also users). Please avoid acronyms. Please describe the feature and the feature's value proposition as well. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:107: // to Google’s online spell checker.” please remove typographic quotes (here and elsewhere) https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:107: // to Google’s online spell checker.” Please copy the text from the spell checker annotation that is also in this CL. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:137: // Human readable description in case the destination points to OTHER. New line before this. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:170: string policy_exception_justification = 5; Please move this into TrafficPolicy. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:173: message ExtractedAuditPolicies { ExtractedNetworkTrafficAnnotation (please fix the messagese / variables below as well). Please add some comments.
Hi. > Is there a doc describing this/how to use it/what the benefits are? Please see here: go/audited-network-requests This is just the very first CL to get feedback from the network team. We'll reach out to them again, once the extractor prototype is in place as well. Best regards, Dominic
All comments addressed. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:11: // Section 1: Location of what is audited. On 2016/10/18 09:05:28, battre wrote: > please delete these lines. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:37: // needs to be put into a special whitelist file. On 2016/10/18 09:05:28, battre wrote: > Please delete this block. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:43: // audit policies and keep the annotation assigned to the policy even when On 2016/10/18 09:05:29, battre wrote: > The purpose of this ID is to give humans a chance to reference > NetworkTrafficAnnotations externally even when those change a little bit (...). Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:47: // network requests by unique_id groups those policies that belong to the On 2016/10/18 09:05:28, battre wrote: > so that sorting all NetworkTrafficAnnotations by unique_id groups those that > belong to the same component together. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:55: // network traffic. On 2016/10/18 09:05:28, battre wrote: > please fix line wrapping. Also below. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:73: // For whitelisted network requests in third_party that cannot be properly On 2016/10/18 09:05:28, battre wrote: > third_party/ Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:77: // For whitelisted network requests in third_party that cannot be properly On 2016/10/18 09:05:28, battre wrote: > third_party/ Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:79: // second, … annotated call. On 2016/10/18 09:05:28, battre wrote: > please replace ... with three proper dots. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:88: // Section 2: Meta information about the network request. On 2016/10/18 09:05:28, battre wrote: > Replace this just with // Meta information about the network request. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:104: // understandable by admins (ideally also users). Please avoid acronyms. On 2016/10/18 09:05:28, battre wrote: > Please describe the feature and the feature's value proposition as well. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:107: // to Google’s online spell checker.” On 2016/10/18 09:05:28, battre wrote: > Please copy the text from the spell checker annotation that is also in this CL. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:107: // to Google’s online spell checker.” On 2016/10/18 09:05:28, battre wrote: > please remove typographic quotes (here and elsewhere) Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:137: // Human readable description in case the destination points to OTHER. On 2016/10/18 09:05:28, battre wrote: > New line before this. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:170: string policy_exception_justification = 5; On 2016/10/18 09:05:28, battre wrote: > Please move this into TrafficPolicy. Done. https://codereview.chromium.org/2421333002/diff/20001/tools/traffic_annotatio... tools/traffic_annotation/traffic_annotation.proto:173: message ExtractedAuditPolicies { On 2016/10/18 09:05:28, battre wrote: > ExtractedNetworkTrafficAnnotation (please fix the messagese / variables below as > well). > > Please add some comments. Done.
rhalavati@chromium.org changed reviewers: + asanka@chromium.org - battre@chromium.org
Hi, This is the primary change list for "Network Traffic Annotation" project, here is the description: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5Numh.... Could you please review it? The main design is in "network_traffic_annotation.h" and "traffic_annotation.proto", changes in net layer are in "url_fetcher" and "url_request_context" files, and the others files are sample usages.
rhalavati@chromium.org changed reviewers: + battre@chromium.org
battre@chromium.org: Please review changes in protobuf and downloader sample.
https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:61: // has annotation, the net::TCPClientSocket() that is used by the nit: has an annotation https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:102: // engine) would be considered websites. I would change this: A website the user visit or interacts with. The distinction from a google owned service can be difficult when the user navigates to google.com or searches with the omnibar. Therefore follow the following guideline: If the source code has hardcoded that the request goes to Google (e.g. for ZeroSuggest), use GOOGLE_OWNED_SERVICE. If the request can go to other domains and is perceived as a part of a website rather than a native browser feature, use WEBSITE. In other cases use OTHER. https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:106: // Other endpoints, e.g. a service hosting a PAC script Other endpoints, e.g. a service hosting a PAC script. In case of doubt, use this category. We will audit it in the future to see whether we need more categories. OTHER = 1000; https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:138: // TODO: repeated enterprise_management.CloudPolicySettings policy = 4; Please try to fix this.
All Comments addressed. Please review. https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:61: // has annotation, the net::TCPClientSocket() that is used by the On 2017/01/19 08:10:15, battre wrote: > nit: has an annotation Done. https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:102: // engine) would be considered websites. On 2017/01/19 08:10:15, battre wrote: > I would change this: > > A website the user visit or interacts with. The distinction from a google owned > service can be difficult when the user navigates to http://google.com or searches with > the omnibar. Therefore follow the following guideline: If the source code has > hardcoded that the request goes to Google (e.g. for ZeroSuggest), use > GOOGLE_OWNED_SERVICE. If the request can go to other domains and is perceived as > a part of a website rather than a native browser feature, use WEBSITE. In other > cases use OTHER. Done. https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:106: // Other endpoints, e.g. a service hosting a PAC script On 2017/01/19 08:10:15, battre wrote: > Other endpoints, e.g. a service hosting a PAC script. In case of doubt, use this > category. We will audit it in the future to see whether we need more categories. > > OTHER = 1000; Done. https://codereview.chromium.org/2421333002/diff/160001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:138: // TODO: repeated enterprise_management.CloudPolicySettings policy = 4; On 2017/01/19 08:10:15, battre wrote: > Please try to fix this. Done.
tools/traffic_annotation LGTM
Quick uneducated pass. I'll do another after I chat with rhalavati@ about the overall approach. https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn#newcode2967 net/BUILD.gn:2967: source_set("traffic_annotation_header") { why is this in a separate source set when url_fetcher.h and url_request_context.h depends on it? https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:30: typedef const unsigned int NetworkTrafficAnnotationTag; using NetworkTrafficAnnotationTag = ... Also, use a fixed sized type if you want this tag to be comparable across platforms. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:59: const char* unique_id, Actually, if the intention is to introduce a parameter that can: * Be passed around by value. * Can only be created by DefineNetworkTrafficAnnotation() * Doesn't depend on |proto| so that the compiler can optimize out the proto then why not just introduce some class with a private constexpr constructor that's a friend of DefineNetworkTrafficAnnotation()? It doesn't need to contain anything unique or significant. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:61: return (NetworkTrafficAnnotationTag)COMPUTE_STRING_HASH(unique_id); sizeof(unique_id) == sizeof(const char*) here. https://codereview.chromium.org/2421333002/diff/180001/net/url_request/url_fe... File net/url_request/url_fetcher.cc (right): https://codereview.chromium.org/2421333002/diff/180001/net/url_request/url_fe... net/url_request/url_fetcher.cc:41: // code analysis and can be ignored here. unnecessary since we are passing |traffic_annotation| along.
I have addressed all comments Asanka, please review. https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn#newcode2967 net/BUILD.gn:2967: source_set("traffic_annotation_header") { On 2017/01/26 20:59:27, asanka wrote: > why is this in a separate source set when url_fetcher.h and > url_request_context.h depends on it? I separated it so that tools that just use annotation definition, like the ones that analyze annotations, create reports, check policy definitions, etc., won't need to define a dependency on the whole net. But it can be done the other way as well if you see that a better choice. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:30: typedef const unsigned int NetworkTrafficAnnotationTag; On 2017/01/26 20:59:28, asanka wrote: > using NetworkTrafficAnnotationTag = ... > > Also, use a fixed sized type if you want this tag to be comparable across > platforms. Done. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:59: const char* unique_id, On 2017/01/26 20:59:28, asanka wrote: > Actually, if the intention is to introduce a parameter that can: > * Be passed around by value. > * Can only be created by DefineNetworkTrafficAnnotation() > * Doesn't depend on |proto| so that the compiler can optimize out the proto > > then why not just introduce some class with a private constexpr constructor > that's a friend of DefineNetworkTrafficAnnotation()? It doesn't need to contain > anything unique or significant. |unique_id|s have two purposes here: 1- They stay in the binary and can be used for auditing purposes and reporting. 2- They remain constant even if the content of proto changes in later revisions and make reviews and tracking changes easier. In this respect, I don't see why defining a class would be a better choice. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:61: return (NetworkTrafficAnnotationTag)COMPUTE_STRING_HASH(unique_id); On 2017/01/26 20:59:28, asanka wrote: > sizeof(unique_id) == sizeof(const char*) here. Yes, but we want the |unique_id| to be kept in the binary and the text to be removed by the optimizer. https://codereview.chromium.org/2421333002/diff/180001/net/url_request/url_fe... File net/url_request/url_fetcher.cc (right): https://codereview.chromium.org/2421333002/diff/180001/net/url_request/url_fe... net/url_request/url_fetcher.cc:41: // code analysis and can be ignored here. On 2017/01/26 20:59:28, asanka wrote: > unnecessary since we are passing |traffic_annotation| along. Done.
Overall I'm strongly leaning towards just storing string a pointer to the string rather than a hash. Also, can you add some guidance for generating, updating, formatting, and validating the proto? Would it make sense to embed the text proto in a block comment instead of a raw string literal? I'm trying to picture the task of someone trying to write it by hand and I don't think our tools are going to help them much. Perhaps the documentation for DefineNetworkTrafficAnnotation could include a couple of easy to copy & paste templates that are well documented. https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn#newcode2967 net/BUILD.gn:2967: source_set("traffic_annotation_header") { On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > On 2017/01/26 20:59:27, asanka wrote: > > why is this in a separate source set when url_fetcher.h and > > url_request_context.h depends on it? > > I separated it so that tools that just use annotation definition, like the ones that analyze annotations, create reports, check policy definitions, etc., won't need to define a dependency on the whole net. But it can be done the other way as well if you see that a better choice. Got it. Can you add :traffic_annotation_header (or rename it to traffic_annotation since the fact that it's a header isn't important to net) to the |deps| of the source sets that depend on it? https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:59: const char* unique_id, On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > On 2017/01/26 20:59:28, asanka wrote: > > Actually, if the intention is to introduce a parameter that can: > > * Be passed around by value. > > * Can only be created by DefineNetworkTrafficAnnotation() > > * Doesn't depend on |proto| so that the compiler can optimize out the proto > > > > then why not just introduce some class with a private constexpr constructor > > that's a friend of DefineNetworkTrafficAnnotation()? It doesn't need to contain > > anything unique or significant. > > |unique_id|s have two purposes here: > 1- They stay in the binary and can be used for auditing purposes and reporting. > 2- They remain constant even if the content of proto changes in later revisions and make reviews and tracking changes easier. > > In this respect, I don't see why defining a class would be a better choice. Using typed ints (a.la enum NetworkTrafficAnnotationTag : uint32_t {}) or a POD class with a single uint32_t member would make it less possible to pass in, say, 0 as the tag. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:61: return (NetworkTrafficAnnotationTag)COMPUTE_STRING_HASH(unique_id); On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > On 2017/01/26 20:59:28, asanka wrote: > > sizeof(unique_id) == sizeof(const char*) here. > > Yes, but we want the |unique_id| to be kept in the binary and the text to be removed by the optimizer. Sure, but what I was pointing out was that you are only hashing size(const char*) bytes of the |unique_id|. sizeof(unique_id) doesn't get you the length of the string. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:7: #include <stdint.h> https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:8: namespace { I know we aren't adding any .text objects here, but let's call this 'internal' and move it inside the net namespace. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:13: return (unsigned int)(recursive_hash<N - 1>(str) * 31 + str[N]) % 138003713; The use of the modulus here is just truncating the domain at 138M instead of letting it grow to 4B. Why? https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:14: } This implementation will always result in a hash that's a multiple of 31 if N starts at sizeof(s)-1 and OOB if N starts at sizeof(s). https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:23: #define COMPUTE_STRING_HASH(S) recursive_hash<sizeof(S)>(S) #include "base/macros.h" and use arraysize() instead? That would catch cases where S has decayed.
Asanka, Dominic, please review. Network traffic annotation tag is changed to const char * and all hash related parts are removed. A sample and a template are added in a new file in tools/traffic_annotation folder besides the proto file. In the next steps we can modify presubmit scripts so that if it finds that a modified file includes the annotation text, it calls a clang tool to check for correct annotation and unique id. https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/180001/net/BUILD.gn#newcode2967 net/BUILD.gn:2967: source_set("traffic_annotation_header") { On 2017/01/27 16:58:00, asanka wrote: > On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > > On 2017/01/26 20:59:27, asanka wrote: > > > why is this in a separate source set when url_fetcher.h and > > > url_request_context.h depends on it? > > > > I separated it so that tools that just use annotation definition, like the > ones that analyze annotations, create reports, check policy definitions, etc., > won't need to define a dependency on the whole net. But it can be done the other > way as well if you see that a better choice. > > Got it. > > Can you add :traffic_annotation_header (or rename it to traffic_annotation since > the fact that it's a header isn't important to net) to the |deps| of the source > sets that depend on it? Done, renamed it to traffic_annotation and added it "net" component" which was the only source set having url_fetcher or url_requestion_context. https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:59: const char* unique_id, On 2017/01/27 16:58:00, asanka wrote: > On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > > On 2017/01/26 20:59:28, asanka wrote: > > > Actually, if the intention is to introduce a parameter that can: > > > * Be passed around by value. > > > * Can only be created by DefineNetworkTrafficAnnotation() > > > * Doesn't depend on |proto| so that the compiler can optimize out the proto > > > > > > then why not just introduce some class with a private constexpr constructor > > > that's a friend of DefineNetworkTrafficAnnotation()? It doesn't need to > contain > > > anything unique or significant. > > > > |unique_id|s have two purposes here: > > 1- They stay in the binary and can be used for auditing purposes and > reporting. > > 2- They remain constant even if the content of proto changes in later > revisions and make reviews and tracking changes easier. > > > > In this respect, I don't see why defining a class would be a better choice. > > Using typed ints (a.la enum NetworkTrafficAnnotationTag : uint32_t {}) or a POD > class with a single uint32_t member would make it less possible to pass in, say, > 0 as the tag. I am not sure if I got your concern clearly. Now that annotation tag is changed to "const char *", is it still an issue? https://codereview.chromium.org/2421333002/diff/180001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:61: return (NetworkTrafficAnnotationTag)COMPUTE_STRING_HASH(unique_id); On 2017/01/27 16:58:00, asanka wrote: > On 2017/01/27 at 07:48:58, Ramin Halavati wrote: > > On 2017/01/26 20:59:28, asanka wrote: > > > sizeof(unique_id) == sizeof(const char*) here. > > > > Yes, but we want the |unique_id| to be kept in the binary and the text to be > removed by the optimizer. > > Sure, but what I was pointing out was that you are only hashing size(const > char*) bytes of the |unique_id|. sizeof(unique_id) doesn't get you the length of > the string. Acknowledged. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:7: On 2017/01/27 16:58:01, asanka wrote: > #include <stdint.h> The type and unique id are now changed to const char * and all this part is removed. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:8: namespace { On 2017/01/27 16:58:01, asanka wrote: > I know we aren't adding any .text objects here, but let's call this 'internal' > and move it inside the net namespace. Acknowledged. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:13: return (unsigned int)(recursive_hash<N - 1>(str) * 31 + str[N]) % 138003713; On 2017/01/27 16:58:01, asanka wrote: > The use of the modulus here is just truncating the domain at 138M instead of > letting it grow to 4B. Why? Acknowledged. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:14: } On 2017/01/27 16:58:01, asanka wrote: > This implementation will always result in a hash that's a multiple of 31 if N > starts at sizeof(s)-1 and OOB if N starts at sizeof(s). Acknowledged. https://codereview.chromium.org/2421333002/diff/200001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:23: #define COMPUTE_STRING_HASH(S) recursive_hash<sizeof(S)>(S) On 2017/01/27 16:58:01, asanka wrote: > #include "base/macros.h" and use arraysize() instead? That would catch cases > where S has decayed. Acknowledged.
My comments are mostly nits and questions. I think at this point we can land and iterate. I'm guessing we are not going to commit to an interface until we've looked further into how the annotation is going to be used. LGTM. https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:40: // (tools/traffic_annotaiton/sample_traffic_annotation.h) spelling https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/sample_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:5: // This file includes a sample and a template for text-coded traffic_annotation. This is a sample file. Let's make it a .cc and have a concrete call to net::DefineNetworkTrafficAnnotation() instead of a comment block. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:6: // For more description on each filed, please refer to *field https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:53: // policy { I'm still a bit unclear on how this policy is checked or enforced. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:58: // [POLICY_NAME] { Does this mean that each network traffic origin needs to introduce a policy for controlling it? Or is this entirely optional? https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:44: int32 line = 3; How stable do you want "TrafficSource" to be? What happens when code moves around? Typically, the binaries that enterprises etc end up using are based on source that can look substantially different from ToT. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:125: // least one is correct). Are there things other than cookies and Channel IDs that need to be described here? https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:131: // exceptions (e.g. SafeBrowsing uses a separate cookie store). Rephrase? We are really asking "which cookie jar are you using? The user's?"
https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... net/url_request/url_fetcher.h:110: // below instead. Shouldn't we remove comments about this being deprecated? I think that for cronet users that are not Chrome, this is perfectly legit to use. https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... net/url_request/url_request_context.h:76: // future. I think that this should not be deprecated. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/sample_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:53: // policy { On 2017/02/01 02:28:28, asanka wrote: > I'm still a bit unclear on how this policy is checked or enforced. For now this is less of an enforcement and more an explanation to the admin how to disable the component that triggers the request. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:58: // [POLICY_NAME] { On 2017/02/01 02:28:28, asanka wrote: > Does this mean that each network traffic origin needs to introduce a policy for > controlling it? Or is this entirely optional? In the first pass this is optional if no matching policy exists. Then we want to check whether we need to build new policies. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:44: int32 line = 3; On 2017/02/01 02:28:28, asanka wrote: > How stable do you want "TrafficSource" to be? What happens when code moves > around? > > Typically, the binaries that enterprises etc end up using are based on source > that can look substantially different from ToT. I think it is ok if these numbers are a bit volatile. You can always check out the right branch and see what was there at the time the binary was created.
rhalavati@chromium.org changed reviewers: + rouslan@chromium.org
Comments addressed. battre@: Do you have any suggestions for asanka@'s comments on cookies in TrafficPolicy segment of protobug? Also please suggest annotation for FileDownloader. rouslan@: We are annotating all network requests in Chromium with the purposes of making auditing and policy enforcement possible. I've modified 3 files related to spell checking and added annotation to them, please review them. For more details, you can also see: https://docs.google.com/document/d/1MsJGxfSJYKHWZHFSG5fUZU-YpW_AR8Vwi9zf_A7cpOM xsrf_token: 4a1f43e1d9452a38de9cda03c52dc5ca https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:40: // (tools/traffic_annotaiton/sample_traffic_annotation.h) On 2017/02/01 02:28:28, asanka wrote: > spelling Done. https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... File net/url_request/url_fetcher.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... net/url_request/url_fetcher.h:110: // below instead. On 2017/02/01 03:48:24, battre (OOO) wrote: > Shouldn't we remove comments about this being deprecated? I think that for > cronet users that are not Chrome, this is perfectly legit to use. Done. https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... File net/url_request/url_request_context.h (right): https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... net/url_request/url_request_context.h:76: // future. On 2017/02/01 03:48:24, battre (OOO) wrote: > I think that this should not be deprecated. Done. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/sample_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:5: // This file includes a sample and a template for text-coded traffic_annotation. On 2017/02/01 02:28:28, asanka wrote: > This is a sample file. Let's make it a .cc and have a concrete call to > net::DefineNetworkTrafficAnnotation() instead of a comment block. Done. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/sample_traffic_annotation.h:6: // For more description on each filed, please refer to On 2017/02/01 02:28:28, asanka wrote: > *field Done. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:44: int32 line = 3; On 2017/02/01 03:48:24, battre (OOO) wrote: > On 2017/02/01 02:28:28, asanka wrote: > > How stable do you want "TrafficSource" to be? What happens when code moves > > around? > > > > Typically, the binaries that enterprises etc end up using are based on source > > that can look substantially different from ToT. > > I think it is ok if these numbers are a bit volatile. You can always check out > the right branch and see what was there at the time the binary was created. Also note that this part is automatically set by the clang tool and the developer doesn't need to enter or update it. The tool extracts the line number applying all build settings.
On 2017/02/01 10:11:14, Ramin Halavati wrote: > Comments addressed. > > battre@: Do you have any suggestions for asanka@'s comments on cookies in > TrafficPolicy segment of protobug? Also please suggest annotation for > FileDownloader. > > rouslan@: > We are annotating all network requests in Chromium with the purposes of making > auditing and policy enforcement possible. I've modified 3 files related to spell > checking and added annotation to them, please review them. > > For more details, you can also see: > https://docs.google.com/document/d/1MsJGxfSJYKHWZHFSG5fUZU-YpW_AR8Vwi9zf_A7cpOM > xsrf_token: 4a1f43e1d9452a38de9cda03c52dc5ca > > https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... > File net/traffic_annotation/network_traffic_annotation.h (right): > > https://codereview.chromium.org/2421333002/diff/220001/net/traffic_annotation... > net/traffic_annotation/network_traffic_annotation.h:40: // > (tools/traffic_annotaiton/sample_traffic_annotation.h) > On 2017/02/01 02:28:28, asanka wrote: > > spelling > > Done. > > https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... > File net/url_request/url_fetcher.h (right): > > https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_fe... > net/url_request/url_fetcher.h:110: // below instead. > On 2017/02/01 03:48:24, battre (OOO) wrote: > > Shouldn't we remove comments about this being deprecated? I think that for > > cronet users that are not Chrome, this is perfectly legit to use. > > Done. > > https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... > File net/url_request/url_request_context.h (right): > > https://codereview.chromium.org/2421333002/diff/220001/net/url_request/url_re... > net/url_request/url_request_context.h:76: // future. > On 2017/02/01 03:48:24, battre (OOO) wrote: > > I think that this should not be deprecated. > > Done. > > https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... > File tools/traffic_annotation/sample_traffic_annotation.h (right): > > https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... > tools/traffic_annotation/sample_traffic_annotation.h:5: // This file includes a > sample and a template for text-coded traffic_annotation. > On 2017/02/01 02:28:28, asanka wrote: > > This is a sample file. Let's make it a .cc and have a concrete call to > > net::DefineNetworkTrafficAnnotation() instead of a comment block. > > Done. > > https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... > tools/traffic_annotation/sample_traffic_annotation.h:6: // For more description > on each filed, please refer to > On 2017/02/01 02:28:28, asanka wrote: > > *field > > Done. > > https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... > File tools/traffic_annotation/traffic_annotation.proto (right): > > https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... > tools/traffic_annotation/traffic_annotation.proto:44: int32 line = 3; > On 2017/02/01 03:48:24, battre (OOO) wrote: > > On 2017/02/01 02:28:28, asanka wrote: > > > How stable do you want "TrafficSource" to be? What happens when code moves > > > around? > > > > > > Typically, the binaries that enterprises etc end up using are based on > source > > > that can look substantially different from ToT. > > > > I think it is ok if these numbers are a bit volatile. You can always check out > > the right branch and see what was there at the time the binary was created. > > Also note that this part is automatically set by the clang tool and the > developer doesn't need to enter or update it. The tool extracts the line number > applying all build settings. Sorry, sent a wrong link for more information. This is the correct one: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU
https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:125: // least one is correct). On 2017/02/01 02:28:28, asanka wrote: > Are there things other than cookies and Channel IDs that need to be described > here? For the time being, I am not aware of anything. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:131: // exceptions (e.g. SafeBrowsing uses a separate cookie store). On 2017/02/01 02:28:28, asanka wrote: > Rephrase? > > We are really asking "which cookie jar are you using? The user's?" // If a request sends or stores cookies/channel IDs/... (i.e. if cookies_allowed is true), we want to know which cookie store is being used. The answer to this question can typically be derived from the URLRequestContext that is being used. The three most common cases will be: - If cookies_allowed is false, leave this field unset. - If the profile's default URLRequestContext is being used (e.g. from Profile::GetRequestContext()), this means that the user's normal cookies are sent. In this case, put "user" here. - If the system URLRequestContext is being used (for example via io_thread()->system_url_request_context_getter()), put "system" here. Otherwise, please explain (e.g. SafeBrowsing uses a separate cookie store). How about this and then renaming cookie_store_exceptions to cookie_store? https://codereview.chromium.org/2421333002/diff/240001/chrome/browser/net/fil... File chrome/browser/net/file_downloader.cc (right): https://codereview.chromium.org/2421333002/diff/240001/chrome/browser/net/fil... chrome/browser/net/file_downloader.cc:30: net::NetworkTrafficAnnotationTag traffic_annotation = FileDownloader should take a net::NetworkTrafficAnnotationTag as a parameter because it can be used as a network abstraction by different components. I suggest to remove it from this CL.
Also, the sample annotation file is missing. https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:131: // exceptions (e.g. SafeBrowsing uses a separate cookie store). On 2017/02/01 at 16:25:57, battre (OOO) wrote: > On 2017/02/01 02:28:28, asanka wrote: > > Rephrase? > > > > We are really asking "which cookie jar are you using? The user's?" > > // If a request sends or stores cookies/channel IDs/... (i.e. if cookies_allowed is true), we want to know which cookie store is being used. The answer to this question can typically be derived from the URLRequestContext that is being used. > > The three most common cases will be: > - If cookies_allowed is false, leave this field unset. > - If the profile's default URLRequestContext is being used (e.g. from Profile::GetRequestContext()), this means that the user's normal cookies are sent. In this case, put "user" here. > - If the system URLRequestContext is being used (for example via io_thread()->system_url_request_context_getter()), put "system" here. > Otherwise, please explain (e.g. SafeBrowsing uses a separate cookie store). > > > How about this and then renaming cookie_store_exceptions to cookie_store? SGTM
spellcheck lgtm. other files: drive by. https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. No "(c)" and 2017. https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:48: #define NO_TRAFFIC_ANNOTATION_YET \ I'd prefer to move this #define outside of the "net" namespace. This will clarify to the reader that this is a global name. (#define does not follow namespace scoping.) https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:52: #define TRAFFIC_ANNOTATION_FOR_TESTS \ Nit: Would be nice to move to network_traffic_annotation_test_helper.h or something similar to avoid polluting global namespace. (#define does not follow namespace scoping.) https://codereview.chromium.org/2421333002/diff/240001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/240001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:1: syntax = "proto3"; Need a license, I think.
rhalavati@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org: Please review changes in tools/traffic_annotation https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/220001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:131: // exceptions (e.g. SafeBrowsing uses a separate cookie store). On 2017/02/01 16:25:57, battre (OOO) wrote: > On 2017/02/01 02:28:28, asanka wrote: > > Rephrase? > > > > We are really asking "which cookie jar are you using? The user's?" > > // If a request sends or stores cookies/channel IDs/... (i.e. if cookies_allowed > is true), we want to know which cookie store is being used. The answer to this > question can typically be derived from the URLRequestContext that is being used. > > The three most common cases will be: > - If cookies_allowed is false, leave this field unset. > - If the profile's default URLRequestContext is being used (e.g. from > Profile::GetRequestContext()), this means that the user's normal cookies are > sent. In this case, put "user" here. > - If the system URLRequestContext is being used (for example via > io_thread()->system_url_request_context_getter()), put "system" here. > Otherwise, please explain (e.g. SafeBrowsing uses a separate cookie store). > > > How about this and then renaming cookie_store_exceptions to cookie_store? Done. https://codereview.chromium.org/2421333002/diff/240001/chrome/browser/net/fil... File chrome/browser/net/file_downloader.cc (right): https://codereview.chromium.org/2421333002/diff/240001/chrome/browser/net/fil... chrome/browser/net/file_downloader.cc:30: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/02/01 16:25:58, battre (OOO) wrote: > FileDownloader should take a net::NetworkTrafficAnnotationTag as a parameter > because it can be used as a network abstraction by different components. I > suggest to remove it from this CL. Done. https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/02/01 22:10:20, rouslan wrote: > No "(c)" and 2017. Done. https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:48: #define NO_TRAFFIC_ANNOTATION_YET \ On 2017/02/01 22:10:20, rouslan wrote: > I'd prefer to move this #define outside of the "net" namespace. This will > clarify to the reader that this is a global name. (#define does not follow > namespace scoping.) Done. https://codereview.chromium.org/2421333002/diff/240001/net/traffic_annotation... net/traffic_annotation/network_traffic_annotation.h:52: #define TRAFFIC_ANNOTATION_FOR_TESTS \ On 2017/02/01 22:10:20, rouslan wrote: > Nit: Would be nice to move to network_traffic_annotation_test_helper.h or > something similar to avoid polluting global namespace. (#define does not follow > namespace scoping.) Done. https://codereview.chromium.org/2421333002/diff/240001/tools/traffic_annotati... File tools/traffic_annotation/traffic_annotation.proto (right): https://codereview.chromium.org/2421333002/diff/240001/tools/traffic_annotati... tools/traffic_annotation/traffic_annotation.proto:1: syntax = "proto3"; On 2017/02/01 22:10:20, rouslan wrote: > Need a license, I think. Done.
Dirk, Please just review the ownership of tools/traffic_annotation.
Description was changed from ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 ========== to ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 TBR=dpranke@chromium.org ==========
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from battre@chromium.org, asanka@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2421333002/#ps300001 (title: "nits")
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
Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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, rouslan@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/2421333002/#ps320001 (title: "Minor update on BUILD.gn")
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
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
rouslan@, asanka@,
I tried to land this CL and got the following two commit errors and made two
changes to the two BUILD.gn files. Please review them.
ERROR at //components/spellcheck/browser/spelling_service_client.h:16:11: Can't
include this header from here.
#include "net/traffic_annotation/network_traffic_annotation.h"
^--------------------------------------------------
The target:
//components/spellcheck/browser:browser
is including a file from the target:
//net:traffic_annotation
It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.
Dependency chain (there may also be others):
//components/spellcheck/browser:browser -->
//net:net --[private]-->
//net:traffic_annotation
ERROR at //net/BUILD.gn:2979:1: Source file not found.
source_set("traffic_annotation") {
^---------------------------------
The target:
//net:traffic_annotation
has a source file:
//net/net/traffic_annotation/network_traffic_annotation.h
which was not found.
___________________
ERROR at //net/BUILD.gn:2979:1: Source file not found.
source_set("traffic_annotation") {
^---------------------------------
The target:
//net:traffic_annotation
has a source file:
//net/net/traffic_annotation/network_traffic_annotation_test_helper.h
which was not found.
msramek@chromium.org changed reviewers: + msramek@chromium.org
Drive-by! Since our goal is to ultimately annotate all requests, maybe we should instead make traffic_annotation a public dependency of //net.
On 2017/02/06 15:03:14, Ramin Halavati wrote:
> rouslan@, asanka@,
>
> I tried to land this CL and got the following two commit errors and made two
> changes to the two BUILD.gn files. Please review them.
>
>
> ERROR at //components/spellcheck/browser/spelling_service_client.h:16:11:
Can't
> include this header from here.
> #include "net/traffic_annotation/network_traffic_annotation.h"
> ^--------------------------------------------------
> The target:
> //components/spellcheck/browser:browser
> is including a file from the target:
> //net:traffic_annotation
>
> It's usually best to depend directly on the destination target.
> In some cases, the destination target is considered a subcomponent
> of an intermediate target. In this case, the intermediate target
> should depend publicly on the destination to forward the ability
> to include headers.
>
> Dependency chain (there may also be others):
> //components/spellcheck/browser:browser -->
> //net:net --[private]-->
> //net:traffic_annotation
>
>
> ERROR at //net/BUILD.gn:2979:1: Source file not found.
> source_set("traffic_annotation") {
> ^---------------------------------
> The target:
> //net:traffic_annotation
> has a source file:
> //net/net/traffic_annotation/network_traffic_annotation.h
> which was not found.
> ___________________
> ERROR at //net/BUILD.gn:2979:1: Source file not found.
> source_set("traffic_annotation") {
> ^---------------------------------
> The target:
> //net:traffic_annotation
> has a source file:
> //net/net/traffic_annotation/network_traffic_annotation_test_helper.h
> which was not found.
https://cs.chromium.org/chromium/src/components/spellcheck/browser/DEPS should
include //net/traffic_annotation, looks like.
I'm a bit lost here. What is the code in //tools doing (or being used for)?
asanka@: I moved Traffic Annotation to public dependency of net BUILD.gn so that we won't need to directly specify it in all usages. Do you agree? dpranke@: I will wait for your approval to land.
On 2017/02/07 10:22:16, Ramin Halavati wrote: > dpranke@: > I will wait for your approval to land. Thanks for the background info. I'm still a bit puzzled by why this is in //tools rather than //net/tools or (some parts of it, at least) in //net/docs ?
https://codereview.chromium.org/2421333002/diff/360001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/360001/net/BUILD.gn#newcode2982 net/BUILD.gn:2982: "traffic_annotation/network_traffic_annotation_test_helper.h", Can test_helper be moved to :test_support ?
On 2017/02/08 02:06:22, Dirk Pranke wrote: > On 2017/02/07 10:22:16, Ramin Halavati wrote: > > dpranke@: > > I will wait for your approval to land. > > Thanks for the background info. I'm still a bit puzzled by why this is in > //tools rather than //net/tools or (some parts of it, at least) in //net/docs ? The concepts in the proto (talking about policies and settings, the components, etc.) are too specific to put them into //net (which is also released as cronet) and would be a layering violation. We don't want //net to depend on //components/policy. The future for this directory is to add more things besides the current protobuf: - Extraction of annotations with a clang plugin. - Various kinds of renderings of the extracted annotations (as another protobuf, as a website, ...) - A script to whitelist OS specific network calls (socket(), open(), ...) - Helper scripts for presubmit scripts (Just a list of things that we need to put somewhere, maybe some of that should live in different places)
asanka@: Comment addressed, please review. https://codereview.chromium.org/2421333002/diff/360001/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/2421333002/diff/360001/net/BUILD.gn#newcode2982 net/BUILD.gn:2982: "traffic_annotation/network_traffic_annotation_test_helper.h", On 2017/02/08 04:37:16, asanka wrote: > Can test_helper be moved to :test_support ? Done.
//net lgtm But things I'd like to see going forward (in this CL or later as we flesh things out): * At the //net layer, document that - the annotation tag is opaque to the network stack and its only purpose is to remain associated with, and be visible from the URLRequest throughout its lifetime. - the annotation itself will be compiled away and not available at runtime. Consumers can look at //tools/foo for an example of how to extract and validate the contents. Different embedders should be able to decide on the format of the annotation given that //net doesn't care. - call sites that get compiled into //chrome targets should adhere to the format described in //tools/foo. This should be enforced via a clang plugin or somesuch. * Have a set of tools in //tools/foo for validating and formatting the proto. Issues with the proto should be discoverable without scheduling a dry run on Reitveld.
It still feels a little weird to be putting this in //tools, but I still don't totally understand this, either. But, my understanding isn't that important and I don't want to block things, if others are fine with the location, so lgtm .
On 2017/02/09 at 02:46:18, dpranke wrote: > It still feels a little weird to be putting this in //tools, but I still don't totally understand this, either. > > But, my understanding isn't that important and I don't want to block things, if others are fine with > the location, so lgtm . Perhaps something like this? - //net provides the API for tagging requests with an opaque identifier. - //chrome/... contains the Chrome specific .proto describing the verbose annotation format that Chrome's callsites are expected to follow. - //tools/traffic_annotation/ contains the tools for extracting/validating such annotations based on an arbitrary .proto. I don't have strong feelings about this yet since I don't know what the tools are going to look like. But if it's going to be Clang plugins etc that are going to be applied all over the entire codebase, then perhaps //tools is the right place for it.
asanka@: Proposed comments added to both citations of network traffic annotation in net, and a bug is filed and assigned for adding format checking tools during presubmit (crbug.com/690323). I will proceed with landing.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, battre@chromium.org, asanka@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2421333002/#ps400001 (title: "More comments added.")
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": 400001, "attempt_start_ts": 1486637937249210,
"parent_rev": "2d9411beed1c3a4d3956a11d834c32da02b85481", "commit_rev":
"a9b551dfda8c2231f7ba4733cdee33bc9747979b"}
Message was sent while issue was closed.
Description was changed from ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 TBR=dpranke@chromium.org ========== to ========== Protobuf for Traffic Annotation and first use by a URLFetcher. This CL introduces a Traffic Annotation data type that allows annotating network traffic issued by Chrome. The annotation is expressed in such a way that neither the network request nor the Chrome binary grow in size. Instead the annotation is written such that a source code analysis tool can detect and extract the information. The network request may, in the future, be extended to store an identifier in the network request, which allows to even do runtime analysis and auditing. Once we settle on interfaces, the old Create() method of URLFetcher will be deprecated and successively migrated to the annotated versio. BUG=656607 TBR=dpranke@chromium.org Review-Url: https://codereview.chromium.org/2421333002 Cr-Commit-Position: refs/heads/master@{#449265} Committed: https://chromium.googlesource.com/chromium/src/+/a9b551dfda8c2231f7ba4733cdee... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/a9b551dfda8c2231f7ba4733cdee... |
