Time-based removal of temporary file systems via BrowsingDataRemover

Time-based removal of temporary file systems via BrowsingDataRemover

QuotaManager takes over much of the functionality that BrowsingDataRemover implemented for FileSystem and Appcache removal. It also handles WebSQL databases, but I've left the database deletion in, as IndexedDBs aren't yet handled correctly, so we need to take care of them explicitly.

BUG=63700
TEST=unit_tests, test_shell_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94913

[Mike West, 2011-06-08 13:39:26]

Hi folks.  I'm stuck, and would appreciate a bit of help.

Following up on our call from yesterday, I'm trying to implement the bits of
workflow that we agree on.  Specifically, I'd like to walk through temporary
filesystems to determine if they've been touched inside the time period, and if
so, remove them.  This seems straightforward enough, but I'm having some issues
getting tests up and running.

Please take a look at the patch, and let me know if I'm just sincerely
misinterpreting what it is that I need to do to get up and running with a
FileSystemFileEnumerator.  At the moment, things basically die inside
`webkit/fileapi/obfuscated_file_system_file_util.cc` on line 574 where the file
path gets checked.  `display_queue_` is never filled, so the enumerator stays
empty.

I assume there's some setup code I need to run to make the filesystem look like
a filesystem, and I tracked down FileSystemOriginTestHelper, which I thought
would solve the problem for me.  It didn't, and now I'm giving up.  I either
need documentation, or one of you standing next to my desk for the next few
days! :)

Thanks!

-Mike

[ericu, 2011-06-08 18:40:23]

http://codereview.chromium.org/7129018/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:600: fileapi::kFileSystemTypeTemporary,
FilePath(), false)));
This isn't the right path to pass to CreateFileEnumerator.  You want to pass in
the virtual path of the directory under which you want to check, which in this
case is "/".  Since you've set the src_origin_url and src_type, the context
tells it everything else it needs to know.

However, this still doesn't quite do what you want.  Thanks to "touch", which
can change modification times, and thanks to the fact that one might delete from
a filesystem without setting a mod time on anything that's still around, you'll
miss some modifications.  [That deletion thing's a bug of mine--I haven't yet
made directories' mtimes get updated by deletions of their contents.]

Kinuko, can he leverage the quota code to find out what's really been touched in
this timeframe?

[Use mkwst_at_chromium.org plz., 2011-06-08 19:00:09]

Thanks!

http://codereview.chromium.org/7129018/diff/1/chrome/browser/browsing_data_re...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/1/chrome/browser/browsing_data_re...
chrome/browser/browsing_data_remover.cc:600: fileapi::kFileSystemTypeTemporary,
FilePath(), false)));
On 2011/06/08 18:40:24, ericu wrote:
> This isn't the right path to pass to CreateFileEnumerator.

Gah.  Yeah, that makes sense.  I'll try it out tomorrow when I'm back in front
of my computer. :)

> Kinuko, can he leverage the quota code to find out what's really been
> touched in this timeframe?

I looked at the quota code, but there didn't seem to be any public methods
hanging off QuotaDatabase to pull last modified; setters for last accessed, but
not exactly what I need.  That does seem to be a much more efficient way of
doing things though, if it's actually recording timestamps when things are
touched.

[michaeln, 2011-06-08 20:22:25]

> > Kinuko, can he leverage the quota code to find out what's really been
> > touched in this timeframe?
> 
> I looked at the quota code, but there didn't seem to be any public methods
> hanging off QuotaDatabase to pull last modified; setters for last accessed,
but
> not exactly what I need.  That does seem to be a much more efficient way of
> doing things though, if it's actually recording timestamps when things are
> touched.

Two additions to the QuotaManager public interface could make it possible for
the BDR to handle all quota managed storage types
(sqlDb,indexedDb,appcache,filesystem).

  void GetOriginsModifiedSince(StorageType /*pers vs temp*/, date_time,
callback);
  void DeleteOriginData(StorageType, origin, callback);

We'd have to store last modified dates/times in the quotaDB (similar to how
we're storing last access times), and have a 'task' to retrieve the list.

[Mike West, 2011-06-09 14:10:20]

