Implement ClearHistory for all ntp_snippets providers

Implement ClearHistory for all ntp_snippets providers

This CL adds ClearHistory implementations:
1) for OfflinePageSuggestionsProvider and
PhysicalWebPageSuggestionsProvider remove dismissed suggestions;
2) for NTPSnippetsService remove all suggestions (including dismissed);
3) for BookmarkSuggestionsProvider remove all last visit dates and
dismissed suggestions.

BUG=641321
Committed: https://crrev.com/0e9455ea24c427d0c55bc85756e11e9356f24e16
Cr-Commit-Position: refs/heads/master@{#416216}

[vitaliii, 2016-09-01 13:33:28]

Patchset #1 (id:1) has been deleted

[vitaliii, 2016-09-01 13:34:33]

vitaliii@chromium.org changed reviewers:
+ treib@chromium.org

[vitaliii, 2016-09-01 13:34:34]

Hi Marc,

PTAL.

[Marc Treib, 2016-09-01 14:11:20]

LGTM with some nits

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:244:
DCHECK(!bookmarks_for_url.empty());
No need to check for this - it shouldn't happen, but also nothing bad will
happen here if it did, so.. eh.

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:195:
ClearDismissedSuggestionsForDebugging(provided_category_);
Should we clear the dismissed first, and then re-fetch?

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:348: NukeAllSnippets();
Since this four-line block now exists twice, we could put it into its own helper
method. Not sure what a good name would be though...

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:880:
service->NukeAllSnippets();
Call ClearHistory instead, so you call a public API method rather than a private
one (which is an implementation details that the test shouldn't have to know
about)?

[vitaliii, 2016-09-01 14:45:15]

Just a fix, no need to look.

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:244:
DCHECK(!bookmarks_for_url.empty());
On 2016/09/01 14:11:20, Marc Treib wrote:
> No need to check for this - it shouldn't happen, but also nothing bad will
> happen here if it did, so.. eh.

Done.

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:195:
ClearDismissedSuggestionsForDebugging(provided_category_);
On 2016/09/01 14:11:20, Marc Treib wrote:
> Should we clear the dismissed first, and then re-fetch?

Done.

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service.cc:348: NukeAllSnippets();
On 2016/09/01 14:11:20, Marc Treib wrote:
> Since this four-line block now exists twice, we could put it into its own
helper
> method. Not sure what a good name would be though...

Acknowledged.

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/ntp_snippets_service_unittest.cc (right):

https://codereview.chromium.org/2303713002/diff/20001/components/ntp_snippets...
components/ntp_snippets/ntp_snippets_service_unittest.cc:880:
service->NukeAllSnippets();
On 2016/09/01 14:11:20, Marc Treib wrote:
> Call ClearHistory instead, so you call a public API method rather than a
private
> one (which is an implementation details that the test shouldn't have to know
> about)?

Done.

[vitaliii, 2016-09-02 08:06:17]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-09-02 08:06:18]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/2303713002/#ps40001
(title: "Marc's nits.")

[commit-bot: I haz the power, 2016-09-02 08:06:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 09:07:25]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-02 09:11:15]

Description was changed from

==========
Implement ClearHistory for all ntp_snippets providers

This CL adds ClearHistory implementations:
1) for OfflinePageSuggestionsProvider and
PhysicalWebPageSuggestionsProvider remove dismissed suggestions;
2) for NTPSnippetsService remove all suggestions (including dismissed);
3) for BookmarkSuggestionsProvider remove all last visit dates and
dismissed suggestions.

BUG=641321
==========

to

==========
Implement ClearHistory for all ntp_snippets providers

This CL adds ClearHistory implementations:
1) for OfflinePageSuggestionsProvider and
PhysicalWebPageSuggestionsProvider remove dismissed suggestions;
2) for NTPSnippetsService remove all suggestions (including dismissed);
3) for BookmarkSuggestionsProvider remove all last visit dates and
dismissed suggestions.

BUG=641321

Committed: https://crrev.com/0e9455ea24c427d0c55bc85756e11e9356f24e16
Cr-Commit-Position: refs/heads/master@{#416216}
==========

[commit-bot: I haz the power, 2016-09-02 09:11:16]

Patchset 2 (id:??) landed as
https://crrev.com/0e9455ea24c427d0c55bc85756e11e9356f24e16
Cr-Commit-Position: refs/heads/master@{#416216}