Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(433)

Issue 2869373002: Partial Protobuf for Traffic Annotation. (Closed)

Created:
3 years, 7 months ago by Ramin Halavati
Modified:
3 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Partial Protobuf for Traffic Annotation. In many use cases, it's better to define some part of a network traffic annotation in one function and the reset of it in another one. like when a function makes the initial request and controls policies, and then passes it to another one to set/unset the cookies and perform the actual request. This CL adds required definitions to make partial annotations. BUG=656607 patch from issue 2815293002 at patchset 160001 (http://crrev.com/2815293002#ps160001) Review-Url: https://codereview.chromium.org/2869373002 Cr-Commit-Position: refs/heads/master@{#474602} Committed: https://chromium.googlesource.com/chromium/src/+/99c6fcef9cff05b0df3a536623f8d8251c2f2a6b

Patch Set 1 #

Patch Set 2 : Types changed to struct. #

Patch Set 3 : Partial Test tag added. #

Patch Set 4 : Runtime checks added for partial annotations. #

Total comments: 4

Patch Set 5 : Comments addressed. #

Patch Set 6 : unique ids changed to hash codes. #

Patch Set 7 : Hash function changed to avoid overflow. #

Patch Set 8 : Hashed unique ids added to auditor outputs. #

Patch Set 9 : Merged. #

Total comments: 13

Patch Set 10 : Comments addressed. #

Total comments: 5

Patch Set 11 : Comments addressed. #

Total comments: 13

Patch Set 12 : Comments addressed. #

Patch Set 13 : Hash function updated. New report added. #

Total comments: 5

Patch Set 14 : Comment addressed, logging header added. #

Patch Set 15 : net/BUILD.gn updated. #

Patch Set 16 : Type changed to int32, Names changed. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+581 lines, -80 lines) Patch
M net/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -1 line 0 comments Download
M net/traffic_annotation/network_traffic_annotation.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +160 lines, -5 lines 0 comments Download
M net/traffic_annotation/network_traffic_annotation_test_helper.h View 1 2 1 chunk +5 lines, -1 line 0 comments Download
M tools/clang/traffic_annotation_extractor/README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +18 lines, -4 lines 0 comments Download
M tools/clang/traffic_annotation_extractor/traffic_annotation_extractor.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +116 lines, -26 lines 0 comments Download
M tools/traffic_annotation/auditor/traffic_annotation_auditor.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 9 chunks +81 lines, -19 lines 0 comments Download
M tools/traffic_annotation/sample_traffic_annotation.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +198 lines, -24 lines 0 comments Download

Messages

