Add ClearHistory() to ContentSuggestionsService and its providers

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}

[vitaliii, 2016-08-29 09:23:54]

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

[vitaliii, 2016-08-29 09:23:55]

Hi Marc,

would you be so kind to have a look?

[Marc Treib, 2016-08-29 09:43:11]

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:496:
content_suggestions_service->ClearHistory();
Should this also get called when the user deletes a history entry from
about:history?

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:499: if ((remove_mask &
REMOVE_HISTORY) && may_delete_history) {
Now you have two identical "if"s - merge them?

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:188: // Not
implemented.
Add a TODO and reference the bug?

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:105: // Removes user
history (for example dismissed suggestions).
I think this deserves a bit more explanation - with this text, I'm wondering how
it is different from ClearDismissedSuggestions. (Maybe dismissed suggestions
aren't the best example?)
What are the implications when this is called? E.g. providers should immediately
clear any history-related suggestions from the UI, that kind of thing.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:571:
TEST_F(ContentSuggestionsServiceTest, ShouldRemoveHistory) {
ShouldForwardClearHistory? The service doesn't really do anything...

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:339: // Not implemented.
Again, TODO and bug?

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:154:
// Not implemented.
^^ :)

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:86:
// Not implemented.
^^ :)

[vitaliii, 2016-08-29 10:37:25]

Hi Marc,

Would you mind having a look?

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:496:
content_suggestions_service->ClearHistory();
On 2016/08/29 09:43:10, Marc Treib wrote:
> Should this also get called when the user deletes a history entry from
> about:history?

The idea was to differentiate these cases, because, as far as I understood, only
NTPSnippetsService needs to know about removing an entry from about:history,
while at least NTPSnippetsService and BookmarkProvider need to handle entire
history removal.

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:499: if ((remove_mask &
REMOVE_HISTORY) && may_delete_history) {
On 2016/08/29 09:43:11, Marc Treib wrote:
> Now you have two identical "if"s - merge them?

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/boo...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:188: // Not
implemented.
On 2016/08/29 09:43:11, Marc Treib wrote:
> Add a TODO and reference the bug?

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_provider.h:105: // Removes user
history (for example dismissed suggestions).
On 2016/08/29 09:43:11, Marc Treib wrote:
> I think this deserves a bit more explanation - with this text, I'm wondering
how
> it is different from ClearDismissedSuggestions. (Maybe dismissed suggestions
> aren't the best example?)
> What are the implications when this is called? E.g. providers should
immediately
> clear any history-related suggestions from the UI, that kind of thing.

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/con...
components/ntp_snippets/content_suggestions_service_unittest.cc:571:
TEST_F(ContentSuggestionsServiceTest, ShouldRemoveHistory) {
On 2016/08/29 09:43:11, Marc Treib wrote:
> ShouldForwardClearHistory? The service doesn't really do anything...

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:339: // Not implemented.
On 2016/08/29 09:43:11, Marc Treib wrote:
> Again, TODO and bug?

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:154:
// Not implemented.
On 2016/08/29 09:43:11, Marc Treib wrote:
> ^^ :)

I cannot understand what you mean.
If adding |^^| after |//|, then it disagrees with "// Ignored" below.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:86:
// Not implemented.
On 2016/08/29 09:43:11, Marc Treib wrote:
> ^^ :)

Acknowledged.

[Marc Treib, 2016-08-29 11:27:36]

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
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 09:43:10, Marc Treib wrote:
> > Should this also get called when the user deletes a history entry from
> > about:history?
> 
> The idea was to differentiate these cases, because, as far as I understood,
only
> NTPSnippetsService needs to know about removing an entry from about:history,
> while at least NTPSnippetsService and BookmarkProvider need to handle entire
> history removal. 

I see - yes, that makes sense.

Note that not necessarily *all* history is cleared here; there can be a time
range. We might want to pass that on to our service?

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:154:
// Not implemented.
On 2016/08/29 10:37:25, vitaliii wrote:
> On 2016/08/29 09:43:11, Marc Treib wrote:
> > ^^ :)
> 
> I cannot understand what you mean.
> If adding |^^| after |//|, then it disagrees with "// Ignored" below.

Sorry, I meant: Same as the previous comment, i.e. add a TODO

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:86:
// Not implemented.
On 2016/08/29 10:37:25, vitaliii wrote:
> On 2016/08/29 09:43:11, Marc Treib wrote:
> > ^^ :)
> 
> Acknowledged.

