Clear network state only when REMOVE_CACHE is set, not unconditionally

Clear network state only when REMOVE_CACHE is set, not unconditionally

Previously, network state like HSTS data was cleared whenever
BrowsingDataRemover::Remove() was called. Some archaeology
(https://codereview.chromium.org/7717023/) reveals that the original
intention was to clear this state when REMOVE_CACHE was set, but due to
a curly brace mishap, we've been clearing it over-aggressively for
years.

When I changed this behavior to remove network state only when
REMOVE_CACHE is set, it revealed that a number of tests are relying on
state being asynchronously cleared. This is no longer the case, as for
example when only REMOVE_DOWNLOADS is set. This CL fixes that by calling
NotifyIfDone() at the end of RemoveImpl(), to catch cases where no state
is being wiped asynchronously. This is a little weird since download
removal does appear to be async -- but it matches the documentation of
BrowsingDataRemover::Observer, which says that the observer fires when
"keywords have been deleted, cache cleared and all other tasks
scheduled".

BUG=603682
Committed: https://crrev.com/c686e3083ac8b46b2b8ce6c036554bf05a7ed9ec
Cr-Commit-Position: refs/heads/master@{#391248}

[estark, 2016-05-02 20:13:17]

estark@chromium.org changed reviewers:
+ mkwst@chromium.org

[estark, 2016-05-02 20:13:18]

mkwst, PTAL?

Also ccing asanka@ who might be able to double-check the new NotifyIfDone() call
at the end of RemoveImpl(). A bunch of downloads-removal-related tests were
timing out without this call, because there's no callback for when download
removal is complete.
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
is an example of a test that was timing out.)

[Mike West, 2016-05-03 05:17:34]

LGTM, thank you. The `NotifyIfDone()` call makes sense to me; we were relying on
callbacks to ensure that we trigger the notifications, but I think you're right
that this would fail given certain combinations of removal options. Adding that
call in here seems like the right way to address the problem.

[estark, 2016-05-03 15:40:02]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-05-03 15:40:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1941073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1941073002/20001

[commit-bot: I haz the power, 2016-05-03 16:16:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-03 16:17:59]

Description was changed from

==========
Clear network state only when REMOVE_CACHE is set, not unconditionally

Previously, network state like HSTS data was cleared whenever
BrowsingDataRemover::Remove() was called. Some archaeology
(https://codereview.chromium.org/7717023/) reveals that the original
intention was to clear this state when REMOVE_CACHE was set, but due to
a curly brace mishap, we've been clearing it over-aggressively for
years.

When I changed this behavior to remove network state only when
REMOVE_CACHE is set, it revealed that a number of tests are relying on
state being asynchronously cleared. This is no longer the case, as for
example when only REMOVE_DOWNLOADS is set. This CL fixes that by calling
NotifyIfDone() at the end of RemoveImpl(), to catch cases where no state
is being wiped asynchronously. This is a little weird since download
removal does appear to be async -- but it matches the documentation of
BrowsingDataRemover::Observer, which says that the observer fires when
"keywords have been deleted, cache cleared and all other tasks
scheduled".

BUG=603682
==========

to

==========
Clear network state only when REMOVE_CACHE is set, not unconditionally

Previously, network state like HSTS data was cleared whenever
BrowsingDataRemover::Remove() was called. Some archaeology
(https://codereview.chromium.org/7717023/) reveals that the original
intention was to clear this state when REMOVE_CACHE was set, but due to
a curly brace mishap, we've been clearing it over-aggressively for
years.

When I changed this behavior to remove network state only when
REMOVE_CACHE is set, it revealed that a number of tests are relying on
state being asynchronously cleared. This is no longer the case, as for
example when only REMOVE_DOWNLOADS is set. This CL fixes that by calling
NotifyIfDone() at the end of RemoveImpl(), to catch cases where no state
is being wiped asynchronously. This is a little weird since download
removal does appear to be async -- but it matches the documentation of
BrowsingDataRemover::Observer, which says that the observer fires when
"keywords have been deleted, cache cleared and all other tasks
scheduled".

BUG=603682

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

[commit-bot: I haz the power, 2016-05-03 16:18:00]

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