Actually delete databases in CookiesTreeModel.

Actually delete databases in CookiesTreeModel.

BUG=34633
TEST=delete a database while it's opened in the renderer

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

[jochen (gone - plz use gerrit), 2010-02-12 20:10:54]

please review.

besides connecting the browsing data database helper to the database tracker
this also adds support for multiple pending delete operations and fixes the
cookie tree model to gracefully handle the case when no nice names for
databases/local storage are present.

it would be great if we could get this in for the dev-channel release

[jochen (gone - plz use gerrit), 2010-02-12 20:13:33]

I need to wait until tryservers catch the correct revision... will restart
tryjobs then.

On 2010/02/12 20:10:54, jochen wrote:
> please review.
> 
> besides connecting the browsing data database helper to the database tracker
> this also adds support for multiple pending delete operations and fixes the
> cookie tree model to gracefully handle the case when no nice names for
> databases/local storage are present.
> 
> it would be great if we could get this in for the dev-channel release

[michaeln, 2010-02-12 21:01:25]

sweet :)

http://codereview.chromium.org/600104/diff/1/2
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/600104/diff/1/2#newcode106
chrome/browser/browsing_data_database_helper.cc:106:
scoped_refptr<webkit_database::DatabaseTracker> tracker_;
why take a reference here?

http://codereview.chromium.org/600104/diff/1/4
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/600104/diff/1/4#newcode149
chrome/browser/cookies_tree_model.cc:149:
local_storage_info->database_identifier)),
Under what circumstances is 'origin' empty and vice versa?

http://codereview.chromium.org/600104/diff/1/4#newcode449
chrome/browser/cookies_tree_model.cc:449: database_info->host :
database_info->origin_identifier;
Similar to the question above, why 'host' in some cases and 'origin_identifier'
in others?

http://codereview.chromium.org/600104/diff/1/4#newcode477
chrome/browser/cookies_tree_model.cc:477: local_storage_info->host :
local_storage_info->database_identifier;
ditto

http://codereview.chromium.org/600104/diff/1/7
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/1/7#newcode112
webkit/database/database_tracker.cc:112: while (callback !=
deletion_callbacks_.end()) {
for loop may be a better choice given how m times ++callback happens below

http://codereview.chromium.org/600104/diff/1/7#newcode113
webkit/database/database_tracker.cc:113: if
(callback->second.find(origin_identifier) == callback->second.end()) {
may be nice to make a local here for the results of the find call so we don't
have to lookup repeatedly below.

http://codereview.chromium.org/600104/diff/1/7#newcode215
webkit/database/database_tracker.cc:215: DeleteOrigin(origin_identifier);
Ooops, I don't think you want to call this method here. This will delete all dbs
still remaining in the origin.

http://codereview.chromium.org/600104/diff/1/7#newcode404
webkit/database/database_tracker.cc:404: net::CompletionCallback* callback) {
since the only caller of this method uses an 'IgnoreCallback'... maybe a NULL
callback should be allowed here... wdyt?

http://codereview.chromium.org/600104/diff/1/7#newcode452
webkit/database/database_tracker.cc:452:
to_be_deleted[*ori].insert(db->database_name);
maybe just insert into the collection here... and test if its not empty() below
instead of setting rv to IO_PENDING here

http://codereview.chromium.org/600104/diff/1/8
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/600104/diff/1/8#newcode150
webkit/database/database_tracker.h:150: typedef
std::map<net::CompletionCallback*, DatabaseSet> PendingCompletionMap;
thnx for making the typedefs

[jochen (gone - plz use gerrit), 2010-02-12 21:09:56]

some replies... I'm boarding now.

http://codereview.chromium.org/600104/diff/1/2
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/600104/diff/1/2#newcode106
chrome/browser/browsing_data_database_helper.cc:106:
scoped_refptr<webkit_database::DatabaseTracker> tracker_;
So the tracker doesn't get deleted before it notified us. Maybe not necessary?

On 2010/02/12 21:01:25, michaeln wrote:
> why take a reference here?

http://codereview.chromium.org/600104/diff/1/4
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/600104/diff/1/4#newcode149
chrome/browser/cookies_tree_model.cc:149:
local_storage_info->database_identifier)),
for schemes without a hostname part, e.g. chrome-extensions
On 2010/02/12 21:01:25, michaeln wrote:
> Under what circumstances is 'origin' empty and vice versa?