Same here.

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:123: // Removes history
from all providers. The data removed depends on a provider.
nit: depends on *the* provider

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:125: // (possibly
dismissed) suggestions for NTPSnippetsService or in case of
I wouldn't list the actual provider examples here; that kind of thing is bound
to get out of sync with reality. Instead, describe what should happen (the first
part of this sentence already does this, more or less).

[vitaliii, 2016-08-29 16:46:42]

Hello Marc,

please have a look.

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:496:
content_suggestions_service->ClearHistory();
On 2016/08/29 11:27:35, Marc Treib wrote:
> On 2016/08/29 10:37:25, vitaliii wrote:
> > On 2016/08/29 09:43:10, Marc Treib wrote:
> > > Should this also get called when the user deletes a history entry from
> > > about:history?
> > 
> > The idea was to differentiate these cases, because, as far as I understood,
> only
> > NTPSnippetsService needs to know about removing an entry from about:history,
> > while at least NTPSnippetsService and BookmarkProvider need to handle entire
> > history removal. 
> 
> I see - yes, that makes sense.
> 
> Note that not necessarily *all* history is cleared here; there can be a time
> range. We might want to pass that on to our service?

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/off...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:154:
// Not implemented.
On 2016/08/29 11:27:35, Marc Treib wrote:
> On 2016/08/29 10:37:25, vitaliii wrote:
> > On 2016/08/29 09:43:11, Marc Treib wrote:
> > > ^^ :)
> > 
> > I cannot understand what you mean.
> > If adding |^^| after |//|, then it disagrees with "// Ignored" below.
> 
> Sorry, I meant: Same as the previous comment, i.e. add a TODO

Done.

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/1/components/ntp_snippets/phy...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:86:
// Not implemented.
On 2016/08/29 11:27:36, Marc Treib wrote:
> On 2016/08/29 10:37:25, vitaliii wrote:
> > On 2016/08/29 09:43:11, Marc Treib wrote:
> > > ^^ :)
> > 
> > Acknowledged.
> 
> Same here.

Done.

[Marc Treib, 2016-08-30 09:50:18]

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:125: // (possibly
dismissed) suggestions for NTPSnippetsService or in case of
On 2016/08/29 11:27:36, Marc Treib wrote:
> I wouldn't list the actual provider examples here; that kind of thing is bound
> to get out of sync with reality. Instead, describe what should happen (the
first
> part of this sentence already does this, more or less).

This is still open - listing the actual examples is IMO not a good idea. Same
applies to the comment on the provider interface.

https://codereview.chromium.org/2284393002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:495: if
(content_suggestions_service)
Braces if the body doesn't fit on one line.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:105: // Removes history
from a given time range. The data removed depends on a
depends on *the* provider

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:107: // example
suggestions, which are based on the time range. Providers should
"which are based on the time range" isn't very clear IMO... "which are based on
history from that time range" maybe?

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:108: // immediately clear
any specified time range history-related data, such as
This doesn't grammar - "clear any history-related data from the specified time
range" ?

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:574: base::Time
begin, end;
Initialize these to something sensible?

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:155:
// TODO(vitaliii): Implement.
Bug?

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:87:
// TODO(vitaliii): Implement.
Bug?

[vitaliii, 2016-08-30 10:58:08]

Patchset #4 (id:60001) has been deleted

[vitaliii, 2016-08-30 11:01:09]

Description was changed from

