Delete empty dom storage databases on destruction.

Delete empty dom storage databases on destruction.

BUG=106763
TEST=


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

[benm (inactive), 2012-02-24 12:17:12]

try bot results aren't my fault! :)

[michaeln, 2012-02-25 02:55:54]

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:47: file_util::Delete(file_path_,
false);
maybe skip this if file_path.empty()?

if (known_to_be_empty_ && !file_path.empty()) {
  Close();
  file_util::Delete(...);
}

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:64: }
known_to_be_empty_ = result->empty();

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:98: values_stored_++;
This count will not be accurate, consider this is just an update.

Keeping count is overkill. All we need is 'known_to_be_empty_' at dtor time, not
a count. I think we can do this in a lighter weight way given a bool data member
to that effect.

- init to false in the ctor
- dont modify LazyOpen in any way (value remains unmodified upon return)
  no extra queries on open
  (its ok to not determine on open because open only happens via the public
   ReadAllValues and CommitChanges methods which will determine the truth
   themselves)
- set to true in the if clear_all_first clause
- set to false here in the INSERT clause, and set a local to 'did_insert'
- set a local to 'did_delete' in the DELETE clause

after the for loop where we INSERT &| DELETE items, and before the COMMIT

if (!known_to_be_empty_ && did_delete && !did_insert) {
  knowns_to_be_empty_ = (select count == 0);
}

COMMIT

wdyt?

[benm (inactive), 2012-02-27 12:06:25]

thanks, new patch on the way!

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:47: file_util::Delete(file_path_,
false);
On 2012/02/25 02:55:55, michaeln wrote:
> maybe skip this if file_path.empty()?
> 
> if (known_to_be_empty_ && !file_path.empty()) {
>   Close();
>   file_util::Delete(...);
> }

Done.

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:64: }
Do we need to do this if we init known_to_be_empty_(false) in the ctor?

When the database is first created, it's empty and we won't write anything on
disk until we get the first CommitChanges. If ReadAllValues gets called on a db
in this state then we'll have already bailed out as LazyOpen(false) would've
returned false. The flag may be false when it could be true in this case, but
the side effect of that is that we won't try and delete the non-existent file in
the dtor.

If the file already exists, with this patch we'll never leave an empty database
on disk so it must have content. So as ReadAllValues does not change the
database, we can assume the database non-empty here if CommitChanges hasn't been
executed. And if CommitChanges has been executed, then we will have updated
known_to_be_empty_ at that point.

http://codereview.chromium.org/9467003/diff/1/webkit/dom_storage/dom_storage_...
webkit/dom_storage/dom_storage_database.cc:98: values_stored_++;
On 2012/02/25 02:55:55, michaeln wrote:
> This count will not be accurate, consider this is just an update.

Good catch, i made the test a bit more robust.

> Keeping count is overkill. All we need is 'known_to_be_empty_' at dtor time,
not
> a count. I think we can do this in a lighter weight way given a bool data
member
> to that effect.
> 
> - init to false in the ctor
> - dont modify LazyOpen in any way (value remains unmodified upon return)
>   no extra queries on open
>   (its ok to not determine on open because open only happens via the public

sg. I kept the small refactoring in LazyOpen, but removed the query.

>    ReadAllValues and CommitChanges methods which will determine the truth
>    themselves)
> - set to true in the if clear_all_first clause
> - set to false here in the INSERT clause, and set a local to 'did_insert'

Do we need the did_insert local? We can just set known_to_be_empty_ false on
insert (as if it's an add or replace we still won't have an empty db). I guess
it would optimise the case that we have a bunch of inserts/deletes in
changed_values_, so that we don't have to run the SELECT COUNT (as there was an
insert which we know will leave the db non-empty)

> - set a local to 'did_delete' in the DELETE clause
> 
> after the for loop where we INSERT &| DELETE items, and before the COMMIT
> 
> if (!known_to_be_empty_ && did_delete && !did_insert) {
>   knowns_to_be_empty_ = (select count == 0);
> }
> 
> COMMIT
> 
> wdyt?

[benm (inactive), 2012-02-27 12:19:24]

http://codereview.chromium.org/9467003/diff/7001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9467003/diff/7001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:116: return transaction.Commit();
hm, if the transaction fails, then we need to switch back to the previous value
of known_to_be_empty_...

[michaeln, 2012-02-27 19:28:47]

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:64: }
i think we should set known_to_be_empty_ here

since chrome can crash, the reasons to assume that an existing database is not
empty are not valid... also there's the matter of the pre-existing db files what
state are they in?

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:109: // See if the database is empty.
consider removing the comment, the code is clear enough

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:113: known_to_be_empty_ =
statement.ColumnInt(0) == 0;
if you think it's more readable, consider...

known_to_be_empty_ = Step() &&
                    (ColumnInt(0) == 0);

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:192: return true;
looks like this introduces a bug in the 'new database' case (up above), if table
creation fails this method returns true.

in the spirit of extreme programming principles, i don't see what the changes in
this method have to do with deleting empties on exit and suggest undoing them

(having said that... imo, an improvement would be to make the 'new database'
case return early always instead of only sometimes, because that would make the
type of error being introduced here less likely to be introduced, i think i
tried to make that point in an earlier code review:)

[benm (inactive), 2012-02-27 19:36:15]

thanks!

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:64: }
Got it.

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:109: // See if the database is empty.
On 2012/02/27 19:28:48, michaeln wrote:
> consider removing the comment, the code is clear enough

Done.

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:113: known_to_be_empty_ =
statement.ColumnInt(0) == 0;
I think I prefer the if () then approach like it is now.

http://codereview.chromium.org/9467003/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:192: return true;
On 2012/02/27 19:28:48, michaeln wrote:
> looks like this introduces a bug in the 'new database' case (up above), if
table
> creation fails this method returns true.

Good spot.

> in the spirit of extreme programming principles, i don't see what the changes
in
> this method have to do with deleting empties on exit and suggest undoing them

Probably for the best. :)

> (having said that... imo, an improvement would be to make the 'new database'
> case return early always instead of only sometimes, because that would make
the
> type of error being introduced here less likely to be introduced, i think i
> tried to make that point in an earlier code review:)

Will do it in another CL :)

[benm (inactive), 2012-02-27 20:03:39]

hopefully this one does the trick. ptal :)

[michaeln, 2012-02-27 20:20:35]

lgtm

[commit-bot: I haz the power, 2012-02-28 10:00:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/9467003/13001

[commit-bot: I haz the power, 2012-02-28 11:24:15]

Change committed as 123960