Implement a task scheduler for BrowsingDataRemover

Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromium.org,stevenjb@chromium.org
BUG=630327
Committed: https://crrev.com/6f166831372df3044052eda2459982671c24ea5e
Cr-Commit-Position: refs/heads/master@{#410016}

[msramek, 2016-07-22 12:41:06]

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

[commit-bot: I haz the power, 2016-07-22 12:41:22]

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-07-22 13:01:47]

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

[commit-bot: I haz the power, 2016-07-22 13:01:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[msramek, 2016-07-22 13:49:34]

Patchset #1 (id:1) has been deleted

[msramek, 2016-07-22 13:50:30]

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

[commit-bot: I haz the power, 2016-07-22 13:50:53]

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-07-22 14:22:36]

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

[commit-bot: I haz the power, 2016-07-22 14:22:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msramek, 2016-07-22 14:50:11]

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

[commit-bot: I haz the power, 2016-07-22 14:50:33]

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

[msramek, 2016-07-22 15:08:44]

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

[commit-bot: I haz the power, 2016-07-22 15:08:47]

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-07-22 15:39:03]

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

[commit-bot: I haz the power, 2016-07-22 15:39:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-22 20:18:47]

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

[commit-bot: I haz the power, 2016-07-22 20:19:30]

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-07-22 20:32:14]

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

[commit-bot: I haz the power, 2016-07-22 20:32:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-22 20:57:12]

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

[commit-bot: I haz the power, 2016-07-22 20:58:08]

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-07-22 21:33:04]

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

[commit-bot: I haz the power, 2016-07-22 21:33:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msramek, 2016-07-22 22:24:11]

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

[commit-bot: I haz the power, 2016-07-22 22:24:39]

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-07-22 23:05:58]

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

[commit-bot: I haz the power, 2016-07-22 23:05:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-22 23:42:04]

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

[commit-bot: I haz the power, 2016-07-22 23:42:22]

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-07-23 00:15:23]

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

[commit-bot: I haz the power, 2016-07-23 00:15:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-25 08:16:43]

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

[commit-bot: I haz the power, 2016-07-25 08:16:53]

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

[msramek, 2016-07-25 09:10:20]

Description was changed from

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back. To ensure that
   observers maintain this hygiene of unregistering themselves, we
   DCHECK that there are no observers left in the destructor.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers. The NotificationService broadcast is also still left in
   this CL to avoid complicating tests.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.
- Remove the NotificationService broadcast. Comments in this CL already
  suggest how that would work, but the change is unrelated to this CL
  and was thus omitted.

BUG=630327
==========

to

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

BUG=630327
==========

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-25 09:36:56]

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

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

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-07-25 10:49:56]

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

[commit-bot: I haz the power, 2016-07-25 10:49:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-07-26 09:59:11]

Patchset #8 (id:160001) has been deleted

[msramek, 2016-07-26 09:59:21]

Patchset #2 (id:40001) has been deleted

[msramek, 2016-07-26 09:59:28]

Patchset #1 (id:20001) has been deleted

[msramek, 2016-07-26 09:59:40]

Patchset #1 (id:60001) has been deleted

[msramek, 2016-07-26 09:59:49]

Patchset #1 (id:80001) has been deleted

[msramek, 2016-07-26 10:00:00]

Patchset #1 (id:100001) has been deleted

[msramek, 2016-07-26 10:00:09]

Patchset #1 (id:120001) has been deleted

[msramek, 2016-07-26 10:00:20]

Patchset #1 (id:140001) has been deleted

[msramek, 2016-07-26 13:07:25]

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

[commit-bot: I haz the power, 2016-07-26 13:07:36]

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

[msramek, 2016-07-26 13:13:14]

Patchset #1 (id:180001) has been deleted

[msramek, 2016-07-26 13:36:39]

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

[msramek, 2016-07-26 13:36:40]

Hi Bernhard,

Can you please have a look?

This is the implementation of the task scheduler that I proposed recently. It's
based atop of the prerequisite https://codereview.chromium.org/2171383002 which
consolidates the observers, but that should not be too relevant for this review.

One change w.r.t. the original proposal is that this CL still uses
AddObserver/RemoveObserver instead of passing base::Callback. The reason is, as
dbeam@ pointed out in the proposal doc, that using callbacks would force us to
add a WeakPtrFactory to every callsite. With observers, the object lifetime
issues are mostly taken care of by base::ObserverList checking in its destructor
that all observers have unregistered themselves. And they do in fact already
unregister themselves properly, which means that this approach requires no
further changes on the callsites.

Thanks,
Martin

[commit-bot: I haz the power, 2016-07-26 14:04:03]

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

