Make ClearBrowsingDataHandler only observe its own removal task

Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d
Cr-Commit-Position: refs/heads/master@{#415925}

[msramek, 2016-08-12 16:52:10]

Description was changed from

==========
Make ClearBrowsingDataHandler only observe its own task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" while we execute the removal task started
by pressing that button - this serves as a feedback to the user that
their removal task has indeed been executed, i.e. that pressing the button
had an effect. However, the user can close ("X") and reopen the dialog,
or open the dialog in another tab, and the button will be enabled again.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

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

to

==========
Make ClearBrowsingDataHandler only observe its own task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" while we execute the removal task started
by pressing that button - this serves as a feedback to the user that
their removal task has indeed been executed, i.e. that pressing the button
had an effect. However, the user can close ("X") and reopen the dialog,
or open the dialog in another tab, and the button will be enabled again.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[msramek, 2016-08-12 17:44:17]

Patchset #1 (id:1) has been deleted

[msramek, 2016-08-16 12:49:49]

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

[msramek, 2016-08-16 12:49:56]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-16 12:50:01]

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-16 12:56:18]

Description was changed from

==========
Make ClearBrowsingDataHandler only observe its own task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" while we execute the removal task started
by pressing that button - this serves as a feedback to the user that
their removal task has indeed been executed, i.e. that pressing the button
had an effect. However, the user can close ("X") and reopen the dialog,
or open the dialog in another tab, and the button will be enabled again.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[msramek, 2016-08-16 13:00:47]

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

[commit-bot: I haz the power, 2016-08-16 13:00: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-08-16 13:00:58]

Patchset #1 (id:40001) has been deleted

[msramek, 2016-08-16 13:09:11]

msramek@chromium.org changed reviewers:
+ dbeam@chromium.org, dpapad@chromium.org

[msramek, 2016-08-16 13:09:12]

Hi guys,

Please have a look! This is the change I mentioned in the email last week.

I ran into trouble with the edge case where the CBD dialog can be closed using
the "X" button, and handled it conservatively - if the user closes and reopens
the dialog while the removal task is still running, we keep the "Clear data"
button disabled. But I'm happy with any other approach if you have other ideas.

Thanks,
Martin

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

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

[commit-bot: I haz the power, 2016-08-16 13:48:06]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-08-16 22:09:09]

https://codereview.chromium.org/2240883002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2240883002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:183:
DCHECK(!webui_callback_id_.empty());
this could currently fail in the [very uncommon case of]:

1) start long clear
2) refresh
3) finish short clear

can we just clear this and only respond if it's not empty when the clearing
succeeds?

is it possible that this flow would have issues if the same observer was
re-added before the long clear succeeds?

[msramek, 2016-08-18 13:15:09]

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

[commit-bot: I haz the power, 2016-08-18 13:15:29]

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-08-18 14:36:56]

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

[commit-bot: I haz the power, 2016-08-18 14:36:56]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-08-25 21:05:41]

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

[commit-bot: I haz the power, 2016-08-25 21:06:24]

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-08-25 21:25:43]

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

[commit-bot: I haz the power, 2016-08-25 21:25:44]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[msramek, 2016-08-26 11:56:17]

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

[commit-bot: I haz the power, 2016-08-26 11:56: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-08-26 11:56:44]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-08-26 12:19:50]

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

[commit-bot: I haz the power, 2016-08-26 12:19:51]

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

[msramek, 2016-08-26 12:37:43]

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

[commit-bot: I haz the power, 2016-08-26 12:37:55]

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-26 13:15:56]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-26 15:04:05]

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

[commit-bot: I haz the power, 2016-08-26 15:04:06]

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-08-26 15:07:22]

Patchset #2 (id:120001) has been deleted

[msramek, 2016-08-26 15:07:27]

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

[commit-bot: I haz the power, 2016-08-26 15:07:45]

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-26 15:09:44]

Hi Dan!

Sorry for the delay on this. I was pondering several options how to do it.

The idea with resetting |webui_callback_id_| when the dialog is closed and
reopened doesn't exactly work - if you execute a task and get a callback, it is
not possible to tell whether it's coming from the task you just executed or from
some other one that was executed earlier (before the dialog was closed and
reopened). Because in both cases |webui_callback_id_| is nonempty.

So now I understood how you came to the idea about task IDs - that would be
helpful here. Well, I basically did that, just in a more OOP fashion - instead
of IDs, we distinguish them by memory addresses, because each task has its own
observer. I think this also creates a nice encapsulation where you can call
task_observer_.reset() to indicate that you don't care about that task anymore.
We do that OnJavascriptDisallowed (before reload), OnInitialize (when the dialog
is reopened) and OnClearingTaskFinished (when a task finishes; though this is
not strictly necessary).

Please have another look!

Cheers,
Martin

[commit-bot: I haz the power, 2016-08-26 17:11:33]

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

[commit-bot: I haz the power, 2016-08-26 17:11:34]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-08-30 01:32:08]

On 2016/08/26 15:09:44, msramek wrote:
> Hi Dan!
> 
> Sorry for the delay on this. I was pondering several options how to do it.
> 
> The idea with resetting |webui_callback_id_| when the dialog is closed and
> reopened doesn't exactly work - if you execute a task and get a callback, it
is
> not possible to tell whether it's coming from the task you just executed or
from
> some other one that was executed earlier (before the dialog was closed and
> reopened). Because in both cases |webui_callback_id_| is nonempty.

OnJavascriptDisallowed() {
  webui_callback_id_.clear();
}

how is this not sufficient?