Thanks to both of you for the feedback.

I think doing this through the quota system makes a lot of sense, as opposed to
brute forcing the solution by walking the filesystem.  I've updated the patch to
do the latter, but I'm happy to change things around to the quota system when
that work is complete.

[michaeln, 2011-06-10 00:47:54]

On 2011/06/09 14:10:20, Mike West wrote:
> Thanks to both of you for the feedback.
> 
> I think doing this through the quota system makes a lot of sense, as opposed
to
> brute forcing the solution by walking the filesystem.  I've updated the patch
to
> do the latter, but I'm happy to change things around to the quota system when
> that work is complete.

I don't see any rush on having tip-of-tree support time-based removal. Do we do
this now and then replace it with a quota based stuff or just get the quota
based solution working instead?

[kinuko, 2011-06-10 03:44:36]

On 2011/06/10 00:47:54, michaeln wrote:
> On 2011/06/09 14:10:20, Mike West wrote:
> > Thanks to both of you for the feedback.
> > 
> > I think doing this through the quota system makes a lot of sense, as opposed
> to
> > brute forcing the solution by walking the filesystem.  I've updated the
patch
> to
> > do the latter, but I'm happy to change things around to the quota system
when
> > that work is complete.
> 
> I don't see any rush on having tip-of-tree support time-based removal. Do we
do
> this now and then replace it with a quota based stuff or just get the quota
> based solution working instead?

Sorry for my lagging response (I've been a bit 'off' after branch cut).  I/we
can make the quota change and it won't take much time (hopefully).  Depending on
how soon we want this solution please go ahead making the changing without quota
support, or you could just hold on now until it becomes available.  (And thanks
for working on this)

[Mike West, 2011-06-10 07:02:00]

Nope, you're entirely correct: there's no rush.  I wanted to get this
implemented while it was top of mind, but I don't at all mind leaving this be
for a while if you guys can get the changes in for an entirely quota-based
system. The code's not going anywhere, and as long as we have a solid
implementation in M14 I'm happy. :)

[Mike West, 2011-06-15 07:14:17]

On 2011/06/10 07:02:00, Mike West wrote:
> Nope, you're entirely correct: there's no rush.

I lied, http://crbug.com/86087 has been marked as blocking the stable release of
M13. :)

I'd suggest that one of you take another look at this patch, and if you're happy
enough with the brute-force workflow to give it an lgtm, I'll commit it to trunk
and work out the details of merging it into the beta.

I'd also create another bug for migrating the BrowsingDataRemover to the quota
system, if one's not already created.

[Mike West, 2011-06-15 10:50:18]

On 2011/06/15 07:14:17, Mike West wrote:

> I lied, http://crbug.com/86087 has been marked as blocking the stable release
of
> M13. :)

Also, if you have any tips about what I might need to do differently on Windows,
I'd appreciate those too.  The tests are failing on that platform, and I assume
there's simply something I've missed. :)

[kinuko, 2011-06-15 11:25:14]

I'm not 100% sure if the 'blocker' really include time-based deletion, anyway I
moved the task to add unified deletion support in quota manager top of my queue.

http://codereview.chromium.org/7129018/diff/10001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7129018/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:161:
sandbox_->GetFileSystemFileUtil());
nit: indent

http://codereview.chromium.org/7129018/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:163: origin, type, FilePath(),
true);
helper.SetUp performs this internally.

http://codereview.chromium.org/7129018/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:181:
op_context->set_src_type(type);
nit: you could also call helper.NewOperationContext() if you hold the helper
instance (stack-allocated in line 159) somewhere in the test class.

http://codereview.chromium.org/7129018/diff/10001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:316:
fileapi::kFileSystemTypePersistent));
Is there any chance that any of the files are not properly closed?  I used to
suffer from windows-only errors when I tried to delete such files.

[Mike West, 2011-06-15 11:45:20]

On 2011/06/15 11:25:14, kinuko wrote:
> I'm not 100% sure if the 'blocker' really include time-based deletion, anyway
I
> moved the task to add unified deletion support in quota manager top of my
queue.