[commit-bot: I haz the power, 2016-07-26 14:04:05]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-07-26 14:51:39]

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:383: DCHECK(!observer ||
observer_list_.HasObserver(observer))
If you allow the |observer| to be null, do you even still need the
Remove(WithFilter) methods without reply?

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:387:
task_queue_.push(RemovalTask(
You could use emplace() for that.

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:81: //  2. Using an
observer.
This doesn't really make it clear what the difference to 3 is. Maybe "one-shot
deletion"?

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:201: // for one observer at
a time. Consider splitting the interface.
I have to agree: I do find it very weird to have the two parts of this interface
used in two completely different ways. Really, for one-shot deletions,
RemoveObserver() is purely used as a signal for cancellation, and then
AddObserver() is just used to allow RemoveObserver() to work. And all of that
just to require fewer changes to callsites, which doesn't seem like it's the
right tradeoff in the longer term.

So, I'd be okay with moving forward with this CL if we can strengthen the TODO
here to "split the interface", not just considering it :)

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:235: struct RemovalTask {
Does this need to be public?

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:250:
std::unique_ptr<OriginFilterBuilder> filter_builder;
You could directly initialize this.

[msramek, 2016-07-28 09:56:08]

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

[msramek, 2016-07-28 09:56:09]

Patchset #2 (id:220001) has been deleted

[commit-bot: I haz the power, 2016-07-28 09:56:27]

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

[msramek, 2016-07-28 09:57:42]

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:383: DCHECK(!observer ||
observer_list_.HasObserver(observer))
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> If you allow the |observer| to be null, do you even still need the
> Remove(WithFilter) methods without reply?

All methods can be reduced to the last one, as demonstrated in the test.

Since the usecase with no filter and no observer should be the most common one,
I think it makes sense to make these simpler versions of the method. It's a few
lines of code here and it lowers the footprint of this CL everywhere else, so it
seems preferable to me. But I'm happy to be convinced otherwise :)

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:387:
task_queue_.push(RemovalTask(
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> You could use emplace() for that.

Done. Nice :)

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:81: //  2. Using an
observer.
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> This doesn't really make it clear what the difference to 3 is. Maybe "one-shot
> deletion"?

Expanded the descriptions a bit. "One-shot deletion" is not exactly correct,
because this class removes the observer in its destructor, so it could run and
observe more deletions.

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:201: // for one observer at
a time. Consider splitting the interface.
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> I have to agree: I do find it very weird to have the two parts of this
interface
> used in two completely different ways. Really, for one-shot deletions,
> RemoveObserver() is purely used as a signal for cancellation, and then
> AddObserver() is just used to allow RemoveObserver() to work. And all of that
> just to require fewer changes to callsites, which doesn't seem like it's the
> right tradeoff in the longer term.
> 
> So, I'd be okay with moving forward with this CL if we can strengthen the TODO
> here to "split the interface", not just considering it :)

I strengthened the TODO, and I readded the deprecation message.
OnBrowsingDataRemoving was added mostly as a quick remedy while this task
queuing mechanism is finished; I'm currently considering that it might not even
be needed anymore. I'll have to talk to dbeam@ first about that though.

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:235: struct RemovalTask {
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> Does this need to be public?

Nope. Done.

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:250:
std::unique_ptr<OriginFilterBuilder> filter_builder;
On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> You could directly initialize this.

Done.

[commit-bot: I haz the power, 2016-07-28 10:19:40]

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

[commit-bot: I haz the power, 2016-07-28 10:19:41]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[msramek, 2016-07-28 10:37:30]

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

[commit-bot: I haz the power, 2016-07-28 10:37:46]

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-07-28 11:24:55]

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

[commit-bot: I haz the power, 2016-07-28 11:24:56]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-03 16:42:46]

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:383: DCHECK(!observer ||
observer_list_.HasObserver(observer))
On 2016/07/28 09:57:42, msramek wrote:
> On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> > If you allow the |observer| to be null, do you even still need the
> > Remove(WithFilter) methods without reply?
> 
> All methods can be reduced to the last one, as demonstrated in the test.
> 
> Since the usecase with no filter and no observer should be the most common
one,
> I think it makes sense to make these simpler versions of the method. It's a
few
> lines of code here and it lowers the footprint of this CL everywhere else, so
it
> seems preferable to me. But I'm happy to be convinced otherwise :)

Hm, could we make one method that takes no observer and one method that takes a
non-null observer (and have both forward to an internal method)? That way
existing callers for the no-observer method don't need to be changed, but we
would catch cases where someone accidentally passes in a null observer.

[msramek, 2016-08-03 21:07:06]

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

[commit-bot: I haz the power, 2016-08-03 21:07:54]

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

[msramek, 2016-08-03 21:16:44]

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2175703002/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:383: DCHECK(!observer ||
observer_list_.HasObserver(observer))
On 2016/08/03 16:42:46, Bernhard Bauer wrote:
> On 2016/07/28 09:57:42, msramek wrote:
> > On 2016/07/26 14:51:38, Bernhard Bauer wrote:
> > > If you allow the |observer| to be null, do you even still need the
> > > Remove(WithFilter) methods without reply?
> > 
> > All methods can be reduced to the last one, as demonstrated in the test.
> > 
> > Since the usecase with no filter and no observer should be the most common
> one,
> > I think it makes sense to make these simpler versions of the method. It's a
> few
> > lines of code here and it lowers the footprint of this CL everywhere else,
so
> it
> > seems preferable to me. But I'm happy to be convinced otherwise :)
> 
> Hm, could we make one method that takes no observer and one method that takes
a
> non-null observer (and have both forward to an internal method)? That way
> existing callers for the no-observer method don't need to be changed, but we
> would catch cases where someone accidentally passes in a null observer.

Yep, that makes more sense actually. Done.

[commit-bot: I haz the power, 2016-08-03 22:02:28]

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

[commit-bot: I haz the power, 2016-08-03 22:02:30]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-08-04 12:30:51]