==========
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,640972
==========

to

==========
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
==========

[vitaliii, 2016-08-30 11:04:22]

Hi, Marc!

Please have a look.

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:123: // Removes history
from all providers. The data removed depends on a provider.
On 2016/08/29 11:27:36, Marc Treib wrote:
> nit: depends on *the* provider

Done.

https://codereview.chromium.org/2284393002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:125: // (possibly
dismissed) suggestions for NTPSnippetsService or in case of
On 2016/08/30 09:50:18, Marc Treib wrote:
> On 2016/08/29 11:27:36, Marc Treib wrote:
> > I wouldn't list the actual provider examples here; that kind of thing is
bound
> > to get out of sync with reality. Instead, describe what should happen (the
> first
> > part of this sentence already does this, more or less).
> 
> This is still open - listing the actual examples is IMO not a good idea. Same
> applies to the comment on the provider interface.

Done.

https://codereview.chromium.org/2284393002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:495: if
(content_suggestions_service)
On 2016/08/30 09:50:18, Marc Treib wrote:
> Braces if the body doesn't fit on one line.

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:105: // Removes history
from a given time range. The data removed depends on a
On 2016/08/30 09:50:18, Marc Treib wrote:
> depends on *the* provider

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:107: // example
suggestions, which are based on the time range. Providers should
On 2016/08/30 09:50:18, Marc Treib wrote:
> "which are based on the time range" isn't very clear IMO... "which are based
on
> history from that time range" maybe?

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_provider.h:108: // immediately clear
any specified time range history-related data, such as
On 2016/08/30 09:50:18, Marc Treib wrote:
> This doesn't grammar - "clear any history-related data from the specified time
> range" ?

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:574: base::Time
begin, end;
On 2016/08/30 09:50:18, Marc Treib wrote:
> Initialize these to something sensible?

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:155:
// TODO(vitaliii): Implement.
On 2016/08/30 09:50:18, Marc Treib wrote:
> Bug?

Done.

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc
(right):

https://codereview.chromium.org/2284393002/diff/40001/components/ntp_snippets...
components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:87:
// TODO(vitaliii): Implement.
On 2016/08/30 09:50:18, Marc Treib wrote:
> Bug?

Done.

[Marc Treib, 2016-08-30 11:09:35]

LGTM, thanks!

[vitaliii, 2016-08-30 11:11:42]

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

[vitaliii, 2016-08-30 11:11:42]

Hi Martin,

Could you please have a look on my browsing_data_remover.cc change?

[msramek, 2016-08-30 11:35:12]

Thanks for following up!

LGTM % nits and a request.

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:493:
ntp_snippets::ContentSuggestionsService* content_suggestions_service =
nit: Please move it a bit lower in the "((remove_mask & REMOVE_HISTORY) &&
may_delete_history)" block. I'd like the call to HistoryService to be first as
the 'primary representant' of the datatypes under REMOVE_HISTORY.

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:493:
ntp_snippets::ContentSuggestionsService* content_suggestions_service =
nit / request:

Apart from the time range begin and end, BrowsingDataRemover::RemoveImpl also
has the |filter_builder_| parameter which specifies restrictions on URLs that
should be deleted. Many backends do not use it *yet*, but in your case you're
intentionally not going to use it. Even if the user uses BrowsingDataRemover to
delete data for just one URL, you will wipe data for all URLs.

