Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

This relands https://codereview.chromium.org/2641853002/ which was reverted
in https://codereview.chromium.org/2641853002/.

Patchset #1 is the exact copy of the original CL.
Patchset #2 is the fix.

The breakage occured in the test
ChromeBrowsingDataRemoverDelegateTest.RemoveMultipleTypesHistoryProhibited
and was not spotted locally, as this test is behind the ifdef:

"defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)"

The problem was that the helper class StoragePartitionRemovalData was
missing, as it had been moved to BrowsingDataRemoverImplTest during
the split. To avoid copying the class into this test file, we changed
the mask from COOKIES to PASSWORDS (which, unlike COOKIES, do not live
in StoragePartition). This does not affect the validy of the test, as its
purpose is to verify that if history deletion is prohibited and HISTORY
is present in the mask, it is not deleted, but other types still are.

BUG=668114
Review-Url: https://codereview.chromium.org/2651673002
Cr-Commit-Position: refs/heads/master@{#445402}
Committed: https://chromium.googlesource.com/chromium/src/+/44727f4d42756a3d89e1466ba662548e40c752c8

[msramek, 2017-01-23 14:38:39]

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

[commit-bot: I haz the power, 2017-01-23 14:38: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, 2017-01-23 14:40:12]

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

[commit-bot: I haz the power, 2017-01-23 14:40:13]

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...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[msramek, 2017-01-23 14:42:04]

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

[commit-bot: I haz the power, 2017-01-23 14:42:17]

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

[msramek, 2017-01-23 14:47:15]

Description was changed from

==========
Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

This relands https://codereview.chromium.org/2641853002/ which was reverted
in https://codereview.chromium.org/2641853002/.

Patchset #1 is the exact copy of the original CL.
Patchset #2 is the fix.

The breakage occured in the test
ChromeBrowsingDataRemoverDelegateTest.RemoveMultipleTypesHistoryProhibited
and was not spotted locally, as this test is behind the ifdef:

"defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)"

The problem was a missing helper class StoragePartitionRemovalData which
moved to BrowsingDataRemoverImpl test during the split. To avoid copying
the class into this test file, we changed the mask from COOKIES to PASSWORDS
(which, unlike COOKIES, are not in StoragePartition).

BUG=668114
==========

to

==========
Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

This relands https://codereview.chromium.org/2641853002/ which was reverted
in https://codereview.chromium.org/2641853002/.

Patchset #1 is the exact copy of the original CL.
Patchset #2 is the fix.

The breakage occured in the test
ChromeBrowsingDataRemoverDelegateTest.RemoveMultipleTypesHistoryProhibited
and was not spotted locally, as this test is behind the ifdef:

"defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)"

The problem was that the helper class StoragePartitionRemovalData was
missing, as it had been moved to BrowsingDataRemoverImplTest during
the split. To avoid copying the class into this test file, we changed
the mask from COOKIES to PASSWORDS (which, unlike COOKIES, do not live
in StoragePartition). This does not affect the validy of the test, as its
purpose is to verify that if history deletion is prohibited and HISTORY
is present in the mask, it is not deleted, but other types still are.

BUG=668114
==========

[msramek, 2017-01-23 15:26:39]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-01-23 15:32:12]

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

[commit-bot: I haz the power, 2017-01-23 15:32:12]

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

[msramek, 2017-01-23 15:46:21]

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

[commit-bot: I haz the power, 2017-01-23 15:46:36]

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-23 15:47:30]

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

[commit-bot: I haz the power, 2017-01-23 15:47:31]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED, no build URL)

[msramek, 2017-01-23 15:48:46]

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

[commit-bot: I haz the power, 2017-01-23 15:48:54]

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

[msramek, 2017-01-23 15:49:43]

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

[msramek, 2017-01-23 15:49:43]

I'm relanding https://codereview.chromium.org/2641853002/ with the broken test
fixed.

Bernhard, can you PTAL? The CL description explains what happened.

[msramek, 2017-01-23 15:54:32]

I also added ChromeOS trybots. Note that two of them are red already, but those
are infra issues.