> 
> So now I understood how you came to the idea about task IDs - that would be
> helpful here. Well, I basically did that, just in a more OOP fashion - instead
> of IDs, we distinguish them by memory addresses, because each task has its own
> observer. I think this also creates a nice encapsulation where you can call
> task_observer_.reset() to indicate that you don't care about that task
anymore.
> We do that OnJavascriptDisallowed (before reload), OnInitialize (when the
dialog
> is reopened) and OnClearingTaskFinished (when a task finishes; though this is
> not strictly necessary).
> 
> Please have another look!
> 
> Cheers,
> Martin

[msramek, 2016-08-30 08:34:32]

On 2016/08/30 01:32:08, Dan Beam wrote:
> On 2016/08/26 15:09:44, msramek wrote:
> > Hi Dan!
> > 
> > Sorry for the delay on this. I was pondering several options how to do it.
> > 
> > The idea with resetting |webui_callback_id_| when the dialog is closed and
> > reopened doesn't exactly work - if you execute a task and get a callback, it
> is
> > not possible to tell whether it's coming from the task you just executed or
> from
> > some other one that was executed earlier (before the dialog was closed and
> > reopened). Because in both cases |webui_callback_id_| is nonempty.
> 
> OnJavascriptDisallowed() {
>   webui_callback_id_.clear();
> }
> 
> how is this not sufficient?

1. Execute task #1. (webui_callback_id_ is nonempty)
2. Reload the page. (reset webui_callback_id_ in OnJavaScriptDisallowed -> it's
now empty)
4. Execute task #2. (webui_callback_id_ is nonempty)
5. Task #1 finishes.
Expected:
6. Dialog is not closed, because we're waiting for task #2 (Task #1 should now
be completely forgotten).
Actual:
6. Completion callback from task #1 sees that webui_callback_id_ is nonempty ->
closes the dialog.

In other words, !webui_callback_id.empty() only means that there is at least one
task in progress. It is not possible to solve this without knowing *how many*
tasks there are, or *which is which*. I implemented the latter.

Furthermore, OnJavaScriptDisallowed() is not sufficient, since it's only called
on reload. We also need to handle closing and reopening of the dialog, i.e.
HandleInitialize().

> 
> > 
> > So now I understood how you came to the idea about task IDs - that would be
> > helpful here. Well, I basically did that, just in a more OOP fashion -
instead
> > of IDs, we distinguish them by memory addresses, because each task has its
own
> > observer. I think this also creates a nice encapsulation where you can call
> > task_observer_.reset() to indicate that you don't care about that task
> anymore.
> > We do that OnJavascriptDisallowed (before reload), OnInitialize (when the
> dialog
> > is reopened) and OnClearingTaskFinished (when a task finishes; though this
is
> > not strictly necessary).
> > 
> > Please have another look!
> > 
> > Cheers,
> > Martin

[Dan Beam, 2016-09-01 02:49:28]

lgtm, sorry for perf review lag

your explanation makes sense.

https://codereview.chromium.org/2240883002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
(right):

https://codereview.chromium.org/2240883002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:46:
class TaskObserver : public BrowsingDataRemover::Observer {
can we just forward-declare this and implement solely in the .cc file?

[msramek, 2016-09-01 08:51:29]

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

[commit-bot: I haz the power, 2016-09-01 08:51: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-09-01 09:46:18]

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

[commit-bot: I haz the power, 2016-09-01 09:46:19]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-09-01 10:34:37]

Thanks! Happy perfing :)

https://codereview.chromium.org/2240883002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h
(right):

https://codereview.chromium.org/2240883002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.h:46:
class TaskObserver : public BrowsingDataRemover::Observer {
On 2016/09/01 02:49:28, Dan Beam wrote:
> can we just forward-declare this and implement solely in the .cc file?

Yep! Done.

[msramek, 2016-09-01 10:35:08]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-09-01 10:35:08]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2240883002/#ps160001
(title: "Only forward-declare in the header file")

[commit-bot: I haz the power, 2016-09-01 10:35:18]

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

[commit-bot: I haz the power, 2016-09-01 10:38:45]

Description was changed from

==========
Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-09-01 10:38:46]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2016-09-01 10:40:20]

Description was changed from

==========
Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Make ClearBrowsingDataHandler only observe its own removal task

In https://codereview.chromium.org/2133893002/, ClearBrowsingDataHandler
was made to observe whether BrowsingDataRemover has any removal task
in progress and disable the "Clear data" button if that is the case.

This is no longer necessary. BrowsingDataRemover is nowadays able to
accept a new removal task at any time and call back (only) the associated
observer when the task is done. Callers no longer need to be aware of
the possibility of other removal tasks running.

We still disable the "Clear data" button while we execute the removal
task started by pressing that button - this serves as a feedback to the
user that their removal task has indeed been executed, i.e. that pressing
the button had an effect. However, opening another instance of a dialog
in a different tab (i.e. tied to a different handler) allows the user
to execute another parallel removal task if they wish to do so.

Finally, we remove the OnBrowsingDataRemoverRemoving(bool) method from
BrowsingDataRemover::Observer, as the previous behavior of
ClearBrowsingDataHandler was its only usecase.

In practice, this CL is mostly a partial revert of
https://codereview.chromium.org/2133893002/

BUG=630327
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a946e92eaa55cb6da801ada49dfdd51d786fa06d
Cr-Commit-Position: refs/heads/master@{#415925}
==========

[commit-bot: I haz the power, 2016-09-01 10:40:22]

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

[Dan Beam, 2016-09-01 15:49:32]

https://codereview.chromium.org/2240883002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2240883002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:75: }
fwiw: i usually find inline function bodies better to read if the interface and
implementation are in the same file, i.e.

TaskObserver(BrowsingDataRemover* remover, const base::Closure& callback)
    : callback_(callback), remover_observer_(this) {}
~TaskObserver() override {}

void OnBrowsingDataRemoverDone() {
  remover_observer_.RemoveAll();
  callback_.Run();
}