Split BrowsingDataRemoverTest into two tests for Impl and Delegate.

Split BrowsingDataRemoverTest into two tests for Impl and Delegate.

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first
   (uploaded as https://codereview.chromium.org/2647683002/).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


BUG=668114
Review-Url: https://codereview.chromium.org/2641853002
Cr-Commit-Position: refs/heads/master@{#445367}
Committed: https://chromium.googlesource.com/chromium/src/+/29b98cde67e8350d241fad23847ef72fbb939de5

[msramek, 2017-01-18 13:34:56]

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

[commit-bot: I haz the power, 2017-01-18 13:35:03]

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-18 13:54:06]

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

[commit-bot: I haz the power, 2017-01-18 13:54:07]

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

[msramek, 2017-01-18 17:56:22]

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

[commit-bot: I haz the power, 2017-01-18 17:56: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, 2017-01-18 18:55:46]

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

[commit-bot: I haz the power, 2017-01-18 18:55:47]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-19 09:44:35]

Description was changed from

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

WIP Not ready for review.

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

to

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

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first (coming soon!).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


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

[msramek, 2017-01-19 09:48:06]

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

[msramek, 2017-01-19 09:48:07]

Hi Bernhard!

Please have a look!

Like last time, I tried to minimize the diff between the original and at least
one of the split classes; the other class is, unfortunately, recognized as a new
file. But it's mostly just copy-paste.

Thanks!
Martin

[msramek, 2017-01-19 15:59:52]

Description was changed from

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

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first (coming soon!).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


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

to

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

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first
   (uploaded as https://codereview.chromium.org/2647683002/).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


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

[Bernhard Bauer, 2017-01-23 09:51:41]

LGTM

Sorry, I added a draft comment, which pushed this CL out of the incoming
section.

https://codereview.chromium.org/2641853002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2641853002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:1: //
Copyright 2012 The Chromium Authors. All rights reserved.
If you are updating the copyright anyway, change it to 2017?

[msramek, 2017-01-23 09:59:38]

Thanks, Bernhard!

https://codereview.chromium.org/2641853002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
(right):

https://codereview.chromium.org/2641853002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc:1: //
Copyright 2012 The Chromium Authors. All rights reserved.
On 2017/01/23 09:51:41, Bernhard Bauer wrote:
> If you are updating the copyright anyway, change it to 2017?

Done.

(I originally didn't update it, as I typically use these dates in code
archaeology to see how old the code really is if the file got moved around. But
I guess it also makes sense for both split portions of the test to use the same
date.)

[msramek, 2017-01-23 09:59:47]

The CQ bit was checked by msramek@chromium.org

[msramek, 2017-01-23 09:59:47]

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/2641853002/#ps40001
(title: "Updated year")

[commit-bot: I haz the power, 2017-01-23 10:00:01]

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 11:51:52]

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

[commit-bot: I haz the power, 2017-01-23 11:51:53]

Try jobs failed on following builders:
  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, 2017-01-23 11:53:18]

The CQ bit was checked by msramek@chromium.org

[commit-bot: I haz the power, 2017-01-23 11:53:39]

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 12:48:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485172398203820,
"parent_rev": "77cbaaa8e8168b2725c4853ebffcfb8ca802e35b", "commit_rev":
"29b98cde67e8350d241fad23847ef72fbb939de5"}

[commit-bot: I haz the power, 2017-01-23 12:48:33]

Description was changed from

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

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first
   (uploaded as https://codereview.chromium.org/2647683002/).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


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

to

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

BrowsingDataRemoverImplTest will eventually be moved to content/ together
with BrowsingDataRemoverImpl.

All tests and their helper classes have been moved to one or the other unittest
based on whether the tested datatype is present in content/ or only in chrome/.

A few changes had to be made:
- The *ImplTest class was converted to use BrowserContext instead of
  TestingProfile, as it will use either BrowserContext or TestingBrowserContext
  after it's moved to content/.
- URLRequestContext is now retrieved through StoragePartition where it
  originally was directly retrieved through TestingProfile. This is because
  BrowserContext does not directly expose a GetURLRequestContext method.
- The downloads test also tested the behavior of ChromeDownloadManagerDelegate.
  As downloads are a content/ datatype, the main part of the test was moved
  to *ImplTest, but a section testing ChromeDownloadManagerDelegate was moved
  to *DelegateTest.

There are a few dependencies that were not yet removed in this CL, but will be
removed in a followup:
1. *DelegateTest needs to see and be friend of BrowsingDataRemoverImpl to test
   the usage of BrowsingDataFilterBuilder. To fix this, we need to improve
   BrowsingDataFilterBuilder first
   (uploaded as https://codereview.chromium.org/2647683002/).
2. *ImplTest refers to extensions, which are not known to content/. This is
   caused by BrowsingDataHelper::EXTENSION mask which is respected in
   StoragePartition deletions. To fix this, BrowsingDataHelper must be improved
   first (also coming soon).
3. *ImplTest works with BrowsingContext, but needs to use a TestingProfile to
   create an instance of BrowsingDataRemoverImpl. This is because the factory
   does not accept a TestingBrowsingContext. This will be fixed naturally when
   we move BrowsingDataRemoverImpl to content/ and replace the factory with
   something like ::CreateFor(BrowsingContext*).


BUG=668114

Review-Url: https://codereview.chromium.org/2641853002
Cr-Commit-Position: refs/heads/master@{#445367}
Committed:
https://chromium.googlesource.com/chromium/src/+/29b98cde67e8350d241fad23847e...
==========

[commit-bot: I haz the power, 2017-01-23 12:48:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/29b98cde67e8350d241fad23847e...

[jkrcal, 2017-01-23 13:10:57]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2648223003/ by jkrcal@chromium.org.

The reason for reverting is: Breaks the buildbot in ChromiumChromiumos on Linux
ChromiumOS Full.

[findit-for-me, 2017-01-23 13:23:51]

FYI: Findit identified this CL at revision 445367 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[msramek, 2017-01-23 14:29:53]

Patchset #4 (id:60001) has been deleted

[msramek, 2017-01-23 17:51:28]

Relanded as https://codereview.chromium.org/2651673002/.