That's a good point; this might be satisfied for M13 with the "Since the
beginning of time" addition in r88119.  I'll follow up with those responsible
for deciding such things. :)

Thanks for the nits, and the tip regarding Windows.  I'll look into those
shortly.

[michaeln, 2011-07-20 20:28:21]

I think the QuotaManager APIs needed for this are available now. Mike, is it
still on your plate to revamp the BDR to utilize those interfaces?

[Mike West, 2011-07-20 20:34:50]

Yup.  Tell me which APIs I should hook into, and I'll take a look at it
first thing tomorrow morning.
On Jul 20, 2011 10:28 PM, <michaeln@chromium.org> wrote:
> I think the QuotaManager APIs needed for this are available now. Mike, is
it
> still on your plate to revamp the BDR to utilize those interfaces?
>
> http://codereview.chromium.org/7129018/

[michaeln, 2011-07-21 02:22:23]

The two new QuotaManager methods are declared in quota_manager.h.
 
  void GetOriginsModifiedSince(
      StorageType type,
      base::Time modified_since,
      GetOriginsCallback* callback);

  void DeleteOriginData(const GURL& origin,
                        StorageType type,
                        StatusCallback* callback);

Given a modified_since time of 0, the first method should return a set of all
origins using storage. These should be called only on the IO thread. The
callback objects are deleted by the QuotaManager after they've been invoked.

[Mike West, 2011-07-21 07:46:29]

On 2011/07/21 02:22:23, michaeln wrote:

> Given a modified_since time of 0, the first method should return a set of all
> origins using storage. These should be called only on the IO thread. The
> callback objects are deleted by the QuotaManager after they've been invoked.

Ok, most of this looks very straightforward, but there was apparently a lot of
refactoring work done in ProfileImpl to use the quota system which hasn't been
ported over to TestingProfile.

Looking at that today... depending on how much work it appears to be, I might do
that in a separate CL.

[Mike West, 2011-07-21 21:48:39]

This is based on the TestingProfile changes I'm still fiddling with in
http://codereview.chromium.org/7464029/

Assuming I get that working, however, this bit seems to be working correctly.
Can you guys take a look? Thanks!

[michaeln, 2011-07-21 23:49:15]

The unit tests could probably be much simpler. What's there now part unit test
and part system integration test. I think you could strip it down to just unit
test the BDR's interactions with the QM. The BDR doesn't really care what the QM
does to return a list of origins.

* Make QM::DeleteOriginData() and QM::GetOriginsModifiedSince() virtual so that
you can mock them up for the BDR tests

* Define a mock that overrides the two methods of interest in the BDR unit test
.cc file. Return whatever list or origins you like and whatever statuscodes
you'd like to test.

* Set and instance of that mock as the profiles QuotaManager.

...

http://codereview.chromium.org/7129018/diff/20001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/20001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:246: bool
waiting_for_clear_file_systems_;
I was thinking that use of the QuotaManager could completely replace the removal
logic for databases, appcaches, and filesystems (for both from-time and
for-all-time removal); so the flags and methods related to dealing with each of
those individually those could be removed. So you'd have some new methods
like...

void ClearQuotaManagedDataOnIOThread();
void OnGotQuotaManagedOrigins(const std::set<GURL>&);
void OnDeletedQuotaManagedOrigin(quota::QuotaStatusCode);

And a bunch of appcache/filesystem/database specific methods and members
removed.

[kinuko, 2011-07-22 02:06:34]

