Adding ClearBlackList to the PreviewsBlackList and plumbing to UI

Adding ClearBlackList to the PreviewsBlackList and plumbing to UI

This adds functionality to clear the previews black list on the IO
thread and the UI thread. Further this exposes other black list calls on
the UI thread.

BUG=639087
Committed: https://crrev.com/84af4a126bf6503d253be5e29b6ed3752e933017
Cr-Commit-Position: refs/heads/master@{#422926}

[RyanSturm, 2016-09-30 23:50:37]

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

[RyanSturm, 2016-09-30 23:51:05]

ryansturm@chromium.org changed reviewers:
+ tbansal@chromium.org

[RyanSturm, 2016-09-30 23:51:06]

tbansal: PTAL

[commit-bot: I haz the power, 2016-09-30 23:51:16]

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, 2016-10-01 00:39:01]

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

[commit-bot: I haz the power, 2016-10-01 00:39:02]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-03 16:48:12]

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

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:733:
previews_service->previews_ui_service()->ClearBlackList(delete_begin_,
Currently, the Clear Browsing Dialogue has "data counters", showing how much
data will be deleted when something is cleared. Those counters should also take
into account the disk space consumed by blacklist if it takes a non-trivial
amount of space.

Can you add a TODO and file a bug to make sure that the counters are fixed
before this is enabled?

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/previews/pre...
File chrome/browser/previews/previews_service_factory.cc (left):

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/previews/pre...
chrome/browser/previews/previews_service_factory.cc:21:
DCHECK_NE(profile->GetProfileType(), Profile::INCOGNITO_PROFILE);
Why was this removed?

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
File components/previews/core/previews_black_list.h (right):

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
components/previews/core/previews_black_list.h:65: // in a null store, resets
all history in the in-memory black list.
Slightly unclear. May be:
s/|end_time|. Reloads/ |end_time|, and reloads/

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
File components/previews/core/previews_black_list_unittest.cc (right):

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
components/previews/core/previews_black_list_unittest.cc:188:
base::RunLoop().RunUntilIdle();
What is this message pump doing given that the TestPreviewsOptOutStore has an
empty implementation for ClearBlackList()?

[RyanSturm, 2016-10-03 17:16:03]

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

[commit-bot: I haz the power, 2016-10-03 17:16:25]

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

[RyanSturm, 2016-10-03 17:23:12]

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

[RyanSturm, 2016-10-03 17:23:13]

tbansal: PTAL
msramek: PTAL @ browsing_data_remover.cc

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

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:733:
previews_service->previews_ui_service()->ClearBlackList(delete_begin_,
On 2016/10/03 16:48:12, tbansal1 wrote:
> Currently, the Clear Browsing Dialogue has "data counters", showing how much
> data will be deleted when something is cleared. Those counters should also
take
> into account the disk space consumed by blacklist if it takes a non-trivial
> amount of space.
> 
> Can you add a TODO and file a bug to make sure that the counters are fixed
> before this is enabled?

Done.

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/previews/pre...
File chrome/browser/previews/previews_service_factory.cc (left):

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/previews/pre...
chrome/browser/previews/previews_service_factory.cc:21:
DCHECK_NE(profile->GetProfileType(), Profile::INCOGNITO_PROFILE);
On 2016/10/03 16:48:12, tbansal1 wrote:
> Why was this removed?

Below will return null for incognito, and the clear browsing history code calls
this.

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
File components/previews/core/previews_black_list.h (right):

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
components/previews/core/previews_black_list.h:65: // in a null store, resets
all history in the in-memory black list.
On 2016/10/03 16:48:12, tbansal1 wrote:
> Slightly unclear. May be:
> s/|end_time|. Reloads/ |end_time|, and reloads/

Done.

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
File components/previews/core/previews_black_list_unittest.cc (right):

https://codereview.chromium.org/2387823002/diff/1/components/previews/core/pr...
components/previews/core/previews_black_list_unittest.cc:188:
base::RunLoop().RunUntilIdle();
On 2016/10/03 16:48:12, tbansal1 wrote:
> What is this message pump doing given that the TestPreviewsOptOutStore has an
> empty implementation for ClearBlackList()?

ClearBlackList actually calls both ClearBlackList and LoadBlackList from the
store.

I imagine this test would fail if no post task were occuring.

[commit-bot: I haz the power, 2016-10-03 18:41:42]

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

[commit-bot: I haz the power, 2016-10-03 18:41:44]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-04 00:17:00]

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_black_list.cc (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_black_list.cc:106:
opt_out_store_->ClearBlackList(begin_time, end_time);
So, this means that opt out store must execute ClearBlackList  synchronously.
Right?

Alternatively, it might be more flexible if you make this async and let the opt
out store call LoadBlackListDone when it is done clearing the blacklist.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_black_list_unittest.cc (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_black_list_unittest.cc:61: void
ClearBlackList(base::Time begin_time, base::Time end_time) override {}
you can add a int variable clear_blacklist_count_
which gets incremented in this function.

Then in the tests below you can check the value of |clear_blacklist_count_| to
verify that it s indeed being called when CLearBlackList() was called on the
profile.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_io_data.h (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_io_data.h:42: // Clears the history of the
black list between |begin_time| and |end_time|.
Add: both inclusive.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_opt_out_store.h (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_opt_out_store.h:55: // Deletes all history in
the store between |begin_time| and |end_time|.
s/Deletes/Synchronously deletes/

Also add: both inclusive
to indicate that both |begin_time| and |end_time| are inclusive.

[tbansal1, 2016-10-04 01:31:46]

lgtm % remaining comments.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_black_list.cc (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_black_list.cc:106:
opt_out_store_->ClearBlackList(begin_time, end_time);
On 2016/10/04 00:16:59, tbansal1 wrote:
> So, this means that opt out store must execute ClearBlackList  synchronously.
> Right?
> 
> Alternatively, it might be more flexible if you make this async and let the
opt
> out store call LoadBlackListDone when it is done clearing the blacklist. 

Discard this comment.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_opt_out_store.h (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_opt_out_store.h:55: // Deletes all history in
the store between |begin_time| and |end_time|.
On 2016/10/04 00:17:00, tbansal1 wrote:
> s/Deletes/Synchronously deletes/
> 
> Also add: both inclusive
> to indicate that both |begin_time| and |end_time| are inclusive.

nm, this makes sense now.

[msramek, 2016-10-04 09:06:18]

browsing_data/ LGTM

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

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:733:
previews_service->previews_ui_service()->ClearBlackList(delete_begin_,
On 2016/10/03 17:23:13, Ryan Sturm wrote:
> On 2016/10/03 16:48:12, tbansal1 wrote:
> > Currently, the Clear Browsing Dialogue has "data counters", showing how much
> > data will be deleted when something is cleared. Those counters should also
> take
> > into account the disk space consumed by blacklist if it takes a non-trivial
> > amount of space.
> > 
> > Can you add a TODO and file a bug to make sure that the counters are fixed
> > before this is enabled?
> 
> Done.

I appreciate the diligence, but this probably won't be needed. For "browsing
history", the counter only shows the number of history entries. It doesn't count
any other (meta)data, as the UI does not have enough space for that.

[RyanSturm, 2016-10-04 19:54:46]

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

[commit-bot: I haz the power, 2016-10-04 19:55:23]

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

[RyanSturm, 2016-10-04 20:27:23]

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

https://codereview.chromium.org/2387823002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:733:
previews_service->previews_ui_service()->ClearBlackList(delete_begin_,
On 2016/10/04 09:06:18, msramek wrote:
> On 2016/10/03 17:23:13, Ryan Sturm wrote:
> > On 2016/10/03 16:48:12, tbansal1 wrote:
> > > Currently, the Clear Browsing Dialogue has "data counters", showing how
much
> > > data will be deleted when something is cleared. Those counters should also
> > take
> > > into account the disk space consumed by blacklist if it takes a
non-trivial
> > > amount of space.
> > > 
> > > Can you add a TODO and file a bug to make sure that the counters are fixed
> > > before this is enabled?
> > 
> > Done.
> 
> I appreciate the diligence, but this probably won't be needed. For "browsing
> history", the counter only shows the number of history entries. It doesn't
count
> any other (meta)data, as the UI does not have enough space for that.

Thanks. I'll remove it.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_black_list_unittest.cc (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_black_list_unittest.cc:61: void
ClearBlackList(base::Time begin_time, base::Time end_time) override {}
On 2016/10/04 00:16:59, tbansal1 wrote:
> you can add a int variable clear_blacklist_count_
> which gets incremented in this function.
> 
> Then in the tests below you can check the value of |clear_blacklist_count_| to
> verify that it s indeed being called when CLearBlackList() was called on the
> profile.

Done.

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
File components/previews/core/previews_io_data.h (right):

https://codereview.chromium.org/2387823002/diff/20001/components/previews/cor...
components/previews/core/previews_io_data.h:42: // Clears the history of the
black list between |begin_time| and |end_time|.
On 2016/10/04 00:16:59, tbansal1 wrote:
> Add: both inclusive.

Done.

[commit-bot: I haz the power, 2016-10-04 20:40:27]

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

[commit-bot: I haz the power, 2016-10-04 20:40:27]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-10-04 20:48:26]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-10-04 20:48:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2387823002/#ps40001
(title: "tbansal comments")

[commit-bot: I haz the power, 2016-10-04 20:49:35]

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-10-04 20:58:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-04 21:01:27]

Description was changed from

==========
Adding ClearBlackList to the PreviewsBlackList and plumbing to UI

This adds functionality to clear the previews black list on the IO
thread and the UI thread. Further this exposes other black list calls on
the UI thread.

BUG=639087
==========

to

==========
Adding ClearBlackList to the PreviewsBlackList and plumbing to UI

This adds functionality to clear the previews black list on the IO
thread and the UI thread. Further this exposes other black list calls on
the UI thread.

BUG=639087

Committed: https://crrev.com/84af4a126bf6503d253be5e29b6ed3752e933017
Cr-Commit-Position: refs/heads/master@{#422926}
==========

[commit-bot: I haz the power, 2016-10-04 21:01:28]

Patchset 3 (id:??) landed as
https://crrev.com/84af4a126bf6503d253be5e29b6ed3752e933017
Cr-Commit-Position: refs/heads/master@{#422926}