http://codereview.chromium.org/600104/diff/1/4#newcode449
chrome/browser/cookies_tree_model.cc:449: database_info->host :
database_info->origin_identifier;
same answer. host looks nicer, but we might not have it.

On 2010/02/12 21:01:25, michaeln wrote:
> Similar to the question above, why 'host' in some cases and
'origin_identifier'
> in others?

http://codereview.chromium.org/600104/diff/1/7
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/1/7#newcode215
webkit/database/database_tracker.cc:215: DeleteOrigin(origin_identifier);
I think it only deletes the origin if it's empty and returns false otherwise.

On 2010/02/12 21:01:25, michaeln wrote:
> Ooops, I don't think you want to call this method here. This will delete all
dbs
> still remaining in the origin.

http://codereview.chromium.org/600104/diff/1/7#newcode404
webkit/database/database_tracker.cc:404: net::CompletionCallback* callback) {
if the callback doesn't need to keep a ref to the tracker, this would be a nice
option.

On 2010/02/12 21:01:25, michaeln wrote:
> since the only caller of this method uses an 'IgnoreCallback'... maybe a NULL
> callback should be allowed here... wdyt?

[michaeln, 2010-02-12 21:21:17]

> So the tracker doesn't get deleted before it notified us. Maybe not necessary?
Don't think its necessary.

> for schemes without a hostname part, e.g. chrome-extensions
Ah... thnx for the explanation

> I think it only deletes the origin if it's empty and returns false otherwise.
Nope, look closer... deletes the origin IF no dbs from the origin are open at
the time of the call.

[Peter Kasting, 2010-02-12 21:22:39]

I'm not really qualified to review the substance of this; that said, it looked
sane enough to me.

http://codereview.chromium.org/600104/diff/10/1012
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/600104/diff/10/1012#newcode147
chrome/browser/cookies_tree_model.cc:147: !local_storage_info->origin.empty() ?
Nit: Just remove ! and reverse the values (three places)

http://codereview.chromium.org/600104/diff/10/1014
File chrome/browser/views/options/cookies_view.cc (right):

http://codereview.chromium.org/600104/diff/10/1014#newcode351
chrome/browser/views/options/cookies_view.cc:351:
cookie_info_view_->SetVisible(view == cookie_info_view_);
Nice!

http://codereview.chromium.org/600104/diff/10/1015
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/10/1015#newcode111
webkit/database/database_tracker.cc:111: PendingCompletionMap::iterator callback
= deletion_callbacks_.begin();
Nit: Please use for() instead of while() here.  You might even be able to save
some logic by putting the increment in the for() declaration, and in the
callback-running case do:

net::CompletionCallback* cb = callback->first;
--callback;
deletion_callbacks_.erase(cb);
cb->Run(net::OK);

http://codereview.chromium.org/600104/diff/10/1017
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/600104/diff/10/1017#newcode1
webkit/database/database_tracker_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
Nit: 2010

[dumi, 2010-02-12 21:40:27]

http://codereview.chromium.org/600104/diff/10/1010
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/600104/diff/10/1010#newcode119
chrome/browser/browsing_data_database_helper.cc:119: callback) !=
net::ERR_IO_PENDING)
should use {} here, since the if-statement is on 2 lines. also, i'm pretty sure
'callback' should not be aligned with 'tracker_'.

http://codereview.chromium.org/600104/diff/10/1012
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/600104/diff/10/1012#newcode149
chrome/browser/cookies_tree_model.cc:149:
local_storage_info->database_identifier)),
we shouldn't use the database_identifier here. i think we should use
schema:port//, if port != 0, or just schema:// otherwise (file://,
chrome-extension://, some-other-schema:1234//). i think you should add a
function here that takes a database_identifier and returns either the origin, or
the schema:port// string.

