Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704383002:
Network traffic annotation added to gcm_network_channel. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 8 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to gcm_network_channel.
Network traffic annotation is added to network request of
gcm_network_channel.
BUG=656607
Review-Url: https://codereview.chromium.org/2704383002
Cr-Commit-Position: refs/heads/master@{#461421}
Committed: https://chromium.googlesource.com/chromium/src/+/e7182dca7960c0cbf1a6e0f0cc5f7b05ad2ef6a5
Patch Set 1 #
Total comments: 28
Patch Set 2 : Annotation updated. #Patch Set 3 : nits #Patch Set 4 : Annotation updated. #
Total comments: 20
Patch Set 5 : Comments addressed. #Patch Set 6 : Comments addressed. #Patch Set 7 : Annotation updated. #
Messages
Total messages: 30 (7 generated)
rhalavati@chromium.org changed reviewers: + asanka@chromium.org, dcheng@chromium.org
asanka@: Please review DEPS as owner of /net. dcheng@: We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified gcm_network_channel and added annotation template to it. Please review it and suggest the required answers for the empty parts. Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
On 2017/02/21 13:50:31, Ramin Halavati wrote: > asanka@: > Please review DEPS as owner of /net. > > dcheng@: > We are annotating all network requests in Chromium with a new > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit > the requests and configure Chrome in a way that meets their security policies > and expectations. Furthermore, it allows us to generate better debugging data in > chrome://net-internals and measure bandwidth consumption on a per-request-type > basis. > > I've modified gcm_network_channel and added annotation template to it. Please > review it and suggest the required answers for the empty parts. Please note that > the claims should be thorough and covering all cases. In case you believe that > annotation should be passed to the modified function instead of originating from > it, please tell me to change the CL accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > And more description on enterprise policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 > > Please tell me if you need any clarifications from my side. If you want to learn > more, see: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. I was under the impression that the extraction and validation tools would be in place before adding annotations. Is that no longer the case? I'm concerned that the content of annotation is difficult for individual OWNERS to maintain.
On 2017/02/21 16:50:40, asanka wrote: > On 2017/02/21 13:50:31, Ramin Halavati wrote: > > asanka@: > > Please review DEPS as owner of /net. > > > > dcheng@: > > We are annotating all network requests in Chromium with a new > > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to > audit > > the requests and configure Chrome in a way that meets their security policies > > and expectations. Furthermore, it allows us to generate better debugging data > in > > chrome://net-internals and measure bandwidth consumption on a per-request-type > > basis. > > > > I've modified gcm_network_channel and added annotation template to it. Please > > review it and suggest the required answers for the empty parts. Please note > that > > the claims should be thorough and covering all cases. In case you believe that > > annotation should be passed to the modified function instead of originating > from > > it, please tell me to change the CL accordingly. > > > > Please take a look at the protobuf scheme in: > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > for the definition of the annotations. > > > > You can find a sample annotation in: > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > Entriprise policy options are here: > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > > > And more description on enterprise policy settings is here: > > http://dev.chromium.org/administrators/policy-list-3 > > > > Please tell me if you need any clarifications from my side. If you want to > learn > > more, see: > > > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. > > I was under the impression that the extraction and validation tools would be in > place before adding annotations. Is that no longer the case? > > I'm concerned that the content of annotation is difficult for individual OWNERS > to maintain. The first version of the tool for extraction and verification of proto texts is ready (https://codereview.chromium.org/2448133006/) and I am using it locally to verify annotations before landing CLs. As at the first step, we are creating annotation templates for the owners and send them the incomplete and syntactically incorrect annotations to fill in, I did not create the CL on adding this check to PRESUBMIT yet, but I will do it as our first round ends.
On 2017/02/21 17:04:03, Ramin Halavati wrote: > On 2017/02/21 16:50:40, asanka wrote: > > On 2017/02/21 13:50:31, Ramin Halavati wrote: > > > asanka@: > > > Please review DEPS as owner of /net. > > > > > > dcheng@: > > > We are annotating all network requests in Chromium with a new > > > NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to > > audit > > > the requests and configure Chrome in a way that meets their security > policies > > > and expectations. Furthermore, it allows us to generate better debugging > data > > in > > > chrome://net-internals and measure bandwidth consumption on a > per-request-type > > > basis. > > > > > > I've modified gcm_network_channel and added annotation template to it. > Please > > > review it and suggest the required answers for the empty parts. Please note > > that > > > the claims should be thorough and covering all cases. In case you believe > that > > > annotation should be passed to the modified function instead of originating > > from > > > it, please tell me to change the CL accordingly. > > > > > > Please take a look at the protobuf scheme in: > > > > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > > for the definition of the annotations. > > > > > > You can find a sample annotation in: > > > > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > > > Entriprise policy options are here: > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > > > > > And more description on enterprise policy settings is here: > > > http://dev.chromium.org/administrators/policy-list-3 > > > > > > Please tell me if you need any clarifications from my side. If you want to > > learn > > > more, see: > > > > > > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. > > > > I was under the impression that the extraction and validation tools would be > in > > place before adding annotations. Is that no longer the case? > > > > I'm concerned that the content of annotation is difficult for individual > OWNERS > > to maintain. > > The first version of the tool for extraction and verification of proto texts is > ready (https://codereview.chromium.org/2448133006/) and I am using it locally to > verify annotations before landing CLs. > As at the first step, we are creating annotation templates for the owners and > send them the incomplete and syntactically incorrect annotations to fill in, I > did not create the CL on adding this check to PRESUBMIT yet, but I will do it as > our first round ends. Ok. Thanks for the explanation.
https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:224: sender: "..." To help me understand, is this something that will be filled in more later? What does ... mean? (I'm not hugely enthusiastic about included text encoded protos as literals here either, as there's zero compile-time validation that the string blob is valid, but perhaps there are unavoidable design constraints here...)
https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:224: sender: "..." On 2017/02/23 19:15:12, dcheng wrote: > To help me understand, is this something that will be filled in more later? What > does ... mean? > > (I'm not hugely enthusiastic about included text encoded protos as literals here > either, as there's zero compile-time validation that the string blob is valid, > but perhaps there are unavoidable design constraints here...) Sorry that the first message was not clear enough. The |...|s are the places that I've asked you to suggest text for them. As we wanted to exclude this text from binary code, we decided on a method were there is no process on annotation text during compile time and therefore it will be optimized out of the binary. A clang tool for presubmit check will be added in next steps to check them before submission. (I've asked you to review its first version for local use in https://codereview.chromium.org/2448133006/ and will write the second version for presubmit once that one is landed.)
dcheng@chromium.org changed reviewers: + pavely@chromium.org
https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:224: sender: "..." On 2017/02/24 10:16:00, Ramin Halavati wrote: > On 2017/02/23 19:15:12, dcheng wrote: > > To help me understand, is this something that will be filled in more later? > What > > does ... mean? > > > > (I'm not hugely enthusiastic about included text encoded protos as literals > here > > either, as there's zero compile-time validation that the string blob is valid, > > but perhaps there are unavoidable design constraints here...) > > Sorry that the first message was not clear enough. The |...|s are the places > that I've asked you to suggest text for them. > > As we wanted to exclude this text from binary code, we decided on a method were > there is no process on annotation text during compile time and therefore it will > be optimized out of the binary. A clang tool for presubmit check will be added > in next steps to check them before submission. (I've asked you to review its > first version for local use in https://codereview.chromium.org/2448133006/ and > will write the second version for presubmit once that one is landed.) I see. I'm probably not a good reviewer for this CL then. +pavely, do you mind helping with the labelling here?
Could you take a look at my comments and let me know if you want me to adjust some messages, include more info. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:224: sender: "..." sender: Invalidation service https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:225: description: "..." Chrome uses cacheinvalidation library to receive push notifications from server about sync items (bookmarks, passwords, preferences, etc.) modified on other clients. It uses GCMClient to receive incoming messages. This request is used for client-to-server communications. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:226: trigger: "..." The first message is sent to register client with the server on chrome startup. It is then sent periodically to confirm that client is still online. After receiving notification about server changes client sends this request to acknowledge that notification is processed. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:228: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER destination: GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:231: cookies_allowed: false/true cookies_allowed: false https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:233: setting: "..." Invalidation service is enabled by default. I don't think there is a way to disable it by flag/ command line option or server experiment.
Annotation updated, please review. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:224: sender: "..." On 2017/02/27 19:29:41, pavely wrote: > sender: Invalidation service Done. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:225: description: "..." On 2017/02/27 19:29:41, pavely wrote: > Chrome uses cacheinvalidation library to receive push notifications from server > about sync items (bookmarks, passwords, preferences, etc.) modified on other > clients. It uses GCMClient to receive incoming messages. This request is used > for client-to-server communications. Done. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:226: trigger: "..." On 2017/02/27 19:29:41, pavely wrote: > The first message is sent to register client with the server on chrome startup. > It is then sent periodically to confirm that client is still online. > After receiving notification about server changes client sends this request to > acknowledge that notification is processed. Done. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:227: data: "..." Please specify what data is sent. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:231: cookies_allowed: false/true On 2017/02/27 19:29:41, pavely wrote: > cookies_allowed: false It seems to me that cookies are not disabled. If they are not required for this request, I can create a CL that disables the cookies and then base this CL on top of that. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:233: setting: "..." On 2017/02/27 19:29:41, pavely wrote: > Invalidation service is enabled by default. I don't think there is a way to > disable it by flag/ command line option or server experiment. Isn't it disabled if Sync is disabled? https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:235: [POLICY_NAME] { Isn't this related? http://dev.chromium.org/administrators/policy-list-3#SyncDisabled https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:240: policy_exception_justification: "..." Do you think it's useful to have a policy to disable this feature?
https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:227: data: "..." On 2017/02/28 11:57:04, Ramin Halavati wrote: > Please specify what data is sent. No PII data is sent in this request. Request contains client_token generated on the server. It includes invalidation object ids (not specific to client) and versions when confirming invalidations. When updating registrations request includes object ids. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:231: cookies_allowed: false/true On 2017/02/28 11:57:03, Ramin Halavati wrote: > On 2017/02/27 19:29:41, pavely wrote: > > cookies_allowed: false > > It seems to me that cookies are not disabled. If they are not required for this > request, I can create a CL that disables the cookies and then base this CL on > top of that. I believe cookies are not required. Authentication is done through HTTP header. Feel free to disable cookies. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:233: setting: "..." On 2017/02/28 11:57:03, Ramin Halavati wrote: > On 2017/02/27 19:29:41, pavely wrote: > > Invalidation service is enabled by default. I don't think there is a way to > > disable it by flag/ command line option or server experiment. > > Isn't it disabled if Sync is disabled? Apart from Sync, InvalidationService is used by couple more components (cloud policy, ChildAccountInfoFetcher). You can see them in the list of registrars in about:invalidations. There is no code that blocks InvalidationService from starting if some consumer requests it, but if no consumer invokes InvalidationService then these requests wouldn't be sent. I know if Sync is disabled it is not triggering InvalidationService initialization. I guess other consumers only use InvalidationService when user is signed in, but I'm not 100% sure. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:235: [POLICY_NAME] { On 2017/02/28 11:57:03, Ramin Halavati wrote: > Isn't this related? > > http://dev.chromium.org/administrators/policy-list-3#SyncDisabled This policy disables sync, it doesn't affect other services. (see my other comment) https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:240: policy_exception_justification: "..." On 2017/02/28 11:57:03, Ramin Halavati wrote: > Do you think it's useful to have a policy to disable this feature? Disabling InvalidationService might break features that depend on it. I think it makes sense to control top level features that use InvalidationService.
pavely@: Comments addressed, please review. asanka@: Please review DEPS as owner of net/. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:227: data: "..." On 2017/02/28 18:38:49, pavely wrote: > On 2017/02/28 11:57:04, Ramin Halavati wrote: > > Please specify what data is sent. > > No PII data is sent in this request. > Request contains client_token generated on the server. It includes invalidation > object ids (not specific to client) and versions when confirming invalidations. > When updating registrations request includes object ids. Done. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:228: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/27 19:29:41, pavely wrote: > destination: GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:231: cookies_allowed: false/true On 2017/02/28 18:38:49, pavely wrote: > On 2017/02/28 11:57:03, Ramin Halavati wrote: > > On 2017/02/27 19:29:41, pavely wrote: > > > cookies_allowed: false > > > > It seems to me that cookies are not disabled. If they are not required for > this > > request, I can create a CL that disables the cookies and then base this CL on > > top of that. > > I believe cookies are not required. Authentication is done through HTTP header. > Feel free to disable cookies. https://codereview.chromium.org/2723043002 created to disable cookies. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:233: setting: "..." On 2017/02/28 18:38:49, pavely wrote: > On 2017/02/28 11:57:03, Ramin Halavati wrote: > > On 2017/02/27 19:29:41, pavely wrote: > > > Invalidation service is enabled by default. I don't think there is a way to > > > disable it by flag/ command line option or server experiment. > > > > Isn't it disabled if Sync is disabled? > > Apart from Sync, InvalidationService is used by couple more components (cloud > policy, ChildAccountInfoFetcher). You can see them in the list of registrars in > about:invalidations. There is no code that blocks InvalidationService from > starting if some consumer requests it, but if no consumer invokes > InvalidationService then these requests wouldn't be sent. > I know if Sync is disabled it is not triggering InvalidationService > initialization. I guess other consumers only use InvalidationService when user > is signed in, but I'm not 100% sure. Acknowledged. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:235: [POLICY_NAME] { On 2017/02/28 18:38:49, pavely wrote: > On 2017/02/28 11:57:03, Ramin Halavati wrote: > > Isn't this related? > > > > http://dev.chromium.org/administrators/policy-list-3#SyncDisabled > > This policy disables sync, it doesn't affect other services. (see my other > comment) Acknowledged. https://codereview.chromium.org/2704383002/diff/1/components/invalidation/imp... components/invalidation/impl/gcm_network_channel.cc:240: policy_exception_justification: "..." On 2017/02/28 18:38:49, pavely wrote: > On 2017/02/28 11:57:03, Ramin Halavati wrote: > > Do you think it's useful to have a policy to disable this feature? > > Disabling InvalidationService might break features that depend on it. I think it > makes sense to control top level features that use InvalidationService. Done.
lgtm
DEPS lgtm
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
Thank you.
I'm not a native speaker, so take me with a grain of salt, but I think there's a lot of missing articles in the description. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:228: "notifications from server about sync items (bookmarks, passwords, " the server https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:235: "client is still online. After receiving notification about server " the client https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:236: "changes client sends this request to acknowledge that " changes, the client https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:237: "notification is processed." the notification https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:239: "No PII data is sent in this request. Request contains " The request. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:241: "object ids (not specific to client) and versions when confirming " a client I also don't fully understand the sentence. Does it contain version numbers of the synced objects? What does "when confirming invalidations" mean? Should that be "used to confirm invalidations", or does it mean that the request does not always contain them, only sometimes, "when confirming invalidations"? https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:242: "invalidations. When updating registrations request includes " registrations, the request https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:251: "features that depend on it. I think it makes sense to control " Keep in mind that this text will be read by enterprise admins. Who is "I"? Why do Chromium developers "think" that this is the case instead of making a clear statement?
Comments addressed, please review. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:228: "notifications from server about sync items (bookmarks, passwords, " On 2017/03/07 10:53:29, msramek wrote: > the server Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:235: "client is still online. After receiving notification about server " On 2017/03/07 10:53:29, msramek wrote: > the client Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:236: "changes client sends this request to acknowledge that " On 2017/03/07 10:53:29, msramek wrote: > changes, the client Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:237: "notification is processed." On 2017/03/07 10:53:29, msramek wrote: > the notification Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:239: "No PII data is sent in this request. Request contains " On 2017/03/07 10:53:29, msramek wrote: > The request. Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:242: "invalidations. When updating registrations request includes " On 2017/03/07 10:53:30, msramek wrote: > registrations, the request Done. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:251: "features that depend on it. I think it makes sense to control " On 2017/03/07 10:53:29, msramek wrote: > Keep in mind that this text will be read by enterprise admins. Who is "I"? Why > do Chromium developers "think" that this is the case instead of making a clear > statement? Done.
https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:241: "object ids (not specific to client) and versions when confirming " On 2017/03/07 10:53:29, msramek wrote: > a client > > I also don't fully understand the sentence. Does it contain version numbers of > the synced objects? What does "when confirming invalidations" mean? Should that > be "used to confirm invalidations", or does it mean that the request does not > always contain them, only sometimes, "when confirming invalidations"? pavely@, can you please clarify this? https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:242: "invalidations. When updating registrations request includes " On 2017/03/07 13:10:35, Ramin Halavati wrote: > On 2017/03/07 10:53:30, msramek wrote: > > registrations, the request > > Done. nit: Still missing the article :)
pavely@: Please view Martin's inline comment on |data|. msramek@: Comment addressed. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:242: "invalidations. When updating registrations request includes " On 2017/03/14 16:17:49, msramek wrote: > On 2017/03/07 13:10:35, Ramin Halavati wrote: > > On 2017/03/07 10:53:30, msramek wrote: > > > registrations, the request > > > > Done. > > nit: Still missing the article :) Done.
https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:241: "object ids (not specific to client) and versions when confirming " On 2017/03/07 10:53:29, msramek wrote: > a client > > I also don't fully understand the sentence. Does it contain version numbers of > the synced objects? As I understand version is internal invalidations concept, each invalidation is marked with version for client to be able to detect the ones that it already received and for server to identify invalidations acknowledged by client. > What does "when confirming invalidations" mean? Should that > be "used to confirm invalidations", or does it mean that the request does not > always contain them, only sometimes, "when confirming invalidations"? It means the request only contains ObjectIDs and versions only when confirming invalidations. I wanted to convey that this message contains protocol details that don't include user data, but wasn't sure how much details to include into description. Here is more structured description of the protocol. The network request is used for: - Initial registration: Doesn't contain user data. - Updating the set of subscriptions. Contains server generated client_token and ObjectIds identifying subscriptions. ObjectId is not unique to user. - Invalidation acknowledgement. Contains client_token, ObjectId and server version for corresponding subscription. Version is not related to sync data, it is internal concept of invalidations protocol.
Annotation updated, please review. https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... File components/invalidation/impl/gcm_network_channel.cc (right): https://codereview.chromium.org/2704383002/diff/60001/components/invalidation... components/invalidation/impl/gcm_network_channel.cc:241: "object ids (not specific to client) and versions when confirming " On 2017/03/15 17:55:46, pavely wrote: > On 2017/03/07 10:53:29, msramek wrote: > > a client > > > > I also don't fully understand the sentence. Does it contain version numbers of > > the synced objects? > As I understand version is internal invalidations concept, each invalidation is > marked with version for client to be able to detect the ones that it already > received and for server to identify invalidations acknowledged by client. > > > > What does "when confirming invalidations" mean? Should that > > be "used to confirm invalidations", or does it mean that the request does not > > always contain them, only sometimes, "when confirming invalidations"? > > It means the request only contains ObjectIDs and versions only when confirming > invalidations. > > > I wanted to convey that this message contains protocol details that don't > include user data, but wasn't sure how much details to include into description. > > Here is more structured description of the protocol. The network request is used > for: > - Initial registration: Doesn't contain user data. > - Updating the set of subscriptions. Contains server generated client_token and > ObjectIds identifying subscriptions. ObjectId is not unique to user. > - Invalidation acknowledgement. Contains client_token, ObjectId and server > version for corresponding subscription. Version is not related to sync data, it > is internal concept of invalidations protocol. Done.
(sorry for the delay, I wasn't looking at this since Ramin was OOO) Thanks for the clarifications, Pavel! The detailed description LGTM.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2704383002/#ps120001 (title: "Annotation updated.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491225131470630,
"parent_rev": "3b0b9b5da27518bbc4f22915ed71eff935b7d3ce", "commit_rev":
"e7182dca7960c0cbf1a6e0f0cc5f7b05ad2ef6a5"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to gcm_network_channel. Network traffic annotation is added to network request of gcm_network_channel. BUG=656607 ========== to ========== Network traffic annotation added to gcm_network_channel. Network traffic annotation is added to network request of gcm_network_channel. BUG=656607 Review-Url: https://codereview.chromium.org/2704383002 Cr-Commit-Position: refs/heads/master@{#461421} Committed: https://chromium.googlesource.com/chromium/src/+/e7182dca7960c0cbf1a6e0f0cc5f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e7182dca7960c0cbf1a6e0f0cc5f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
