Respect time range in browsing data removal for last-visited data.

Respect time range in browsing data removal for last-visited data.

This CL changes the data removal logic in last-visited data to only remove metadata
of nodes with a last-visited data inside the provided time-range.
Previously, that metdata was deleted from all nodes. This had the result that no
bookmarks were present in content suggestions unless they had been visited again.

BUG=674178
Review-Url: https://codereview.chromium.org/2616633002
Cr-Commit-Position: refs/heads/master@{#442424}
Committed: https://chromium.googlesource.com/chromium/src/+/26e30ced5c0018ccada8063ee8b0bcd2fac293af

[tschumann, 2017-01-04 11:00:52]

tschumann@chromium.org changed reviewers:
+ jkrcal@chromium.org, msramek@chromium.org

[msramek, 2017-01-04 11:47:12]

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

https://codereview.chromium.org/2616633002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:296: //
TODO(vitaliii): Do not remove all dates, but only the ones matched by the
nit: This TODO seems to be addressed.

https://codereview.chromium.org/2616633002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:301:
ntp_snippets::RemoveLastVisitedDatesBetween(delete_begin_, delete_end_,
Could you update BrowsingDataRemoverTest? You can use
AddURLWithCreationTimeAndMetaInfo to add entries with timestamps in the past.

[jkrcal, 2017-01-04 12:14:22]

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:285:
base::Callback<bool(const GURL& url)> filter,
Why do you accept |filter| when you not use it? Why not call the filter with
|url_and_title.url|?

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:200: //
Both should get removed.
This is more a question to the implementation: are you sure both should get
removed? This way, you remove more than desired, in my opinion.

[tschumann, 2017-01-04 13:32:32]

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

https://codereview.chromium.org/2616633002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:296: //
TODO(vitaliii): Do not remove all dates, but only the ones matched by the
On 2017/01/04 11:47:12, msramek wrote:
> nit: This TODO seems to be addressed.

Done.

https://codereview.chromium.org/2616633002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc:301:
ntp_snippets::RemoveLastVisitedDatesBetween(delete_begin_, delete_end_,
On 2017/01/04 11:47:12, msramek wrote:
> Could you update BrowsingDataRemoverTest? You can use
> AddURLWithCreationTimeAndMetaInfo to add entries with timestamps in the past.

Done. Deeper test coverage is in bookmark_last_visit_utils_unittest.

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:285:
base::Callback<bool(const GURL& url)> filter,
On 2017/01/04 12:14:22, jkrcal wrote:
> Why do you accept |filter| when you not use it? Why not call the filter with
> |url_and_title.url|?

Good point! Will add this functionality. (Sending my responses now to get
Martin's input on the other comment).

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:200: //
Both should get removed.
On 2017/01/04 12:14:22, jkrcal wrote:
> This is more a question to the implementation: are you sure both should get
> removed? This way, you remove more than desired, in my opinion.

This was my understanding of Dominic's comment:
https://bugs.chromium.org/p/chromium/issues/detail?id=674178#c3

+msramek What is your take on this?

[jkrcal, 2017-01-04 14:09:26]

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:200: //
Both should get removed.
On 2017/01/04 13:32:32, tschumann wrote:
> On 2017/01/04 12:14:22, jkrcal wrote:
> > This is more a question to the implementation: are you sure both should get
> > removed? This way, you remove more than desired, in my opinion.
> 
> This was my understanding of Dominic's comment:
> https://bugs.chromium.org/p/chromium/issues/detail?id=674178#c3
> 
> +msramek What is your take on this?

Interesting :) 

My interpretation of Dominic's comment is "also delete visits from the other
platform (if they satisfy the time range)" and not "also delete visits from the
other platform irrespective of the time".

[msramek, 2017-01-04 14:19:07]

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils_unittest.cc:200: //
Both should get removed.
On 2017/01/04 14:09:25, jkrcal wrote:
> On 2017/01/04 13:32:32, tschumann wrote:
> > On 2017/01/04 12:14:22, jkrcal wrote:
> > > This is more a question to the implementation: are you sure both should
get
> > > removed? This way, you remove more than desired, in my opinion.
> > 
> > This was my understanding of Dominic's comment:
> > https://bugs.chromium.org/p/chromium/issues/detail?id=674178#c3
> > 
> > +msramek What is your take on this?
> 
> Interesting :) 
> 
> My interpretation of Dominic's comment is "also delete visits from the other
> platform (if they satisfy the time range)" and not "also delete visits from
the
> other platform irrespective of the time".

I agree with Jan. Definitely take timestamps into account where applicable.

(...this reminds me of the joke about the programmer buying milk and bread rolls
:) )

[msramek, 2017-01-04 14:24:04]

browsing_data/ LGTM

https://codereview.chromium.org/2616633002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2616633002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3084: const
base::Time delete_end = delete_begin + base::TimeDelta::FromDays(2);
nit: This is unnecessary - we can use just base::Time::Max().

[tschumann, 2017-01-04 15:08:44]

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:285:
base::Callback<bool(const GURL& url)> filter,
On 2017/01/04 13:32:32, tschumann wrote:
> On 2017/01/04 12:14:22, jkrcal wrote:
> > Why do you accept |filter| when you not use it? Why not call the filter with
> > |url_and_title.url|?
> 
> Good point! Will add this functionality. (Sending my responses now to get
> Martin's input on the other comment).

Done.
+Martin: Can you double-check I got the filter semantics right? (couldn't find a
clear documentation if filtering out means out of the history or out of the
removal process).
Somebody remind me that I shouldn't go grocery shopping today [milk / bread
rolls...] xD

https://codereview.chromium.org/2616633002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2616633002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3084: const
base::Time delete_end = delete_begin + base::TimeDelta::FromDays(2);
On 2017/01/04 14:24:04, msramek wrote:
> nit: This is unnecessary - we can use just base::Time::Max().

Done.

[msramek, 2017-01-04 15:16:26]

Still LGTM.

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

https://codereview.chromium.org/2616633002/diff/1/components/ntp_snippets/boo...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:285:
base::Callback<bool(const GURL& url)> filter,
On 2017/01/04 15:08:44, tschumann wrote:
> On 2017/01/04 13:32:32, tschumann wrote:
> > On 2017/01/04 12:14:22, jkrcal wrote:
> > > Why do you accept |filter| when you not use it? Why not call the filter
with
> > > |url_and_title.url|?
> > 
> > Good point! Will add this functionality. (Sending my responses now to get
> > Martin's input on the other comment).
> 
> Done.
> +Martin: Can you double-check I got the filter semantics right? (couldn't find
a
> clear documentation if filtering out means out of the history or out of the
> removal process).
> Somebody remind me that I shouldn't go grocery shopping today [milk / bread
> rolls...] xD

Yeah, sorry for that. I guess we really should have used the word 'predicate'
primarily... url should be deleted iff filter(url), so you got the semantics
right.

[tschumann, 2017-01-04 15:30:35]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 15:30:48]

