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

Issue 2284393002: Add ClearHistory() to ContentSuggestionsService and its providers (Closed)

Created:
4 years, 3 months ago by vitaliii
Modified:
4 years, 3 months ago
Reviewers:
Marc Treib, msramek
CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, ntp-dev+reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add ClearHistory() to ContentSuggestionsService and its providers This CL adds ClearHistory() to ContentSuggestionsService and its providers, which is called when the history is removed using Clear Browsing Data dialog, so that the providers can remove their history. BUG=641321 Committed: https://crrev.com/685fdfaad7f4ee4aa4b49ccf1d9d2db318b4341a Cr-Commit-Position: refs/heads/master@{#415609}

Patch Set 1 #

Total comments: 22

Patch Set 2 : Marc's comments. #

Total comments: 5

Patch Set 3 : Marc's comments. #

Total comments: 14

Patch Set 4 : Marc's comments. #

Total comments: 6

Patch Set 5 : msramek@'s comments and filter. #

Total comments: 8

Patch Set 6 : |const &| and comments. #

Patch Set 7 : |bookmark_model| can be null in tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -1 line) Patch
M chrome/browser/browsing_data/browsing_data_remover.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/ntp_snippets/content_suggestions_service_factory.cc View 1 2 3 4 5 6 1 chunk +3 lines, -1 line 0 comments Download
M components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestions_provider.h View 1 2 3 4 5 1 chunk +11 lines, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestions_service.h View 1 2 3 4 5 2 chunks +11 lines, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestions_service.cc View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestions_service_unittest.cc View 1 2 3 4 5 2 chunks +14 lines, -0 lines 0 comments Download
M components/ntp_snippets/ntp_snippets_service.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/ntp_snippets/ntp_snippets_service.cc View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M components/ntp_snippets/offline_pages/offline_page_suggestions_provider.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc View 1 2 3 4 5 1 chunk +7 lines, -0 lines 0 comments Download
M components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 36 (15 generated)
vitaliii
Hi Marc, would you be so kind to have a look?
4 years, 3 months ago (2016-08-29 09:23:55 UTC) #2
Marc Treib
https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc#newcode496 chrome/browser/browsing_data/browsing_data_remover.cc:496: content_suggestions_service->ClearHistory(); Should this also get called when the user ...
4 years, 3 months ago (2016-08-29 09:43:11 UTC) #3
vitaliii
Hi Marc, Would you mind having a look? https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc#newcode496 chrome/browser/browsing_data/browsing_data_remover.cc:496: content_suggestions_service->ClearHistory(); ...
4 years, 3 months ago (2016-08-29 10:37:25 UTC) #4
Marc Treib
https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc#newcode496 chrome/browser/browsing_data/browsing_data_remover.cc:496: content_suggestions_service->ClearHistory(); On 2016/08/29 10:37:25, vitaliii wrote: > On 2016/08/29 ...
4 years, 3 months ago (2016-08-29 11:27:36 UTC) #5
vitaliii
Hello Marc, please have a look. https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_data/browsing_data_remover.cc#newcode496 chrome/browser/browsing_data/browsing_data_remover.cc:496: content_suggestions_service->ClearHistory(); On 2016/08/29 ...
4 years, 3 months ago (2016-08-29 16:46:42 UTC) #6
Marc Treib
https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets/content_suggestions_service.h File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets/content_suggestions_service.h#newcode125 components/ntp_snippets/content_suggestions_service.h:125: // (possibly dismissed) suggestions for NTPSnippetsService or in case ...
4 years, 3 months ago (2016-08-30 09:50:18 UTC) #7
vitaliii
Hi, Marc! Please have a look. https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets/content_suggestions_service.h File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets/content_suggestions_service.h#newcode123 components/ntp_snippets/content_suggestions_service.h:123: // Removes history ...
4 years, 3 months ago (2016-08-30 11:04:22 UTC) #10
Marc Treib
LGTM, thanks!
4 years, 3 months ago (2016-08-30 11:09:35 UTC) #11
vitaliii
Hi Martin, Could you please have a look on my browsing_data_remover.cc change?
4 years, 3 months ago (2016-08-30 11:11:42 UTC) #13
msramek
Thanks for following up! LGTM % nits and a request. https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing_data/browsing_data_remover.cc#newcode493 ...
4 years, 3 months ago (2016-08-30 11:35:12 UTC) #14
vitaliii
Hi Marc, Would you mind checking the wording in CleanHistory() comments? https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing_data/browsing_data_remover.cc File chrome/browser/browsing_data/browsing_data_remover.cc (right): ...
4 years, 3 months ago (2016-08-30 12:54:46 UTC) #15
Marc Treib
https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right): https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h#newcode47 components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:47: base::Callback<bool(const GURL& url)> filter) override; This should be a ...
4 years, 3 months ago (2016-08-30 13:01:56 UTC) #16
msramek
LGTM. https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/ntp_snippets_service.cc File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/ntp_snippets_service.cc#newcode341 components/ntp_snippets/ntp_snippets_service.cc:341: base::Callback<bool(const GURL& url)> filter) { nit: If these ...
4 years, 3 months ago (2016-08-30 13:02:22 UTC) #17
vitaliii
https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right): https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h#newcode47 components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:47: base::Callback<bool(const GURL& url)> filter) override; On 2016/08/30 13:01:56, Marc ...
4 years, 3 months ago (2016-08-30 14:17:48 UTC) #18
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/2284393002/120001
4 years, 3 months ago (2016-08-30 14:19:42 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/287336)
4 years, 3 months ago (2016-08-30 14:42:36 UTC) #23
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/2284393002/120001
4 years, 3 months ago (2016-08-31 05:49:45 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/133584)
4 years, 3 months ago (2016-08-31 06:35:34 UTC) #27
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/2284393002/180001
4 years, 3 months ago (2016-08-31 09:43:58 UTC) #32
commit-bot: I haz the power
Committed patchset #7 (id:180001)
4 years, 3 months ago (2016-08-31 11:26:03 UTC) #34
commit-bot: I haz the power
4 years, 3 months ago (2016-08-31 11:27:27 UTC) #36
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/685fdfaad7f4ee4aa4b49ccf1d9d2db318b4341a
Cr-Commit-Position: refs/heads/master@{#415609}

Powered by Google App Engine
This is Rietveld 408576698