Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794343002:
Network traffic annotation added to image_data_fetcher. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago Reviewers:
gambard, Bernhard Bauer, pkotwicz, jkrcal, msramek, Marc Treib, markusheintz_, achuithb, sfiera CC:
chromium-reviews, battre Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to image_data_fetcher.
Network traffic annotation is added to network request of
components/image_fetcher/core/image_data_fetcher.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2794343002
Cr-Commit-Position: refs/heads/master@{#473188}
Committed: https://chromium.googlesource.com/chromium/src/+/6072a6b7d7d0992e45f8cc64cc62d2e8b766786c
Patch Set 1 #
Total comments: 8
Patch Set 2 : Annotation moved to callers. #
Total comments: 60
Patch Set 3 : All comments addressed. #Patch Set 4 : All comments addressed. #
Total comments: 26
Patch Set 5 : All comments addressed. #
Total comments: 2
Patch Set 6 : Comment addressed. #Patch Set 7 : Android and iOS files added. #
Total comments: 14
Patch Set 8 : Favicon Annotation updated. #
Total comments: 7
Patch Set 9 : Comments addressed, new files added. #Patch Set 10 : Android update. #
Total comments: 16
Patch Set 11 : Annotation updated. #
Total comments: 7
Patch Set 12 : Comments addressed. #Messages
Total messages: 106 (56 generated)
rhalavati@chromium.org changed reviewers: + markusheintz@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 image_data_fetcher and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... 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/04/04 13:51:34, Ramin Halavati wrote: > 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 image_data_fetcher and added annotation template to it. Please > review it and suggest the required answers for the empty parts (marked with > '...'). Please note that the claims should be thorough and covering all cases. > In case you believe that annotation should be passed to the modified function > instead of originating from it, please tell me to change the CL accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > 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. Hi, A gentle reminder on this.
On 2017/04/11 06:41:22, Ramin Halavati wrote: > On 2017/04/04 13:51:34, Ramin Halavati wrote: > > 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 image_data_fetcher and added annotation template to it. Please > > review it and suggest the required answers for the empty parts (marked with > > '...'). Please note that the claims should be thorough and covering all cases. > > In case you believe that annotation should be passed to the modified function > > instead of originating from it, please tell me to change the CL accordingly. > > > > Please take a look at the protobuf scheme in: > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > for the definition of the annotations. > > > > You can find a sample annotation in: > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > Entriprise policy options are here: > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > > > 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. > > Hi, > > A gentle reminder on this. Sorry about the delay will take a look.
On 2017/04/11 08:56:34, markusheintz_ wrote: > On 2017/04/11 06:41:22, Ramin Halavati wrote: > > On 2017/04/04 13:51:34, Ramin Halavati wrote: > > > 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 image_data_fetcher and added annotation template to it. Please > > > review it and suggest the required answers for the empty parts (marked with > > > '...'). Please note that the claims should be thorough and covering all > cases. > > > In case you believe that annotation should be passed to the modified > function > > > instead of originating from it, please tell me to change the CL accordingly. > > > > > > Please take a look at the protobuf scheme in: > > > > > > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > > > for the definition of the annotations. > > > > > > You can find a sample annotation in: > > > > > > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > > > > > Entriprise policy options are here: > > > > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... > > > > > > 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. > > > > Hi, > > > > A gentle reminder on this. > > Sorry about the delay will take a look. Hi Markus, A friendly reminder on this.
https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... File components/image_fetcher/core/image_data_fetcher.cc (right): https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:72: sender: "..." Is there a description of the fields? E.g. sender The image_data_fetcher is a utility that can be used by many different features in Chrome. So what should I put into this field? https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:74: trigger: "..." What do you expect here? This list of trigger is dynamic and may change over time. https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:75: data: "..." raw image data is fetch for a given URL https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:76: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER It could be all of the options. Not sure how client will use it. https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:79: cookies_allowed: false/true false. https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:80: cookies_store: "..." NA https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:81: setting: "..." none. clients have to do the right thing https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... components/image_fetcher/core/image_data_fetcher.cc:82: chrome_policy { There exists no policy. It the responsibility of the client to add a policy. What shall I put here in this case?
On 2017/05/03 09:04:08, markusheintz_ wrote: > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > File components/image_fetcher/core/image_data_fetcher.cc (right): > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:72: sender: "..." > Is there a description of the fields? > > E.g. sender The image_data_fetcher is a utility that can be used by many > different features in Chrome. So what should I put into this field? > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:74: trigger: "..." > What do you expect here? This list of trigger is dynamic and may change over > time. > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:75: data: "..." > raw image data is fetch for a given URL > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:76: destination: > WEBSITE/GOOGLE_OWNED_SERVICE/OTHER > It could be all of the options. Not sure how client will use it. > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:79: cookies_allowed: > false/true > false. > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:80: cookies_store: "..." > NA > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:81: setting: "..." > none. clients have to do the right thing > > https://codereview.chromium.org/2794343002/diff/1/components/image_fetcher/co... > components/image_fetcher/core/image_data_fetcher.cc:82: chrome_policy { > There exists no policy. It the responsibility of the client to add a policy. > What shall I put here in this case? If this request is doing several things based on different use cases, would it be good if annotation would be passed to it? That way the function that uses this can specify the sender, description, and trigger, precisely.
rhalavati@chromium.org changed reviewers: + pkotwicz@chromium.org, treib@chromium.org
Markus, Annotation moved to arguments, please review: components/image_fetcher/core/* Marc: Please suggest annotations for: chrome/browser/search/thumbnail_source.cc chrome/browser/search/suggestions/suggestions_service_factory.cc chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc Peter: Please suggest annotations for: chrome/browser/favicon/large_icon_service_factory.cc Here is the intro to annotations: 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. Please review modified files and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... 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.
treib@chromium.org changed reviewers: + sfiera@chromium.org
+sfiera for chrome_most_visited_sites_factory https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... File chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:117: net::NetworkTrafficAnnotationTag traffic_annotation = icon_cacher_traffic_annotation https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:90: net::NetworkTrafficAnnotationTag traffic_annotation = image_manager_traffic_annotation (otherwise it's easy to mistake it for the "main" annotation for the SuggestionsService) https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:91: net::DefineNetworkTrafficAnnotation("...", R"( suggestions_service https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:93: sender: "..." SuggestionsService https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:94: description: "..." Retrieves thumbnails for site suggestions based on the user's synced browsing history, for use e.g. on the New Tab page. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:95: trigger: "..." Triggered when a thumbnail for a suggestion is required, and no local thumbnail is available. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:96: data: "..." None. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:97: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:100: cookies_allowed: false/true false https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:102: setting: "..." This cannot be disabled individually, but users can disable the whole SuggestionsService, see here: https://cs.chromium.org/chromium/src/components/suggestions/suggestions_servi... https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:21: net::DefineNetworkTrafficAnnotation("...", R"( thumbnail_source https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:23: sender: "..." ThumbnailSource https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:24: description: "..." Retrieves thumbnails for site suggestions based on the user's synced browsing history, for use e.g. on the New Tab page. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:25: trigger: "..." Triggered when a thumbnail for a suggestion is required (e.g. on the New Tab page), and no local thumbnail is available. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:26: data: "..." None. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:30: cookies_allowed: false/true false https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:32: setting: "..." Same here: Cannot be disabled individually, but users can disable the whole SuggestionsService, see here: https://cs.chromium.org/chromium/src/components/suggestions/suggestions_servi... https://codereview.chromium.org/2794343002/diff/20001/components/image_fetche... File components/image_fetcher/core/image_data_fetcher.h (right): https://codereview.chromium.org/2794343002/diff/20001/components/image_fetche... components/image_fetcher/core/image_data_fetcher.h:98: const net::NetworkTrafficAnnotationTag& traffic_annotation_; This should probably not be a reference, but an instance. Otherwise it'll break if whoever created the fetcher doesn't hold on to the annotation object it created (which I think is most cases).
https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... File chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:120: sender: "..." Popular Sites New Tab Fetch https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:121: description: "..." Google Chrome may display a list of regionally-popular web sites on the New Tab Page. This service fetches icons from those sites. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:122: trigger: "..." Whenever a popular site would be displayed, but its icon is not yet cached in the browser. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:123: data: "..." None https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:124: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL WEBSITE https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:127: cookies_allowed: false/true false https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:129: setting: "..." This is in progress—an experiment exists, but nothing generally available yet. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:151: profile->GetRequestContext(), traffic_annotation)), Isn't it necessary to update the iOS factory too? It feels wrong to me for the factory to declare this and not the code that uses the fetcher. Can this be bassed as part of StartOrQueueNetworkRequest()?
All comments addressed, please review. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... File chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:117: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/09 09:43:46, Marc Treib wrote: > icon_cacher_traffic_annotation Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:120: sender: "..." On 2017/05/09 10:08:17, sfiera wrote: > Popular Sites New Tab Fetch Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:121: description: "..." On 2017/05/09 10:08:17, sfiera wrote: > Google Chrome may display a list of regionally-popular web sites on the New Tab > Page. This service fetches icons from those sites. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:122: trigger: "..." On 2017/05/09 10:08:17, sfiera wrote: > Whenever a popular site would be displayed, but its icon is not yet cached in > the browser. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:123: data: "..." On 2017/05/09 10:08:17, sfiera wrote: > None Shouldn't it be URL of the required icon? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:124: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/09 10:08:17, sfiera wrote: > WEBSITE Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:127: cookies_allowed: false/true On 2017/05/09 10:08:17, sfiera wrote: > false Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:129: setting: "..." On 2017/05/09 10:08:17, sfiera wrote: > This is in progress—an experiment exists, but nothing generally available yet. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:151: profile->GetRequestContext(), traffic_annotation)), On 2017/05/09 10:08:17, sfiera wrote: > Isn't it necessary to update the iOS factory too? > > It feels wrong to me for the factory to declare this and not the code that uses > the fetcher. Can this be bassed as part of StartOrQueueNetworkRequest()? I didn't quite get it. You mean instead of constructor of image_fetcher_imp, annotation would be given to StartOrQueueNetworkRequest? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:90: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/09 09:43:47, Marc Treib wrote: > image_manager_traffic_annotation (otherwise it's easy to mistake it for the > "main" annotation for the SuggestionsService) Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:91: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/09 09:43:46, Marc Treib wrote: > suggestions_service Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:93: sender: "..." On 2017/05/09 09:43:46, Marc Treib wrote: > SuggestionsService Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:94: description: "..." On 2017/05/09 09:43:46, Marc Treib wrote: > Retrieves thumbnails for site suggestions based on the user's synced browsing > history, for use e.g. on the New Tab page. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:95: trigger: "..." On 2017/05/09 09:43:46, Marc Treib wrote: > Triggered when a thumbnail for a suggestion is required, and no local thumbnail > is available. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:96: data: "..." On 2017/05/09 09:43:46, Marc Treib wrote: > None. Shouldn't it sent links to required thumbnails? And aren't they based on user history? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:97: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/09 09:43:46, Marc Treib wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:100: cookies_allowed: false/true On 2017/05/09 09:43:46, Marc Treib wrote: > false Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_factory.cc:102: setting: "..." On 2017/05/09 09:43:46, Marc Treib wrote: > This cannot be disabled individually, but users can disable the whole > SuggestionsService, see here: > https://cs.chromium.org/chromium/src/components/suggestions/suggestions_servi... Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:21: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/09 09:43:47, Marc Treib wrote: > thumbnail_source Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:23: sender: "..." On 2017/05/09 09:43:47, Marc Treib wrote: > ThumbnailSource Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:24: description: "..." On 2017/05/09 09:43:47, Marc Treib wrote: > Retrieves thumbnails for site suggestions based on the user's synced browsing > history, for use e.g. on the New Tab page. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:25: trigger: "..." On 2017/05/09 09:43:47, Marc Treib wrote: > Triggered when a thumbnail for a suggestion is required (e.g. on the New Tab > page), and no local thumbnail is available. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:26: data: "..." On 2017/05/09 09:43:47, Marc Treib wrote: > None. Doesn't it sent the URL to that thumbnail? Isn't it related to user history? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:27: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL GOOGLE_OWNED_SERVICE? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:30: cookies_allowed: false/true On 2017/05/09 09:43:47, Marc Treib wrote: > false Done. https://codereview.chromium.org/2794343002/diff/20001/components/image_fetche... File components/image_fetcher/core/image_data_fetcher.h (right): https://codereview.chromium.org/2794343002/diff/20001/components/image_fetche... components/image_fetcher/core/image_data_fetcher.h:98: const net::NetworkTrafficAnnotationTag& traffic_annotation_; On 2017/05/09 09:43:47, Marc Treib wrote: > This should probably not be a reference, but an instance. Otherwise it'll break > if whoever created the fetcher doesn't hold on to the annotation object it > created (which I think is most cases). Done.
https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... File chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:123: data: "..." On 2017/05/09 11:08:23, Ramin Halavati wrote: > On 2017/05/09 10:08:17, sfiera wrote: > > None > > Shouldn't it be URL of the required icon? Well, yes, all HTTP requests include the URL of the thing they're requesting, but is that useful to list here? https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:151: profile->GetRequestContext(), traffic_annotation)), On 2017/05/09 11:08:23, Ramin Halavati wrote: > On 2017/05/09 10:08:17, sfiera wrote: > > Isn't it necessary to update the iOS factory too? > > > > It feels wrong to me for the factory to declare this and not the code that > uses > > the fetcher. Can this be bassed as part of StartOrQueueNetworkRequest()? > > I didn't quite get it. You mean instead of constructor of image_fetcher_imp, > annotation would be given to StartOrQueueNetworkRequest? Yes. I think it's better for the annotation to be declared once by IconCacherImpl, rather than multiple times by different factories. Only IconCacherImpl can really say how it uses the image fetcher.
https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:26: data: "..." On 2017/05/09 11:08:24, Ramin Halavati wrote: > On 2017/05/09 09:43:47, Marc Treib wrote: > > None. > > Doesn't it sent the URL to that thumbnail? Isn't it related to user history? Right, the request URL includes the URL of the page for which we want a thumbnail as a parameter. It looks like this: https://www.google.com/webpagethumbnail?c=63&d=https://cs.chromium.org/search.... So how about: The URL for which to retrieve a thumbnail. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:27: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/09 11:08:24, Ramin Halavati wrote: > GOOGLE_OWNED_SERVICE? Yes. Sorry, missed that one.
Patchset #4 (id:60001) has been deleted
All comments addressed. I moved the annotations based on my understanding of the new locations, please review. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... File chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/ntp_tile... chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.cc:123: data: "..." On 2017/05/09 11:20:20, sfiera wrote: > On 2017/05/09 11:08:23, Ramin Halavati wrote: > > On 2017/05/09 10:08:17, sfiera wrote: > > > None > > > > Shouldn't it be URL of the required icon? > > Well, yes, all HTTP requests include the URL of the thing they're requesting, > but is that useful to list here? Acknowledged. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:26: data: "..." On 2017/05/09 11:38:34, Marc Treib wrote: > On 2017/05/09 11:08:24, Ramin Halavati wrote: > > On 2017/05/09 09:43:47, Marc Treib wrote: > > > None. > > > > Doesn't it sent the URL to that thumbnail? Isn't it related to user history? > > Right, the request URL includes the URL of the page for which we want a > thumbnail as a parameter. It looks like this: > https://www.google.com/webpagethumbnail?c=63&d=https://cs.chromium.org/search.... > > So how about: > The URL for which to retrieve a thumbnail. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:27: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/09 11:38:34, Marc Treib wrote: > On 2017/05/09 11:08:24, Ramin Halavati wrote: > > GOOGLE_OWNED_SERVICE? > > Yes. Sorry, missed that one. Done. https://codereview.chromium.org/2794343002/diff/20001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:32: setting: "..." On 2017/05/09 09:43:47, Marc Treib wrote: > Same here: Cannot be disabled individually, but users can disable the whole > SuggestionsService, see here: > https://cs.chromium.org/chromium/src/components/suggestions/suggestions_servi... Done.
LGTM for ntp_tiles with one change Thanks, this location looks much better. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_tiles/ic... File components/ntp_tiles/icon_cacher_impl.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/ntp_tiles/ic... components/ntp_tiles/icon_cacher_impl.cc:92: data: "The URL for which to retrieve a thumbnail." This does not fetch a thumbnail, just an icon. e.g. it might be a request for https://chromium.org/favicon.ico.
https://codereview.chromium.org/2794343002/diff/80001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/80001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:76: chrome_policy { nit: Misaligned? (I think SyncDisabled above is indented one level too much) https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:292: net::NetworkTrafficAnnotationTag traffic_annotation = I think this one was copied from the wrong place. This instance is quite different. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:293: net::DefineNetworkTrafficAnnotation("suggestions_service_factory", R"( remote_suggestions_service https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:295: sender: "suggestions_service" Content Suggestion thumbnail fetch https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:298: "synced browsing history, for use e.g. on the New Tab page." Retrieves thumbnails for content suggestions, for display on the New Tab page or Chrome Home. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:301: "local thumbnail is available." Triggered when the user looks at a content suggestion (and its thumbnail isn't cached yet). https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:302: data: "The URL for which to retrieve a thumbnail." None. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:308: "Users can disable this feature by signing out of Chrome, or " Currently not available, but in progress: crbug.com/703684 https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:311: chrome_policy { chrome_policy { NTPContentSuggestionsEnabled { policy_options {mode: MANDATORY} NTPContentSuggestionsEnabled: false } } https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... File components/suggestions/image_manager.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:44: net::DefineNetworkTrafficAnnotation("image_manager", R"( nit: I don't know what this name is used for, but "image_manager" seems overly generic. Maybe at least "suggestions_image_manager"? https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:46: sender: "suggestions_service" optional: This field seems to be a bit more verbose in other places. "SuggestionsService thumbnail fetch"? https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:63: SyncDisabled { Also here: misaligned?
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Thank you very much, all comments addressed, please review. https://codereview.chromium.org/2794343002/diff/80001/chrome/browser/search/t... File chrome/browser/search/thumbnail_source.cc (right): https://codereview.chromium.org/2794343002/diff/80001/chrome/browser/search/t... chrome/browser/search/thumbnail_source.cc:76: chrome_policy { On 2017/05/09 13:40:36, Marc Treib wrote: > nit: Misaligned? (I think SyncDisabled above is indented one level too much) Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:292: net::NetworkTrafficAnnotationTag traffic_annotation = On 2017/05/09 13:40:36, Marc Treib wrote: > I think this one was copied from the wrong place. This instance is quite > different. Acknowledged. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:293: net::DefineNetworkTrafficAnnotation("suggestions_service_factory", R"( On 2017/05/09 13:40:36, Marc Treib wrote: > remote_suggestions_service Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:295: sender: "suggestions_service" On 2017/05/09 13:40:36, Marc Treib wrote: > Content Suggestion thumbnail fetch Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:298: "synced browsing history, for use e.g. on the New Tab page." On 2017/05/09 13:40:36, Marc Treib wrote: > Retrieves thumbnails for content suggestions, for display on the New Tab page or > Chrome Home. Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:301: "local thumbnail is available." On 2017/05/09 13:40:36, Marc Treib wrote: > Triggered when the user looks at a content suggestion (and its thumbnail isn't > cached yet). Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:302: data: "The URL for which to retrieve a thumbnail." On 2017/05/09 13:40:36, Marc Treib wrote: > None. Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:308: "Users can disable this feature by signing out of Chrome, or " On 2017/05/09 13:40:36, Marc Treib wrote: > Currently not available, but in progress: crbug.com/703684 Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:311: chrome_policy { On 2017/05/09 13:40:36, Marc Treib wrote: > chrome_policy { > NTPContentSuggestionsEnabled { > policy_options {mode: MANDATORY} > NTPContentSuggestionsEnabled: false > } > } Done. https://codereview.chromium.org/2794343002/diff/80001/components/ntp_tiles/ic... File components/ntp_tiles/icon_cacher_impl.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/ntp_tiles/ic... components/ntp_tiles/icon_cacher_impl.cc:92: data: "The URL for which to retrieve a thumbnail." On 2017/05/09 13:25:55, sfiera wrote: > This does not fetch a thumbnail, just an icon. > > e.g. it might be a request for https://chromium.org/favicon.ico. Done. https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... File components/suggestions/image_manager.cc (right): https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:44: net::DefineNetworkTrafficAnnotation("image_manager", R"( On 2017/05/09 13:40:36, Marc Treib wrote: > nit: I don't know what this name is used for, but "image_manager" seems overly > generic. Maybe at least "suggestions_image_manager"? Done. https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:46: sender: "suggestions_service" On 2017/05/09 13:40:36, Marc Treib wrote: > optional: This field seems to be a bit more verbose in other places. > "SuggestionsService thumbnail fetch"? Done. https://codereview.chromium.org/2794343002/diff/80001/components/suggestions/... components/suggestions/image_manager.cc:63: SyncDisabled { On 2017/05/09 13:40:36, Marc Treib wrote: > Also here: misaligned? Done.
Thanks! c/b/search, components/image_fetcher, components/ntp_snippets, components/suggestions all LGTM. https://codereview.chromium.org/2794343002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2794343002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:293: net::DefineNetworkTrafficAnnotation("remote_suggestions_service", R"( Sorry, this should be remote_suggestions_provider
components/favicon LGTM
Thank you very much, comment addressed. pkotwicz@: Thank you but I also need you to fill the missing items for "components/favicon/core/large_icon_service.cc". I repeat the intro here if required: Please review the modified file and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. https://codereview.chromium.org/2794343002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right): https://codereview.chromium.org/2794343002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:293: net::DefineNetworkTrafficAnnotation("remote_suggestions_service", R"( On 2017/05/09 14:08:19, Marc Treib wrote: > Sorry, this should be remote_suggestions_provider Done.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #7 (id:180001) has been deleted
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:200002) has been deleted
rhalavati@chromium.org changed reviewers: + achuith@chromium.org, bauerb@chromium.org, gambard@chromium.org
I have added a few Android and iOS files for compatibility issues. As we don't need to annotate them at this phase, I've just added flags stating that no network annotation is needed. gambard@, Could you please review "ios_image_data_fetcher_wrapper.mm" bauerb@: Could you please review "logo_bridge.cc" achuith@: Could you please review "chrome/browser/chromeos/hats/*"
android LGTM
ios_image_data_fetcher_wrapper.mm lgtm
https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:343: net::DefineNetworkTrafficAnnotation("...", R"( favicon_component https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:345: sender: "..." Favicon component https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:346: description: "..." Sends a request to a Google server to retrieve the favicon bitmap for a page URL https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: trigger: "..." A request can be sent for signed-in users when Chrome does not have a favicon for a particular page. For instance, this is used by the new tab page for the "most visited" tiles. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:348: data: "..." Page URL and desired icon size https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:349: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:361: policy_exception_justification: "..." For the policy block, I think it should say this: policy { cookies_allowed: false setting: "This feature cannot be disabled by settings." policy_exception_justification: "Not implemented." }
https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:343: net::DefineNetworkTrafficAnnotation("...", R"( favicon_component https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:345: sender: "..." Favicon component https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:346: description: "..." Sends a request to a Google server to retrieve the favicon bitmap for a page URL https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: trigger: "..." A request can be sent for signed-in users when Chrome does not have a favicon for a particular page. For instance, this is used by the new tab page for the "most visited" tiles. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:348: data: "..." Page URL and desired icon size https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:349: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:361: policy_exception_justification: "..." For the policy block, I think it should say this: policy { cookies_allowed: false setting: "This feature cannot be disabled by settings." policy_exception_justification: "Not implemented." }
Peter, Thank you very much, Favicon annotation updated, please review. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:343: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/12 17:10:31, pkotwicz wrote: > favicon_component Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:345: sender: "..." On 2017/05/12 17:10:31, pkotwicz wrote: > Favicon component Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:346: description: "..." On 2017/05/12 17:10:31, pkotwicz wrote: > Sends a request to a Google server to retrieve the favicon bitmap for a page URL Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: trigger: "..." On 2017/05/12 17:10:31, pkotwicz wrote: > A request can be sent for signed-in users when Chrome does not have a favicon > for a particular page. For instance, this is used by the new tab page for the > "most visited" tiles. Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:348: data: "..." On 2017/05/12 17:10:31, pkotwicz wrote: > Page URL and desired icon size Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:349: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/12 17:10:31, pkotwicz wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2794343002/diff/250001/components/favicon/cor... components/favicon/core/large_icon_service.cc:361: policy_exception_justification: "..." On 2017/05/12 17:10:31, pkotwicz wrote: > For the policy block, I think it should say this: > > policy { > cookies_allowed: false > setting: "This feature cannot be disabled by settings." > policy_exception_justification: "Not implemented." > } Done.
components/favicon/ LGTM
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Peter. Martin, Any comments?
https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google server to retrieve the favicon bitmap " Since the payload is sensitive, can you please elaborate more where the URL comes from?
On 2017/05/18 11:25:18, msramek wrote: > https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... > File components/favicon/core/large_icon_service.cc (right): > > https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... > components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google > server to retrieve the favicon bitmap " > Since the payload is sensitive, can you please elaborate more where the URL > comes from? Peter, Could you please elaborate on this?
https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google server to retrieve the favicon bitmap " There are multiple places where the payload can come from as GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() is used by multiple call sites Currently the NTP is the only caller for NTP thumbnails and content suggestions. I think that the iOS folks want to use this for bookmarks. In crbug.com/672478 there is a proposal of using this function for passwords in chrome://settings
https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google server to retrieve the favicon bitmap " On 2017/05/18 15:47:43, pkotwicz wrote: > There are multiple places where the payload can come from as > GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() is used by > multiple call sites > > Currently the NTP is the only caller for NTP thumbnails and content suggestions. > I think that the iOS folks want to use this for bookmarks. In crbug.com/672478 > there is a proposal of using this function for passwords in chrome://settings So, my point is that we generally only share user's URLs with Google if history sync is enabled. Since history sync is not listed as a setting that controls this request, I would like this annotation to justify the cases where we send URL without history sync enabled. Please don't worry about the future proposals now - they will have to go through a privacy review, where we can remind them to update the annotation. If you think that the callers are too varied to be listed here, perhaps we should split annotation so that the description is defined on upper layers. This is not supported right now, but will be soon.
There is this warning in the comment in LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() // WARNING: This function will share the |page_url| with a Google server. This // Can be used only for urls that are not privacy sensitive or for users that // sync their history with Google servers. How would I list "signed into Chrome" as a setting which controls this request My understanding is that this function is only called when the user is signed into Google.
Description was changed from ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 ==========
pkotwicz@chromium.org changed reviewers: + jkrcal@chromium.org
Description was changed from ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 ==========
pkotwicz@chromium.org changed reviewers: + jkrcal@chromium.org
I have added jkrcal@ as a reviewer who is familiar about the callers of GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() (and who wrote the function)
chromeos lgtm
https://codereview.chromium.org/2794343002/diff/270001/chrome/browser/chromeo... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2794343002/diff/270001/chrome/browser/chromeo... chrome/browser/chromeos/hats/hats_notification_controller.cc:239: NO_TRAFFIC_ANNOTATION_YET); Are we going to change this to something more specific in the future?
large_icon_service* lgtm Thanks! https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google server to retrieve the favicon bitmap " On 2017/05/18 16:02:16, msramek wrote: > On 2017/05/18 15:47:43, pkotwicz wrote: > > There are multiple places where the payload can come from as > > GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() is used by > > multiple call sites > > > > Currently the NTP is the only caller for NTP thumbnails and content > suggestions. > > I think that the iOS folks want to use this for bookmarks. In crbug.com/672478 > > there is a proposal of using this function for passwords in chrome://settings > > So, my point is that we generally only share user's URLs with Google if history > sync is enabled. Since history sync is not listed as a setting that controls > this request, I would like this annotation to justify the cases where we send > URL without history sync enabled. > > Please don't worry about the future proposals now - they will have to go through > a privacy review, where we can remind them to update the annotation. > > If you think that the callers are too varied to be listed here, perhaps we > should split annotation so that the description is defined on upper layers. This > is not supported right now, but will be soon. I would replace the "For instance, ..." sentence by: This is used: - for article suggestions on the new tab page (URLs are public and provided by Google), - for server-suggested most visited tiles on the new tab page (user gets these URLs from Google, only if history sync is enabled). I agree with privacy requiring each new user to update the annotation.
Thank you very much, comments addressed. Marc, Could you please check the newly added file: components/doodle/doodle_service.cc https://codereview.chromium.org/2794343002/diff/270001/chrome/browser/chromeo... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2794343002/diff/270001/chrome/browser/chromeo... chrome/browser/chromeos/hats/hats_notification_controller.cc:239: NO_TRAFFIC_ANNOTATION_YET); On 2017/05/18 21:29:26, achuithb wrote: > Are we going to change this to something more specific in the future? Yes, at this stage, our focus is to cover annotations for desktop Windows and Linux. We will cover other targets later. https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/270001/components/favicon/cor... components/favicon/core/large_icon_service.cc:347: "Sends a request to a Google server to retrieve the favicon bitmap " On 2017/05/19 06:18:21, jkrcal wrote: > On 2017/05/18 16:02:16, msramek wrote: > > On 2017/05/18 15:47:43, pkotwicz wrote: > > > There are multiple places where the payload can come from as > > > GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache() is used by > > > multiple call sites > > > > > > Currently the NTP is the only caller for NTP thumbnails and content > > suggestions. > > > I think that the iOS folks want to use this for bookmarks. In > crbug.com/672478 > > > there is a proposal of using this function for passwords in > chrome://settings > > > > So, my point is that we generally only share user's URLs with Google if > history > > sync is enabled. Since history sync is not listed as a setting that controls > > this request, I would like this annotation to justify the cases where we send > > URL without history sync enabled. > > > > Please don't worry about the future proposals now - they will have to go > through > > a privacy review, where we can remind them to update the annotation. > > > > If you think that the callers are too varied to be listed here, perhaps we > > should split annotation so that the description is defined on upper layers. > This > > is not supported right now, but will be soon. > > I would replace the "For instance, ..." sentence by: > This is used: > - for article suggestions on the new tab page (URLs are public and provided by > Google), > - for server-suggested most visited tiles on the new tab page (user gets these > URLs from Google, only if history sync is enabled). > > I agree with privacy requiring each new user to update the annotation. Done.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:111: net::DefineNetworkTrafficAnnotation("...", R"( doodle_service https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:113: sender: "..." Doodle service https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:114: description: "..." Downloads the Doodle image if Google is your configured search provider. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:115: trigger: "..." Displaying the new tab page on Android. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:116: data: "..." None. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:117: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:121: setting: "..." Choosing a non-Google search engine in Chromium settings under 'Search Engine' will disable this feature. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:128: policy_exception_justification: "..." Not implemented, considered not useful as it does not upload any data and just downloads a logo image.
Thank you very much Marc. Please review. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:111: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/05/19 08:29:11, Marc Treib wrote: > doodle_service Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:113: sender: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > Doodle service Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:114: description: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > Downloads the Doodle image if Google is your configured search provider. Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:115: trigger: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > Displaying the new tab page on Android. Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:116: data: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > None. Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:117: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/19 08:29:12, Marc Treib wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:121: setting: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > Choosing a non-Google search engine in Chromium settings under 'Search Engine' > will disable this feature. Done. https://codereview.chromium.org/2794343002/diff/310001/components/doodle/dood... components/doodle/doodle_service.cc:128: policy_exception_justification: "..." On 2017/05/19 08:29:12, Marc Treib wrote: > Not implemented, considered not useful as it does not upload any data and just > downloads a logo image. Done.
components/doodle LGTM
Thanks a lot for the clarification! LGTM % few more comments. https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... components/doodle/doodle_service.cc:127: "Not implemented, considered not useful as it does not upload any " Wouldn't this be https://www.chromium.org/administrators/policy-list-3#DefaultSearchProviderEn... ? https://codereview.chromium.org/2794343002/diff/330001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/favicon/cor... components/favicon/core/large_icon_service.cc:388: setting: "This feature cannot be disabled by settings." Since the trigger says "signed-in users", should this be SignInAllowed? https://codereview.chromium.org/2794343002/diff/330001/components/suggestions... File components/suggestions/image_manager.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/suggestions... components/suggestions/image_manager.cc:59: "Users can disable this feature by signing out of Chrome, or " nit: Offset.
Thank you very much Martin, comment addressed. Marc, Please review the change in: components/doodle/doodle_service.cc https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... components/doodle/doodle_service.cc:127: "Not implemented, considered not useful as it does not upload any " On 2017/05/19 08:47:38, msramek wrote: > Wouldn't this be > https://www.chromium.org/administrators/policy-list-3#DefaultSearchProviderEn... > ? Sorry, missed again. https://codereview.chromium.org/2794343002/diff/330001/components/favicon/cor... File components/favicon/core/large_icon_service.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/favicon/cor... components/favicon/core/large_icon_service.cc:388: setting: "This feature cannot be disabled by settings." On 2017/05/19 08:47:38, msramek wrote: > Since the trigger says "signed-in users", should this be SignInAllowed? But scenario one is not covered by this policy. I updated trigger instead. https://codereview.chromium.org/2794343002/diff/330001/components/suggestions... File components/suggestions/image_manager.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/suggestions... components/suggestions/image_manager.cc:59: "Users can disable this feature by signing out of Chrome, or " On 2017/05/19 08:47:38, msramek wrote: > nit: Offset. Done.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
doodle_service.cc still lgtm https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... components/doodle/doodle_service.cc:127: "Not implemented, considered not useful as it does not upload any " On 2017/05/19 09:03:37, Ramin Halavati wrote: > On 2017/05/19 08:47:38, msramek wrote: > > Wouldn't this be > > > https://www.chromium.org/administrators/policy-list-3#DefaultSearchProviderEn... > > ? > > Sorry, missed again. Yep, sounds like this policy would do the trick (if you force the default search engine to be non-Google). The same then also applies to a few other annotation, like in doodle_fetcher_impl.cc.
On 2017/05/19 09:19:24, Marc Treib wrote: > doodle_service.cc still lgtm > > https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... > File components/doodle/doodle_service.cc (right): > > https://codereview.chromium.org/2794343002/diff/330001/components/doodle/dood... > components/doodle/doodle_service.cc:127: "Not implemented, considered not useful > as it does not upload any " > On 2017/05/19 09:03:37, Ramin Halavati wrote: > > On 2017/05/19 08:47:38, msramek wrote: > > > Wouldn't this be > > > > > > https://www.chromium.org/administrators/policy-list-3#DefaultSearchProviderEn... > > > ? > > > > Sorry, missed again. > > Yep, sounds like this policy would do the trick (if you force the default search > engine to be non-Google). > The same then also applies to a few other annotation, like in > doodle_fetcher_impl.cc. Thank you Marc. I will updated that in another CL, we will do a review on all annotations hopefully in the next few weeks, and will update all such missing cases.
The CQ bit was unchecked by rhalavati@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 sfiera@chromium.org, gambard@chromium.org, bauerb@chromium.org, pkotwicz@chromium.org, jkrcal@chromium.org, achuith@chromium.org, msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2794343002/#ps350001 (title: "Comments addressed.")
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": 350001, "attempt_start_ts": 1495185954952720,
"parent_rev": "c3a8b7831c0598f303b34495a7396f06d4760d8e", "commit_rev":
"3b72ae90e7307aa787ad0001121813a8d873e693"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by rhalavati@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
For LargeIconService I think that "perhaps we should split annotation so that the description is defined on upper layers. This is not supported right now, but will be soon." makes sense
CQ is committing da patch.
Bot data: {"patchset_id": 350001, "attempt_start_ts": 1495205225869900,
"parent_rev": "3fe737215d4305f8766db04b2a4dfe15087b5b9a", "commit_rev":
"6072a6b7d7d0992e45f8cc64cc62d2e8b766786c"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to image_data_fetcher. Network traffic annotation is added to network request of components/image_fetcher/core/image_data_fetcher.cc BUG=656607 Review-Url: https://codereview.chromium.org/2794343002 Cr-Commit-Position: refs/heads/master@{#473188} Committed: https://chromium.googlesource.com/chromium/src/+/6072a6b7d7d0992e45f8cc64cc62... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:350001) as https://chromium.googlesource.com/chromium/src/+/6072a6b7d7d0992e45f8cc64cc62...
Message was sent while issue was closed.
On 2017/05/19 14:51:15, pkotwicz wrote: > For LargeIconService I think that > "perhaps we should split annotation so that the description is defined on upper > layers. This > is not supported right now, but will be soon." > makes sense OK Peter, I will land this CL, then create a new one and modify just this file. |
