Adding methods that will be used by the quota management UI....

Adding methods that will be used by the quota management UI.

TEST=none
BUG=none



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

[dumi, 2009-12-22 02:28:49]

hey michael,

i think this patch should be ready for review. please take a look at it when you
have time.

thanks,
dumi

[michaeln, 2009-12-29 02:55:26]

http://codereview.chromium.org/507014/diff/7009/4060
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/507014/diff/7009/4060#newcode74
chrome/browser/renderer_host/database_dispatcher_host.cc:74: for
(OpenedDatabases::const_iterator it = opened_databases_.begin();
maybe origin_iter

http://codereview.chromium.org/507014/diff/7009/4060#newcode76
chrome/browser/renderer_host/database_dispatcher_host.cc:76: DatabaseConnections
database_connections = it->second;
you can avoid a copy of the collection here...
  DatabseConnections& connections = it->second;

http://codereview.chromium.org/507014/diff/7009/4060#newcode77
chrome/browser/renderer_host/database_dispatcher_host.cc:77: for
(DatabaseConnections::const_iterator itt = database_connections.begin();
and connection_iter

http://codereview.chromium.org/507014/diff/7009/4060#newcode384
chrome/browser/renderer_host/database_dispatcher_host.cc:384: if
(IsDatabaseOpened(origin_identifier, database_name)) {
Do we still have to send this message to all child processes that have ANY
database open from this origin so they can update the space_available for the
origin?

http://codereview.chromium.org/507014/diff/7009/4060#newcode398
chrome/browser/renderer_host/database_dispatcher_host.cc:398: return
(opened_databases_[origin_identifier][database_name] > 0);
this will insert a entry for 'origin_identifier' if one isn't present in the map
already

http://codereview.chromium.org/507014/diff/7009/4055
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/7009/4055#newcode25
webkit/database/database_tracker.cc:25: const int kCurrentVersion = 1;
maybe bump this to version 2 since you're adding a table

http://codereview.chromium.org/507014/diff/7009/4054
File webkit/database/databases_table.cc (right):

http://codereview.chromium.org/507014/diff/7009/4054#newcode32
webkit/database/databases_table.cc:32: "CREATE INDEX databases_origin_index ON
Databases (origin)") &&
Not sure we need to make this change to the schema. Some systems will have an
index called 'origin_index' and others will have an index called
'databses_origin_index'.

http://codereview.chromium.org/507014/diff/7009/4057
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/7009/4057#newcode43
webkit/database/quota_table.cc:43: SQL_FROM_HERE, "UPDATE Quota SET quota = ?
WHERE origin = ?"));
I think you could use INSERT OR REPLACE here if the schema constrained things to
one row per origin... ala CREATE UNIQUE INDEX

[dumi, 2009-12-30 02:08:36]

please take another look.

http://codereview.chromium.org/507014/diff/7009/4060
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/507014/diff/7009/4060#newcode74
chrome/browser/renderer_host/database_dispatcher_host.cc:74: for
(OpenedDatabases::const_iterator it = opened_databases_.begin();
On 2009/12/29 02:55:26, michaeln wrote:
> maybe origin_iter

done.

http://codereview.chromium.org/507014/diff/7009/4060#newcode76
chrome/browser/renderer_host/database_dispatcher_host.cc:76: DatabaseConnections
database_connections = it->second;
On 2009/12/29 02:55:26, michaeln wrote:
> you can avoid a copy of the collection here...
>   DatabseConnections& connections = it->second;

fixed.

http://codereview.chromium.org/507014/diff/7009/4060#newcode77
chrome/browser/renderer_host/database_dispatcher_host.cc:77: for
(DatabaseConnections::const_iterator itt = database_connections.begin();
On 2009/12/29 02:55:26, michaeln wrote:
> and connection_iter

changed to database_iter. i think it makes it a bit more consistent with other
methods that deal with origins/origin_identifiers and databases/database_names.

http://codereview.chromium.org/507014/diff/7009/4060#newcode384
chrome/browser/renderer_host/database_dispatcher_host.cc:384: if
(IsDatabaseOpened(origin_identifier, database_name)) {
On 2009/12/29 02:55:26, michaeln wrote:
> Do we still have to send this message to all child processes that have ANY
> database open from this origin so they can update the space_available for the
> origin?

good catch. fixed.

http://codereview.chromium.org/507014/diff/7009/4060#newcode398
chrome/browser/renderer_host/database_dispatcher_host.cc:398: return
(opened_databases_[origin_identifier][database_name] > 0);
On 2009/12/29 02:55:26, michaeln wrote:
> this will insert a entry for 'origin_identifier' if one isn't present in the
map
> already

yeah. that was fine as it was, since we didn't check anywhere if there was any
DB opened in an origin. we do it now, so i changed this code. i also removed the
comment at the end of RemoveOpenedDatabase() and added the clean up code.

http://codereview.chromium.org/507014/diff/7009/4055
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/7009/4055#newcode25
webkit/database/database_tracker.cc:25: const int kCurrentVersion = 1;
On 2009/12/29 02:55:26, michaeln wrote:
> maybe bump this to version 2 since you're adding a table

done.

http://codereview.chromium.org/507014/diff/7009/4054
File webkit/database/databases_table.cc (right):

http://codereview.chromium.org/507014/diff/7009/4054#newcode32
webkit/database/databases_table.cc:32: "CREATE INDEX databases_origin_index ON
Databases (origin)") &&
On 2009/12/29 02:55:26, michaeln wrote:
> Not sure we need to make this change to the schema. Some systems will have an
> index called 'origin_index' and others will have an index called
> 'databses_origin_index'.
> 

reverted.

http://codereview.chromium.org/507014/diff/7009/4057
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/7009/4057#newcode43
webkit/database/quota_table.cc:43: SQL_FROM_HERE, "UPDATE Quota SET quota = ?
WHERE origin = ?"));
On 2009/12/29 02:55:26, michaeln wrote:
> I think you could use INSERT OR REPLACE here if the schema constrained things
to
> one row per origin... ala CREATE UNIQUE INDEX

done. made 'origin' the primary key.

[michaeln, 2009-12-30 21:06:58]

http://codereview.chromium.org/507014/diff/8002/8007
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/8002/8007#newcode40
webkit/database/quota_table.cc:40: SQL_FROM_HERE, "REPLACE INTO Quota VALUES (?,
?)"));
so 'REPLACE' is short hand for INSERT or REPLACE... just checking

http://codereview.chromium.org/507014/diff/8017/9004
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/8017/9004#newcode83
webkit/database/database_tracker.cc:83:
opened_databases_.erase(origin_identifier);
The opened_databases_ data structure here looks a lot like that used in the
dispatcher host class. Consider sharing this code somehow.

Also, there are repeated map lookups in the logic here (and elsewhere when
accessing this data structure) that could be streamlined.

http://codereview.chromium.org/507014/diff/8017/9004#newcode193
webkit/database/database_tracker.cc:193: origins_info->push_back(origin_info);
Seems dicey to hand out ptrs to these internal structures that can get cleared
at arbitrary times.

http://codereview.chromium.org/507014/diff/8017/9004#newcode254
webkit/database/database_tracker.cc:254: !db_->DoesTableExist("meta"))) {
maybe use the static MetaTable::DoesTableExist(&db_) to avoid the dependency on
the table name used by the MetaTable class... i guess the MetaTable class could
have a public const char* data member for the its table name?

http://codereview.chromium.org/507014/diff/8017/9007
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/507014/diff/8017/9007#newcode115
webkit/database/database_tracker.h:115: bool DeleteOrigin(const string16&
origin_identifier);
This is a lot of new public API for this class. Which of these methods are
really needed by what callers. I'd like to understand how this stuff will be
used prior to putting all of this in the public section.

http://codereview.chromium.org/507014/diff/8017/9003
File webkit/database/databases_table.cc (right):

http://codereview.chromium.org/507014/diff/8017/9003#newcode154
webkit/database/databases_table.cc:154: return (delete_statement.Run() &&
db_->GetLastChangeCount());
I wonder if trying to delete rows for an origin without records in the db should
return true or false?

http://codereview.chromium.org/507014/diff/8017/9006
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/8017/9006#newcode37
webkit/database/quota_table.cc:37: int64 quota) {
maybe DCHECK(quota >= 0)?

http://codereview.chromium.org/507014/diff/8017/9008
File webkit/database/quota_table.h (right):

http://codereview.chromium.org/507014/diff/8017/9008#newcode24
webkit/database/quota_table.h:24: bool SetOriginQuota(const string16&
origin_identifier, int64 quota);
Have you considered a ClearOriginQuota method for use when the user resets an
origin to the 'default' quota (in the UI)?

http://codereview.chromium.org/507014/diff/8017/9013
File webkit/database/quota_table_unittest.cc (right):

http://codereview.chromium.org/507014/diff/8017/9013#newcode44
webkit/database/quota_table_unittest.cc:44:
EXPECT_TRUE(db.Open(temp_dir.path().Append(FILE_PATH_LITERAL("tracker.db"))));
maybe u can use an in-memory db for these tests... db.OpenInMemory()

http://codereview.chromium.org/507014/diff/8017/9013#newcode59
webkit/database/quota_table_unittest.cc:59: 
maybe test that we only have a single row in the db here to ensure the
insert/replace logic is correct

[dumi, 2010-01-05 01:50:06]

http://codereview.chromium.org/507014/diff/8002/8007
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/8002/8007#newcode40
webkit/database/quota_table.cc:40: SQL_FROM_HERE, "REPLACE INTO Quota VALUES (?,
?)"));
On 2009/12/30 21:06:58, michaeln wrote:
> so 'REPLACE' is short hand for INSERT or REPLACE... just checking

yes. it's officially supported by sqlite (apparently to be more compatible with
MySql).

http://codereview.chromium.org/507014/diff/8017/9004
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/8017/9004#newcode83
webkit/database/database_tracker.cc:83:
opened_databases_.erase(origin_identifier);
On 2009/12/30 21:07:00, michaeln wrote:
> The opened_databases_ data structure here looks a lot like that used in the
> dispatcher host class. Consider sharing this code somehow.
> 
> Also, there are repeated map lookups in the logic here (and elsewhere when
> accessing this data structure) that could be streamlined.

done.

http://codereview.chromium.org/507014/diff/8017/9004#newcode193
webkit/database/database_tracker.cc:193: origins_info->push_back(origin_info);
On 2009/12/30 21:07:00, michaeln wrote:
> Seems dicey to hand out ptrs to these internal structures that can get cleared
> at arbitrary times.

added a copy-constructor to OriginInfo and changed the API to return
vector<OriginInfo>.

http://codereview.chromium.org/507014/diff/8017/9004#newcode254
webkit/database/database_tracker.cc:254: !db_->DoesTableExist("meta"))) {
On 2009/12/30 21:07:00, michaeln wrote:
> maybe use the static MetaTable::DoesTableExist(&db_) to avoid the dependency
on
> the table name used by the MetaTable class... i guess the MetaTable class
could
> have a public const char* data member for the its table name?

done. wasn't aware of that method.

http://codereview.chromium.org/507014/diff/8017/9007
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/507014/diff/8017/9007#newcode115
webkit/database/database_tracker.h:115: bool DeleteOrigin(const string16&
origin_identifier);
On 2009/12/30 21:07:00, michaeln wrote:
> This is a lot of new public API for this class. Which of these methods are
> really needed by what callers. I'd like to understand how this stuff will be
> used prior to putting all of this in the public section.

per our discussion, left only GetAllOriginsInfo(), DeleteDatabase(),
DeleteOrigin() and SetOriginQuota() public.

http://codereview.chromium.org/507014/diff/8017/9003
File webkit/database/databases_table.cc (right):

http://codereview.chromium.org/507014/diff/8017/9003#newcode154
webkit/database/databases_table.cc:154: return (delete_statement.Run() &&
db_->GetLastChangeCount());
On 2009/12/30 21:07:00, michaeln wrote:
> I wonder if trying to delete rows for an origin without records in the db
should
> return true or false?

i think it should return false. just like trying to update the record for an
origin that's not in the database.

http://codereview.chromium.org/507014/diff/8017/9006
File webkit/database/quota_table.cc (right):

http://codereview.chromium.org/507014/diff/8017/9006#newcode37
webkit/database/quota_table.cc:37: int64 quota) {
On 2009/12/30 21:07:00, michaeln wrote:
> maybe DCHECK(quota >= 0)?

done.

http://codereview.chromium.org/507014/diff/8017/9008
File webkit/database/quota_table.h (right):

http://codereview.chromium.org/507014/diff/8017/9008#newcode24
webkit/database/quota_table.h:24: bool SetOriginQuota(const string16&
origin_identifier, int64 quota);
On 2009/12/30 21:07:00, michaeln wrote:
> Have you considered a ClearOriginQuota method for use when the user resets an
> origin to the 'default' quota (in the UI)?

added.

http://codereview.chromium.org/507014/diff/8017/9013
File webkit/database/quota_table_unittest.cc (right):

http://codereview.chromium.org/507014/diff/8017/9013#newcode44
webkit/database/quota_table_unittest.cc:44:
EXPECT_TRUE(db.Open(temp_dir.path().Append(FILE_PATH_LITERAL("tracker.db"))));
On 2009/12/30 21:07:00, michaeln wrote:
> maybe u can use an in-memory db for these tests... db.OpenInMemory()

done.

http://codereview.chromium.org/507014/diff/8017/9013#newcode59
webkit/database/quota_table_unittest.cc:59: 
On 2009/12/30 21:07:00, michaeln wrote:
> maybe test that we only have a single row in the db here to ensure the
> insert/replace logic is correct

'origin' is a primary key in this table now, which should guarantee that there's
only one row.

[michaeln, 2010-01-05 22:29:31]

http://codereview.chromium.org/507014/diff/9041/16036
File webkit/database/database_connections.cc (right):

http://codereview.chromium.org/507014/diff/9041/16036#newcode24
webkit/database/database_connections.cc:24: DBConnections origin_connections =
origin_it->second;
This is making a copy of the the collection, i think you probably want to use a
reference here to avoid that.

http://codereview.chromium.org/507014/diff/9041/16036#newcode68
webkit/database/database_connections.cc:68:
connections_.erase(origin_identifier);
As mentioned before, there are several duplicate map lookups here (6 against one
map, 4 against another) that could de-duped. Here's a similar amount of code
that i think does the same thing with 1 lookup per map.

DCHECK(IsDatabaseOpened(id, name));

OriginConnections::const_iterator found =
    connections_by_origin_.find(origin_identifier);
DBConnections& connections = found->second;
int& count = connections[database_name];
DCHECK(count >= num_connections);
count -= num_connections;
if (!count)
  connections_by_origin_.erase(found);

http://codereview.chromium.org/507014/diff/9041/16037
File webkit/database/database_connections.h (right):

http://codereview.chromium.org/507014/diff/9041/16037#newcode27
webkit/database/database_connections.h:27: void RemoveAllConnections(const
DatabaseConnections& connections);
google c++ style generally avoids overloading methods... maybe...

a. RemoveAllConnections() or ClearConnections()
b. RemoveConnections(const DatabaseConnections& connecstions);

and for the private helper...

c. RemoveConnectionsHelper(id,name,count);

http://codereview.chromium.org/507014/diff/9041/16025
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9041/16025#newcode127
webkit/database/database_tracker.cc:127: size_t length = origins_info->size();
i think you can DCHECK(origins_info->empty()) above, and call
origins_info->clear() prior to the early return below

http://codereview.chromium.org/507014/diff/9041/16025#newcode156
webkit/database/database_tracker.cc:156: // Try to delete the file on the hard
drive.
What if the user has manually deleted the file from the file system? Are we
stuck with a row in the db that can't be deleted?

http://codereview.chromium.org/507014/diff/9041/16025#newcode343
webkit/database/database_tracker.cc:343: bool
DatabaseTracker::GetAllDatabasesForOrigin(
Is this method called by anything (other than a unittest). If not, i'd suggest
removing the method.

[dumi, 2010-01-06 00:03:22]

http://codereview.chromium.org/507014/diff/9041/16036
File webkit/database/database_connections.cc (right):

http://codereview.chromium.org/507014/diff/9041/16036#newcode24
webkit/database/database_connections.cc:24: DBConnections origin_connections =
origin_it->second;
On 2010/01/05 22:29:31, michaeln wrote:
> This is making a copy of the the collection, i think you probably want to use
a
> reference here to avoid that.

fixed.

http://codereview.chromium.org/507014/diff/9041/16036#newcode68
webkit/database/database_connections.cc:68:
connections_.erase(origin_identifier);
On 2010/01/05 22:29:31, michaeln wrote:
> As mentioned before, there are several duplicate map lookups here (6 against
one
> map, 4 against another) that could de-duped. Here's a similar amount of code
> that i think does the same thing with 1 lookup per map.
> 
> DCHECK(IsDatabaseOpened(id, name));
> 
> OriginConnections::const_iterator found =
>     connections_by_origin_.find(origin_identifier);
> DBConnections& connections = found->second;
> int& count = connections[database_name];
> DCHECK(count >= num_connections);
> count -= num_connections;
> if (!count)
>   connections_by_origin_.erase(found);

refactored.

http://codereview.chromium.org/507014/diff/9041/16037
File webkit/database/database_connections.h (right):

http://codereview.chromium.org/507014/diff/9041/16037#newcode27
webkit/database/database_connections.h:27: void RemoveAllConnections(const
DatabaseConnections& connections);
On 2010/01/05 22:29:31, michaeln wrote:
> google c++ style generally avoids overloading methods... maybe...
> 
> a. RemoveAllConnections() or ClearConnections()
> b. RemoveConnections(const DatabaseConnections& connecstions);
> 
> and for the private helper...
> 
> c. RemoveConnectionsHelper(id,name,count);

done.

http://codereview.chromium.org/507014/diff/9041/16025
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9041/16025#newcode127
webkit/database/database_tracker.cc:127: size_t length = origins_info->size();
On 2010/01/05 22:29:31, michaeln wrote:
> i think you can DCHECK(origins_info->empty()) above, and call
> origins_info->clear() prior to the early return below

done. even though it seems to me like we're adding a restriction that we could
easily work around.

http://codereview.chromium.org/507014/diff/9041/16025#newcode156
webkit/database/database_tracker.cc:156: // Try to delete the file on the hard
drive.
On 2010/01/05 22:29:31, michaeln wrote:
> What if the user has manually deleted the file from the file system? Are we
> stuck with a row in the db that can't be deleted?

yes. i think this is something for GetCachedOriginInfo() to take care of. i
updated that method to do that.

http://codereview.chromium.org/507014/diff/9041/16025#newcode343
webkit/database/database_tracker.cc:343: bool
DatabaseTracker::GetAllDatabasesForOrigin(
On 2010/01/05 22:29:31, michaeln wrote:
> Is this method called by anything (other than a unittest). If not, i'd suggest
> removing the method.

removed. also removed GetAllOrigins(), GetDatabaseSize(), GetOriginQuota() and
GetOriginUsage(), and refactored the code that used them.

[michaeln, 2010-01-06 00:44:36]

http://codereview.chromium.org/507014/diff/9041/16036
File webkit/database/database_connections.cc (right):

http://codereview.chromium.org/507014/diff/9041/16036#newcode68
webkit/database/database_connections.cc:68:
connections_.erase(origin_identifier);
On 2010/01/06 00:03:22, dumi wrote:
> On 2010/01/05 22:29:31, michaeln wrote:
> > As mentioned before, there are several duplicate map lookups here..
> refactored.

thnx

http://codereview.chromium.org/507014/diff/9042/16059
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9042/16059#newcode158
webkit/database/database_tracker.cc:158: FilePath db_file =
GetFullDBFilePath(origin_identifier, database_name);
maybe test if the file exists prior to trying to delete it

http://codereview.chromium.org/507014/diff/9042/16059#newcode287
webkit/database/database_tracker.cc:287: origin_identifier, it->database_name);
Are there windows of time where a DatabaseDetails record exists, but there is no
file on disk... as an expected condition... i'm thinking about when databases
are first created/opened? If so, this logic may not be valid since the cached
info can get dropped at arbitrary times.

[dumi, 2010-01-06 01:50:48]

http://codereview.chromium.org/507014/diff/9042/16059
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9042/16059#newcode158
webkit/database/database_tracker.cc:158: FilePath db_file =
GetFullDBFilePath(origin_identifier, database_name);
On 2010/01/06 00:44:36, michaeln wrote:
> maybe test if the file exists prior to trying to delete it

seems redundant.

http://codereview.chromium.org/507014/diff/9042/16059#newcode287
webkit/database/database_tracker.cc:287: origin_identifier, it->database_name);
On 2010/01/06 00:44:36, michaeln wrote:
> Are there windows of time where a DatabaseDetails record exists, but there is
no
> file on disk... as an expected condition... i'm thinking about when databases
> are first created/opened? If so, this logic may not be valid since the cached
> info can get dropped at arbitrary times.

good catch. we add a row to the Databases table before we write anything to a
database. reverted this change. we can worry about cleaning up the Databases
table later.

[michaeln, 2010-01-06 02:09:10]

http://codereview.chromium.org/507014/diff/9042/16059
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9042/16059#newcode158
webkit/database/database_tracker.cc:158: FilePath db_file =
GetFullDBFilePath(origin_identifier, database_name);
On 2010/01/06 01:50:51, dumi wrote:
> On 2010/01/06 00:44:36, michaeln wrote:
> > maybe test if the file exists prior to trying to delete it
> 
> seems redundant.

I don't think its redundant because the code branches based on the return value
of file_util::Delete (which returns false for files that do not exist).

Without something here, we have the bug where if the db file was deleted in the
file system (thru whatever means), it can't be deleted from the tracking system
thru this method. Seems like a proximate fix for that bug.

[dumi, 2010-01-06 02:33:10]

http://codereview.chromium.org/507014/diff/9042/16059
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/507014/diff/9042/16059#newcode158
webkit/database/database_tracker.cc:158: FilePath db_file =
GetFullDBFilePath(origin_identifier, database_name);
On 2010/01/06 02:09:10, michaeln wrote:
> On 2010/01/06 01:50:51, dumi wrote:
> > On 2010/01/06 00:44:36, michaeln wrote:
> > > maybe test if the file exists prior to trying to delete it
> > 
> > seems redundant.
> 
> I don't think its redundant because the code branches based on the return
value
> of file_util::Delete (which returns false for files that do not exist).
> 
> Without something here, we have the bug where if the db file was deleted in
the
> file system (thru whatever means), it can't be deleted from the tracking
system
> thru this method. Seems like a proximate fix for that bug.
> 

i see your point. done.

[michaeln, 2010-01-06 03:27:33]

LGTM