LGTM!

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:286: Observer* observer);
You might want to make these non-virtual anymore.

[msramek, 2016-08-04 12:48:57]

Thanks Bernhard!

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:286: Observer* observer);
On 2016/08/04 12:30:51, Bernhard Bauer wrote:
> You might want to make these non-virtual anymore.

It's actually "not yet" rather than "anymore". This part was landed in
https://codereview.chromium.org/2161583002 which was extracted from
https://codereview.chromium.org/2025683003/. In the latter CL, I'm mocking
RemoveWithFilter() so I need it to be virtual.

Should I perhaps instead declare all four as virtual, so that readers of this
code don't have to wonder why the first two aren't and the last two are?

[Bernhard Bauer, 2016-08-04 12:57:41]

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:286: Observer* observer);
On 2016/08/04 12:48:57, msramek wrote:
> On 2016/08/04 12:30:51, Bernhard Bauer wrote:
> > You might want to make these non-virtual anymore.
> 
> It's actually "not yet" rather than "anymore". This part was landed in
> https://codereview.chromium.org/2161583002 which was extracted from
> https://codereview.chromium.org/2025683003/. In the latter CL, I'm mocking
> RemoveWithFilter() so I need it to be virtual.
> 
> Should I perhaps instead declare all four as virtual, so that readers of this
> code don't have to wonder why the first two aren't and the last two are?

Could you just declare the Internal method as virtual? That's where all calls
end up anyway.

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:286: Observer* observer);
On 2016/08/04 12:48:57, msramek wrote:
> On 2016/08/04 12:30:51, Bernhard Bauer wrote:
> > You might want to make these non-virtual anymore.
> 
> It's actually "not yet" rather than "anymore". This part was landed in
> https://codereview.chromium.org/2161583002 which was extracted from
> https://codereview.chromium.org/2025683003/. In the latter CL, I'm mocking
> RemoveWithFilter() so I need it to be virtual.
> 
> Should I perhaps instead declare all four as virtual, so that readers of this
> code don't have to wonder why the first two aren't and the last two are?

Could you just declare the Internal method as virtual? That's where it all ends
up anyway.

[msramek, 2016-08-04 13:19:47]

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

[commit-bot: I haz the power, 2016-08-04 13:19:59]

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

[msramek, 2016-08-04 13:20:27]

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/2175703002/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:286: Observer* observer);
On 2016/08/04 12:57:41, Bernhard Bauer wrote:
> On 2016/08/04 12:48:57, msramek wrote:
> > On 2016/08/04 12:30:51, Bernhard Bauer wrote:
> > > You might want to make these non-virtual anymore.
> > 
> > It's actually "not yet" rather than "anymore". This part was landed in
> > https://codereview.chromium.org/2161583002 which was extracted from
> > https://codereview.chromium.org/2025683003/. In the latter CL, I'm mocking
> > RemoveWithFilter() so I need it to be virtual.
> > 
> > Should I perhaps instead declare all four as virtual, so that readers of
this
> > code don't have to wonder why the first two aren't and the last two are?
> 
> Could you just declare the Internal method as virtual? That's where all calls
> end up anyway.

...yes :) Apparently I'm a bit slow today. Done. Also made it protected.

[msramek, 2016-08-04 13:35:36]

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

