Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(27)

Issue 2710703002: Network traffic annotation added to web_history_service. (Closed)

Created:
3 years, 10 months ago by Ramin Halavati
Modified:
3 years, 6 months ago
CC:
chromium-reviews, sky
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+308 lines, -75 lines) Patch
M chrome/browser/extensions/api/hotword_private/hotword_private_apitest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -5 lines 0 comments Download
M chrome/browser/history/browsing_history_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +57 lines, -6 lines 0 comments Download
M chrome/browser/search/hotword_audio_history_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/browsing_history_handler_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +12 lines, -7 lines 0 comments Download
M components/browsing_data/core/counters/history_counter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +25 lines, -1 line 0 comments Download
M components/browsing_data/core/history_notice_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +55 lines, -6 lines 0 comments Download
M components/history/core/browser/history_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +26 lines, -1 line 0 comments Download
M components/history/core/browser/web_history_service.h View 1 2 3 4 5 6 7 8 9 3 chunks +28 lines, -9 lines 0 comments Download
M components/history/core/browser/web_history_service.cc View 1 2 3 4 5 6 7 8 9 10 11 13 chunks +56 lines, -20 lines 0 comments Download
M components/history/core/browser/web_history_service_unittest.cc View 1 2 3 4 5 6 7 8 9 7 chunks +24 lines, -10 lines 0 comments Download
M components/history/core/test/fake_web_history_service.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M components/history/core/test/fake_web_history_service.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/history/history_service_facade.mm View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +7 lines, -3 lines 0 comments Download

Messages