On 2011/07/21 23:49:15, michaeln wrote:
> The unit tests could probably be much simpler. What's there now part unit test
> and part system integration test. I think you could strip it down to just unit
> test the BDR's interactions with the QM. The BDR doesn't really care what the
QM
> does to return a list of origins.
> 
> * Make QM::DeleteOriginData() and QM::GetOriginsModifiedSince() virtual so
that
> you can mock them up for the BDR tests
> 
> * Define a mock that overrides the two methods of interest in the BDR unit
test
> .cc file. Return whatever list or origins you like and whatever statuscodes
> you'd like to test.
> 
> * Set and instance of that mock as the profiles QuotaManager.
> 
> ...
> 
>
http://codereview.chromium.org/7129018/diff/20001/chrome/browser/browsing_dat...
> File chrome/browser/browsing_data_remover.h (right):
> 
>
http://codereview.chromium.org/7129018/diff/20001/chrome/browser/browsing_dat...
> chrome/browser/browsing_data_remover.h:246: bool
> waiting_for_clear_file_systems_;
> I was thinking that use of the QuotaManager could completely replace the
removal
> logic for databases, appcaches, and filesystems (for both from-time and
> for-all-time removal); so the flags and methods related to dealing with each
of
> those individually those could be removed. So you'd have some new methods
> like...
> 
> void ClearQuotaManagedDataOnIOThread();
> void OnGotQuotaManagedOrigins(const std::set<GURL>&);
> void OnDeletedQuotaManagedOrigin(quota::QuotaStatusCode);
> 
> And a bunch of appcache/filesystem/database specific methods and members
> removed.

+1 to this design!  Hopefully we should be able to cleanup bunch of BDR code
now.  Thanks michael for taking care of this.

[Mike West, 2011-07-25 18:13:41]

I've eventually gone the route michaeln suggested by mocking out the bits of
QuotaManager that are relevant to BDR. I confess that the quota code ended up
being a bit more complex than I expected (especially with regard to tasks), so
I'm not at all confident with the solution I've implemented.

There's one failing test that I'm pretty sure boils down to something tasky
that's going wrong when running deletion tasks for persistent and temporary
tasks together. Everything else should be working, and I'd appreciate you taking
a look at the core of it now rather than waiting for me to hammer all the bits
into place.

Regardless, I'm rolling the whole change into this CL, as it's not going to make
the cutoff. I'd like to make a potential merge into a release branch as simple
as possible.

---

michaeln, kinuko: Could you both take a look at that mock code, as well as the
quota bits of BDR?

jochen_: Could you review the BDR code?

Thanks!

[jochen (gone - plz use gerrit), 2011-07-26 08:55:33]

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:10: #include "base/logging.h"
why separate from the other includes?

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:52: #include
"webkit/quota/quota_manager.h"
alphabetical ordering

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:577: // TODO(mkwst): Not really anything
pretty I can think of to do here.
DLOG the status at least?

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:580:
--temporary_quota_origins_to_delete_count_;
DCHECK for >= 0?

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:152: // temporary data
dot at end of comment

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:156: // core of
`ClearQuotaManagedDataOnIOThread`
please use regular quotes (")

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:22: #include
"webkit/quota/mock_quota_manager.h"
alphabetical ordering

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:109: explicit
RemoveQuotaManagedDataTester() {}
remove explicit

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:110:
~RemoveQuotaManagedDataTester() {}
virtual

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:20: // MockQuotaManager implement RefCounted.
but isn't the base class, quota manager already refcounted?

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:28: const std::set<GURL>& origins,
GetOriginsCallback* callback)
all arguments on their own line, and aligned

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:47: };
disallow copy/assign

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:10: #include <string>
empty line between sys and chrome includes

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:16: #include "webkit/quota/quota_manager.h"
alphabetical ordering

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:20: 
only one empty line

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:49: bool OriginHasData(const GURL& origin,
StorageType type);
const?

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:63: };
disallow copy & assign?

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:65: } // namespace quota
two spaces between } and //

[marja, 2011-07-26 11:53:06]

Commented some minor things I bumped into when reading through this cl.

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:12: #include "base/logging.h"
Is this intentionally left in?

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:109:
temporary_quota_origins_to_delete_count_(0) {
Why initialize temporary_quota_origins_to_delete_count_, but not
persistent_quota_origins_to_delete_count_? Both are set to 0 later though, so no
real harm done here, just confusing. (The same in the other BDR ctor.)

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:557: bool deleting = false;
This bool seems unnecessary.

[Mike West, 2011-07-26 12:08:32]

Thanks Jochen and Marja for your comments.

I think I've tracked down the race condition I generated, and I'm keeping my
fingers crossed that the trybots approve. :)

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:10: #include "base/logging.h"
On 2011/07/26 08:55:33, jochen wrote:
> why separate from the other includes?