http://codereview.chromium.org/600104/diff/10/1015
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/10/1015#newcode113
webkit/database/database_tracker.cc:113: if
(callback->second.find(origin_identifier) == callback->second.end()) {
might be nice to use a temp variable: DatabaseSet& db_set = callback->second.
and i agree with peter that a for loop would simplify this code a lot.

http://codereview.chromium.org/600104/diff/10/1015#newcode205
webkit/database/database_tracker.cc:205: // TODO(jochen): Delete journal files
associated with this database.
i don't think you need to worry about journal files. it's up to sqlite to keep
track of them, and a closed database should never have journal files hanging
around.

http://codereview.chromium.org/600104/diff/10/1015#newcode409
webkit/database/database_tracker.cc:409:
DCHECK(deletion_callbacks_.find(callback) == deletion_callbacks_.end());
we'll probably never run into this case anyway, but i still think we should
replace this DCHECK with:

if (deletion_callbacks_.find(callback) != deletion_callbacks_.end())
  return net:ERR_IO_PENDING;

http://codereview.chromium.org/600104/diff/10/1015#newcode428
webkit/database/database_tracker.cc:428:
DCHECK(deletion_callbacks_.find(callback) == deletion_callbacks_.end());
same comment.

http://codereview.chromium.org/600104/diff/10/1016
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/600104/diff/10/1016#newcode212
webkit/database/database_tracker.h:212: DatabaseSet dbs_to_be_deleted_;
i don't think we need this field anymore.

[Peter Kasting, 2010-02-12 21:41:57]

http://codereview.chromium.org/600104/diff/10/1010
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/600104/diff/10/1010#newcode119
chrome/browser/browsing_data_database_helper.cc:119: callback) !=
net::ERR_IO_PENDING)
On 2010/02/12 21:40:27, dumi wrote:
> should use {} here, since the if-statement is on 2 lines.

That isn't the rule.  The rule applies to _bodies_ bigger than two lines.  We
don't normally use braces for conditionals on two lines.

> also, i'm pretty sure
> 'callback' should not be aligned with 'tracker_'.

It's legal.

[dumi, 2010-02-12 21:43:14]

http://codereview.chromium.org/600104/diff/10/1016
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/600104/diff/10/1016#newcode179
webkit/database/database_tracker.h:179: bool DeleteOrigin(const string16&
origin_identifier);
forgot to mention that we need a DeleteOrigin() method similar to the
DeleteDatabase() one for when the user selects a "Web Database" node and hits
delete: all databases for that origin should be deleted.

[jochen (gone - plz use gerrit), 2010-02-15 16:40:12]

http://codereview.chromium.org/600104/diff/1/2
File chrome/browser/browsing_data_database_helper.cc (right):

http://codereview.chromium.org/600104/diff/1/2#newcode106
chrome/browser/browsing_data_database_helper.cc:106:
scoped_refptr<webkit_database::DatabaseTracker> tracker_;
I removed this callback and allow a NULL callback for DeleteDatabase.

On 2010/02/12 21:09:56, jochen wrote:
> So the tracker doesn't get deleted before it notified us. Maybe not necessary?
> 
> On 2010/02/12 21:01:25, michaeln wrote:
> > why take a reference here?
> 
>

http://codereview.chromium.org/600104/diff/1/7
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/1/7#newcode113
webkit/database/database_tracker.cc:113: if
(callback->second.find(origin_identifier) == callback->second.end()) {
On 2010/02/12 21:01:25, michaeln wrote:
> may be nice to make a local here for the results of the find call so we don't
> have to lookup repeatedly below.

Done.

http://codereview.chromium.org/600104/diff/1/7#newcode215
webkit/database/database_tracker.cc:215: DeleteOrigin(origin_identifier);
On 2010/02/12 21:09:56, jochen wrote:
> I think it only deletes the origin if it's empty and returns false otherwise.
> 
> On 2010/02/12 21:01:25, michaeln wrote:
> > Ooops, I don't think you want to call this method here. This will delete all
> dbs
> > still remaining in the origin.
> 
> 

Done.

http://codereview.chromium.org/600104/diff/1/7#newcode404
webkit/database/database_tracker.cc:404: net::CompletionCallback* callback) {
On 2010/02/12 21:09:56, jochen wrote:
> if the callback doesn't need to keep a ref to the tracker, this would be a
nice
> option.
> 
> On 2010/02/12 21:01:25, michaeln wrote:
> > since the only caller of this method uses an 'IgnoreCallback'... maybe a
NULL
> > callback should be allowed here... wdyt?
> 
> 

Done.

http://codereview.chromium.org/600104/diff/1/7#newcode452
webkit/database/database_tracker.cc:452:
to_be_deleted[*ori].insert(db->database_name);
On 2010/02/12 21:01:25, michaeln wrote:
> maybe just insert into the collection here... and test if its not empty()
below
> instead of setting rv to IO_PENDING here

Done.

http://codereview.chromium.org/600104/diff/10/1012
File chrome/browser/cookies_tree_model.cc (right):

http://codereview.chromium.org/600104/diff/10/1012#newcode147
chrome/browser/cookies_tree_model.cc:147: !local_storage_info->origin.empty() ?
On 2010/02/12 21:22:39, Peter Kasting wrote:
> Nit: Just remove ! and reverse the values (three places)

Done.

http://codereview.chromium.org/600104/diff/10/1012#newcode149
chrome/browser/cookies_tree_model.cc:149:
local_storage_info->database_identifier)),
I don't think so - we would throw together information from different sources
e.g. for chrome-extension: scheme.