[Bernhard Bauer, 2017-01-23 16:10:47]

LGTM, but the underlying issue seems somewhat ugly to me... I know that's not
your code, but would you mind adding a TODO or something? Thanks!

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

https://codereview.chromium.org/2651673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1003:
// here.
Yeah, um... If this is logic we want to test, it might be better to make that
part of the interface (and return a success value or something) rather than
enforcing it internally via DCHECK.

[msramek, 2017-01-23 16:39:51]

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

[commit-bot: I haz the power, 2017-01-23 16:40:05]

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

[msramek, 2017-01-23 16:43:31]

Thanks again, Bernhard!

I added a TODO. A success boolean value sounds reasonable here; in fact, I was
considering adding that before, but most data storage backends do not
communicate their success in the first place...

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

https://codereview.chromium.org/2651673002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc:1003:
// here.
On 2017/01/23 16:10:47, Bernhard Bauer wrote:
> Yeah, um... If this is logic we want to test, it might be better to make that
> part of the interface (and return a success value or something) rather than
> enforcing it internally via DCHECK.

Acknowledged. Added a TODO.

[msramek, 2017-01-23 16:43:42]

The CQ bit was unchecked by msramek@chromium.org

[msramek, 2017-01-23 16:43:51]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-23 16:43:52]

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/2651673002/#ps60001
(title: "Added a TODO.")

[commit-bot: I haz the power, 2017-01-23 16:44:06]

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-23 17:49:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485189831453240,
"parent_rev": "59c02c3aa8f983c2adecb654bd71e2ada5fb4d8e", "commit_rev":
"44727f4d42756a3d89e1466ba662548e40c752c8"}

[commit-bot: I haz the power, 2017-01-23 17:50:12]

Description was changed from

==========
Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

This relands https://codereview.chromium.org/2641853002/ which was reverted
in https://codereview.chromium.org/2641853002/.

Patchset #1 is the exact copy of the original CL.
Patchset #2 is the fix.

The breakage occured in the test
ChromeBrowsingDataRemoverDelegateTest.RemoveMultipleTypesHistoryProhibited
and was not spotted locally, as this test is behind the ifdef:

"defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)"

The problem was that the helper class StoragePartitionRemovalData was
missing, as it had been moved to BrowsingDataRemoverImplTest during
the split. To avoid copying the class into this test file, we changed
the mask from COOKIES to PASSWORDS (which, unlike COOKIES, do not live
in StoragePartition). This does not affect the validy of the test, as its
purpose is to verify that if history deletion is prohibited and HISTORY
is present in the mask, it is not deleted, but other types still are.

BUG=668114
==========

to

==========
Reland "Split BrowsingDataRemoverTest into two tests for Impl and Delegate."

This relands https://codereview.chromium.org/2641853002/ which was reverted
in https://codereview.chromium.org/2641853002/.

Patchset #1 is the exact copy of the original CL.
Patchset #2 is the fix.

The breakage occured in the test
ChromeBrowsingDataRemoverDelegateTest.RemoveMultipleTypesHistoryProhibited
and was not spotted locally, as this test is behind the ifdef:

"defined(NDEBUG) && !defined(DCHECK_ALWAYS_ON)"

The problem was that the helper class StoragePartitionRemovalData was
missing, as it had been moved to BrowsingDataRemoverImplTest during
the split. To avoid copying the class into this test file, we changed
the mask from COOKIES to PASSWORDS (which, unlike COOKIES, do not live
in StoragePartition). This does not affect the validy of the test, as its
purpose is to verify that if history deletion is prohibited and HISTORY
is present in the mask, it is not deleted, but other types still are.

BUG=668114

Review-Url: https://codereview.chromium.org/2651673002
Cr-Commit-Position: refs/heads/master@{#445402}
Committed:
https://chromium.googlesource.com/chromium/src/+/44727f4d42756a3d89e1466ba662...
==========

[commit-bot: I haz the power, 2017-01-23 17:50:14]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/44727f4d42756a3d89e1466ba662...