I'd intended to remove it before committing, but given your suggestion below
regarding DLOGging the status code, I'll move it back into the block. :)

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:52: #include
"webkit/quota/quota_manager.h"
On 2011/07/26 08:55:33, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:577: // TODO(mkwst): Not really anything
pretty I can think of to do here.
On 2011/07/26 08:55:33, jochen wrote:
> DLOG the status at least?

Done. But it's pretty useless to be honest; the callback doesn't provide the
origin that's failed.

@kinuko, michaeln: Can we add this into the StatusCallback, or is it also used
in circumstances where no origin is available?

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:580:
--temporary_quota_origins_to_delete_count_;
On 2011/07/26 08:55:33, jochen wrote:
> DCHECK for >= 0?

Good idea. Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:152: // temporary data
On 2011/07/26 08:55:33, jochen wrote:
> dot at end of comment

Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:156: // core of
`ClearQuotaManagedDataOnIOThread`
On 2011/07/26 08:55:33, jochen wrote:
> please use regular quotes (")

Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover_unittest.cc (right):

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:22: #include
"webkit/quota/mock_quota_manager.h"
On 2011/07/26 08:55:33, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:109: explicit
RemoveQuotaManagedDataTester() {}
On 2011/07/26 08:55:33, jochen wrote:
> remove explicit

Done.

http://codereview.chromium.org/7129018/diff/36001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover_unittest.cc:110:
~RemoveQuotaManagedDataTester() {}
On 2011/07/26 08:55:33, jochen wrote:
> virtual

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.cc (right):

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:20: // MockQuotaManager implement RefCounted.
On 2011/07/26 08:55:33, jochen wrote:
> but isn't the base class, quota manager already refcounted?

You're right. I don't know why I thought I needed to add this.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:28: const std::set<GURL>& origins,
GetOriginsCallback* callback)
On 2011/07/26 08:55:33, jochen wrote:
> all arguments on their own line, and aligned

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.cc:47: };
On 2011/07/26 08:55:33, jochen wrote:
> disallow copy/assign

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
File webkit/quota/mock_quota_manager.h (right):

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:10: #include <string>
On 2011/07/26 08:55:33, jochen wrote:
> empty line between sys and chrome includes

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:16: #include "webkit/quota/quota_manager.h"
On 2011/07/26 08:55:33, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:20: 
On 2011/07/26 08:55:33, jochen wrote:
> only one empty line

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:49: bool OriginHasData(const GURL& origin,
StorageType type);
On 2011/07/26 08:55:33, jochen wrote:
> const?

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:63: };
On 2011/07/26 08:55:33, jochen wrote:
> disallow copy & assign?

Done.

http://codereview.chromium.org/7129018/diff/36001/webkit/quota/mock_quota_man...
webkit/quota/mock_quota_manager.h:65: } // namespace quota
On 2011/07/26 08:55:33, jochen wrote:
> two spaces between } and //

Done.

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:12: #include "base/logging.h"
On 2011/07/26 11:53:07, Marja Hölttä wrote:
> Is this intentionally left in?

Yes. I DLOG the status code if an origin can't be deleted for some reason.

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:109:
temporary_quota_origins_to_delete_count_(0) {
On 2011/07/26 11:53:07, Marja Hölttä wrote:
> Why initialize temporary_quota_origins_to_delete_count_, but not
> persistent_quota_origins_to_delete_count_?

Copy/paste error. Nice catch!

 Both are set to 0 later though, so no
> real harm done here, just confusing. (The same in the other BDR ctor.)

http://codereview.chromium.org/7129018/diff/35004/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:557: bool deleting = false;
On 2011/07/26 11:53:07, Marja Hölttä wrote:
> This bool seems unnecessary.

It in fact _is_ unnecessary.

[michaeln, 2011-07-26 22:40:19]

http://codereview.chromium.org/7129018/diff/28001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (left):

http://codereview.chromium.org/7129018/diff/28001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:235:
&BrowsingDataRemover::ClearAppCacheOnIOThread));
hoooray for deleted code!