Total messages: 65 (38 generated)
Ramin Halavati
Daniel, Dominic, I've enclosed another version of partial annotations with 4 functions and 2 partial ...
3 years, 7 months ago (2017-05-10 13:09:58 UTC) #2
Ramin Halavati
On 2017/05/10 13:09:58, Ramin Halavati wrote: > Daniel, Dominic, > > I've enclosed another version ...
3 years, 7 months ago (2017-05-17 13:22:48 UTC) #5
Ramin Halavati
Asanka, Daniel, I would be delighted to have your approval on this. :)
3 years, 7 months ago (2017-05-22 08:22:24 UTC) #8
dcheng
LGTM Sorry for the delay, traveling + a series of laptop failures so I missed ...
3 years, 7 months ago (2017-05-23 05:13:36 UTC) #9
Ramin Halavati
Thank you very much Daniel, comments addressed. Asanka, Martin, Dominic, Please review. https://codereview.chromium.org/2869373002/diff/120001/net/traffic_annotation/network_traffic_annotation.h File net/traffic_annotation/network_traffic_annotation.h ...
3 years, 7 months ago (2017-05-23 06:38:12 UTC) #10
Ramin Halavati
On 2017/05/23 06:38:12, Ramin Halavati wrote: > Thank you very much Daniel, comments addressed. > ...
3 years, 7 months ago (2017-05-23 11:16:04 UTC) #11
Ramin Halavati
+Helen
3 years, 7 months ago (2017-05-23 14:00:10 UTC) #23
xunjieli
Thanks! a couple of nits and a question on how to run the clang tool. ...
3 years, 7 months ago (2017-05-23 15:17:50 UTC) #25
Ramin Halavati
Thank you Helen, comments addressed, please review. https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation/network_traffic_annotation.h File net/traffic_annotation/network_traffic_annotation.h (right): https://codereview.chromium.org/2869373002/diff/240001/net/traffic_annotation/network_traffic_annotation.h#newcode10 net/traffic_annotation/network_traffic_annotation.h:10: // Recuresively ...
3 years, 7 months ago (2017-05-23 16:30:43 UTC) #28
xunjieli
I like the part about storing a hash code. I thinks it makes it easier ...
3 years, 7 months ago (2017-05-23 18:37:10 UTC) #29
Ramin Halavati
Thanks Helen, comments addressed, please review. Yes, the plan is to have hash collision checking ...
3 years, 7 months ago (2017-05-23 18:56:08 UTC) #30
xunjieli
net/ LGTM > Presubmit check does not check the whole repository for speed issues and ...
3 years, 7 months ago (2017-05-23 23:51:50 UTC) #31
dcheng
I'm fine with this overall approach. It might be nice to document how collision resistant ...
3 years, 7 months ago (2017-05-24 03:39:52 UTC) #32
dcheng
On 2017/05/24 03:39:52, dcheng wrote: > I'm fine with this overall approach. It might be ...
3 years, 7 months ago (2017-05-24 03:42:30 UTC) #33
Ramin Halavati
Daniel, Helen, All comments addressed, thank you very much. I changed the denominator of hash ...
3 years, 7 months ago (2017-05-24 06:56:13 UTC) #34
battre
Sorry, I have an issue with the words Extend vs. Expand. They are the same ...
3 years, 7 months ago (2017-05-24 12:12:52 UTC) #39
battre
LGTM % naming
3 years, 7 months ago (2017-05-24 12:17:09 UTC) #40
Ramin Halavati
Thank you Dominic, comment addressed. What about the followings? CompleteNetworkTrafficAnnotation and CompleteBranchedNetworkTrafficAnnotation https://codereview.chromium.org/2869373002/diff/360001/net/traffic_annotation/network_traffic_annotation.h File net/traffic_annotation/network_traffic_annotation.h ...
3 years, 7 months ago (2017-05-24 12:27:28 UTC) #42
battre
On 2017/05/24 12:27:28, Ramin Halavati wrote: > Thank you Dominic, comment addressed. > > What ...
3 years, 7 months ago (2017-05-24 12:39:49 UTC) #43
Ramin Halavati
On 2017/05/24 12:39:49, battre wrote: > On 2017/05/24 12:27:28, Ramin Halavati wrote: > > Thank ...
3 years, 7 months ago (2017-05-24 12:41:29 UTC) #44
xunjieli
On 2017/05/24 12:41:29, Ramin Halavati wrote: > On 2017/05/24 12:39:49, battre wrote: > > On ...
3 years, 7 months ago (2017-05-24 13:13:44 UTC) #45
xunjieli
One more question. Can we use signed int32 instead of unsigned int32 for the hash ...
3 years, 7 months ago (2017-05-24 17:26:04 UTC) #51
dcheng
On 2017/05/24 17:26:04, xunjieli wrote: > One more question. Can we use signed int32 instead ...
3 years, 7 months ago (2017-05-24 17:35:43 UTC) #52
dcheng
On 2017/05/24 06:56:13, Ramin Halavati wrote: > Daniel, Helen, > All comments addressed, thank you ...
3 years, 7 months ago (2017-05-24 17:38:33 UTC) #53
Ramin Halavati
Thank you. Type changed to int32_t, completing function names updated.
3 years, 7 months ago (2017-05-25 06:10:58 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2869373002/460001
3 years, 7 months ago (2017-05-25 07:50:10 UTC) #62
commit-bot: I haz the power
3 years, 7 months ago (2017-05-25 07:53:18 UTC) #65
Message was sent while issue was closed.
Committed patchset #16 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/99c6fcef9cff05b0df3a536623f8...

Powered by Google App Engine
This is Rietveld 408576698