Total messages: 86 (44 generated)
Ramin Halavati
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows ...
3 years, 10 months ago (2017-02-21 13:26:02 UTC) #2
Ramin Halavati
On 2017/02/21 13:26:02, Ramin Halavati wrote: > We are annotating all network requests in Chromium ...
3 years, 9 months ago (2017-02-28 12:14:14 UTC) #3
sdefresne
I may not be the best reviewer for this. It may be better to involve ...
3 years, 9 months ago (2017-02-28 18:02:00 UTC) #4
Ramin Halavati
I think if this request is only in charge of web history synchronization, we don't ...
3 years, 9 months ago (2017-03-01 06:38:10 UTC) #6
msramek
I'll review this more closely later today. I don't own the code, but I know ...
3 years, 9 months ago (2017-03-01 12:43:43 UTC) #7
msramek
Looking at this again, I think the usecases and controls are quite different. For example, ...
3 years, 9 months ago (2017-03-01 15:07:14 UTC) #8
Ramin Halavati
Annotation moved one layer upper. Please advise text for the six sites.
3 years, 9 months ago (2017-03-02 07:45:43 UTC) #9
Ramin Halavati
On 2017/03/02 07:45:43, Ramin Halavati wrote: > Annotation moved one layer upper. > Please advise ...
3 years, 9 months ago (2017-03-09 07:29:59 UTC) #10
msramek
I updated 4/6 sections that I'm familiar with. I'm not familiar with the remaining part ...
3 years, 9 months ago (2017-03-09 14:10:17 UTC) #12
Ramin Halavati
msramek@: 4 annotations updated, please review. sdefresne@: Please review other changes. https://codereview.chromium.org/2710703002/diff/40001/components/history/core/browser/web_history_service.cc File components/history/core/browser/web_history_service.cc (right): ...
3 years, 9 months ago (2017-03-09 14:39:02 UTC) #13
rpetterson
Hi, it's been over 2 years since I've actively worked on any features in Chrome. ...
3 years, 9 months ago (2017-03-09 18:40:46 UTC) #14
msramek
Alright, since we really want the annotations to be as precise as possible, it would ...
3 years, 9 months ago (2017-03-09 18:47:21 UTC) #16
Ramin Halavati
On 2017/03/09 18:47:21, msramek wrote: > Alright, since we really want the annotations to be ...
3 years, 9 months ago (2017-03-15 11:55:47 UTC) #17
Matt Giuca
On 2017/03/15 11:55:47, Ramin Halavati wrote: > On 2017/03/09 18:47:21, msramek wrote: > > Alright, ...
3 years, 9 months ago (2017-03-16 07:39:59 UTC) #18
Ramin Halavati
On 2017/03/16 07:39:59, Matt Giuca wrote: > On 2017/03/15 11:55:47, Ramin Halavati wrote: > > ...
3 years, 8 months ago (2017-04-05 09:09:07 UTC) #19
Ramin Halavati
+zea@, +sky@, Could you please suggest a reviewer for the two missing annotations on Audio ...
3 years, 8 months ago (2017-04-11 06:05:29 UTC) #21
Matt Giuca
+Vlad who I talked to in a private thread. He wrote: "The linked review does ...
3 years, 8 months ago (2017-04-11 06:48:51 UTC) #23
msramek
On 2017/04/11 06:48:51, Matt Giuca wrote: > +Vlad who I talked to in a private ...
3 years, 8 months ago (2017-04-11 09:19:48 UTC) #24
sky
sky->brettw
3 years, 8 months ago (2017-04-11 16:56:41 UTC) #27
sky
https://codereview.chromium.org/2710703002/diff/80001/components/history/core/browser/web_history_service.cc File components/history/core/browser/web_history_service.cc (right): https://codereview.chromium.org/2710703002/diff/80001/components/history/core/browser/web_history_service.cc#newcode394 components/history/core/browser/web_history_service.cc:394: net::NetworkTrafficAnnotationTag traffic_annotation = I have a number of concerns ...
3 years, 8 months ago (2017-04-11 18:27:48 UTC) #29
Ramin Halavati
I moved the annotations to the caller functions. Our future plan is to add tools ...
3 years, 8 months ago (2017-04-12 14:00:27 UTC) #46
sky
https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc#newcode1080 components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes ...
3 years, 8 months ago (2017-04-12 15:23:00 UTC) #47
battre
https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc#newcode1080 components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes ...
3 years, 8 months ago (2017-04-12 20:34:01 UTC) #48
sky
https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc#newcode1080 components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes ...
3 years, 8 months ago (2017-04-12 22:30:53 UTC) #49
Matt Giuca
https://codereview.chromium.org/2710703002/diff/160001/chrome/browser/search/hotword_audio_history_handler.cc File chrome/browser/search/hotword_audio_history_handler.cc (right): https://codereview.chromium.org/2710703002/diff/160001/chrome/browser/search/hotword_audio_history_handler.cc#newcode63 chrome/browser/search/hotword_audio_history_handler.cc:63: sender: "Web History" On 2017/04/11 09:19:48, msramek wrote: > ...
3 years, 8 months ago (2017-04-13 06:52:15 UTC) #51
battre
https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc#newcode1080 components/history/core/browser/history_service.cc:1080: "If a user who syncs their browsing history deletes ...
3 years, 8 months ago (2017-04-13 09:18:29 UTC) #52
sky
If you would like a VC I think Wednesday morning (8AMish PDT) likely aligns best ...
3 years, 8 months ago (2017-04-17 16:29:29 UTC) #53
sky
https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc#newcode1081 components/history/core/browser/history_service.cc:1081: "history item(s), Chromium sends a request to history.google.com " ...
3 years, 8 months ago (2017-04-19 15:57:01 UTC) #54
Ramin Halavati
Martin, Changed all annotations to partial. Should I move all one level up? https://codereview.chromium.org/2710703002/diff/160001/components/history/core/browser/history_service.cc File ...
3 years, 6 months ago (2017-05-30 09:49:23 UTC) #60
msramek
Sorry yet again for the delays, Ramin! I left some comments. https://codereview.chromium.org/2710703002/diff/200001/components/browsing_data/core/counters/history_counter.cc File components/browsing_data/core/counters/history_counter.cc (right): ...
3 years, 6 months ago (2017-06-06 19:56:07 UTC) #63
Ramin Halavati
Thank you Martin, comments addressed, please review. Marc, Could you please help for annotating chrome/browser/search/hotword_audio_history_handler.cc? ...
3 years, 6 months ago (2017-06-07 06:52:48 UTC) #65
Marc Treib
On 2017/06/07 06:52:48, Ramin Halavati wrote: > Thank you Martin, comments addressed, please review. > ...
3 years, 6 months ago (2017-06-07 09:07:44 UTC) #66
Ramin Halavati
On 2017/06/07 09:07:44, Marc Treib (OOO till June 7) wrote: > On 2017/06/07 06:52:48, Ramin ...
3 years, 6 months ago (2017-06-07 16:55:32 UTC) #67
msramek
https://codereview.chromium.org/2710703002/diff/260001/components/history/core/browser/history_service.cc File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2710703002/diff/260001/components/history/core/browser/history_service.cc#newcode1081 components/history/core/browser/history_service.cc:1081: "Deleting one or more local history items." The other ...
3 years, 6 months ago (2017-06-07 19:15:45 UTC) #68
msramek
Thanks. All seems to be correct now, except the annotation in BrowsingHistoryService which still incorrectly ...
3 years, 6 months ago (2017-06-08 21:30:36 UTC) #71
Ramin Halavati
Thanks Martin, comment addressed, please review. P.S., seems that I forgot to publish my previous ...
3 years, 6 months ago (2017-06-09 04:42:37 UTC) #72
msramek
LGTM, thanks!
3 years, 6 months ago (2017-06-09 14:12:17 UTC) #77
Ramin Halavati
On 2017/06/09 14:12:17, msramek (slow) wrote: > LGTM, thanks! Thank you Martin. Sky@, Could you ...
3 years, 6 months ago (2017-06-09 17:03:46 UTC) #78
sky
I'm about to go on vacation, so sky -> brettw
3 years, 6 months ago (2017-06-09 18:14:05 UTC) #80
brettw
owners lgtm, I did not review any of the actual annotations.
3 years, 6 months ago (2017-06-09 20:56:51 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2710703002/320001
3 years, 6 months ago (2017-06-10 05:39:43 UTC) #83
commit-bot: I haz the power
3 years, 6 months ago (2017-06-10 06:34:14 UTC) #86
Message was sent while issue was closed.
Committed patchset #14 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/65bc8270dae726bd5e9bd7d91a4e...

Powered by Google App Engine
This is Rietveld 408576698