On 2010/02/12 21:40:27, dumi wrote:
> we shouldn't use the database_identifier here. i think we should use
> schema:port//, if port != 0, or just schema:// otherwise (file://,
> chrome-extension://, some-other-schema:1234//). i think you should add a
> function here that takes a database_identifier and returns either the origin,
or
> the schema:port// string.

http://codereview.chromium.org/600104/diff/10/1015
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/10/1015#newcode111
webkit/database/database_tracker.cc:111: PendingCompletionMap::iterator callback
= deletion_callbacks_.begin();
This doesn't work if callback == begin() 

actually, deleting from a set invalidates all iterators, so I'll collect the
keys in a different set and erase them after the now-to-be for loop

On 2010/02/12 21:22:39, Peter Kasting wrote:
> Nit: Please use for() instead of while() here.  You might even be able to save
> some logic by putting the increment in the for() declaration, and in the
> callback-running case do:
> 
> net::CompletionCallback* cb = callback->first;
> --callback;
> deletion_callbacks_.erase(cb);
> cb->Run(net::OK);

http://codereview.chromium.org/600104/diff/10/1015#newcode205
webkit/database/database_tracker.cc:205: // TODO(jochen): Delete journal files
associated with this database.
On 2010/02/12 21:40:27, dumi wrote:
> i don't think you need to worry about journal files. it's up to sqlite to keep
> track of them, and a closed database should never have journal files hanging
> around.

Done.

http://codereview.chromium.org/600104/diff/10/1015#newcode409
webkit/database/database_tracker.cc:409:
DCHECK(deletion_callbacks_.find(callback) == deletion_callbacks_.end());
I don't know whether that's a good idea. I prefer the DCHECK because it will
force whoever might use this interface in the future to look at the code.

Since I'm allowing NULL callbacks now, I changed the DCHECK to only check on
non-NULL callbacks.
On 2010/02/12 21:40:27, dumi wrote:
> we'll probably never run into this case anyway, but i still think we should
> replace this DCHECK with:
> 
> if (deletion_callbacks_.find(callback) != deletion_callbacks_.end())
>   return net:ERR_IO_PENDING;
>

http://codereview.chromium.org/600104/diff/10/1016
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/600104/diff/10/1016#newcode179
webkit/database/database_tracker.h:179: bool DeleteOrigin(const string16&
origin_identifier);
That would require syncing the calls to the DeleteDatabase with the DeleteOrigin
call. Instead, I now delete the origin if it does contain no databases anymore
in DeleteDatabase()

On 2010/02/12 21:43:15, dumi wrote:
> forgot to mention that we need a DeleteOrigin() method similar to the
> DeleteDatabase() one for when the user selects a "Web Database" node and hits
> delete: all databases for that origin should be deleted.

http://codereview.chromium.org/600104/diff/10/1016#newcode212
webkit/database/database_tracker.h:212: DatabaseSet dbs_to_be_deleted_;
we still need it for IsDatabaseScheduledForDeletion

On 2010/02/12 21:40:27, dumi wrote:
> i don't think we need this field anymore.

http://codereview.chromium.org/600104/diff/10/1017
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/600104/diff/10/1017#newcode1
webkit/database/database_tracker_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
On 2010/02/12 21:22:39, Peter Kasting wrote:
> Nit: 2010

Done.

[michaeln, 2010-02-15 21:18:08]

http://codereview.chromium.org/600104/diff/2005/3008
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/2005/3008#newcode111
webkit/database/database_tracker.cc:111: std::set<net::CompletionCallback*>
to_be_deleted;
i'd have used a std::vector<> here...
  std::vector<> completed_callbacks;

