|
|
Created:
3 years, 10 months ago by Ramin Halavati Modified:
3 years, 6 months ago Reviewers:
msramek, Vladislav Kaznacheev, Matt Giuca, rpetterson, sdefresne, Marc Treib, kcarattini, brettw, Nicolas Zea, battre CC:
chromium-reviews, sky Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to web_history_service.
Network traffic annotation is added to network request of
web_history_service.
BUG=656607
Review-Url: https://codereview.chromium.org/2710703002
Cr-Commit-Position: refs/heads/master@{#478511}
Committed: https://chromium.googlesource.com/chromium/src/+/65bc8270dae726bd5e9bd7d91a4e0fb86c7d237e
Patch Set 1 #
Total comments: 20
Patch Set 2 : Annotation updated. #Patch Set 3 : Annotations separated. #
Total comments: 56
Patch Set 4 : Annotations updated, unittest added. #Patch Set 5 : policy_options corrected. #
Total comments: 1
Patch Set 6 : Annotations moved to caller functions. #Patch Set 7 : Unittest updated. #Patch Set 8 : Another unittest and ios file added. #Patch Set 9 : Another test file added. #
Total comments: 16
Patch Set 10 : Annotations changed to partial. #
Total comments: 16
Patch Set 11 : Comments addressed. #
Total comments: 6
Patch Set 12 : Comments addressed. hotword_audio change to not yet tag. #Patch Set 13 : Unnescessary policy_options removed, two policies updated. #
Total comments: 2
Patch Set 14 : Comment addressed. #Messages
Total messages: 86 (44 generated)
rhalavati@chromium.org changed reviewers: + sdefresne@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 web_history_service and added annotation template to it. Please review it and suggest the required answers for the empty parts. Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
On 2017/02/21 13:26:02, 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 web_history_service and added annotation template to it. Please > review it and suggest the required answers for the empty parts. Please note that > the claims should be thorough and covering all cases. In case you believe that > annotation should be passed to the modified function instead of originating from > it, please tell me to change the CL accordingly. > > Please take a look at the protobuf scheme in: > https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... > for the definition of the annotations. > > You can find a sample annotation in: > https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... > > Entriprise policy options are here: > https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/cl... > > And more description on enterprise policy settings is here: > http://dev.chromium.org/administrators/policy-list-3 > > Please tell me if you need any clarifications from my side. If you want to learn > more, see: > https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. Hi, A friendly reminder on this review.
I may not be the best reviewer for this. It may be better to involve someone from the sync team (zea@ maybe). https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:182: net::DefineNetworkTrafficAnnotation("...", R"( There's multiple context where this can be used (add lookup, change, deletion in the web history), so I think this must be moved to the calling function (up to WebHistoryService::CreateRequest as only the caller of that function knows which type of request this is). https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:184: sender: "..." "web_history" https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:185: description: "..." "Google Chrome can synchronise history of signed-in users." https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:186: trigger: "..." Again, this will depends on the caller I guess. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:187: data: "..." ditto. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:188: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:191: cookies_allowed: false/true I don't know what this mean. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:192: cookies_store: "..." ditto https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:193: setting: "..." "You can control whether synchronisation of history is enabled in advanced sync settings." https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:195: [POLICY_NAME] { I don't know what this mean.
rhalavati@chromium.org changed reviewers: + battre@chromium.org, msramek@chromium.org
I think if this request is only in charge of web history synchronization, we don't need to get more fine grained. I made some changes, if you agree and feel it possible, please add suggestions for 'trigger' and 'data' which mean in which situations this request is called and what is sent. If you still believe it must move to lower level, please tell me to change it accordingly. +battre@, msramek@ Do you agree? https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:182: net::DefineNetworkTrafficAnnotation("...", R"( On 2017/02/28 18:02:00, sdefresne wrote: > There's multiple context where this can be used (add lookup, change, deletion in > the web history), so I think this must be moved to the calling function (up to > WebHistoryService::CreateRequest as only the caller of that function knows which > type of request this is). Acknowledged. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:184: sender: "..." On 2017/02/28 18:02:00, sdefresne wrote: > "web_history" Done. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:185: description: "..." On 2017/02/28 18:02:00, sdefresne wrote: > "Google Chrome can synchronise history of signed-in users." Done. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:186: trigger: "..." On 2017/02/28 18:02:00, sdefresne wrote: > Again, this will depends on the caller I guess. Acknowledged. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:187: data: "..." On 2017/02/28 18:02:00, sdefresne wrote: > ditto. Acknowledged. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:188: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/02/28 18:02:00, sdefresne wrote: > GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:191: cookies_allowed: false/true On 2017/02/28 18:02:00, sdefresne wrote: > I don't know what this mean. It means whether this request uses cookies are not. As you have the following lines of code, I assume the answer is false: fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES); https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:192: cookies_store: "..." On 2017/02/28 18:02:00, sdefresne wrote: > ditto Acknowledged. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:193: setting: "..." On 2017/02/28 18:02:00, sdefresne wrote: > "You can control whether synchronisation of history is enabled in advanced sync > settings." Done. https://codereview.chromium.org/2710703002/diff/1/components/history/core/bro... components/history/core/browser/web_history_service.cc:195: [POLICY_NAME] { On 2017/02/28 18:02:00, sdefresne wrote: > I don't know what this mean. This part asks about the enterprise policy that can disable this feature.
I'll review this more closely later today. I don't own the code, but I know it well. WebHistoryService is used to download your history from history.google.com, and also issue deletion requests. It is not, however, used to sync (upload) your history. That's done by ProfileSyncService and what's actually synced are open tabs, history is derived from them.
Looking at this again, I think the usecases and controls are quite different. For example, you can view your web history if history sync is enabled; you can delete it if it's enabled AND the admin hasn't forbidden deleting it. Etc. I would suggest moving a layer above. Each of the methods (QueryHistory, ExpireHistory, etc.) should have its own annotation which is then passed to RequestImpl.
Annotation moved one layer upper. Please advise text for the six sites.
On 2017/03/02 07:45:43, Ramin Halavati wrote: > Annotation moved one layer upper. > Please advise text for the six sites. Hi, A friendly ping.
msramek@chromium.org changed reviewers: + rlp@chromium.org
I updated 4/6 sections that I'm familiar with. I'm not familiar with the remaining part about audio history. CC'ing the author rlp@ to please help. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:395: net::DefineNetworkTrafficAnnotation("query_history", R"( web_history_query https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:398: description: "..." If history sync is enabled, this downloads the synced history from history.google.com. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:399: trigger: "..." Synced history is downloaded when user opens the history page, searches on the history page, or scrolls down the history page to see more results. This is only the case if the user is signed in and history sync is enabled. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:400: data: "..." The history query text (or empty strings if all results are to be fetched), the begin and end timestamps, and the maximum number of results to be fetched. The request also includes a version info token to resolve transaction conflicts, and an OAuth2 token authenticating the user. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:405: setting: "..." In Settings, under the "People" section, users can either sign out or enter the Sync settings and disable history sync. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:406: policy { SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:462: net::DefineNetworkTrafficAnnotation("expire_history", R"( web_history_expire https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:465: description: "..." If a user who syncs their browsing history deletes one or more history item(s), Chrome sends a request to history.google.com to execute the corresponding deletion serverside. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:466: trigger: "..." Deleting one or more history items form the history page, or deleting history in bulk from the Clear Browsing Data dialog. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:467: data: "..." For bulk history deletion, the begin and end timestamps of the selected time range. For deletion of individual history items, the selected items represented by an URL and timestamp. The request also includes a version info token to resolve transaction conflicts, and an OAuth2 token authenticating the user. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:472: setting: "..." In Settings, under the "People" section, users can either sign out or enter the Sync settings and disable history sync. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:473: policy { SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:511: net::DefineNetworkTrafficAnnotation("get_audio_hisory_enabled", R"( web_history_get_audio_enabled https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:514: description: "..." I'm not familiar with audio history, please find another owner. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:548: net::DefineNetworkTrafficAnnotation("set_audio_hisory_enabled", R"( web_history_set_audio_enabled https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:551: description: "..." I'm not familiar with audio history, please find another owner. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:597: net::DefineNetworkTrafficAnnotation("query_web_history_activity", R"( web_history_swaa https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:600: description: "..." Queries history.google.com to find out if user has the 'Include Chrome browsing history and activity from websites and apps that use Google services' option enabled in the Activity controls of their Google account. This is done for users who sync their browsing history without a custom passphrase in order to show information about history.google.com on the history page and in the Clear Browsing Data dialog. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:601: trigger: "..." This request is sent when user opens the history page or the Clear Browsing Data dialog and history sync without a custom passphrase is (re)enabled. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:602: data: "..." An OAuth2 token authenticating the user. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:607: setting: "..." In Settings, under the "People" section, users can either sign out or enter the Sync settings and disable history sync. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:608: policy { SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:642: net::DefineNetworkTrafficAnnotation("query_other_history_activity", R"( web_history_query_other_forms_of_history https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:645: description: "..." Determines if the user has other forms of browsing history (than Chrome browsing history) stored in their Google account. This is used to inform the users about the existence of other forms of browsing history when they delete their Chrome browsing history from the Clear Browsing Data dialog. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:646: trigger: "..." This request is sent when user opens the Clear Browsing Data dialog and history sync without a custom passphrase is (re)enabled. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:647: data: "..." An OAuth2 token authenticating the user. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:652: setting: "..." In Settings, under the "People" section, users can either sign out or enter the Sync settings and disable history sync. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:653: policy { SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true }
msramek@: 4 annotations updated, please review. sdefresne@: Please review other changes. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:395: net::DefineNetworkTrafficAnnotation("query_history", R"( On 2017/03/09 14:10:17, msramek wrote: > web_history_query Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:398: description: "..." On 2017/03/09 14:10:16, msramek wrote: > If history sync is enabled, this downloads the synced history from > http://history.google.com. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:399: trigger: "..." On 2017/03/09 14:10:16, msramek wrote: > Synced history is downloaded when user opens the history page, searches on the > history page, or scrolls down the history page to see more results. This is only > the case if the user is signed in and history sync is enabled. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:400: data: "..." On 2017/03/09 14:10:17, msramek wrote: > The history query text (or empty strings if all results are to be fetched), the > begin and end timestamps, and the maximum number of results to be fetched. The > request also includes a version info token to resolve transaction conflicts, and > an OAuth2 token authenticating the user. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:405: setting: "..." On 2017/03/09 14:10:16, msramek wrote: > In Settings, under the "People" section, users can either sign out or enter the > Sync settings and disable history sync. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:406: policy { On 2017/03/09 14:10:17, msramek wrote: > SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:462: net::DefineNetworkTrafficAnnotation("expire_history", R"( On 2017/03/09 14:10:16, msramek wrote: > web_history_expire Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:465: description: "..." On 2017/03/09 14:10:16, msramek wrote: > If a user who syncs their browsing history deletes one or more history item(s), > Chrome sends a request to http://history.google.com to execute the corresponding > deletion serverside. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:466: trigger: "..." On 2017/03/09 14:10:17, msramek wrote: > Deleting one or more history items form the history page, or deleting history in > bulk from the Clear Browsing Data dialog. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:467: data: "..." On 2017/03/09 14:10:17, msramek wrote: > For bulk history deletion, the begin and end timestamps of the selected time > range. For deletion of individual history items, the selected items represented > by an URL and timestamp. The request also includes a version info token to > resolve transaction conflicts, and an OAuth2 token authenticating the user. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:472: setting: "..." On 2017/03/09 14:10:17, msramek wrote: > In Settings, under the "People" section, users can either sign out or enter the > Sync settings and disable history sync. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:473: policy { On 2017/03/09 14:10:16, msramek wrote: > SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:511: net::DefineNetworkTrafficAnnotation("get_audio_hisory_enabled", R"( On 2017/03/09 14:10:16, msramek wrote: > web_history_get_audio_enabled Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:514: description: "..." On 2017/03/09 14:10:17, msramek wrote: > I'm not familiar with audio history, please find another owner. Acknowledged. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:548: net::DefineNetworkTrafficAnnotation("set_audio_hisory_enabled", R"( On 2017/03/09 14:10:17, msramek wrote: > web_history_set_audio_enabled Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:551: description: "..." On 2017/03/09 14:10:16, msramek wrote: > I'm not familiar with audio history, please find another owner. Acknowledged. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:597: net::DefineNetworkTrafficAnnotation("query_web_history_activity", R"( On 2017/03/09 14:10:16, msramek wrote: > web_history_swaa Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:600: description: "..." On 2017/03/09 14:10:17, msramek wrote: > Queries http://history.google.com to find out if user has the 'Include Chrome browsing > history and activity from websites and apps that use Google services' option > enabled in the Activity controls of their Google account. This is done for users > who sync their browsing history without a custom passphrase in order to show > information about http://history.google.com on the history page and in the Clear > Browsing Data dialog. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:601: trigger: "..." On 2017/03/09 14:10:17, msramek wrote: > This request is sent when user opens the history page or the Clear Browsing Data > dialog and history sync without a custom passphrase is (re)enabled. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:602: data: "..." On 2017/03/09 14:10:16, msramek wrote: > An OAuth2 token authenticating the user. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:607: setting: "..." On 2017/03/09 14:10:16, msramek wrote: > In Settings, under the "People" section, users can either sign out or enter the > Sync settings and disable history sync. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:608: policy { On 2017/03/09 14:10:16, msramek wrote: > SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:642: net::DefineNetworkTrafficAnnotation("query_other_history_activity", R"( On 2017/03/09 14:10:16, msramek wrote: > web_history_query_other_forms_of_history Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:645: description: "..." On 2017/03/09 14:10:17, msramek wrote: > Determines if the user has other forms of browsing history (than Chrome browsing > history) stored in their Google account. This is used to inform the users about > the existence of other forms of browsing history when they delete their Chrome > browsing history from the Clear Browsing Data dialog. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:646: trigger: "..." On 2017/03/09 14:10:16, msramek wrote: > This request is sent when user opens the Clear Browsing Data dialog and history > sync without a custom passphrase is (re)enabled. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:647: data: "..." On 2017/03/09 14:10:17, msramek wrote: > An OAuth2 token authenticating the user. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:652: setting: "..." On 2017/03/09 14:10:17, msramek wrote: > In Settings, under the "People" section, users can either sign out or enter the > Sync settings and disable history sync. Done. https://codereview.chromium.org/2710703002/diff/40001/components/history/core... components/history/core/browser/web_history_service.cc:653: policy { On 2017/03/09 14:10:16, msramek wrote: > SyncDisabled { policy_options: {mode: MANDATORY}, SyncDisabled: true } Done.
Hi, it's been over 2 years since I've actively worked on any features in Chrome. I'm happy to help where I can, but am unlikely to be best reviewer. If you still wish me to look at something particular, please indicate which part.
msramek@chromium.org changed reviewers: + kcarattini@chromium.org, mgiuca@chromium.org
Alright, since we really want the annotations to be as precise as possible, it would be better to find a current owner. Thanks anyway :) +kcarattini@ +mgiuca@ as hotword owners. Can you please fill in the blanks in the network annotation of audio history requests?
On 2017/03/09 18:47:21, msramek wrote: > Alright, since we really want the annotations to be as precise as possible, it > would be better to find a current owner. Thanks anyway :) > > +kcarattini@ +mgiuca@ as hotword owners. Can you please fill in the blanks in > the network annotation of audio history requests? kcarattini@, mgiuca@: Can you help on this?
On 2017/03/15 11:55:47, Ramin Halavati wrote: > On 2017/03/09 18:47:21, msramek wrote: > > Alright, since we really want the annotations to be as precise as possible, it > > would be better to find a current owner. Thanks anyway :) > > > > +kcarattini@ +mgiuca@ as hotword owners. Can you please fill in the blanks in > > the network annotation of audio history requests? > > kcarattini@, mgiuca@: > Can you help on this? kcarattini@ is out for a few months. I never worked on hotwording and I don't know how this fits into the picture. Everybody else who worked on this is gone :(. I will ask around if anyone is maintaining this, otherwise will try to look at it myself.
On 2017/03/16 07:39:59, Matt Giuca wrote: > On 2017/03/15 11:55:47, Ramin Halavati wrote: > > On 2017/03/09 18:47:21, msramek wrote: > > > Alright, since we really want the annotations to be as precise as possible, > it > > > would be better to find a current owner. Thanks anyway :) > > > > > > +kcarattini@ +mgiuca@ as hotword owners. Can you please fill in the blanks > in > > > the network annotation of audio history requests? > > > > kcarattini@, mgiuca@: > > Can you help on this? > > kcarattini@ is out for a few months. > > I never worked on hotwording and I don't know how this fits into the picture. > Everybody else who worked on this is gone :(. I will ask around if anyone is > maintaining this, otherwise will try to look at it myself. kcarattini@, mgiuca@, A friendly reminder on this.
rhalavati@chromium.org changed reviewers: + sky@chromium.org, zea@chromium.org
+zea@, +sky@, Could you please suggest a reviewer for the two missing annotations on Audio History? (WebHistoryService::GetAudioHistoryEnabled and WebHistoryService::SetAudioHistoryEnabled functions)
mgiuca@chromium.org changed reviewers: + kaznacheev@chromium.org
+Vlad who I talked to in a private thread. He wrote: "The linked review does not look like hotwording-related to me, what am I missing?" This is audio history, not hotwording. I know audio history is used by the hotwording code, but I don't think it is directly related to hotwording.
On 2017/04/11 06:48:51, Matt Giuca wrote: > +Vlad who I talked to in a private thread. > > He wrote: > "The linked review does not look like hotwording-related to me, what am I > missing?" > > This is audio history, not hotwording. I know audio history is used by the > hotwording code, but I don't think it is directly related to hotwording. The two methods were added because of Hotword - see https://codereview.chromium.org/687803004, the CL title says "[Hotword]". The only non-test callsite of these methods is in HotwordAudioHistoryHandler. So if we're trying to add an annotation describing the ping, the answer is "This is used by Hotword <why>, triggered <when>, and contains <what>". We can figure out the <what> part, but we need someone from Hotword to answer <why> and <when>.
Description was changed from ========== Network traffic annotation added to web_history_service. Network traffic annotation is added to network request of web_history_service. BUG=656607 ========== to ========== Network traffic annotation added to web_history_service. Network traffic annotation is added to network request of web_history_service. BUG=656607 ==========
sky@chromium.org changed reviewers: + brettw@chromium.org - sky@chromium.org
sky->brettw
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/2710703002/diff/80001/components/history/core... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/80001/components/history/core... components/history/core/browser/web_history_service.cc:394: net::NetworkTrafficAnnotationTag traffic_annotation = I have a number of concerns with this: This is the code for doing the actual fetching and is quite removed from the UI surfaces that may end up using it. The information you are adding here is specific to how the UI is implemented and how this is called *now*. If we change the UI that surfaces the results of these functions in anyway, then all these comments are out of date and there is really no way to know that. *If* we want this level of detail then consumers of this API should pass in the details and it shouldn't be a function doing the query that knows how it is used. I'm also of the opinion these comments do not belong here. Rather there should be an enumeration that can be used to map to specifics. I think think it's fair for the code here to take an arbitrary int that maps to some enum, but the code here shouldn't know about the level of details you are adding.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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_ozone_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.
I moved the annotations to the caller functions. Our future plan is to add tools to let definition of partial annotations, so that common parts of the annotations will be defined where the actual URL request is made (like in this class), and specific parts will be defined in the functions that call them. With that change, we try to keep each part of annotation at closest point to the code, so that it will result in highest possibility of updated when code changes. Martin, I moved the annotations from this file to the caller functions, please review them and suggest required changes.
https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes one or more " Why do we need all this meta information in the source? We don't do this for uma macros. And again, these annotation belong in the UI calling it, not in the service itself.
https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes one or more " On 2017/04/12 15:23:00, sky wrote: > Why do we need all this meta information in the source? We don't do this for uma > macros. Putting these annotations away from the source code increases the chance of incorrect information. The very point of this effort is to move information that we theoretically have in the privacy whitepaper into source code to minimize this chance. We want to give enterprises information that they can rely on. This is different from UMA, where incorrect annotations have no consequence. > And again, these annotation belong in the UI calling it, not in the service > itself. I agree that in this case we have a situation where it would be best to split the "what is being sent" from the "when is it being sent" as you pointed out into two different locations. Can we keep this as a TODO? (it's in our very interest to reflect these two questions at the right location to keep the data up to date). Ramin is working on a design proposal for having partial annotations that get joined together.
https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes one or more " On 2017/04/12 20:34:01, battre wrote: > On 2017/04/12 15:23:00, sky wrote: > > Why do we need all this meta information in the source? We don't do this for > uma > > macros. > > Putting these annotations away from the source code increases the chance of > incorrect information. I understand that desire, but it seems to me the level of detail in these comments means they are going to very easily get out of date. See below for more. > The very point of this effort is to move information that > we theoretically have in the privacy whitepaper into source code to minimize > this chance. We want to give enterprises information that they can rely on. This > is different from UMA, where incorrect annotations have no consequence. > > > And again, these annotation belong in the UI calling it, not in the service > > itself. > > I agree that in this case we have a situation where it would be best to split > the "what is being sent" from the "when is it being sent" as you pointed out > into two different locations. Can we keep this as a TODO? I would rather we get this right now, and plumb through the appropriate information as needed. > (it's in our very > interest to reflect these two questions at the right location to keep the data > up to date). Ramin is working on a design proposal for having partial > annotations that get joined together. Could you outline why this is a big string? It seems there is a format here and without compile time checking it's going to be too easy to end up with unparsable data. For example, can I have a trailing ',' after '}' in an array? What if I misspell 'false' in cookies_allowed? What are the expected items in SyncDisabled? I'm also concerned about the level of description needed here. What is the right level? Who decides that? For example, the comment explicitly mentions 'history.google.com', is that important? What if the url is changed? It's going to be very easy to miss changes of that sort. Similarly what if the title of 'Clear Browsing Data dialog' changes, or we move away from OAuth2. The comment mentions 'People' below, that in particular could very easily change... I have a hard time believing we can keep comments of this sort up to date with the level of detail that is here now.
mgiuca@chromium.org changed reviewers: + yitingc@chromium.org
https://codereview.chromium.org/2710703002/diff/160001/chrome/browser/search/... File chrome/browser/search/hotword_audio_history_handler.cc (right): https://codereview.chromium.org/2710703002/diff/160001/chrome/browser/search/... chrome/browser/search/hotword_audio_history_handler.cc:63: sender: "Web History" On 2017/04/11 09:19:48, msramek wrote: > The two methods were added because of Hotword - see > https://codereview.chromium.org/687803004, the CL title says "[Hotword]". The > only non-test callsite of these methods is in HotwordAudioHistoryHandler. > > So if we're trying to add an annotation describing the ping, the answer is "This > is used by Hotword <why>, triggered <when>, and contains <what>". We can figure > out the <what> part, but we need someone from Hotword to answer <why> and > <when>. This code was added by rlp@ who left the team a few years ago leaving this code in the hands of amistry@ (who left) and kcarattini@ (who is away and hasn't worked on Hotword code for two years also). I was essentially a caretaker for some time after that (I don't have any of the context of why this was added) and I no longer work on this either. (I can go digging through the code but it won't be any easier for me to find these answers than anybody else.) From memory, the hotwording is tied to the audio history service simply because the subsequent voice searches are stored with the user's account. I'm not exactly sure why it's linked to hotwording (as opposed to normal voice searches), but I think you have to have audio history enabled to use hotwording. The best way to get an explanation of why and when is to ask the privacy team, who should have records on this. +yitingc who was involved at the time.
https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes one or more " On 2017/04/12 22:30:53, sky (OOO) wrote: > On 2017/04/12 20:34:01, battre wrote: > > On 2017/04/12 15:23:00, sky wrote: > > > Why do we need all this meta information in the source? We don't do this for > > uma > > > macros. > > > > Putting these annotations away from the source code increases the chance of > > incorrect information. > > I understand that desire, but it seems to me the level of detail in these > comments means they are going to very easily get out of date. See below for > more. > > > The very point of this effort is to move information that > > we theoretically have in the privacy whitepaper into source code to minimize > > this chance. We want to give enterprises information that they can rely on. > This > > is different from UMA, where incorrect annotations have no consequence. > > > > > And again, these annotation belong in the UI calling it, not in the service > > > itself. > > > > I agree that in this case we have a situation where it would be best to split > > the "what is being sent" from the "when is it being sent" as you pointed out > > into two different locations. Can we keep this as a TODO? > > I would rather we get this right now, and plumb through the appropriate > information as needed. > > > (it's in our very > > interest to reflect these two questions at the right location to keep the data > > up to date). Ramin is working on a design proposal for having partial > > annotations that get joined together. > > Could you outline why this is a big string? This was a lengthy discussion with the network stack team to not taint the dependency graph with compile time dependencies. cronet was happy to accept a string but not a dependency to more Chrome-y concepts such as enterprise policies. The current design is the consequence of several in-depth meetings. > It seems there is a format here and without compile time checking it's going to > be too easy to end up with unparsable data. For example, can I have a trailing > ',' after '}' in an array? What if I misspell 'false' in cookies_allowed? What > are the expected items in SyncDisabled? A precommit-script is intended to address this issue. The syntax is text-serialized protobuf, which is pretty standard but apparently not standardized. We should get a link to the format specification. > I'm also concerned about the level of description needed here. What is the right > level? Who decides that? PCounsel will be notified periodically if messages change or are added to review them. > For example, the comment explicitly mentions > 'history.google.com', is that important? What if the url is changed? It's going > to be very easy to miss changes of that sort. Similarly what if the title of > 'Clear Browsing Data dialog' changes, or we move away from OAuth2. The comment > mentions 'People' below, that in particular could very easily change... I have a > hard time believing we can keep comments of this sort up to date with the level > of detail that is here now. I share the concern of outdated comments. We should try the use language that is sufficiently robust to getting outdated while still providing sufficient information to be useful. Do you want to have a VC to discuss this in person?
If you would like a VC I think Wednesday morning (8AMish PDT) likely aligns best with your timezone. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes one or more " On 2017/04/13 09:18:29, battre wrote: > On 2017/04/12 22:30:53, sky (OOO) wrote: > > On 2017/04/12 20:34:01, battre wrote: > > > On 2017/04/12 15:23:00, sky wrote: > > > > Why do we need all this meta information in the source? We don't do this > for > > > uma > > > > macros. > > > > > > Putting these annotations away from the source code increases the chance of > > > incorrect information. > > > > I understand that desire, but it seems to me the level of detail in these > > comments means they are going to very easily get out of date. See below for > > more. > > > > > The very point of this effort is to move information that > > > we theoretically have in the privacy whitepaper into source code to minimize > > > this chance. We want to give enterprises information that they can rely on. > > This > > > is different from UMA, where incorrect annotations have no consequence. > > > > > > > And again, these annotation belong in the UI calling it, not in the > service > > > > itself. > > > > > > I agree that in this case we have a situation where it would be best to > split > > > the "what is being sent" from the "when is it being sent" as you pointed out > > > into two different locations. Can we keep this as a TODO? > > > > I would rather we get this right now, and plumb through the appropriate > > information as needed. > > > > > (it's in our very > > > interest to reflect these two questions at the right location to keep the > data > > > up to date). Ramin is working on a design proposal for having partial > > > annotations that get joined together. > > > > Could you outline why this is a big string? > > This was a lengthy discussion with the network stack team to not taint the > dependency graph with compile time dependencies. cronet was happy to accept a > string but not a dependency to more Chrome-y concepts such as enterprise > policies. The current design is the consequence of several in-depth meetings. Did you consider using a real class/structure but converting to a string? It sure would be nice to have more type safety here. > > It seems there is a format here and without compile time checking it's going > to > > be too easy to end up with unparsable data. For example, can I have a trailing > > ',' after '}' in an array? What if I misspell 'false' in cookies_allowed? What > > are the expected items in SyncDisabled? > > A precommit-script is intended to address this issue. The syntax is > text-serialized protobuf, which is pretty standard but apparently not > standardized. We should get a link to the format specification. > > > I'm also concerned about the level of description needed here. What is the > right > > level? Who decides that? > > PCounsel will be notified periodically if messages change or are added to review > them. > > > For example, the comment explicitly mentions > > 'history.google.com', is that important? What if the url is changed? It's > going > > to be very easy to miss changes of that sort. Similarly what if the title of > > 'Clear Browsing Data dialog' changes, or we move away from OAuth2. The comment > > mentions 'People' below, that in particular could very easily change... I have > a > > hard time believing we can keep comments of this sort up to date with the > level > > of detail that is here now. > > I share the concern of outdated comments. We should try the use language that is > sufficiently robust to getting outdated while still providing sufficient > information to be useful. Do you want to have a VC to discuss this in person? I'm happy to have a VC if you feel it's necessary. The blockers for me at this point are: 1. Should we be using a structure vs a string? 2. The amount of detail specified. Specifically what is written here is way too detailed. 3. Making sure code like this does not assume it's being used in a certain way. By that I mean UI code should be passing in the annotations and lower level functions shouldn't assume they are being used in a certain way.
https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1081: "history item(s), Chromium sends a request to history.google.com " 'to history.google.com' -> 'to a google.com host' https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1084: "Deleting one or more history items form the history page, or " form -> from https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1087: "For bulk history deletion, the begin and end timestamps of the " Bulk history deletion may be specified using a time range. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1089: "the selected items represented by an URL and timestamp. The " are represented by URL and timestamp. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1090: "request also includes a version info token to resolve " I'm not familiar with the last part, someone from sync should verify that. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1098: "In Chromium Settings, under the 'People' section, users can " How about: In settings users can either sign out or disable history sync.
Patchset #10 (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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Martin, Changed all annotations to partial. Should I move all one level up? https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1081: "history item(s), Chromium sends a request to history.google.com " On 2017/04/19 15:57:00, sky wrote: > 'to history.google.com' -> 'to a http://google.com host' Done. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1084: "Deleting one or more history items form the history page, or " On 2017/04/19 15:57:00, sky wrote: > form -> from Done. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1089: "the selected items represented by an URL and timestamp. The " On 2017/04/19 15:57:00, sky wrote: > are represented by URL and timestamp. Done. https://codereview.chromium.org/2710703002/diff/160001/components/history/cor... components/history/core/browser/history_service.cc:1098: "In Chromium Settings, under the 'People' section, users can " On 2017/04/19 15:57:00, sky wrote: > How about: > In settings users can either sign out or disable history sync. Done.
Patchset #11 (id:220001) has been deleted
Patchset #11 (id:240001) has been deleted
Sorry yet again for the delays, Ramin! I left some comments. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... File components/browsing_data/core/counters/history_counter.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:96: "If history sync is enabled, this downloads the synced history " If history sync is enabled, this queries history.google.com to determine if there is any synced history. This information is displayed in the Clear Browsing Data dialog. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:99: "Synced history is downloaded when user opens the history page, " Checking the "Browsing history" option in the Clear Browsing Data dialog, or enabling history sync while the dialog is open. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:104: "The history query text (or empty strings if all results are to be " The first sentence is very general and not really interesting here. We always send parameters min=base::Time(), max=base::Time::Max(), and count=1, so technically no information. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... File components/browsing_data/core/history_notice_utils.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/history_notice_utils.cc:134: browsing history from the Clear Browsing Data dialog." Missing quotation mark. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1078: "history item(s), Chromium sends a request to a google.com host " nit: As we agreed in the meantime, let's use "Chrome" here. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1081: "Deleting one or more history items from the history page, or " The history page trigger takes a different route. BrowsingHistoryService::RemoveVisits() -> WebHistoryService::ExpireHistory(). The deletion of individual *local* items does go through here, but the deletion of individual *synced* items goes directly to WebHistoryService. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1082: "deleting history in bulk from the Clear Browsing Data dialog." I just realized that this is wrong. Bulk history deletion can be requested by Clear Browsing Data dialog, but of course also extensions and Clear-Site-Data (https://w3c.github.io/webappsec-clear-site-data/). https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1085: "selected time range. For deletion of individual history items, " Ditto here. The deletion of individual history items does not go through here.
rhalavati@chromium.org changed reviewers: + treib@chromium.org - yitingc@chromium.org
Thank you Martin, comments addressed, please review. Marc, Could you please help for annotating chrome/browser/search/hotword_audio_history_handler.cc? https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... File components/browsing_data/core/counters/history_counter.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:96: "If history sync is enabled, this downloads the synced history " On 2017/06/06 19:56:06, msramek (slow) wrote: > If history sync is enabled, this queries http://history.google.com to determine if > there is any synced history. This information is displayed in the Clear Browsing > Data dialog. Done. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:99: "Synced history is downloaded when user opens the history page, " On 2017/06/06 19:56:06, msramek (slow) wrote: > Checking the "Browsing history" option in the Clear Browsing Data dialog, or > enabling history sync while the dialog is open. Done. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/counters/history_counter.cc:104: "The history query text (or empty strings if all results are to be " On 2017/06/06 19:56:06, msramek (slow) wrote: > The first sentence is very general and not really interesting here. We always > send parameters min=base::Time(), max=base::Time::Max(), and count=1, so > technically no information. Done. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... File components/browsing_data/core/history_notice_utils.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/browsing_da... components/browsing_data/core/history_notice_utils.cc:134: browsing history from the Clear Browsing Data dialog." On 2017/06/06 19:56:07, msramek (slow) wrote: > Missing quotation mark. Done. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1078: "history item(s), Chromium sends a request to a google.com host " On 2017/06/06 19:56:07, msramek (slow) wrote: > nit: As we agreed in the meantime, let's use "Chrome" here. Done. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1081: "Deleting one or more history items from the history page, or " On 2017/06/06 19:56:07, msramek (slow) wrote: > The history page trigger takes a different route. > > BrowsingHistoryService::RemoveVisits() -> WebHistoryService::ExpireHistory(). > > The deletion of individual *local* items does go through here, but the deletion > of individual *synced* items goes directly to WebHistoryService. Done. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1082: "deleting history in bulk from the Clear Browsing Data dialog." On 2017/06/06 19:56:07, msramek (slow) wrote: > I just realized that this is wrong. Bulk history deletion can be requested by > Clear Browsing Data dialog, but of course also extensions and Clear-Site-Data > (https://w3c.github.io/webappsec-clear-site-data/). Done. https://codereview.chromium.org/2710703002/diff/200001/components/history/cor... components/history/core/browser/history_service.cc:1085: "selected time range. For deletion of individual history items, " On 2017/06/06 19:56:07, msramek (slow) wrote: > Ditto here. The deletion of individual history items does not go through here. So, what is sent for local items? URL and time stamps?
On 2017/06/07 06:52:48, Ramin Halavati wrote: > Thank you Martin, comments addressed, please review. > > > Marc, > > Could you please help for annotating > chrome/browser/search/hotword_audio_history_handler.cc? Sorry - while I technically own this code, I've never touched it and am not at all familiar with it. (I'm not even sure it's still used - wasn't the hotwording stuff removed a while back? But maybe this is something else.) Anyway, I'm not comfortable answering these rather intricate questions here. Hopefully one of the original authors can help, otherwise I'll have to investigate, but that'll take a while.
On 2017/06/07 09:07:44, Marc Treib (OOO till June 7) wrote: > On 2017/06/07 06:52:48, Ramin Halavati wrote: > > Thank you Martin, comments addressed, please review. > > > > > > Marc, > > > > Could you please help for annotating > > chrome/browser/search/hotword_audio_history_handler.cc? > > Sorry - while I technically own this code, I've never touched it and am not at > all familiar with it. (I'm not even sure it's still used - wasn't the hotwording > stuff removed a while back? But maybe this is something else.) > Anyway, I'm not comfortable answering these rather intricate questions here. > Hopefully one of the original authors can help, otherwise I'll have to > investigate, but that'll take a while. Thanks Marc. I will add NO_TRAFFIC_ANNOTATION_YET to these two cases for now to proceed with the rest of CL, and investigate them in a separate CL.
https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/history_service.cc:1081: "Deleting one or more local history items." The other way around. This handles bulk deletion. "Deleting browsing history for a given time range, e.g. from the Clear Browsing Data dialog, by an extension, or the Clear-Site-Data header." BrowsingHistoryService::RemoveVisits() handles individual history entries. Please add an annotation for individual deletion there. https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/history_service.cc:1083: "A version info token to resolve transaction conflicts, and an " Please re-add the part about begin and end timestamps. I wanted to remove those from the HistoryCounter, because there they have uninteresting values. But here, they're actually user-supplied. https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/web_history_service.cc:197: chrome_policy { Should we pull this to upper layers as well? It's true that WebHistoryService requests won't happen if history is disabled, but e.g. for history deletion, maybe https://www.chromium.org/administrators/policy-list-3#AllowDeletingBrowserHis... is more interesting.
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...
Thanks. All seems to be correct now, except the annotation in BrowsingHistoryService which still incorrectly refers to bulk deletion. https://codereview.chromium.org/2710703002/diff/300001/chrome/browser/history... File chrome/browser/history/browsing_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/300001/chrome/browser/history... chrome/browser/history/browsing_history_service.cc:337: "Deleting one or more history items form the history page, or " This is still not addressed: RemoveVisits() *only* handles individual history items deletion. So please remove the part about bulk deletion from all sections of this proto.
Thanks Martin, comment addressed, please review. P.S., seems that I forgot to publish my previous commit's replies. https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/history_service.cc:1081: "Deleting one or more local history items." On 2017/06/07 19:15:44, msramek (slow) wrote: > The other way around. This handles bulk deletion. > > "Deleting browsing history for a given time range, e.g. from the Clear Browsing > Data dialog, by an extension, or the Clear-Site-Data header." > > BrowsingHistoryService::RemoveVisits() handles individual history entries. > Please add an annotation for individual deletion there. Updated here, but BrowsingHistoryService::RemoveVisits calls HistoryService::ExpireHistory which calls HistoryBackend::ExpireHistory and I did not reach a network request needing annotation in it. https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/history_service.cc:1083: "A version info token to resolve transaction conflicts, and an " On 2017/06/07 19:15:44, msramek (slow) wrote: > Please re-add the part about begin and end timestamps. > > I wanted to remove those from the HistoryCounter, because there they have > uninteresting values. But here, they're actually user-supplied. Done. https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/260001/components/history/cor... components/history/core/browser/web_history_service.cc:197: chrome_policy { On 2017/06/07 19:15:44, msramek (slow) wrote: > Should we pull this to upper layers as well? It's true that WebHistoryService > requests won't happen if history is disabled, but e.g. for history deletion, > maybe > https://www.chromium.org/administrators/policy-list-3#AllowDeletingBrowserHis... > is more interesting. Move policy, kept settings. OK? https://codereview.chromium.org/2710703002/diff/300001/chrome/browser/history... File chrome/browser/history/browsing_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/300001/chrome/browser/history... chrome/browser/history/browsing_history_service.cc:337: "Deleting one or more history items form the history page, or " On 2017/06/08 21:30:36, msramek (slow) wrote: > This is still not addressed: RemoveVisits() *only* handles individual history > items deletion. So please remove the part about bulk deletion from all sections > of this proto. 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: This issue passed the CQ dry run.
LGTM, thanks!
On 2017/06/09 14:12:17, msramek (slow) wrote: > LGTM, thanks! Thank you Martin. Sky@, Could you please take a look.
sky@chromium.org changed reviewers: - sky@chromium.org
I'm about to go on vacation, so sky -> brettw
owners lgtm, I did not review any of the actual annotations.
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...
CQ is committing da patch. Bot data: {"patchset_id": 320001, "attempt_start_ts": 1497073170728530, "parent_rev": "1e963ee8ad0c396688f3ce5d86d880a60d3e19d8", "commit_rev": "65bc8270dae726bd5e9bd7d91a4e0fb86c7d237e"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to web_history_service. Network traffic annotation is added to network request of web_history_service. BUG=656607 ========== to ========== Network traffic annotation added to web_history_service. Network traffic annotation is added to network request of web_history_service. BUG=656607 Review-Url: https://codereview.chromium.org/2710703002 Cr-Commit-Position: refs/heads/master@{#478511} Committed: https://chromium.googlesource.com/chromium/src/+/65bc8270dae726bd5e9bd7d91a4e... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001) as https://chromium.googlesource.com/chromium/src/+/65bc8270dae726bd5e9bd7d91a4e... |