BrowsingDataRemover, (re)use StoragePartition deletion code.

BrowsingDataRemover, (re)use StoragePartition deletion code.

This CL removes code duplication around storage partition related data removal code from BrowsingDataRemover and re-uses StoragePartition::ClearData* method. 

BUG=180118
Test=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233365

[lazyboy, 2013-10-23 22:10:14]

+Albert for review.
I'll add more reviewers later once we're happy with the CL.

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
File chrome/browser/browsing_data/browsing_data_remover.cc (left):

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:616: void
BrowsingDataRemover::OverrideQuotaManagerForTesting(
The quota related unit tests need update, I'll fix them.
(BrowsingDataRemoverTest.RemoveQuotaManged* tests)

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:1046: // TODO(mkwst):
Clean this up, it's slow. http://crbug.com/130746
I'm not entirely sure about this TODO. The bug # seems to be pointing to
something else.

My understanding is that it should either be
already handled by
StoragePartitionImpl::QuotaManagedDataDeletionHelper::ClearDataOnIOThread() or
the code there is incorrect.

Noting so I don't forget.

[awong, 2013-10-23 22:16:47]

Forwarding to miket@.

Mike, let me know if you want me to take a first pass at this, but I'd like to
ensure that whomever is taking ownership of StoragePartition starts doing these
reviews.

[miket_OOO, 2013-10-24 21:33:27]

On 2013/10/23 22:16:47, awong wrote:
> Forwarding to miket@.
> 
> Mike, let me know if you want me to take a first pass at this, but I'd like to
> ensure that whomever is taking ownership of StoragePartition starts doing
these
> reviews.

100% mine. After I send this comment I'll remove you from the reviewers line.
Thanks!

[lazyboy, 2013-10-25 23:50:21]

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
File chrome/browser/browsing_data/browsing_data_remover.cc (left):

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:616: void
BrowsingDataRemover::OverrideQuotaManagerForTesting(
On 2013/10/23 22:10:14, lazyboy wrote:
> The quota related unit tests need update, I'll fix them.
> (BrowsingDataRemoverTest.RemoveQuotaManged* tests)

The unit tests are now fixed.

https://chromiumcodereview.appspot.com/37843003/diff/340001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:1046: // TODO(mkwst):
Clean this up, it's slow. http://crbug.com/130746
On 2013/10/23 22:10:14, lazyboy wrote:
> I'm not entirely sure about this TODO. The bug # seems to be pointing to
> something else.
> 
> My understanding is that it should either be
> already handled by
> StoragePartitionImpl::QuotaManagedDataDeletionHelper::ClearDataOnIOThread() or
> the code there is incorrect.
> 
> Noting so I don't forget.

I think I've figured out what the TODO is talking about, put the TODO back in
the moved function
StoragePartitionImpl::QuotaManagedDataDeletionHelper::ClearOriginsOnIOThread().

[miket_OOO, 2013-10-28 20:08:36]

LGTM

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
~content::StoragePartition::QUOTA_MANAGED_STORAGE_MASK_PERSISTENT;
I see what you're doing here, but do you really need to mask _ALL explicitly?
It's -1, and I'd be really surprised if anyone subdivided the bitspace in the
future such that _ALL wasn't -1.

It all compiles to the same thing, so it's just a readability thing. OK to
ignore.

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:564: if (delete_begin_ ==
base::Time() ||
I wish there were a Time::Min(), which would make this more readable.

https://chromiumcodereview.appspot.com/37843003/diff/420001/content/browser/s...
File content/browser/storage_partition_impl.cc (right):

https://chromiumcodereview.appspot.com/37843003/diff/420001/content/browser/s...
content/browser/storage_partition_impl.cc:592: (*deletion_task_count)++;
Why zero then inc? why not just start out with 1? Is it to visually balance the
(*)-- down below?

[lazyboy, 2013-10-28 22:14:50]

+mkwst@ for revewing chrome/browser/browsing_data/* changes.

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:562:
~content::StoragePartition::QUOTA_MANAGED_STORAGE_MASK_PERSISTENT;
On 2013/10/28 20:08:37, miket wrote:
> I see what you're doing here, but do you really need to mask _ALL explicitly?
> It's -1, and I'd be really surprised if anyone subdivided the bitspace in the
> future such that _ALL wasn't -1.
It used to be that one of the Masks in
content/public/browser/storage_partition.h didn't have _ALL set to -1 (it was
doing 0b00011111 pattern), so I did this explicitly to be safe. But since _ALL
is now -1 anyway, it doesn't matter. Removed masking _ALL.
> 
> It all compiles to the same thing, so it's just a readability thing. OK to
> ignore.

https://chromiumcodereview.appspot.com/37843003/diff/420001/chrome/browser/br...
chrome/browser/browsing_data/browsing_data_remover.cc:564: if (delete_begin_ ==
base::Time() ||
On 2013/10/28 20:08:37, miket wrote:
> I wish there were a Time::Min(), which would make this more readable.

Right.

https://chromiumcodereview.appspot.com/37843003/diff/420001/content/browser/s...
File content/browser/storage_partition_impl.cc (right):

https://chromiumcodereview.appspot.com/37843003/diff/420001/content/browser/s...
content/browser/storage_partition_impl.cc:592: (*deletion_task_count)++;
On 2013/10/28 20:08:37, miket wrote:
> Why zero then inc? why not just start out with 1? Is it to visually balance
the
> (*)-- down below?

Yes (to visually balance).

[Mike West, 2013-10-29 11:14:31]

The BrowsingDataRemover code changes lgtm (much cleaner!), but I think removing
all the BDR tests is a mistake. Is there a good reason to kill these tests that
I'm missing? See below:

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:409:
base::Unretained(this), base::Unretained(sb_context)));
Are these cookies not tied to a StoragePartition?

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (left):

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:972:
TEST_F(BrowsingDataRemoverTest, RemoveQuotaManagedDataForeverBoth) {
Why remove all these tests? I think you can certainly trim them down quite a bit
now that the functionality is pushed off to the StoragePartition, but there's
currently nothing that's testing that BrowsingDataRemover actually removes these
types of data.

Especially given the somewhat weird and unintuitive behavior around persistent
and temporary data, I don't think we can just remove these tests. I'd expect at
least some sort of check that the StoragePartition was actually instructed to
remove data, and called with the expected parameters.

[lazyboy, 2013-10-30 00:33:23]

@mkwst: I've added back the BDR tests /w modifications you have suggested.

+John for content/* review:
content/browser/storage_partition_impl.h
content/browser/storage_partition_impl.cc
content/browser/storage_partition_impl_unittest.cc
content/public/browser/storage_partition.h

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover.cc:409:
base::Unretained(this), base::Unretained(sb_context)));
On 2013/10/29 11:14:32, Mike West wrote:
> Are these cookies not tied to a StoragePartition?

These are related to SafeBrowsingService, which is chrome/ concept, so
StoragePartition (which is content/ concept) cannot directly do this.
I'm not sure this should be somehow moved to content/ or not.

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (left):

https://codereview.chromium.org/37843003/diff/490001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:972:
TEST_F(BrowsingDataRemoverTest, RemoveQuotaManagedDataForeverBoth) {
On 2013/10/29 11:14:32, Mike West wrote:
> Why remove all these tests? I think you can certainly trim them down quite a
bit
> now that the functionality is pushed off to the StoragePartition, but there's
> currently nothing that's testing that BrowsingDataRemover actually removes
these
> types of data.
> 
> Especially given the somewhat weird and unintuitive behavior around persistent
> and temporary data, I don't think we can just remove these tests. I'd expect
at
> least some sort of check that the StoragePartition was actually instructed to
> remove data, and called with the expected parameters.

Good point, I was hesitant translating and moving all of these tests to
StoragePartitionImplTest as well, since we were losing testing the functionality
you mention.
I've added all of these tests back, making sure StoragePartition is correctly
instructed to remove these data.

[jam, 2013-10-30 18:15:24]

https://codereview.chromium.org/37843003/diff/560001/content/public/browser/s...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/37843003/diff/560001/content/public/browser/s...
content/public/browser/storage_partition.h:123: virtual void
ClearDataForOriginWithinRange(
can we combine this with the above, and have a GURL pointer that is NULL if for
all origins?

[lazyboy, 2013-10-30 19:55:34]

Diff is patchset #9 vs patchset #7.

https://codereview.chromium.org/37843003/diff/560001/content/public/browser/s...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/37843003/diff/560001/content/public/browser/s...
content/public/browser/storage_partition.h:123: virtual void
ClearDataForOriginWithinRange(
On 2013/10/30 18:15:24, jam wrote:
> can we combine this with the above, and have a GURL pointer that is NULL if
for
> all origins?

Sure, I've merged 3 of the 4 ClearData* functions into one ClearData() function.
The remaining one requires a URLContextGetter*, so left it out (some todo says
it will go away).

[Mike West, 2013-10-30 20:26:18]

BrowsingDataRemover code and test changes LGTM. Thanks!

[jam, 2013-10-31 00:05:48]

lgtm

(I assume miket reviewed the content/browser changes)

[jam, 2013-10-31 00:06:05]

https://codereview.chromium.org/37843003/diff/660001/content/public/browser/s...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/37843003/diff/660001/content/public/browser/s...
content/public/browser/storage_partition.h:55: OriginMatcherFunction;
nit: move this to line 108, convention for content api is that every callback
typedef is right above the method which uses it

[lazyboy, 2013-10-31 20:55:29]

+boliu for android_webview/* Review.

@mkwst:
I've modified more tests in BrowsingDataRemoverTest similar to last round (for
some reason I thought they didn't need modification). Those are also copied to
StoragePartitionImplTest like before.
Can you skim through them?

[diff is patchset #11 vs patchset #10]

https://codereview.chromium.org/37843003/diff/660001/content/public/browser/s...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/37843003/diff/660001/content/public/browser/s...
content/public/browser/storage_partition.h:55: OriginMatcherFunction;
On 2013/10/31 00:06:05, jam wrote:
> nit: move this to line 108, convention for content api is that every callback
> typedef is right above the method which uses it

Done.

[lazyboy, 2013-11-01 03:55:38]

Actually adding boliu@ this time.

[boliu, 2013-11-01 04:10:17]

android_webview lgtm

[commit-bot: I haz the power, 2013-11-06 00:41:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/37843003/1060001

[commit-bot: I haz the power, 2013-11-06 04:44:12]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 16:54:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/37843003/1060001

[commit-bot: I haz the power, 2013-11-06 17:16:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-11-06 17:27:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/37843003/1060001

[commit-bot: I haz the power, 2013-11-06 21:10:49]

Change committed as 233365

[Nico, 2013-11-07 20:01:29]

https://codereview.chromium.org/37843003/diff/1060001/content/browser/storage...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/37843003/diff/1060001/content/browser/storage...
content/browser/storage_partition_impl_unittest.cc:257: //  if
(IsWebSafeSchemeForTest(origin.scheme())) {
Did you intentionally land this code commented out?

[lazyboy, 2013-11-07 20:17:17]

https://codereview.chromium.org/37843003/diff/1060001/content/browser/storage...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/37843003/diff/1060001/content/browser/storage...
content/browser/storage_partition_impl_unittest.cc:257: //  if
(IsWebSafeSchemeForTest(origin.scheme())) {
On 2013/11/07 20:01:30, Nico (ooo until Nov 12) wrote:
> Did you intentionally land this code commented out?

Mistake from me. Uncommenting them in other CL.