http://codereview.chromium.org/600104/diff/2005/3008#newcode118
webkit/database/database_tracker.cc:118: if (databases.empty()) {
These are complicated data structures, tricky to visually verify things are
correct here, some database_tracker_unittest could really help.

It looks like we may be invoking the completion callback prior to actually
deleting all of the things scheduled for deletion on its behalf? The 'databases'
collection is just the collection of dbs for a particular origin, there may be
other dbs from other origins that should be deleted prior to invoking the
callback... is that right?

If so, do we want to...

if (databases.empty()) {
  callback->second.remove(dbiter);
  if (callback->second.empty()) {
    callback->first->Run();
    completed_callbacks.push_back(callback->first);
  }
}

http://codereview.chromium.org/600104/diff/2005/3008#newcode417
webkit/database/database_tracker.cc:417:
deletion_callbacks_[callback][origin_identifier].insert(database_name);
I think we can avoid adding NULL callbacks to this structure. Wdyt?

http://codereview.chromium.org/600104/diff/2005/3008#newcode464
webkit/database/database_tracker.cc:464: deletion_callbacks_[callback] =
to_be_deleted;
Ditto

http://codereview.chromium.org/600104/diff/2005/3009
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/600104/diff/2005/3009#newcode113
webkit/database/database_tracker.h:113: void DeleteDatabaseIfNeeded(const
string16& origin_identifier,
should this method be private?

http://codereview.chromium.org/600104/diff/2005/3009#newcode135
webkit/database/database_tracker.h:135: // failure, or net::ERR_IO_PENDING and
|callback| is invoked upon completion.
maybe mention |callback| may be NULL

[michaeln, 2010-02-15 21:38:10]

Also, I think the original intent behind DeleteOrigin() was to hook that up to a
ui gesture in the cookie tree to delete the node associated with that origin.
Since its not being used for that purpose i wonder two things...

1) What does happen on that ui gesture? Does that end up calling
DeleteDatabase(ori, db) for each child node?

2) Do we really need the DeleteOrigin() call? The net effect of that call is the
remove the directory in the file system, and to remove any stray journal files
that we failed to cleanup earlier.

Also, about stray journal files, I know dumi said sqlite should have taken care
of them, but i don't think that's entirely true. Sqlite only has an opportunity
to act on them when when a db open (or opening/closing). If the system crashes
there could be stray files left laying about. Then, if the user gestures to
delete something upon next run prior to any webapp accessing them they'll still
be laying around.

[jochen (gone - plz use gerrit), 2010-02-15 21:55:38]

On 2010/02/15 21:38:10, michaeln wrote:
> Also, I think the original intent behind DeleteOrigin() was to hook that up to
a
> ui gesture in the cookie tree to delete the node associated with that origin.
> Since its not being used for that purpose i wonder two things...
> 
> 1) What does happen on that ui gesture? Does that end up calling
> DeleteDatabase(ori, db) for each child node?
> 

Yes, right now the cookies tree model will always trigger delete for all child
nodes before removing the origin node. Although that could be changed of course

> 2) Do we really need the DeleteOrigin() call? The net effect of that call is
the
> remove the directory in the file system, and to remove any stray journal files
> that we failed to cleanup earlier.
> 
I think deleting the origin if empty is disireable from a privacy point of view

> Also, about stray journal files, I know dumi said sqlite should have taken
care
> of them, but i don't think that's entirely true. Sqlite only has an
opportunity
> to act on them when when a db open (or opening/closing). If the system crashes
> there could be stray files left laying about. Then, if the user gestures to
> delete something upon next run prior to any webapp accessing them they'll
still
> be laying around.

[Peter Kasting, 2010-02-15 22:28:30]

http://codereview.chromium.org/600104/diff/10/1015
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/10/1015#newcode111
webkit/database/database_tracker.cc:111: PendingCompletionMap::iterator callback
= deletion_callbacks_.begin();
On 2010/02/15 16:40:15, jochen wrote:
> This doesn't work if callback == begin() 

True, argh.

> actually, deleting from a set invalidates all iterators,

Oh, is this a set?  I thought it was a map.  Deleting one iterator out of a map
leaves the others untouched.

[michaeln, 2010-02-16 00:59:39]

> > 1) What does happen on that ui gesture? Does that end up calling
> > DeleteDatabase(ori, db) for each child node?
> 
> Yes, right now the cookies tree model will always trigger delete for all child
> nodes before removing the origin node. Although that could be changed of
course

That strategy seems fine, i was just wondering how it worked is all.