http://codereview.chromium.org/7129018/diff/28001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/28001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:209:
profile_->GetWebKitContext()->DeleteDataModifiedSince(delete_begin_);
Unfortunately, removal of "WebDOMLocalStorage" is not taken care of via the
QuotaManager, so this is still needed.

http://codereview.chromium.org/7129018/diff/28001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:232:
&BrowsingDataRemover::ClearDatabasesOnFILEThread));
Removal of WebSQLDatabases is taken care of by going thru the QuotaManager, so i
think this isn't needed anymore.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:481: }
can be deleted?

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:496: }
ditto?

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:543: // isn't protected.
There is a codepath where the QuotaManager will invoke the callback prior to
return from DeleteOriginData. It's not really an expected case in chrome, but it
does exist. Not sure if it's worth it, but consider computing the full 'count'
prior to making the DeleteOriginData calls.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:569:
&BrowsingDataRemover::OnPersistentQuotaManagedOriginDeletion));
There's a fair amount of code duplication between

OnTemporaryQuotaManagedOriginDeletion()
OnPersistentQuotaManagedOriginDeletio()
and
CheckTemporaryQuotaManagedDataDeletionStatus()
CheckPersistentQuotaManagedDataDeletionStatus()

Maybe condense this to fewer methods.

void OnQuotaManagerOriginDeleted() {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  --quota_origins_to_delete_count_;  // TEMP and PERSISTENT
  if (!quota_origins_to_delete_count_)
    PostTask(&OnClearedQuotaManager);
}

void OnClearedQuotaManager() {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
  waiting_for_clear_quota_manager_ = false;
  NotifyAndDeleteIfDone();
}

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:179: void OnClearedDatabases(int rv);
can this be deleted too?

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:182: void ClearDatabasesOnFILEThread();
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:186: void OnClearedFileSystems();
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:189: void ClearFileSystemsOnFILEThread();
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:233:
scoped_refptr<webkit_database::DatabaseTracker> database_tracker_;
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:235:
net::CompletionCallbackImpl<BrowsingDataRemover> database_cleared_callback_;
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:251: bool waiting_for_clear_databases_;
ditto

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:254: bool
waiting_for_clear_persistent_quota_managed_data_;
see comments in the .cc file, maybe consolidate these two members into one...
waiting_for_clear_quota_manager_;

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:262: int
persistent_quota_origins_to_delete_count_;
ditto consolidation

