Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2707363004:
Network traffic annotation added to sync. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, sync-reviews_chromium.org, net-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to sync.
Network traffic annotation is added to network request of sync.
BUG=656607
Review-Url: https://codereview.chromium.org/2707363004
Cr-Commit-Position: refs/heads/master@{#455732}
Committed: https://chromium.googlesource.com/chromium/src/+/75a414cfbc5ef07035f7e5ce184059a1061fa044
Patch Set 1 #
Total comments: 14
Patch Set 2 : Annotations updated. #Patch Set 3 : nits #
Total comments: 2
Patch Set 4 : Annotation updated. #
Total comments: 9
Patch Set 5 : Comments addressed. #Patch Set 6 : policy changed to chrome_policy. #Patch Set 7 : policy changed to chrome_policy. #
Messages
Total messages: 34 (11 generated)
rhalavati@chromium.org changed reviewers: + stanisc@chromium.org
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 4 files related to sunc and added annotation templates to them. Please review them 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.
stanisc@chromium.org changed reviewers: + zea@chromium.org
zea@, could you please review this one?
One comment. Also, have you given thought to perhaps combining this annotation with the data measurement annotations that already exist? https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:66: net::DefineNetworkTrafficAnnotation("...", R"( It would be nice if we didn't have to repeat this literal across all these files. Perhaps hard code it somewhere?
https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:66: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/23 22:15:54, Nicolas Zea wrote: > It would be nice if we didn't have to repeat this literal across all these > files. Perhaps hard code it somewhere? Please elaborate me if I didn't understand your comment correctly. The main annotation text will be dropped off during compile time and the hardcoded |unique_id| will be kept and a pointer to that will be passed to other functions. As we want the annotation text not to be part of binary and run-time, we need it to be in the same compilation unit as where it is used, to extract it using clang tools. So if there are two usage sites that need to have similar annotation, might we have a function that creates URLFetcher for both of them and assigns the annotation to it there?
https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:66: net::DefineNetworkTrafficAnnotation("...", R"( I was just thinking have the R"..." string be saved into a header (maybe within net/traffic_annotation?) as a constant, and then use that constant here and in the other sync callsites (or any other places with the same semantics/policy. I'm mainly thinking about readability/maintainability of the code. That said, I'm not sure how you're planning to use these annotations. If we had say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync callsites, would that defeat what you're trying to accomplish?
rhalavati@chromium.org changed reviewers: + battre@chromium.org
https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:66: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/24 20:37:38, Nicolas Zea wrote: > I was just thinking have the R"..." string be saved into a header (maybe within > net/traffic_annotation?) as a constant, and then use that constant here and in > the other sync callsites (or any other places with the same semantics/policy. > I'm mainly thinking about readability/maintainability of the code. > > That said, I'm not sure how you're planning to use these annotations. If we had > say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync callsites, > would that defeat what you're trying to accomplish? Current plan is extraction of R"..." strings using clang tools in separate reporting and auditing tools and use them there. Clang tool considers the R"..." boundary and does not apply macros or constants to it. I think serialized protobuf is quite readable in code and the reports will be created after parsing the protobufs. A presubmit tool will be added shortly to check it the format is correct to help maintainability. battre@: Any comments?
On 2017/02/27 09:23:17, Ramin Halavati wrote: > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > File components/sync/driver/sync_stopped_reporter.cc (right): > > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > components/sync/driver/sync_stopped_reporter.cc:66: > net::DefineNetworkTrafficAnnotation("...", R"( > On 2017/02/24 20:37:38, Nicolas Zea wrote: > > I was just thinking have the R"..." string be saved into a header (maybe > within > > net/traffic_annotation?) as a constant, and then use that constant here and in > > the other sync callsites (or any other places with the same semantics/policy. > > I'm mainly thinking about readability/maintainability of the code. > > > > That said, I'm not sure how you're planning to use these annotations. If we > had > > say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync callsites, > > would that defeat what you're trying to accomplish? > > Current plan is extraction of R"..." strings using clang tools in separate > reporting and auditing tools and use them there. Clang tool considers the R"..." > boundary and does not apply macros or constants to it. > I think serialized protobuf is quite readable in code and the reports will be > created after parsing the protobufs. A presubmit tool will be added shortly to > check it the format is correct to help maintainability. > > battre@: > Any comments? But why can't you do the extraction of the strings using clang and replace duplicate strings with a macro/constant? It just improves the readability of the code, and makes helps ensure there aren't any typos if for example we introduce somewhere else in sync that we want to trigger a similar url requrest?
On 2017/02/28 22:00:59, Nicolas Zea wrote: > On 2017/02/27 09:23:17, Ramin Halavati wrote: > > > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > > File components/sync/driver/sync_stopped_reporter.cc (right): > > > > > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > > components/sync/driver/sync_stopped_reporter.cc:66: > > net::DefineNetworkTrafficAnnotation("...", R"( > > On 2017/02/24 20:37:38, Nicolas Zea wrote: > > > I was just thinking have the R"..." string be saved into a header (maybe > > within > > > net/traffic_annotation?) as a constant, and then use that constant here and > in > > > the other sync callsites (or any other places with the same > semantics/policy. > > > I'm mainly thinking about readability/maintainability of the code. > > > > > > That said, I'm not sure how you're planning to use these annotations. If we > > had > > > say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync > callsites, > > > would that defeat what you're trying to accomplish? > > > > Current plan is extraction of R"..." strings using clang tools in separate > > reporting and auditing tools and use them there. Clang tool considers the > R"..." > > boundary and does not apply macros or constants to it. > > I think serialized protobuf is quite readable in code and the reports will be > > created after parsing the protobufs. A presubmit tool will be added shortly to > > check it the format is correct to help maintainability. > > > > battre@: > > Any comments? > > But why can't you do the extraction of the strings using clang and replace > duplicate strings with a macro/constant? It just improves the readability of the > code, and makes helps ensure there aren't any typos if for example we introduce > somewhere else in sync that we want to trigger a similar url requrest? Code replacement requires clang-plugin instead of clang tool which is much harder to maintain and clang team strongly advises against using that unless it's absolutely needed. I think with current approach, the developer can specify the changes quite easily and inline with actual use, and syntax checking is done during presubmit checks. The only draw back is the cases that one annotation is used in two different parts of codes, and I think this situations suggest adding a new function that both call sites use.
On 2017/03/01 07:23:15, Ramin Halavati wrote: > On 2017/02/28 22:00:59, Nicolas Zea wrote: > > On 2017/02/27 09:23:17, Ramin Halavati wrote: > > > > > > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > > > File components/sync/driver/sync_stopped_reporter.cc (right): > > > > > > > > > https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... > > > components/sync/driver/sync_stopped_reporter.cc:66: > > > net::DefineNetworkTrafficAnnotation("...", R"( > > > On 2017/02/24 20:37:38, Nicolas Zea wrote: > > > > I was just thinking have the R"..." string be saved into a header (maybe > > > within > > > > net/traffic_annotation?) as a constant, and then use that constant here > and > > in > > > > the other sync callsites (or any other places with the same > > semantics/policy. > > > > I'm mainly thinking about readability/maintainability of the code. > > > > > > > > That said, I'm not sure how you're planning to use these annotations. If > we > > > had > > > > say a GOOGLE_OWNED constant (or maybe even macro?) used at the sync > > callsites, > > > > would that defeat what you're trying to accomplish? > > > > > > Current plan is extraction of R"..." strings using clang tools in separate > > > reporting and auditing tools and use them there. Clang tool considers the > > R"..." > > > boundary and does not apply macros or constants to it. > > > I think serialized protobuf is quite readable in code and the reports will > be > > > created after parsing the protobufs. A presubmit tool will be added shortly > to > > > check it the format is correct to help maintainability. > > > > > > battre@: > > > Any comments? > > > > But why can't you do the extraction of the strings using clang and replace > > duplicate strings with a macro/constant? It just improves the readability of > the > > code, and makes helps ensure there aren't any typos if for example we > introduce > > somewhere else in sync that we want to trigger a similar url requrest? > > Code replacement requires clang-plugin instead of clang tool which is much > harder to maintain and clang team strongly advises against using that unless > it's absolutely needed. > I think with current approach, the developer can specify the changes quite > easily and inline with actual use, and syntax checking is done during presubmit > checks. > The only draw back is the cases that one annotation is used in two different > parts of codes, and I think this situations suggest adding a new function that > both call sites use. Sure, a new function sounds fine. My main point isn't how we replace the string, just that repeating a long string like that across multiple related files seems prone to developer error. Are there any places outside of sync that use that same string? If so, it seems like the function should be defined in net/traffic_annotation. If that string is specific to sync though, you can probably define the function in sync_util.h
Hey, Pavel just explained to me the purpose of this change, and that you actually want me to fill out the strings, rather than review them. Sorry about that, I completely misunderstood what this CL was for (I missed your initial message, and was just looking at the CL description). Take a look at the strings I've included. Now that I understand that the strings are unique (mostly) to each site, I'm fine with not pulling the string out into a separate file. https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:68: sender: "..." sender: "chrome sync" description: "A network request to inform Chrome Sync that sync has been disabled for this device." trigger: "User disabled sync." data: "the sync device id and store birthday" cookies: false cookies_store: false settings: "NA" policy: UNSET policy exception: "This network request is only performed after the user has opted in to sync and then disables sync" https://codereview.chromium.org/2707363004/diff/1/components/sync/engine/net/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine/net/... components/sync/engine/net/http_bridge.cc:241: sender: "..." sender: "chrome sync" description: "Chrome Sync will synchronize profile data between chrome clients and Google for a given user account." trigger: "User makes a change to syncable profile data after enabling sync on the device" data: "the device and user identifiers, along with any profile data that is changing" cookies: false policy settings: "" policy: SyncDisabled { options: MANDATORY value: true} } https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... File components/sync/engine_impl/attachments/attachment_downloader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... components/sync/engine_impl/attachments/attachment_downloader_impl.cc:219: net::DefineNetworkTrafficAnnotation("...", R"( same as the http bridge https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... File components/sync/engine_impl/attachments/attachment_uploader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... components/sync/engine_impl/attachments/attachment_uploader_impl.cc:218: net::NetworkTrafficAnnotationTag traffic_annotation = same as the http bridge
Annotations updated, please review. https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:68: sender: "..." On 2017/03/03 22:39:55, Nicolas Zea wrote: > sender: "chrome sync" > description: "A network request to inform Chrome Sync that sync has been > disabled for this device." > trigger: "User disabled sync." > data: "the sync device id and store birthday" > cookies: false > cookies_store: false > settings: "NA" > policy: UNSET > policy exception: "This network request is only performed after the user has > opted in to sync and then disables sync" Thanks, could you elaborate on "Store Birthday" Can't we use 'SyncDisabled' as a policy here as well? By preventing enabling Sync, disabling it will also be prevented. https://codereview.chromium.org/2707363004/diff/1/components/sync/engine/net/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine/net/... components/sync/engine/net/http_bridge.cc:241: sender: "..." On 2017/03/03 22:39:55, Nicolas Zea wrote: > sender: "chrome sync" > description: "Chrome Sync will synchronize profile data between chrome clients > and Google for a given user account." > trigger: "User makes a change to syncable profile data after enabling sync on > the device" > data: "the device and user identifiers, along with any profile data that is > changing" > > cookies: false > policy settings: "" > policy: SyncDisabled { options: MANDATORY value: true} > } Done. https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... File components/sync/engine_impl/attachments/attachment_downloader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... components/sync/engine_impl/attachments/attachment_downloader_impl.cc:219: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/03/03 22:39:56, Nicolas Zea wrote: > same as the http bridge Done. https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... File components/sync/engine_impl/attachments/attachment_uploader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/engine_impl... components/sync/engine_impl/attachments/attachment_uploader_impl.cc:218: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/03/03 22:39:56, Nicolas Zea wrote: > same as the http bridge Done.
https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:68: sender: "..." On 2017/03/06 06:55:45, Ramin Halavati wrote: > On 2017/03/03 22:39:55, Nicolas Zea wrote: > > sender: "chrome sync" > > description: "A network request to inform Chrome Sync that sync has been > > disabled for this device." > > trigger: "User disabled sync." > > data: "the sync device id and store birthday" > > cookies: false > > cookies_store: false > > settings: "NA" > > policy: UNSET > > policy exception: "This network request is only performed after the user has > > opted in to sync and then disables sync" > > Thanks, could you elaborate on "Store Birthday" Store birthday is the identifier for the lifetime of the account data that gets reset every time the user clears their sync data via the google dashboard. Perhaps we can just refer to this as "sync device identifier and metadata"? > > Can't we use 'SyncDisabled' as a policy here as well? By preventing enabling > Sync, disabling it will also be prevented. That's true. I wasn't sure if the policy was meant to be tied to the specific request or not. I'm fine with repeating SyncDsabled here.
https://codereview.chromium.org/2707363004/diff/40001/components/sync/engine/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/40001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:255: setting: "This feature cannot be disabled by settings." Looks like I forgot to specify this string. It should be "The user can disable Chrome Sync by going into the profile settings and choosing to Sign Out".
Annotations updated, please review. https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... File components/sync/driver/sync_stopped_reporter.cc (right): https://codereview.chromium.org/2707363004/diff/1/components/sync/driver/sync... components/sync/driver/sync_stopped_reporter.cc:68: sender: "..." On 2017/03/06 17:49:20, Nicolas Zea wrote: > On 2017/03/06 06:55:45, Ramin Halavati wrote: > > On 2017/03/03 22:39:55, Nicolas Zea wrote: > > > sender: "chrome sync" > > > description: "A network request to inform Chrome Sync that sync has been > > > disabled for this device." > > > trigger: "User disabled sync." > > > data: "the sync device id and store birthday" > > > cookies: false > > > cookies_store: false > > > settings: "NA" > > > policy: UNSET > > > policy exception: "This network request is only performed after the user has > > > opted in to sync and then disables sync" > > > > Thanks, could you elaborate on "Store Birthday" > > Store birthday is the identifier for the lifetime of the account data that gets > reset every time the user clears their sync data via the google dashboard. > Perhaps we can just refer to this as "sync device identifier and metadata"? > > > > > Can't we use 'SyncDisabled' as a policy here as well? By preventing enabling > > Sync, disabling it will also be prevented. > > That's true. I wasn't sure if the policy was meant to be tied to the specific > request or not. I'm fine with repeating SyncDsabled here. Done. https://codereview.chromium.org/2707363004/diff/40001/components/sync/engine/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/40001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:255: setting: "This feature cannot be disabled by settings." On 2017/03/06 17:56:35, Nicolas Zea wrote: > Looks like I forgot to specify this string. It should be "The user can disable > Chrome Sync by going into the profile settings and choosing to Sign Out". Done.
lgtm
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you.
https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:238: net::NetworkTrafficAnnotationTag traffic_annotation = Is it possible toe extract this annotation to one place to avoid repetition? https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:239: net::DefineNetworkTrafficAnnotation("http_bridge", R"( This sounds quite generic. I would at least prefix all the IDs with "sync_" to avoid namespace collisions. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... File components/sync/engine_impl/attachments/attachment_downloader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... components/sync/engine_impl/attachments/attachment_downloader_impl.cc:219: net::DefineNetworkTrafficAnnotation("attachment_downloader", R"( Ditto. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... File components/sync/engine_impl/attachments/attachment_uploader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... components/sync/engine_impl/attachments/attachment_uploader_impl.cc:219: net::DefineNetworkTrafficAnnotation("attachment_uploader", R"( Ditto.
Comments addressed, please review. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:238: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/03/09 12:02:32, msramek wrote: > Is it possible toe extract this annotation to one place to avoid repetition? We need to define annotations in the same compilation unit as the code that uses it. There isn't a common class now that the three usages get their URLFetcher from. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:239: net::DefineNetworkTrafficAnnotation("http_bridge", R"( On 2017/03/09 12:02:32, msramek wrote: > This sounds quite generic. I would at least prefix all the IDs with "sync_" to > avoid namespace collisions. Done. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... File components/sync/engine_impl/attachments/attachment_downloader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... components/sync/engine_impl/attachments/attachment_downloader_impl.cc:219: net::DefineNetworkTrafficAnnotation("attachment_downloader", R"( On 2017/03/09 12:02:32, msramek wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... File components/sync/engine_impl/attachments/attachment_uploader_impl.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine_... components/sync/engine_impl/attachments/attachment_uploader_impl.cc:219: net::DefineNetworkTrafficAnnotation("attachment_uploader", R"( On 2017/03/09 12:02:32, msramek wrote: > Ditto. Acknowledged.
LGTM https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... File components/sync/engine/net/http_bridge.cc (right): https://codereview.chromium.org/2707363004/diff/60001/components/sync/engine/... components/sync/engine/net/http_bridge.cc:238: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/03/09 12:31:20, Ramin Halavati wrote: > On 2017/03/09 12:02:32, msramek wrote: > > Is it possible toe extract this annotation to one place to avoid repetition? > > We need to define annotations in the same compilation unit as the code that uses > it. There isn't a common class now that the three usages get their URLFetcher > from. Right, I was thinking about creating one - perhaps components/sync/base/traffic_annotation.h. But this is up to the local code owners, so if Nicolas is happy, I'm fine.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2707363004/#ps100001 (title: "policy changed to chrome_policy")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2729423003 Patch 1). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/2707363004/#ps120001 (title: "policy changed to chrome_policy.")
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": 1489064128081010,
"parent_rev": "caa17f475b1ccf16ce0d4f89902663f4462396a8", "commit_rev":
"75a414cfbc5ef07035f7e5ce184059a1061fa044"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to sync. Network traffic annotation is added to network request of sync. BUG=656607 ========== to ========== Network traffic annotation added to sync. Network traffic annotation is added to network request of sync. BUG=656607 Review-Url: https://codereview.chromium.org/2707363004 Cr-Commit-Position: refs/heads/master@{#455732} Committed: https://chromium.googlesource.com/chromium/src/+/75a414cfbc5ef07035f7e5ce1840... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/75a414cfbc5ef07035f7e5ce1840...
Message was sent while issue was closed.
Yeah, FWIW the attachments code is something that we're likely to remove in the near future as it's not in use anywhere. As such I don't feel strongly about pulling out the strings into a separate file. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