> > 2) Do we really need the DeleteOrigin() call? The net effect of that call is
> the
> > remove the directory in the file system, and to remove any stray journal
files
> > that we failed to cleanup earlier.
> > 
> I think deleting the origin if empty is disireable from a privacy point of
view

I didn't mean to suggest that we shouldn't delete the directory on disk when we
could. Was just providing some history as to why DeleteOrigin() existed as a
public method and what the intended use was.

[dumi, 2010-02-16 20:15:39]

On 2010/02/15 21:55:38, jochen wrote:
> On 2010/02/15 21:38:10, michaeln wrote:
> > Also, I think the original intent behind DeleteOrigin() was to hook that up
to
> a
> > ui gesture in the cookie tree to delete the node associated with that
origin.
> > Since its not being used for that purpose i wonder two things...
> > 
> > 1) What does happen on that ui gesture? Does that end up calling
> > DeleteDatabase(ori, db) for each child node?
> > 
> 
> Yes, right now the cookies tree model will always trigger delete for all child
> nodes before removing the origin node. Although that could be changed of
course

i think we should make it call DeleteOrigin() directly and then change
DeleteOrigin() to something like:

dbData = db_table_->GetAllDatabaseDetailsForOrigin();
for each db in dbData : DeleteDatabase(db);
wait for all open dbs to be closed and deleted if needed
if (file_util::Delete(origin_dir, true)) db_table_->DeleteOrigin();

this would make sure that:
1. journal files are deleted too
2. if the user manually removes some DBs, the 'Databases' table will have
records for inexistent databases. this code would delete those records.

[jochen (gone - plz use gerrit), 2010-02-17 19:19:08]

ping

[michaeln, 2010-02-17 19:41:23]

LGTM, modulo try happiness a few very minor things to look at

http://codereview.chromium.org/600104/diff/13001/14001
File base/file_util.h (right):

http://codereview.chromium.org/600104/diff/13001/14001#newcode333
base/file_util.h:333: bool GetFileInfo(const std::wstring& file_path, FileInfo*
info);
style: probably should add a blank line here (when in rome rule)

http://codereview.chromium.org/600104/diff/13001/14003
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/600104/diff/13001/14003#newcode1406
base/file_util_unittest.cc:1406: // Observe that this timestamp is divisible by
two (seconds) - FAT stores
Maybe 'Note' instead of 'Observe', when i read this comment i was expecting the
test code to make that observation before understanding that it was a note to
the reader :)

http://codereview.chromium.org/600104/diff/13001/14012
File webkit/database/database_tracker.cc (left):

http://codereview.chromium.org/600104/diff/13001/14012#oldcode196
webkit/database/database_tracker.cc:196: // TODO(jochen): Delete journal files
associated with this database.
please keep the TODO about journal files here, you can put my name in it if you
like

http://codereview.chromium.org/600104/diff/13001/14012
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/600104/diff/13001/14012#newcode110
webkit/database/database_tracker.cc:110:
dbs_to_be_deleted_.erase(origin_identifier);
maybe a blank line here to separate the completion callback logic from the other
logic

http://codereview.chromium.org/600104/diff/13001/14012#newcode119
webkit/database/database_tracker.cc:119:
callback->second.erase(origin_identifier);
i think you could call erase(dbiter) here to avoid searching again, although, it
is more clear what's going on by saying erase(origin)

maybe dbiter could use a better name, 'found_ori' ?

http://codereview.chromium.org/600104/diff/13001/14012#newcode459
webkit/database/database_tracker.cc:459:
to_be_deleted[*ori].insert(db->database_name);
style: since these if and else clause are both one-liners, these should not have
brackets

http://codereview.chromium.org/600104/diff/13001/14012#newcode470
webkit/database/database_tracker.cc:470: ori != to_be_deleted.end(); ++ori)
style: this for loop body should have brackets

http://codereview.chromium.org/600104/diff/13001/14014
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/600104/diff/13001/14014#newcode75
webkit/database/database_tracker_unittest.cc:75: TEST(DatabaseTrackerTest,
DeleteOpenDatabase) {
thnx for a adding the tests

http://codereview.chromium.org/600104/diff/13001/14014#newcode117
webkit/database/database_tracker_unittest.cc:117: EXPECT_EQ(net::ERR_IO_PENDING,
result);
maybe assert that the callback has not yet been invoked?

http://codereview.chromium.org/600104/diff/13001/14014#newcode136
webkit/database/database_tracker_unittest.cc:136: 
nit: there's an extra blank line here that can be removed