http://codereview.chromium.org/7129018/diff/26039/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/7129018/diff/26039/chrome/test/testing_profile...
chrome/test/testing_profile.cc:549: quota_manager_ = new
quota::MockQuotaManager(
Returning a non-null object is going to have some affect on other tests. To make
this change have less ripple effect, maybe add a setter that only your test
calls, and leave all other tests with a null object. That's more flexible too
since other tests would be free to setup a different 'mock' without impacting
your test.

[Mike West, 2011-07-27 12:49:18]

Thanks for the review.  One or two open questions which I'll try to ping you
about sometime when we're both awake. Otherwise I think I've taken care of most
of your notes.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:481: void
BrowsingDataRemover::OnClearedDatabases(int rv) {
On 2011/07/26 22:40:19, michaeln wrote:
> can be deleted?

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:496: void
BrowsingDataRemover::ClearDatabasesOnFILEThread() {
On 2011/07/26 22:40:19, michaeln wrote:
> ditto?

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:543:
++temporary_quota_origins_to_delete_count_;
On 2011/07/26 22:40:19, michaeln wrote:
> There is a codepath where the QuotaManager will invoke the callback prior to
> return from DeleteOriginData. It's not really an expected case in chrome, but
it
> does exist. Not sure if it's worth it, but consider computing the full 'count'
> prior to making the DeleteOriginData calls.

I'll ping you about this.  I'm not sure I understand why I'd want to do that
(or, really, how I'd go about doing it without a lot of overhead. :) ).

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:569: void
BrowsingDataRemover::OnTemporaryQuotaManagedOriginDeletion(
On 2011/07/26 22:40:19, michaeln wrote:
> There's a fair amount of code duplication between
> ... 
> Maybe condense this to fewer methods.

I agree in part, and I've made some changes here, but I don't see a way around
at least some duplication. See comments in .h and the new unit tests.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:179: void OnClearedDatabases(int rv);
On 2011/07/26 22:40:19, michaeln wrote:
> can this be deleted too?

I was under the impression that this handled IndexedDB as well, but Jochen
assures me that it doesn't. Throwing it out the window.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:182: void ClearDatabasesOnFILEThread();
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:186: void OnClearedFileSystems();
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:189: void ClearFileSystemsOnFILEThread();
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:192: void OnClearedGearsData();
Throwing these away too, as we've removed Gears.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:233:
scoped_refptr<webkit_database::DatabaseTracker> database_tracker_;
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:235:
net::CompletionCallbackImpl<BrowsingDataRemover> database_cleared_callback_;
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:251: bool waiting_for_clear_databases_;
On 2011/07/26 22:40:19, michaeln wrote:
> ditto

Done.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:254: bool
waiting_for_clear_persistent_quota_managed_data_;
On 2011/07/26 22:40:19, michaeln wrote:
> see comments in the .cc file, maybe consolidate these two members into one...
> waiting_for_clear_quota_manager_;

I've done this, but I don't really see a way around flags for the
temp/persistent queues. See the CC file, as well as the three new tests I've
added. In short, I can't tell the difference between having no origins of a
certain type, and having no origins period.

http://codereview.chromium.org/7129018/diff/26039/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:262: int
persistent_quota_origins_to_delete_count_;
On 2011/07/26 22:40:19, michaeln wrote:
> ditto consolidation

ditto explanation. :)

http://codereview.chromium.org/7129018/diff/26039/chrome/test/testing_profile.cc
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/7129018/diff/26039/chrome/test/testing_profile...
chrome/test/testing_profile.cc:549: quota_manager_ = new
quota::MockQuotaManager(
On 2011/07/26 22:40:19, michaeln wrote:
> That's more flexible too since other tests would be free to setup
> a different 'mock' without impacting your test.

Fair enough. I wanted to move as much boilerplate as possible out of the tests,
but you're probably right.

[michaeln, 2011-07-28 19:17:47]

this just about lgtm, please see the one comment about code reuse in particular

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:458:
waiting_for_temporary_quota_managed_origin_list_ = true;
You may be able to get a little more code reuse if instead you kept a count how
many origin_lists_ you were waiting on instead of distinct flags per type. I
think OnGotTemporaryQuotaManagedOrigins and OnGotPersistentQuotaManagedOrigins
could be collapsed into one.

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (left):

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:175: void
ClearGearsDataOnFILEThread(const FilePath& profile_dir);
nice to see all this deleted code :)

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.h (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.h:186: // the QuotaManager is owned by the
profile; we can use a raw pointer here,
capital T in The

http://codereview.chromium.org/7129018/diff/39001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/test/testing_profile...
chrome/test/testing_profile.h:395: // The QuotaManager, created if
GetQuotaManager is invoked.
comment is stale

[Use mkwst_at_chromium.org plz., 2011-07-28 19:59:09]

Answered your question about code reuse: I'm not sure it's possible without a
bit more work.

There's also persistent timeouts in the history removal tests on mac and windows
(it of course works fine on Linux). I'm hoping to grab Jochen to take a look at
that with me tomorrow.

Other than that...

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:458:
waiting_for_temporary_quota_managed_origin_list_ = true;
On 2011/07/28 19:17:48, michaeln wrote:
> You may be able to get a little more code reuse if instead you kept a count
how
> many origin_lists_ you were waiting on instead of distinct flags per type. I
> think OnGotTemporaryQuotaManagedOrigins and OnGotPersistentQuotaManagedOrigins
> could be collapsed into one.

Combining the boolean flags into a counter makes sense. But I don't think I'll
be able to get rid of the two methods. The `GetOriginsCallback` gives me a set
of origins, but doesn't tell me which type I'm dealing with. I need to hard-code
that in the method that handles the callback, otherwise I can't call
`DeleteOriginData`.

I talked with Kinuko yesterday about changing the API to return both a set of
origins _and_ a type. She's on board, and I'm happy to make the change, but I
think that's better done in a separate CL, as I'll need to touch more than a few
files to change the method signature.

[Mike West, 2011-07-29 16:39:58]

Jochen tracked down some uninitialized variables that might have been causing
the broken tests on Mac and Windows. Throwing them to the trybots now. :)

I'd like to land this next week, and ask for permission to merge it into M14. To
that end, I've split off the cleanup of the GetOriginsCallback code into a
separate CL that Kinuko LGTMd today: http://codereview.chromium.org/7533013/ I'd
like to land that after this to reduce dependencies for possible merging (this
CL is big enough already. :) ).

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:458:
waiting_for_temporary_quota_managed_origin_list_ = true;
Replaced the flags with a counter, filed http://crbug.com/90966 for the API
change.

http://codereview.chromium.org/7129018/diff/39001/chrome/test/testing_profile.h
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/7129018/diff/39001/chrome/test/testing_profile...
chrome/test/testing_profile.h:395: // The QuotaManager, created if
GetQuotaManager is invoked.
On 2011/07/28 19:17:48, michaeln wrote:
> comment is stale

Done.

[michaeln, 2011-07-29 19:00:10]

> On 2011/07/28 19:17:48, michaeln wrote:
> > You may be able to get a little more code reuse if instead you kept a count
> how
> > many origin_lists_ you were waiting on instead of distinct flags per type. I
> > think OnGotTemporaryQuotaManagedOrigins and
OnGotPersistentQuotaManagedOrigins
> > could be collapsed into one.
> 
> Combining the boolean flags into a counter makes sense. But I don't think I'll
> be able to get rid of the two methods. The `GetOriginsCallback` gives me a set
> of origins, but doesn't tell me which type I'm dealing with. I need to
hard-code
> that in the method that handles the callback, otherwise I can't call
> `DeleteOriginData`.
> 
> I talked with Kinuko yesterday about changing the API to return both a set of
> origins _and_ a type. She's on board, and I'm happy to make the change, but I
> think that's better done in a separate CL, as I'll need to touch more than a
few
> files to change the method signature.

Right. that makes sense.

On 2011/07/29 16:39:58, Mike West wrote:
> Jochen tracked down some uninitialized variables that might have been causing
> the broken tests on Mac and Windows. Throwing them to the trybots now. :)
> 
> I'd like to land this next week, and ask for permission to merge it into M14.
To
> that end, I've split off the cleanup of the GetOriginsCallback code into a
> separate CL that Kinuko LGTMd today: http://codereview.chromium.org/7533013/
I'd
> like to land that after this to reduce dependencies for possible merging (this
> CL is big enough already. :) ).

sgtm!

[Mike West, 2011-08-01 09:04:21]

Jochen's fix seems to have solved the issues with the Win and Mac trybots.

I've rebased this onto ToT, running it through the trybots now. If they're
happy, I'll land it this afternoon, rebase
http://codereview.chromium.org/7533013/ on top of it, and land that as well.

[commit-bot: I haz the power, 2011-08-01 10:41:11]

Presubmit check for 7129018-47001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Presubmit checks took 1.6s to calculate.

[Mike West, 2011-08-01 10:52:05]

On 2011/08/01 10:41:11, I haz the power (commit-bot) wrote:

> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Bah. This is why I shouldn't clean up code as I go. ;)

Hello lovely `profile_impl.cc` owners (mirandac@, erg@, and sail@)! Would one of
you mind taking a quick look at this patch? Among other things, this CL drops
Gears support from the BrowsingDataRemover, which requires dropping the call
from the profile's dtor. That's the only impact to the profile's code.

Thanks!

-Mike

[jochen (gone - plz use gerrit), 2011-08-01 10:54:32]

I think in this case it's ok to submit with TBR=mirandac

[Miranda Callahan, 2011-08-01 14:40:51]

On 2011/08/01 10:54:32, jochen wrote:
> I think in this case it's ok to submit with TBR=mirandac

LGTM!

[commit-bot: I haz the power, 2011-08-01 16:23:43]

Change committed as 94913