Can you please add the explanation as a comment? The same thing that Marc told
me in the privacy review.

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:496:
content_suggestions_service->ClearHistory(time_range.begin,
nit: Please use |delete_begin_| and |delete_end_| for consistency with the other
calls (it's the same, the value is just cached there).

[vitaliii, 2016-08-30 12:54:46]

Hi Marc,

Would you mind checking the wording in CleanHistory() comments?

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:493:
ntp_snippets::ContentSuggestionsService* content_suggestions_service =
On 2016/08/30 11:35:12, msramek wrote:
> nit: Please move it a bit lower in the "((remove_mask & REMOVE_HISTORY) &&
> may_delete_history)" block. I'd like the call to HistoryService to be first as
> the 'primary representant' of the datatypes under REMOVE_HISTORY.

Done.

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:493:
ntp_snippets::ContentSuggestionsService* content_suggestions_service =
On 2016/08/30 11:35:12, msramek wrote:
> nit / request:
> 
> Apart from the time range begin and end, BrowsingDataRemover::RemoveImpl also
> has the |filter_builder_| parameter which specifies restrictions on URLs that
> should be deleted. Many backends do not use it *yet*, but in your case you're
> intentionally not going to use it. Even if the user uses BrowsingDataRemover
to
> delete data for just one URL, you will wipe data for all URLs.
> 
> Can you please add the explanation as a comment? The same thing that Marc told
> me in the privacy review.

We decided to pass the filter, since some providers may use it,
but we will not use it for now.

https://codereview.chromium.org/2284393002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover.cc:496:
content_suggestions_service->ClearHistory(time_range.begin,
On 2016/08/30 11:35:12, msramek wrote:
> nit: Please use |delete_begin_| and |delete_end_| for consistency with the
other
> calls (it's the same, the value is just cached there).

Done.

[Marc Treib, 2016-08-30 13:01:56]

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:47:
base::Callback<bool(const GURL& url)> filter) override;
This should be a const &

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_provider.h:109: // matched by
|filter|.
It's really an intersection between the time range and the filter, right? I.e.
remove everything that is in that range and that also matches the filter? Then
I'd say something like
"..from the specified time range where the URL matches the |filter|."

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:128: // matched by
|filter|.
Same here

[msramek, 2016-08-30 13:02:22]

LGTM.

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:341: base::Callback<bool(const
GURL& url)> filter) {
nit: If these deletions are going to happen on the same (UI) thread, consider
passing const ref instead of a copy.

[vitaliii, 2016-08-30 14:17:48]

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
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 Treib wrote:
> This should be a const &

Done.

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_provider.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_provider.h:109: // matched by
|filter|.
On 2016/08/30 13:01:56, Marc Treib wrote:
> It's really an intersection between the time range and the filter, right? I.e.
> remove everything that is in that range and that also matches the filter? Then
> I'd say something like
> "..from the specified time range where the URL matches the |filter|."

Done.

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:128: // matched by
|filter|.
On 2016/08/30 13:01:56, Marc Treib wrote:
> Same here

Done.

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2284393002/diff/100001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:341: base::Callback<bool(const
GURL& url)> filter) {
On 2016/08/30 13:02:22, msramek wrote:
> nit: If these deletions are going to happen on the same (UI) thread, consider
> passing const ref instead of a copy.

Done.

[vitaliii, 2016-08-30 14:19:03]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-08-30 14:19:03]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2284393002/#ps120001
(title: "|const &| and comments.")

[commit-bot: I haz the power, 2016-08-30 14:19:42]

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-08-30 14:42:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 14:42:36]

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_...)

[vitaliii, 2016-08-31 05:49:29]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2016-08-31 05:49: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-08-31 06:35:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 06:35:34]

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_androi...)

[vitaliii, 2016-08-31 09:39:50]

Patchset #7 (id:140001) has been deleted

[vitaliii, 2016-08-31 09:41:27]

Patchset #7 (id:160001) has been deleted

[vitaliii, 2016-08-31 09:43:33]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-08-31 09:43:34]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2284393002/#ps180001
(title: "|bookmark_model| can be null in tests.")

[commit-bot: I haz the power, 2016-08-31 09:43:58]

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-08-31 11:26:01]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-08-31 11:26:03]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-08-31 11:27:26]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-08-31 11:27:27]

Patchset 7 (id:??) landed as
https://crrev.com/685fdfaad7f4ee4aa4b49ccf1d9d2db318b4341a
Cr-Commit-Position: refs/heads/master@{#415609}