Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682263002:
Network traffic annotation added to chrome::BitmapFetcher. (Closed)
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 10 months ago Reviewers:
vasilii, David Trainor- moved to gerrit, Peter Kasting, Mike Lerman, Devlin, gone, Rahul Chaturvedi, Justin Donnelly, tbarzic, battre CC:
chromium-reviews, jdonnelly+watch_chromium.org, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org, msramek, Rahul Chaturvedi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to chrome::BitmapFetcher.
Network traffic annotation is added to network request(s) of chrome::BitmapFetcher and all functions that are sourcing calls to it.
BUG=656607
Review-Url: https://codereview.chromium.org/2682263002
Cr-Commit-Position: refs/heads/master@{#452793}
Committed: https://chromium.googlesource.com/chromium/src/+/e9cefd65b1f3a9f4648852cb25933894e994022f
Patch Set 1 : Annotations added. #Patch Set 2 : Unittests added. #
Total comments: 26
Patch Set 3 : nits #Patch Set 4 : Comments addressed. #
Total comments: 8
Patch Set 5 : nits #Patch Set 6 : Extra file removed. #
Total comments: 18
Patch Set 7 : Comments Addressed. #Patch Set 8 : Minor correction. #
Total comments: 31
Patch Set 9 : Annotations updated. #
Total comments: 5
Patch Set 10 : Comments addressed. #
Total comments: 6
Patch Set 11 : nits #
Total comments: 9
Patch Set 12 : Annotations updated. #Patch Set 13 : Android compatibility. #Patch Set 14 : Android compatibility. #
Total comments: 5
Patch Set 15 : Minor update. #
Total comments: 3
Patch Set 16 : Chrome changed to Chromium #
Total comments: 2
Patch Set 17 : nits #Messages
Total messages: 66 (16 generated)
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 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/chrome/tools/traffic_annotation/traffic_... for the definition of the annotations. You can find a sample annotation in https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s.... 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. dfalcantara@: I've modified 6 files in browser/bitmap_fetcher and added NetworkTrafficAnnotationTag as input parameter to BitmapFetcher, please review them. pkasting@: I've modified 2 files in browser/autocomplete and browser/ui/omnibox, and added annotation template to them. Please review them and suggest the required answers for the empty parts. vasilii@: I've modified 2 files in browser/ui/passwords and have added annotation template to it. Please review them and suggest the required answers for the empty parts. rdevlin.cronin@: I've modified webstore_install_helper.cc and have added annotation template to it. Please review it and suggest the required answers for the empty parts. mlerman@: I've modified profile_avatar_downloader.cc and have added annotation template to it. Please review it and suggest the required answers for the empty parts.
On 2017/02/09 13:14:32, 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. > > 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/chrome/tools/traffic_annotation/traffic_... > for the definition of the annotations. > > You can find a sample annotation in > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s.... > > 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. > > > dfalcantara@: > I've modified 6 files in browser/bitmap_fetcher and added > NetworkTrafficAnnotationTag as input parameter to BitmapFetcher, please review > them. > > pkasting@: > I've modified 2 files in browser/autocomplete and browser/ui/omnibox, and added > annotation template to them. Please review them and suggest the required answers > for the empty parts. > > vasilii@: > I've modified 2 files in browser/ui/passwords and have added annotation template > to it. Please review them and suggest the required answers for the empty parts. > > rdevlin.cronin@: > I've modified webstore_install_helper.cc and have added annotation template to > it. Please review it and suggest the required answers for the empty parts. > > mlerman@: > I've modified profile_avatar_downloader.cc and have added annotation template to > it. Please review it and suggest the required answers for the empty parts. Sorry, typo: 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.
https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:34: sender: "" SEMANTICS: sender: profiles description: The Google Chrome binary comes bundles with low-resolution versions of the people avatars that are selecting in chrome://settings. When an avatar is chosen for a profile, Chrome will download the high-resolution version from Google's static content servers, for use in the people manager UI. trigger: User selects a new avatar in chrome://settings for their profile data: none; only the filename of the png to download. destination: GOOGLE_OWNED_SERVICE POLICY: policy_exception_justification: No content is being uploaded or saved; this request merely downloads a publicly available PNG file.
https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:18: net::DefineNetworkTrafficAnnotation("", R"( "credenential avatar" https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:20: sender: "" Chrome Password manager https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:21: description: "" Every credential saved in Chrome via the Credential Management API can have an avatar URL. The URL is essentially provided by the site calling the API. The avatar is used in the account chooser UI and auto signin toast which appear when a site calls navigator.credentials.get(). The avatar is retrieved before showing the UI. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:22: trigger: "" User visits a site that calls navigator.credentials.get(). Assuming there are matching credentials in the Chrome password store, the avatars are retrieved. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:23: data: "" No outbound data. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:24: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER WEBSITE https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:27: cookies_allowed: false/true false https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:29: setting: "" One can disable saving new credentials in the settings (see "Passwords and forms"). There is no setting to disable the API. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:30: policy { N/A https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:37: fetcher_.reset(new chrome::BitmapFetcher(url, this, traffic_annotation)); I don't think we need a unique_ptr here. Can you create a function that returns net::NetworkTrafficAnnotationTag and simply init fetcher_ the same way as |delegate_|? https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.h (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.h:42: std::unique_ptr<chrome::BitmapFetcher> fetcher_; Fix the indent.
All comments addressed, review please. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:34: sender: "" On 2017/02/09 13:40:16, Mike Lerman wrote: > SEMANTICS: > > sender: profiles > description: The Google Chrome binary comes bundles with low-resolution versions > of the people avatars that are selecting in chrome://settings. When an avatar is > chosen for a profile, Chrome will download the high-resolution version from > Google's static content servers, for use in the people manager UI. > trigger: User selects a new avatar in chrome://settings for their profile > data: none; only the filename of the png to download. > destination: GOOGLE_OWNED_SERVICE > > POLICY: > policy_exception_justification: No content is being uploaded or saved; this > request merely downloads a publicly available PNG file. Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:18: net::DefineNetworkTrafficAnnotation("", R"( On 2017/02/09 13:47:46, vasilii wrote: > "credenential avatar" Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:20: sender: "" On 2017/02/09 13:47:45, vasilii wrote: > Chrome Password manager Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:21: description: "" On 2017/02/09 13:47:45, vasilii wrote: > Every credential saved in Chrome via the Credential Management API can have an > avatar URL. The URL is essentially provided by the site calling the API. The > avatar is used in the account chooser UI and auto signin toast which appear when > a site calls navigator.credentials.get(). The avatar is retrieved before showing > the UI. Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:22: trigger: "" On 2017/02/09 13:47:45, vasilii wrote: > User visits a site that calls navigator.credentials.get(). Assuming there are > matching credentials in the Chrome password store, the avatars are retrieved. Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:23: data: "" On 2017/02/09 13:47:45, vasilii wrote: > No outbound data. Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:24: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/09 13:47:45, vasilii wrote: > WEBSITE Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:27: cookies_allowed: false/true On 2017/02/09 13:47:46, vasilii wrote: > false Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:29: setting: "" On 2017/02/09 13:47:45, vasilii wrote: > One can disable saving new credentials in the settings (see "Passwords and > forms"). There is no setting to disable the API. Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:30: policy { On 2017/02/09 13:47:45, vasilii wrote: > N/A Done. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:37: fetcher_.reset(new chrome::BitmapFetcher(url, this, traffic_annotation)); On 2017/02/09 13:47:46, vasilii wrote: > I don't think we need a unique_ptr here. Can you create a function that returns > net::NetworkTrafficAnnotationTag and simply init fetcher_ the same way as > |delegate_|? The tag is now generated by net::DefineNetworkTrafficAnnotation function, but do you suggest that this function call and its entire big input text would be moved up? https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.h (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.h:42: std::unique_ptr<chrome::BitmapFetcher> fetcher_; On 2017/02/09 13:47:46, vasilii wrote: > Fix the indent. Done.
https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:37: fetcher_.reset(new chrome::BitmapFetcher(url, this, traffic_annotation)); On 2017/02/09 14:22:51, Ramin Halavati wrote: > On 2017/02/09 13:47:46, vasilii wrote: > > I don't think we need a unique_ptr here. Can you create a function that > returns > > net::NetworkTrafficAnnotationTag and simply init fetcher_ the same way as > > |delegate_|? > > The tag is now generated by net::DefineNetworkTrafficAnnotation function, but do > you suggest that this function call and its entire big input text would be moved > up? Right, to the anonymous namespace.
profiles LGTM
those files lgtm
https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:40: "content servers, for use in the people manager UI." Grammar: The Google Chrome binary comes with a bundle of low-resolution versions of avatar images. When the user selects an avatar in chrome://settings, Chrome will download a high-resolution version from Google's static content servers for use in the people manager UI. https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:39: } @vasilii: Don't we have a policy to disable the entire password manager?
https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:39: } On 2017/02/10 08:52:34, battre (OOO) wrote: > @vasilii: Don't we have a policy to disable the entire password manager? "PasswordManagerEnabled" is the policy. However, it disables saving new credentials. Obviously, without credentials this class will never be instantiated. However, there is no policy that suppresses the API. https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:51: net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_MAYBE_USER_GESTURE); I liked the previous formatting more.
vasilii@, battre@: Comments addressed, please review. https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/20001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:37: fetcher_.reset(new chrome::BitmapFetcher(url, this, traffic_annotation)); On 2017/02/09 14:25:53, vasilii wrote: > On 2017/02/09 14:22:51, Ramin Halavati wrote: > > On 2017/02/09 13:47:46, vasilii wrote: > > > I don't think we need a unique_ptr here. Can you create a function that > > returns > > > net::NetworkTrafficAnnotationTag and simply init fetcher_ the same way as > > > |delegate_|? > > > > The tag is now generated by net::DefineNetworkTrafficAnnotation function, but > do > > you suggest that this function call and its entire big input text would be > moved > > up? > > Right, to the anonymous namespace. Done. https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_avatar_downloader.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_avatar_downloader.cc:40: "content servers, for use in the people manager UI." On 2017/02/10 08:52:34, battre (OOO) wrote: > Grammar: > > The Google Chrome binary comes with a bundle of low-resolution versions of > avatar images. When the user selects an avatar in chrome://settings, Chrome will > download a high-resolution version from Google's static content servers for use > in the people manager UI. Done. https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:39: } On 2017/02/10 09:38:01, vasilii wrote: > On 2017/02/10 08:52:34, battre (OOO) wrote: > > @vasilii: Don't we have a policy to disable the entire password manager? > > "PasswordManagerEnabled" is the policy. However, it disables saving new > credentials. Obviously, without credentials this class will never be > instantiated. However, there is no policy that suppresses the API. So I think as that policy stops generating this network request, we can still add it here. Do you agree? https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:51: net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_MAYBE_USER_GESTURE); On 2017/02/10 09:38:01, vasilii wrote: > I liked the previous formatting more. Sorry, reverted. git cl format had done it behind my back!
LGTM chrome/browser/ui/passwords/account_avatar_fetcher.cc https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... File chrome/browser/ui/passwords/account_avatar_fetcher.cc (right): https://codereview.chromium.org/2682263002/diff/60001/chrome/browser/ui/passw... chrome/browser/ui/passwords/account_avatar_fetcher.cc:39: } On 2017/02/10 14:40:57, Ramin Halavati wrote: > On 2017/02/10 09:38:01, vasilii wrote: > > On 2017/02/10 08:52:34, battre (OOO) wrote: > > > @vasilii: Don't we have a policy to disable the entire password manager? > > > > "PasswordManagerEnabled" is the policy. However, it disables saving new > > credentials. Obviously, without credentials this class will never be > > instantiated. However, there is no policy that suppresses the API. > > So I think as that policy stops generating this network request, we can still > add it here. Do you agree? I agree.
https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:60: sender: "" Webstore Install Helper https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:61: description: "" Fetches the bitmap corresponding to an extension icon. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:62: trigger: "" This can happen in a few different circumstances: User initiated an install from the Chrome Web Store User initiated an inline installation from another website ...Something with kiosk apps - maybe steel@ knows more about that one. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:63: data: "" The url of the icon for the extension, which includes the extension id (potentially PII). https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:64: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER Chrome Web Store == GOOGLE_OWNED_SERVICE. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:67: cookies_allowed: false/true Good question - how would we determine this? https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:69: setting: "" There's no direct Chrome setting to disable this, but you could uninstall all extensions and not install (or begin the installation flow for) any more. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:72: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} I don't see policy_options in the traffic_annotation proto?
Description was changed from ========== Network traffic annotation added to chrome::BitmapFetcher. Network traffic annotation is added to network request(s) of chrome::BitmapFetcher and all functions that are sourcing calls to it. BUG=656607 ========== to ========== Network traffic annotation added to chrome::BitmapFetcher. Network traffic annotation is added to network request(s) of chrome::BitmapFetcher and all functions that are sourcing calls to it. BUG=656607 ==========
pkasting@chromium.org changed reviewers: + jdonnelly@chromium.org - pkasting@chromium.org
Switching reviewer from me to jdonnelly. Justin, the omnibox/autocomplete changes here are both about fetching images for AiS -- one is prefetching them, one is fetching them. So these should probably both be annotated the same way. (Maybe we should construct the annotation in some common helper?) I think you may have better information than me on how to properly annotate the desired information for these AiS images, e.g. should we document these as "Google owned service", or should we say "website" since in principle other engines could provide answers? Etc. P.S. Seems weird that we prefetch images for both the "first line" and "second line" of an answer but only show images from the "second line". That sounds like a potential bandwidth waste. Maybe something to think about?
rdevlin.cronin@: All comments addressed. steel@: Could you please add comment on chrome/browser/extensions/webstore_install_helper.cc https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:60: sender: "" On 2017/02/13 23:35:56, Devlin wrote: > Webstore Install Helper Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:61: description: "" On 2017/02/13 23:35:56, Devlin wrote: > Fetches the bitmap corresponding to an extension icon. Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:62: trigger: "" On 2017/02/13 23:35:56, Devlin wrote: > This can happen in a few different circumstances: > User initiated an install from the Chrome Web Store > User initiated an inline installation from another website > ...Something with kiosk apps - maybe steel@ knows more about that one. Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:63: data: "" On 2017/02/13 23:35:56, Devlin wrote: > The url of the icon for the extension, which includes the extension id > (potentially PII). Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:64: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/13 23:35:56, Devlin wrote: > Chrome Web Store == GOOGLE_OWNED_SERVICE. Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:67: cookies_allowed: false/true On 2017/02/13 23:35:56, Devlin wrote: > Good question - how would we determine this? In current case, it is specified in the icon_fetcher_->Init() call, where net::LOAD_DO_NOT_SAVE_COOKIES and net::LOAD_DO_NOT_SEND_COOKIES are specified. Hence I set it to false. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:69: setting: "" On 2017/02/13 23:35:56, Devlin wrote: > There's no direct Chrome setting to disable this, but you could uninstall all > extensions and not install (or begin the installation flow for) any more. Done. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:72: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/13 23:35:57, Devlin wrote: > I don't see policy_options in the traffic_annotation proto? The policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on policy settings is here: http://dev.chromium.org/administrators/policy-list-3
https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: sender: "" As pkasting points out, these should be identical to values I specified in my response for chrome_omnibox_client.cc. Can you add a function here that would return the proto and use it in both locations? Or would you prefer I do that in a separate CL? https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:296: sender: "" suggest (Alternatively, "omnibox" if a more general description is preferred. This is the suggest feature of the omnibox component.) https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:297: description: "" Google Chrome provides answers in the suggestion list to the queries you type in the omnibox. This request retrieves a small image (for example, an icon illustrating the current weather conditions) when this can add information to the answer. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:298: trigger: "" Query typed by the user in the omnibox. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:299: data: "" The only data sent is the path to an image. No user data is included, although the general weather condition (sunny, rainy, etc.) in the user's current location could be inferred from the name of the image in the path. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:300: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER Yeah, this is a tough one. Currently, no search engine other than Google provides answers in the suggest feature, though they could do so at any time. Moreover, even the answers provided by Google could point to images on other web sites (this request is for the image, not the suggestions themselves). On the other hand, this feature is almost always set to have Google as the backend and is perceived by the user to be part of the browser. In the balance, I'd choose WEBSITE based on what is possible here. But GOOGLE_OWNED_SERVICE would also be reasonable given the current behavior and how users probably perceive it. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:303: cookies_allowed: false/true false (requests are to ssl.gstatic.com) https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:305: setting: "" Disable ‘Use a prediction service to help complete searches and URLs typed in the address bar' in Chrome’s settings under Advanced. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:307: [POLICY_NAME] { SearchSuggestEnabled https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:308: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} I'm honestly not sure. I see that there's a CloudPolicySettings entry called SearchSuggestEnabled but I don't know how that interacts with the user setting (prefs::kSearchSuggestEnabled). You can see where we consult this pref in IsSuggestEnabled* but there's no explicit check that I can find for the policy. Does the cloud policy just override the user pref somehow? * https://cs.chromium.org/chromium/src/chrome/browser/search/search.cc?type=cs&...
On 2017/02/14 00:57:06, Peter Kasting wrote: > P.S. Seems weird that we prefetch images for both the "first line" and "second > line" of an answer but only show images from the "second line". That sounds > like a potential bandwidth waste. Maybe something to think about? Granted, it's a little odd. We chose the simpler approach in both cases. The fetching code doesn't assume that only the second line can have an image because that would make it more complicated and it is possible in the future we would want that. The UI code does assume only an image in the first line because to allow for both would require additional layout logic that we wouldn't be using. Currently, images are never sent with the first line so no bandwidth is wasted. And we have a good channel of communication with the Suggest team so it's unlikely they would change that without telling us and gating any changes on a Chrome version that included support for images in the first line.
steel@chromium.org changed reviewers: + steel@chromium.org, tbarzic@chromium.org
Toni is more familiar with this code. Adding him.
https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:67: "3-...Something with kiosk apps - maybe steel@ knows more about " Loading of kiosk app data on Chrome OS (provided that the kiosk app is a Web Store app). Basically, if a kiosk app is set on a Chrome OS device, KioskAppManager will use WebstoreInstallHelper to fetch the kiosk app data - primarily app icon and the required platform version information. (note that the fetched data is then cached locally, so this should happen only on initial app load, or if local data get's corrupted).
https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:72: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/14 10:05:54, Ramin Halavati wrote: > On 2017/02/13 23:35:57, Devlin wrote: > > I don't see policy_options in the traffic_annotation proto? > > The policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > And more description on policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 But what are the policy options indicative of? There's quite a giant list there, but what's the question here?
jdonnelly@, rdevlin.cronin@, tbrazic@: Comments addressed, please review. https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:72: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/15 00:36:08, Devlin wrote: > On 2017/02/14 10:05:54, Ramin Halavati wrote: > > On 2017/02/13 23:35:57, Devlin wrote: > > > I don't see policy_options in the traffic_annotation proto? > > > > The policy options are here: > > > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > > > And more description on policy settings is here: > > http://dev.chromium.org/administrators/policy-list-3 > > But what are the policy options indicative of? There's quite a giant list > there, but what's the question here? The questions is if any of the policy options will disable this request. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: sender: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > As pkasting points out, these should be identical to values I specified in my > response for chrome_omnibox_client.cc. Can you add a function here that would > return the proto and use it in both locations? Or would you prefer I do that in > a separate CL? To keep the clang tool that extracts annotation simple and remove the need to use run time data, we would like to have annotations as constants defined in the same compilation unit that initializes the call. Therefore I think if both these functions need exactly similar annotation, may be we can create a function Foo that both these call sites call Foo and Foo creates the annotation and fetches or prefetches the image. How does it sound? https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:67: "3-...Something with kiosk apps - maybe steel@ knows more about " On 2017/02/14 21:24:57, tbarzic wrote: > Loading of kiosk app data on Chrome OS (provided that the kiosk app is a Web > Store app). > > Basically, if a kiosk app is set on a Chrome OS device, KioskAppManager will use > WebstoreInstallHelper to fetch the kiosk app data - primarily app icon and the > required platform version information. (note that the fetched data is then > cached locally, so this should happen only on initial app load, or if local data > get's corrupted). Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:296: sender: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > suggest (Alternatively, "omnibox" if a more general description is preferred. > This is the suggest feature of the omnibox component.) Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:297: description: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > Google Chrome provides answers in the suggestion list to the queries you type in > the omnibox. This request retrieves a small image (for example, an icon > illustrating the current weather conditions) when this can add information to > the answer. Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:298: trigger: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > Query typed by the user in the omnibox. Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:299: data: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > The only data sent is the path to an image. No user data is included, although > the general weather condition (sunny, rainy, etc.) in the user's current > location could be inferred from the name of the image in the path. Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:300: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/14 17:01:05, Justin Donnelly wrote: > Yeah, this is a tough one. Currently, no search engine other than Google > provides answers in the suggest feature, though they could do so at any time. > Moreover, even the answers provided by Google could point to images on other web > sites (this request is for the image, not the suggestions themselves). On the > other hand, this feature is almost always set to have Google as the backend and > is perceived by the user to be part of the browser. > > In the balance, I'd choose WEBSITE based on what is possible here. But > GOOGLE_OWNED_SERVICE would also be reasonable given the current behavior and how > users probably perceive it. Can we add a DCHECK to insure that Google owned service is used here and if it changes in future, the one who changes it changes the annotation as well? https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:303: cookies_allowed: false/true On 2017/02/14 17:01:05, Justin Donnelly wrote: > false (requests are to http://ssl.gstatic.com) I wonder as the default URLFetcher allows to send and store and this is not specified anywhere, should this be false or true? https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:305: setting: "" On 2017/02/14 17:01:05, Justin Donnelly wrote: > Disable ‘Use a prediction service to help complete searches and URLs typed in > the address bar' in Chrome’s settings under Advanced. Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:307: [POLICY_NAME] { On 2017/02/14 17:01:05, Justin Donnelly wrote: > SearchSuggestEnabled Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:308: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/14 17:01:05, Justin Donnelly wrote: > I'm honestly not sure. > > I see that there's a CloudPolicySettings entry called SearchSuggestEnabled but I > don't know how that interacts with the user setting > (prefs::kSearchSuggestEnabled). You can see where we consult this pref in > IsSuggestEnabled* but there's no explicit check that I can find for the policy. > Does the cloud policy just override the user pref somehow? > > * > https://cs.chromium.org/chromium/src/chrome/browser/search/search.cc?type=cs&... It seems that selecting false value for this will disable it and prevent user from changing it: http://dev.chromium.org/administrators/policy-list-3#SearchSuggestEnabled https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:295: DCHECK(...); Can we have a simple check here to ensure that Google owned service is called?
https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: sender: "" On 2017/02/15 11:52:02, Ramin Halavati wrote: > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > As pkasting points out, these should be identical to values I specified in my > > response for chrome_omnibox_client.cc. Can you add a function here that would > > return the proto and use it in both locations? Or would you prefer I do that > in > > a separate CL? > > To keep the clang tool that extracts annotation simple and remove the need to > use run time data, we would like to have annotations as constants defined in the > same compilation unit that initializes the call. Therefore I think if both these > functions need exactly similar annotation, may be we can create a function Foo > that both these call sites call Foo and Foo creates the annotation and fetches > or prefetches the image. How does it sound? That seems reasonable. But if we're going to introduce a new file and/or class with methods delegating both prefetch and fetch with a callback I wonder if we're better off just keeping it simple and duplicating the two specifications with a comment in each directing people to change both at once. Peter, wdyt? https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:300: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/15 11:52:03, Ramin Halavati wrote: > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > Yeah, this is a tough one. Currently, no search engine other than Google > > provides answers in the suggest feature, though they could do so at any time. > > Moreover, even the answers provided by Google could point to images on other > web > > sites (this request is for the image, not the suggestions themselves). On the > > other hand, this feature is almost always set to have Google as the backend > and > > is perceived by the user to be part of the browser. > > > > In the balance, I'd choose WEBSITE based on what is possible here. But > > GOOGLE_OWNED_SERVICE would also be reasonable given the current behavior and > how > > users probably perceive it. > > Can we add a DCHECK to insure that Google owned service is used here and if it > changes in future, the one who changes it changes the annotation as well? Response in other comment. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:303: cookies_allowed: false/true On 2017/02/15 11:52:03, Ramin Halavati wrote: > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > false (requests are to http://ssl.gstatic.com) > > I wonder as the default URLFetcher allows to send and store and this is not > specified anywhere, should this be false or true? Yeah, if the annotation is meant to be a description of what's possible (rather than a policy that will be enforced) then you're right, true is more accurate. We (Chrome) don't even control the source of the Google URLs (they come from the Suggest team's backends). https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:308: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/15 11:52:03, Ramin Halavati wrote: > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > I'm honestly not sure. > > > > I see that there's a CloudPolicySettings entry called SearchSuggestEnabled but > I > > don't know how that interacts with the user setting > > (prefs::kSearchSuggestEnabled). You can see where we consult this pref in > > IsSuggestEnabled* but there's no explicit check that I can find for the > policy. > > Does the cloud policy just override the user pref somehow? > > > > * > > > https://cs.chromium.org/chromium/src/chrome/browser/search/search.cc?type=cs&... > > It seems that selecting false value for this will disable it and prevent user > from changing it: > > http://dev.chromium.org/administrators/policy-list-3#SearchSuggestEnabled Cool, thanks for the reference, that's helpful. https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:295: DCHECK(...); On 2017/02/15 11:52:03, Ramin Halavati wrote: > Can we have a simple check here to ensure that Google owned service is called? We have no control over whether and when other Suggest providers start providing answers with image URLs so a DCHECK strikes me as risky. I'd prefer to change the destination to WEBSITE to account for the fact that we can't control the source of the suggestions. https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:302: "The Google Chrome provides answers in the suggestion list to " Remove "The".
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: sender: "" On 2017/02/15 21:39:44, Justin Donnelly wrote: > On 2017/02/15 11:52:02, Ramin Halavati wrote: > > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > > As pkasting points out, these should be identical to values I specified in > my > > > response for chrome_omnibox_client.cc. Can you add a function here that > would > > > return the proto and use it in both locations? Or would you prefer I do that > > in > > > a separate CL? > > > > To keep the clang tool that extracts annotation simple and remove the need to > > use run time data, we would like to have annotations as constants defined in > the > > same compilation unit that initializes the call. Therefore I think if both > these > > functions need exactly similar annotation, may be we can create a function Foo > > that both these call sites call Foo and Foo creates the annotation and fetches > > or prefetches the image. How does it sound? > > That seems reasonable. But if we're going to introduce a new file and/or class > with methods delegating both prefetch and fetch with a callback I wonder if > we're better off just keeping it simple and duplicating the two specifications > with a comment in each directing people to change both at once. > > Peter, wdyt? If the tools can handle the following proposal, it's what I would do, but I don't know if they can: * Create a helper function here that takes a Callback * The helper creates the traffic annotation and executes the Callback, passing it the annotation * Callers bind the Prefetch() or RequestImage() calls up into the necessary callback and pass it in This way the helper is simpler and doesn't need to take args that would only be used for the non-prefetch case. I would try to avoid duplicating the annotation in two places, though. If all else failed, I'd go ahead with a helper that took more args and was uglier. :/
pkasting@, jdonnelli@: Comments addressed, review please. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: sender: "" On 2017/02/16 00:05:21, Peter Kasting wrote: > On 2017/02/15 21:39:44, Justin Donnelly wrote: > > On 2017/02/15 11:52:02, Ramin Halavati wrote: > > > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > > > As pkasting points out, these should be identical to values I specified in > > my > > > > response for chrome_omnibox_client.cc. Can you add a function here that > > would > > > > return the proto and use it in both locations? Or would you prefer I do > that > > > in > > > > a separate CL? > > > > > > To keep the clang tool that extracts annotation simple and remove the need > to > > > use run time data, we would like to have annotations as constants defined in > > the > > > same compilation unit that initializes the call. Therefore I think if both > > these > > > functions need exactly similar annotation, may be we can create a function > Foo > > > that both these call sites call Foo and Foo creates the annotation and > fetches > > > or prefetches the image. How does it sound? > > > > That seems reasonable. But if we're going to introduce a new file and/or class > > with methods delegating both prefetch and fetch with a callback I wonder if > > we're better off just keeping it simple and duplicating the two specifications > > with a comment in each directing people to change both at once. > > > > Peter, wdyt? > > If the tools can handle the following proposal, it's what I would do, but I > don't know if they can: > > * Create a helper function here that takes a Callback > * The helper creates the traffic annotation and executes the Callback, passing > it the annotation > * Callers bind the Prefetch() or RequestImage() calls up into the necessary > callback and pass it in > > This way the helper is simpler and doesn't need to take args that would only be > used for the non-prefetch case. > > I would try to avoid duplicating the annotation in two places, though. If all > else failed, I'd go ahead with a helper that took more args and was uglier. :/ The Callback solution seems good. If you agree, I would duplicate the annotation here and add a note on top of both annotations that these will be merged in another CL. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:300: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/15 21:39:44, Justin Donnelly wrote: > On 2017/02/15 11:52:03, Ramin Halavati wrote: > > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > > Yeah, this is a tough one. Currently, no search engine other than Google > > > provides answers in the suggest feature, though they could do so at any > time. > > > Moreover, even the answers provided by Google could point to images on other > > web > > > sites (this request is for the image, not the suggestions themselves). On > the > > > other hand, this feature is almost always set to have Google as the backend > > and > > > is perceived by the user to be part of the browser. > > > > > > In the balance, I'd choose WEBSITE based on what is possible here. But > > > GOOGLE_OWNED_SERVICE would also be reasonable given the current behavior and > > how > > > users probably perceive it. > > > > Can we add a DCHECK to insure that Google owned service is used here and if it > > changes in future, the one who changes it changes the annotation as well? > > Response in other comment. Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:303: cookies_allowed: false/true On 2017/02/15 21:39:44, Justin Donnelly wrote: > On 2017/02/15 11:52:03, Ramin Halavati wrote: > > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > > false (requests are to http://ssl.gstatic.com) > > > > I wonder as the default URLFetcher allows to send and store and this is not > > specified anywhere, should this be false or true? > > Yeah, if the annotation is meant to be a description of what's possible (rather > than a policy that will be enforced) then you're right, true is more accurate. > We (Chrome) don't even control the source of the Google URLs (they come from the > Suggest team's backends). Done. https://codereview.chromium.org/2682263002/diff/140001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:308: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/02/15 21:39:44, Justin Donnelly wrote: > On 2017/02/15 11:52:03, Ramin Halavati wrote: > > On 2017/02/14 17:01:05, Justin Donnelly wrote: > > > I'm honestly not sure. > > > > > > I see that there's a CloudPolicySettings entry called SearchSuggestEnabled > but > > I > > > don't know how that interacts with the user setting > > > (prefs::kSearchSuggestEnabled). You can see where we consult this pref in > > > IsSuggestEnabled* but there's no explicit check that I can find for the > > policy. > > > Does the cloud policy just override the user pref somehow? > > > > > > * > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/search/search.cc?type=cs&... > > > > It seems that selecting false value for this will disable it and prevent user > > from changing it: > > > > http://dev.chromium.org/administrators/policy-list-3#SearchSuggestEnabled > > Cool, thanks for the reference, that's helpful. Done. https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:295: DCHECK(...); On 2017/02/15 21:39:44, Justin Donnelly wrote: > On 2017/02/15 11:52:03, Ramin Halavati wrote: > > Can we have a simple check here to ensure that Google owned service is called? > > We have no control over whether and when other Suggest providers start providing > answers with image URLs so a DCHECK strikes me as risky. I'd prefer to change > the destination to WEBSITE to account for the fact that we can't control the > source of the suggestions. Done. https://codereview.chromium.org/2682263002/diff/160001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:302: "The Google Chrome provides answers in the suggestion list to " On 2017/02/15 21:39:44, Justin Donnelly wrote: > Remove "The". Done.
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
On 2017/02/16 07:03:30, Ramin Halavati wrote: > pkasting@, jdonnelli@: > Comments addressed, review please. (I'm not reviewing, don't wait for me)
lgtm https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:253: // TODO(jdonnelly@, rhalavati@): Create a helper function with Callback to I've never seen @ used in a TODO. Unless there was some recent decision to change this style, can you change this to TODO(jdonnelly, rhalavati)? https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:294: // TODO(jdonnelly@, rhalavati@): Create a helper function with Callback Same here.
On 2017/02/16 07:20:09, Peter Kasting wrote: > (I'm not reviewing, don't wait for me) He'll need an OWNERS approval, though.
extensions lgtm https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:82: "Not implemented, considered not useful." or similar to the avatar case, "No content is being uploaded or saved; this request merely downloads a publicly available PNG file."
rhalavati@chromium.org changed reviewers: + pkasting@chromium.org
All comments addressed. pkasting@: OWNERS approval please. https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:253: // TODO(jdonnelly@, rhalavati@): Create a helper function with Callback to On 2017/02/16 14:53:18, Justin Donnelly wrote: > I've never seen @ used in a TODO. Unless there was some recent decision to > change this style, can you change this to TODO(jdonnelly, rhalavati)? Done. https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/webstore_install_helper.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/webstore_install_helper.cc:82: "Not implemented, considered not useful." On 2017/02/16 18:00:50, Devlin wrote: > or similar to the avatar case, "No content is being uploaded or saved; this > request merely downloads a publicly available PNG file." I think this is implied in |data| part and it's sufficient to say it's not useful here. https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/180001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:294: // TODO(jdonnelly@, rhalavati@): Create a helper function with Callback On 2017/02/16 14:53:18, Justin Donnelly wrote: > Same here. Done.
LGTM for owners. My comments below apply to both files. Most is wordsmithing. You might want Justin to chime in again on the substantive question about inferring user data. https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: // the one in "c/b/ui/omnibox/chrome_omnibox_client.cc". Nit: Remove quotes and spell out the full pathname https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:261: "Google Chrome provides answers in the suggestion list to the " Nit: to the -> for certain https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:264: "weather conditions) when this can add information to the " Nit: the -> an https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:271: "included, although the general weather condition (sunny, " Nit: "...although some might be inferrable (e.g. whether the weather is sunny or rainy in the user's current location) from the name..." (This goes along with the weather being an example of an image, rather than the only image, which this requests. That said... don't we only send back weather answers for queries like "weather london", in which case there's no guarantee that "london" is the user's location, so even this inference is weak? I wonder if we should just delete this.) Also, this comment talks about "the user's" location, while other comments refer to "you". Be consistent about the use of second or third person to refer to the user.
All comments addressed, landing. https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:255: // the one in "c/b/ui/omnibox/chrome_omnibox_client.cc". On 2017/02/17 07:11:48, Peter Kasting wrote: > Nit: Remove quotes and spell out the full pathname Done. https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:261: "Google Chrome provides answers in the suggestion list to the " On 2017/02/17 07:11:48, Peter Kasting wrote: > Nit: to the -> for certain Done. https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:264: "weather conditions) when this can add information to the " On 2017/02/17 07:11:48, Peter Kasting wrote: > Nit: the -> an Done. https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:271: "included, although the general weather condition (sunny, " On 2017/02/17 07:11:48, Peter Kasting wrote: > Nit: "...although some might be inferrable (e.g. whether the weather is sunny or > rainy in the user's current location) from the name..." > > (This goes along with the weather being an example of an image, rather than the > only image, which this requests. That said... don't we only send back weather > answers for queries like "weather london", in which case there's no guarantee > that "london" is the user's location, so even this inference is weak? I wonder > if we should just delete this.) > > Also, this comment talks about "the user's" location, while other comments refer > to "you". Be consistent about the use of second or third person to refer to the > user. Done, I think it's a clarifying example.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlerman@chromium.org, dfalcantara@chromium.org, vasilii@chromium.org, rdevlin.cronin@chromium.org, jdonnelly@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2682263002/#ps220001 (title: "Annotations updated.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
dtrainor@: We are annotating network requests in Chrome for desktop version and a file is modified that is used in android version as well. I've added empty annotation to 2 required positions in 2 android files, contextual_search_scene_layer.cc and answers_image_bridge.cc. Please review them.
https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/200001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:271: "included, although the general weather condition (sunny, " On 2017/02/17 08:09:21, Ramin Halavati wrote: > On 2017/02/17 07:11:48, Peter Kasting wrote: > > Nit: "...although some might be inferrable (e.g. whether the weather is sunny > or > > rainy in the user's current location) from the name..." > > > > (This goes along with the weather being an example of an image, rather than > the > > only image, which this requests. That said... don't we only send back weather > > answers for queries like "weather london", in which case there's no guarantee > > that "london" is the user's location, so even this inference is weak? I > wonder > > if we should just delete this.) > > > > Also, this comment talks about "the user's" location, while other comments > refer > > to "you". Be consistent about the use of second or third person to refer to > the > > user. > > Done, I think it's a clarifying example. Sorry, I missed this discussion earlier. Peter: to answer your question, we don't require a location to send a weather answer. For example [weather today] should trigger.
On 2017/02/21 16:10:03, Justin Donnelly wrote: > Sorry, I missed this discussion earlier. Peter: to answer your question, we > don't require a location to send a weather answer. For example [weather today] > should trigger. And provides an answer based on the user's current location, to be clear.
rhalavati@chromium.org changed reviewers: + dtrainor@chromium.org
dtrainor@: We are annotating network requests in Chrome for desktop version and a file is modified that is used in android version as well. I've added empty annotation to 2 required positions in 2 android files, contextual_search_scene_layer.cc and answers_image_bridge.cc. Please review them.
https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:260: sender: "Omnibox Suggest Prefetch" I wonder if this is too fine-grained. Over in https://codereview.chromium.org/2709543002 I'm about to propose a sender of "omnibox" for another annotation. I think that's the right level of granularity here too.
https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:262: "Google Chrome provides answers in the suggestion list for certain " (Sorry for all the separate mails on this stuff) Is it really appropriate to say "Google Chrome" (or even "Chrome") in these annotations, when this code can be used in Chromium or third-party products? What about this first sentence: "The omnibox suggestion list can contain direct answers to certain user queries." https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:282: "address bar.' in Chrome's settings under Advanced. The " ...similarly here "...in the settings page..."
rhalavati@chromium.org changed reviewers: + battre@chromium.org
https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:260: sender: "Omnibox Suggest Prefetch" On 2017/02/24 02:38:22, Peter Kasting wrote: > I wonder if this is too fine-grained. > > Over in https://codereview.chromium.org/2709543002 I'm about to propose a sender > of "omnibox" for another annotation. I think that's the right level of > granularity here too. We need to have different |sender| texts for different annotations. So I think if it's OK, lets keep both more granular so that they refer more precisely to the part of code that is making the request. battre@: Any comments?
https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/260001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:260: sender: "Omnibox Suggest Prefetch" On 2017/02/24 07:44:01, Ramin Halavati wrote: > On 2017/02/24 02:38:22, Peter Kasting wrote: > > I wonder if this is too fine-grained. > > > > Over in https://codereview.chromium.org/2709543002 I'm about to propose a > sender > > of "omnibox" for another annotation. I think that's the right level of > > granularity here too. > > We need to have different |sender| texts for different annotations. So I think > if it's OK, lets keep both more granular so that they refer more precisely to > the part of code that is making the request. > > battre@: > Any comments? Actually, I would prefer if the same component reuses the same |sender| text. I was envisioning this as a grouping function.
battre@, pkasting@: Comments addressed. Please review. battre@: What is your idea on Peter's Google Chrome vs. Chromium comments? https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:301: sender: "Omnibox Suggest" Should this be changed to Omnibox?
On 2017/02/24 08:05:51, Ramin Halavati wrote: > battre@, pkasting@: > Comments addressed. Please review. > > battre@: > What is your idea on Peter's Google Chrome vs. Chromium comments? > > https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... > File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): > > https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... > chrome/browser/ui/omnibox/chrome_omnibox_client.cc:301: sender: "Omnibox > Suggest" > Should this be changed to Omnibox? I am supportive of using Chromium where appropriate. I think for UMA we would still use Google Chrome because it is only enabled for Chrome but not for Chromium. Best regards, Dominic
https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:301: sender: "Omnibox Suggest" On 2017/02/24 08:05:51, Ramin Halavati wrote: > Should this be changed to Omnibox? Yes, I think that would be reasonable.
battre@, pkasting@, Comments addressed, please review. https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/chrome_omnibox_client.cc (right): https://codereview.chromium.org/2682263002/diff/280001/chrome/browser/ui/omni... chrome/browser/ui/omnibox/chrome_omnibox_client.cc:301: sender: "Omnibox Suggest" On 2017/02/24 08:09:21, battre wrote: > On 2017/02/24 08:05:51, Ramin Halavati wrote: > > Should this be changed to Omnibox? > > Yes, I think that would be reasonable. Done.
LGTM. I prefer "Chromium" to "Google Chrome". I still think it's even clearer to reword things to avoid a product name at all when doing so is feasible, and tried to suggest wordings to accomplish this. But if you guys don't feel this is helpful, I'm OK with that outcome. https://codereview.chromium.org/2682263002/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:263: "queries that user types in the omnibox. This request retrieves a " Nit: user -> the user
pkasting@: Comment addressed. 'Google Chrome' and 'Chrome' were changed to 'Chromium' in this CL's annotations and will be used in all future annotations. Already landed annotations will be updated in the next round of reviewing and corrections. https://codereview.chromium.org/2682263002/diff/300001/chrome/browser/autocom... File chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc (right): https://codereview.chromium.org/2682263002/diff/300001/chrome/browser/autocom... chrome/browser/autocomplete/chrome_autocomplete_provider_client.cc:263: "queries that user types in the omnibox. This request retrieves a " On 2017/02/24 09:02:10, Peter Kasting wrote: > Nit: user -> the user Done.
chrome/browser/android/* lgtm sorry for the delays
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, rdevlin.cronin@chromium.org, mlerman@chromium.org, dfalcantara@chromium.org, jdonnelly@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2682263002/#ps320001 (title: "nits")
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": 320001, "attempt_start_ts": 1487931426079850,
"parent_rev": "30f4687dc9ad36e50a7f2ec0f5a63806d14b8585", "commit_rev":
"e9cefd65b1f3a9f4648852cb25933894e994022f"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to chrome::BitmapFetcher. Network traffic annotation is added to network request(s) of chrome::BitmapFetcher and all functions that are sourcing calls to it. BUG=656607 ========== to ========== Network traffic annotation added to chrome::BitmapFetcher. Network traffic annotation is added to network request(s) of chrome::BitmapFetcher and all functions that are sourcing calls to it. BUG=656607 Review-Url: https://codereview.chromium.org/2682263002 Cr-Commit-Position: refs/heads/master@{#452793} Committed: https://chromium.googlesource.com/chromium/src/+/e9cefd65b1f3a9f4648852cb2593... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/e9cefd65b1f3a9f4648852cb2593... |