Dry run: 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, 2017-01-04 16:14:29]

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

[commit-bot: I haz the power, 2017-01-04 16:14:30]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-04 20:27:20]

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

https://codereview.chromium.org/2616633002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:3124:
BrowsingDataRemover::TimeRange(delete_begin, base::Time::Max()),
FYI, I landed https://codereview.chromium.org/2578723002 which deprecated
BrowsingDataRemover::TimeRange and changed the interface here to take two
base::Time parameters.

Please rebase and replace this line with "delete_begin, base::Time::Max()".

[tschumann, 2017-01-05 12:48:10]

rebased. over to Jan :-)

[jkrcal, 2017-01-05 13:28:55]

lgtm (modulo this one comment)

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

https://codereview.chromium.org/2616633002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:283:
model->DeleteNodeMetaInfo(&node, kBookmarkDismissedFromNTP);
Can you please remove the dismissed bit only if there is no visit date
remaining? This way, it can lead into tricky situations (something re-appearing
on NTP because you delete history).

[tschumann, 2017-01-05 19:25:17]

with the other change, the UI will immediately refetch content after
ClearHistory events. So I prepared the provider for this, too.

Note to myself: bookmark suggestions provider is not unit-tested -- change that.

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

https://codereview.chromium.org/2616633002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:283:
model->DeleteNodeMetaInfo(&node, kBookmarkDismissedFromNTP);
On 2017/01/05 13:28:55, jkrcal wrote:
> Can you please remove the dismissed bit only if there is no visit date
> remaining? This way, it can lead into tricky situations (something
re-appearing
> on NTP because you delete history).

let's not remove this here at all -- the content suggestion's bookmarks provider
is taking care of that data.

[tschumann, 2017-01-05 19:25:28]

The CQ bit was checked by tschumann@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 19:26:01]

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

[jkrcal, 2017-01-05 20:01:50]

still lgtm

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

https://codereview.chromium.org/2616633002/diff/40001/components/ntp_snippets...
components/ntp_snippets/bookmarks/bookmark_last_visit_utils.cc:283:
model->DeleteNodeMetaInfo(&node, kBookmarkDismissedFromNTP);
On 2017/01/05 19:25:17, tschumann wrote:
> On 2017/01/05 13:28:55, jkrcal wrote:
> > Can you please remove the dismissed bit only if there is no visit date
> > remaining? This way, it can lead into tricky situations (something
> re-appearing
> > on NTP because you delete history).
> 
> let's not remove this here at all -- the content suggestion's bookmarks
provider
> is taking care of that data.

Acknowledged.

[commit-bot: I haz the power, 2017-01-05 20:25:23]

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

[commit-bot: I haz the power, 2017-01-05 20:25:24]

Dry run: This issue passed the CQ dry run.

[tschumann, 2017-01-09 22:43:40]

The CQ bit was checked by tschumann@chromium.org

[tschumann, 2017-01-09 22:43:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2616633002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-09 22:44:16]

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, 2017-01-10 00:36:51]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484001820472350,
"parent_rev": "035c57234344f21d73954f65483bf946d90bc51b", "commit_rev":
"26e30ced5c0018ccada8063ee8b0bcd2fac293af"}

[commit-bot: I haz the power, 2017-01-10 00:37:20]

Description was changed from

==========
Respect time range in browsing data removal for last-visited data.

This CL changes the data removal logic in last-visited data to only remove
metadata
of nodes with a last-visited data inside the provided time-range.
Previously, that metdata was deleted from all nodes. This had the result that no
bookmarks were present in content suggestions unless they had been visited
again.

BUG=674178
==========

to

==========
Respect time range in browsing data removal for last-visited data.

This CL changes the data removal logic in last-visited data to only remove
metadata
of nodes with a last-visited data inside the provided time-range.
Previously, that metdata was deleted from all nodes. This had the result that no
bookmarks were present in content suggestions unless they had been visited
again.

BUG=674178

Review-Url: https://codereview.chromium.org/2616633002
Cr-Commit-Position: refs/heads/master@{#442424}
Committed:
https://chromium.googlesource.com/chromium/src/+/26e30ced5c0018ccada8063ee8b0...
==========

[commit-bot: I haz the power, 2017-01-10 00:37:21]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/26e30ced5c0018ccada8063ee8b0...