[commit-bot: I haz the power, 2016-08-04 13:35:54]

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

[msramek, 2016-08-04 13:45:45]

Description was changed from

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

BUG=630327
==========

to

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromi...
BUG=630327
==========

[msramek, 2016-08-04 13:47:04]

msramek@chromium.org changed reviewers:
+ engedy@chromium.org, johnme@chromium.org, meacer@chromium.org,
mmenke@chromium.org, stevenjb@chromium.org

[msramek, 2016-08-04 13:47:05]

The signature of Remove() calls has changed, so this CL also updates the
callsites. Adding owners to TBR.

TL;DR Every observer of BrowsingDataRemover will now only receive
OnBrowsingDataRemoverDone callbacks from its own removal task. This doesn't
change anything in practice, since executing more than one removal task at the
same time previously carried high risk of a crash, so no one was doing that.

mmenke@ for c/b/net and c/b/prerender
engedy@ for c/b/profile_resetter
meacer@ for c/b/ssl
johnme@ for c/b/push_messaging
stevenjb@ for c/b/ui and c/b/chromeos

[msramek, 2016-08-04 13:49:11]

And explicitly +cc dbeam@ FYI this will probably be able to supersede your
solution for the MD CBD dialog in https://codereview.chromium.org/2133893002/,
but let's talk about that in a separate email thread :)

[mmenke, 2016-08-04 13:52:18]

On 2016/08/04 13:49:11, msramek wrote:
> And explicitly +cc dbeam@ FYI this will probably be able to supersede your
> solution for the MD CBD dialog in https://codereview.chromium.org/2133893002/,
> but let's talk about that in a separate email thread :)

chrome/browser/net, chrome/browser/prerender LGTM

[engedy, 2016-08-04 14:36:52]

chrome/browser/profile_resetter/ LGTM.

[commit-bot: I haz the power, 2016-08-04 14:37:25]

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

[commit-bot: I haz the power, 2016-08-04 14:37:27]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-08-04 17:21:38]

chrome/browser/ssl LGTM

[johnme, 2016-08-04 17:28:56]

push_messaging/ lgtm

[stevenjb, 2016-08-04 23:48:01]

RS owner lgtm

[msramek, 2016-08-05 08:40:49]

Thanks everyone! I added y'all to TBR, but then decided to wait a day just in
case...

[msramek, 2016-08-05 08:41:00]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-08-05 08:41:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2175703002/#ps340001
(title: "Formatting.")

[commit-bot: I haz the power, 2016-08-05 08:41:10]

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-05 08:44:48]

Description was changed from

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromi...
BUG=630327
==========

to

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromi...
BUG=630327
==========

[commit-bot: I haz the power, 2016-08-05 08:44:50]

Committed patchset #7 (id:340001)

[commit-bot: I haz the power, 2016-08-05 08:46:48]

Description was changed from

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromi...
BUG=630327
==========

to

==========
Implement a task scheduler for BrowsingDataRemover

BrowsingDataRemover now processes each Remove() call by adding a task
to a queue; the tasks are then processed serially.

Design doc and discussion:
https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/

Changes in this CL:
1. Add a RemovalTask struct that will represent an enqueued task.
2. Since BrowsingDataFilterBuilder and its subclasses are not copyable,
   change the interface so that it's passed in a unique_ptr (it must
   be owned by a RemovalTask so that it outlives the Remove() call).
3. Split the Remove() and RemoveWithFilter() interfaces into two versions,
   one with an observer and one without. Thus, RemoveAndReply() and
   RemoveWithFilterAndReply() are added. They must explicitly specify
   a *single* observer that will receive a callback from that particular
   task.
4. Despite the change #3, every observer must still explicitly register
   and unregister itself. This is needed so that if a caller is destroyed
   before its scheduled tasks are executed, it can unregister itself
   and we know that we should *not* call it back.
5. The recently added OnBrowsingDataRemoving() call which communicates
   a busy / idle state of BrowsingDataRemover still calls all registered
   observers.
6. Updated the tests with the new interface.

To consider in follow-up CLs:
- Use a base::Callback instead of an Observer pointer (see the discussion
  in the design doc), or use a task handles whose destruction will
  cancel the task.
- Consider changing BrowsingDataFilterBuilder to be copyable. This is
  still not needed in production, but could simplify tests.

TBR=mmenke@chromium.org,engedy@chromium.org,meacer@chromium.org,johnme@chromi...
BUG=630327

Committed: https://crrev.com/6f166831372df3044052eda2459982671c24ea5e
Cr-Commit-Position: refs/heads/master@{#410016}
==========

[commit-bot: I haz the power, 2016-08-05 08:46:50]

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