Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
334039:
Adding Chromium's database tracker. (Closed)
|
Created:
11 years, 1 month ago by dumi Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdding Chromium's database tracker.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30747
Patch Set 1 #Patch Set 2 : Minor change that will hopefully make gcc happy. #Patch Set 3 : Replacing base::hash_{map|set} with std::{map|set}, to avoid providing a hash function for Listener. #Patch Set 4 : Fixing deps. #Patch Set 5 : All Michael's comments should be addressed, minus database_tracker_unittest. #
Total comments: 48
Patch Set 6 : Minor change to databases_table_unittest that will hopefully make gcc happy. #Patch Set 7 : Addressed Michael's comments. #Patch Set 8 : Do not store the name of the DB files in the tracker DB. #
Total comments: 16
Patch Set 9 : Addressed Michael's comments. #
Total comments: 14
Patch Set 10 : '' #
Total comments: 8
Patch Set 11 : database_tracker_unittest.cc + minor signature changes to a couple of DatabaseTracker methods #
Total comments: 4
Patch Set 12 : Addressed Michael's latest comments. #
Total comments: 18
Patch Set 13 : '' #
Total comments: 4
Patch Set 14 : DatabaseSizeChanged --> OnDatabaseSizeChanged #Patch Set 15 : '' #
Total comments: 6
Patch Set 16 : '' #
Total comments: 2
Patch Set 17 : Final version? #
Messages
Total messages: 19 (0 generated)
before looking real close... what about these things that were mentioned last time around? 1) unit tests 2) using a meta_table to version this database 3) detangling the SQL code out of the C++ logic 4) using the ObserverList<T> class
hey... its starting to shape up http://codereview.chromium.org/334039/diff/7001/7003 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/7001/7003#newcode36 Line 36: db_->Close(); as mentioned before... i think ~Connection() will take care of Close()ing for you, so you don't need to call it here. http://codereview.chromium.org/334039/diff/7001/7003#newcode57 Line 57: if (*space_available < 0) Maybe GetOriginSpaceAvailable should not return negative values. http://codereview.chromium.org/334039/diff/7001/7003#newcode86 Line 86: ClearAllCachedOriginInfo(); code comments about why we're clearing the cache would be good http://codereview.chromium.org/334039/diff/7001/7003#newcode95 Line 95: FilePath DatabaseTracker::GetTrackerDatabaseFileName() const { Since this one-liner is only used in LazyInit, it doesn't feel like it warrants its own method, maybe just construct the path within the body of LazyInit. http://codereview.chromium.org/334039/diff/7001/7003#newcode110 Line 110: (meta_table_->GetCompatibleVersionNumber() <= kCurrentVersion); Probably want to rearrange the order to put the version checks before the Databases table creation code. The reason being don't give the Databases table a chance to muck with a database that's too new. http://codereview.chromium.org/334039/diff/7001/7003#newcode125 Line 125: databases_table_->UpdateDatabaseDetails(details); I think it would be considerably better to only call 'update' if values have changed. The reason for this is two-fold... 1) To avoid the perf hit associated with fsync'ing 2) Less surface area for tracker database corruption (the less you write, the less chance something can go wrong). In general, hitting the disk less often is a good thing. http://codereview.chromium.org/334039/diff/7001/7003#newcode150 Line 150: void DatabaseTracker::CacheDatabaseSizes(const string16& origin_identifier) { I think it would be good to move this method into the body of GetCachedOriginInfo. That way its more clear that there is exactly one code path to construct/initialize/populate one of these structures. http://codereview.chromium.org/334039/diff/7001/7003#newcode169 Line 169: DatabaseTracker::CachedOriginInfo& DatabaseTracker::GetCachedOriginInfo( Google-style generally frowns upon non-const references, this can just as easily be a ptr type. And that may even have a semantic benefit... are they're any opportunities to get an error when reading from the db in order to populate the 'info' struct... if so... a return value of NULL could indicate that class of error condition. http://codereview.chromium.org/334039/diff/7001/7003#newcode178 Line 178: return GetCachedOriginInfo(origin_identifier). wrapping on the . looks odd to my eye... maybe spring for a local variable? http://codereview.chromium.org/334039/diff/7001/7004 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/7001/7004#newcode37 Line 37: : public base::RefCountedThreadSafe<DatabaseTracker> { does this not fit on one line... just checking? http://codereview.chromium.org/334039/diff/7001/7004#newcode41 Line 41: virtual void DatabaseSizeChanged(const string16& origin_identifier, The convention for observer'y/callback'y/listener'y interfaces is to prefix the callback method names with On... OnDatabaseSizeChanged(...) ... i think that works here, wdyt? http://codereview.chromium.org/334039/diff/7001/7004#newcode59 Line 59: void DatabaseClosed(const string16& origin_identifier, until we have a use case for this, maybe nix it for now? http://codereview.chromium.org/334039/diff/7001/7004#newcode67 Line 67: const FilePath& database_directory() const { return db_dir_; } yes... const FilePath&... thank you http://codereview.chromium.org/334039/diff/7001/7004#newcode90 Line 90: FilePath GetTrackerDatabaseFileName() const; per comments in .cc file, consider removing this helper and moving the function it performs into LazyInit(). http://codereview.chromium.org/334039/diff/7001/7004#newcode98 Line 98: void CacheDatabaseSizes(const string16& origin_identifier); per comments in .cc file, consider removing this helper and moving the function it performs into GetCachedOriginInfo(). http://codereview.chromium.org/334039/diff/7001/7004#newcode111 Line 111: int64 GetOriginSpaceAvailable(const string16& origin_identifier); Can you similar'ify the naming convention for these three methods? GetOriginUsage() GetQuotaForOrigin() <--- this one is arbitrarily different GetOriginSpaceAvailable() GetUsageForOrigin() GetOriginQuota() <--- this one is arbitrarily different GetSpaceAvailableForOrigin() http://codereview.chromium.org/334039/diff/7001/7004#newcode117 Line 117: scoped_ptr<sql::MetaTable> meta_table_; versioning... thank you http://codereview.chromium.org/334039/diff/7001/7007 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/7001/7007#newcode100 Line 100: return delete_statement.Run(); similarly, maybe only return true if something was deleted? http://codereview.chromium.org/334039/diff/7001/7008 File webkit/database/databases_table.h (right): http://codereview.chromium.org/334039/diff/7001/7008#newcode26 Line 26: FilePath file_path; Maybe we don't need the 'file_path' column just yet. The c++ code more oftent than not ignores the value stored in the details record and just re-computes the path as needed. Eventually, i suspect we'll have a layer of indirection here (where the actually file_path is not necesarilly computable from <origin/databaseName>)... but that day is not today. http://codereview.chromium.org/334039/diff/7001/7008#newcode38 Line 38: bool UpdateDatabaseDetails(const DatabaseDetails& details); as mentioned in the unittest.cc file, i think returning 'false' when the caller tries to update a row that doesn't exist would be an improvement http://codereview.chromium.org/334039/diff/7001/7009 File webkit/database/databases_table_unittest.cc (right): http://codereview.chromium.org/334039/diff/7001/7009#newcode16 Line 16: const DatabaseDetails& d2) { indent is off http://codereview.chromium.org/334039/diff/7001/7009#newcode26 Line 26: SQL_FROM_HERE, "SELECT * FROM Databases;")); Maybe "SELECT Count(*) FROM Databases" for this query? As coded, i think some other kinds of errors could cause a false positive. nit: the trailing ';' is not needed within the sql string, here and throughout including in the databases_table.cc file, i think omitting them makes the SQL (when wrapped up in c++ constructs) more readable. http://codereview.chromium.org/334039/diff/7001/7009#newcode52 Line 52: EXPECT_TRUE(databases_table.UpdateDatabaseDetails(details_in1)); The return value of 'true' is strange for this method in this case. I understand the update statement succeeded, but just that no rows matched. Maybe the return value should AND in (db.GetLastChangeCount() == 1). Wdyt? http://codereview.chromium.org/334039/diff/7001/7009#newcode68 Line 68: // Insert details for another database with the same origin A test that tries (and fails) to insert a duplicate row would be good. http://codereview.chromium.org/334039/diff/7001/7009#newcode90 Line 90: EXPECT_EQ(details_out_origin3.size(), 0); gcc will complain about signed/unsigned comparisons here... one option... size_t(0) also the EXPECT_EQ macro wants... EXPECT_EQ(expected_value, actual_value) ... so the test case output reads more accurately http://codereview.chromium.org/334039/diff/7001/7009#newcode100 Line 100: DatabaseDetailsAreEqual(details_out_origin1[1], details_in1))); too bad about the sort order... i wonder if you are guaranteed a particular order based on the index your using here (origin,name)... another option would be to ORDER BY so you get the results back in a known order... since this is how they're indexed, i don't think there would be any performance penalty to ORDER BY'ing? http://codereview.chromium.org/334039/diff/7001/7009#newcode110 Line 110: db.Close(); If you rearrange the order of the locals in this method, you can avoid having to call db.Close() here... ScopedTempDir temp_dir; sql::Connection db; ... declared in that order, the ~Connection will run first when the stack unwinds, and ~Connection will Close file connection automatically.
http://codereview.chromium.org/334039/diff/7001/7003 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/7001/7003#newcode36 Line 36: db_->Close(); On 2009/10/28 18:23:05, michaeln wrote: > as mentioned before... i think ~Connection() will take care of Close()ing for > you, so you don't need to call it here. removed. http://codereview.chromium.org/334039/diff/7001/7003#newcode57 Line 57: if (*space_available < 0) On 2009/10/28 18:23:05, michaeln wrote: > Maybe GetOriginSpaceAvailable should not return negative values. done. http://codereview.chromium.org/334039/diff/7001/7003#newcode86 Line 86: ClearAllCachedOriginInfo(); On 2009/10/28 18:23:05, michaeln wrote: > code comments about why we're clearing the cache would be good done. http://codereview.chromium.org/334039/diff/7001/7003#newcode95 Line 95: FilePath DatabaseTracker::GetTrackerDatabaseFileName() const { On 2009/10/28 18:23:05, michaeln wrote: > Since this one-liner is only used in LazyInit, it doesn't feel like it warrants > its own method, maybe just construct the path within the body of LazyInit. done. http://codereview.chromium.org/334039/diff/7001/7003#newcode110 Line 110: (meta_table_->GetCompatibleVersionNumber() <= kCurrentVersion); On 2009/10/28 18:23:05, michaeln wrote: > Probably want to rearrange the order to put the version checks before the > Databases table creation code. The reason being don't give the Databases table a > chance to muck with a database that's too new. done. http://codereview.chromium.org/334039/diff/7001/7003#newcode125 Line 125: databases_table_->UpdateDatabaseDetails(details); On 2009/10/28 18:23:05, michaeln wrote: > I think it would be considerably better to only call 'update' if values have > changed. The reason for this is two-fold... > > 1) To avoid the perf hit associated with fsync'ing > 2) Less surface area for tracker database corruption (the less you write, the > less chance something can go wrong). > > In general, hitting the disk less often is a good thing. > fixed. http://codereview.chromium.org/334039/diff/7001/7003#newcode150 Line 150: void DatabaseTracker::CacheDatabaseSizes(const string16& origin_identifier) { On 2009/10/28 18:23:05, michaeln wrote: > I think it would be good to move this method into the body of > GetCachedOriginInfo. That way its more clear that there is exactly one code path > to construct/initialize/populate one of these structures. > done. http://codereview.chromium.org/334039/diff/7001/7003#newcode169 Line 169: DatabaseTracker::CachedOriginInfo& DatabaseTracker::GetCachedOriginInfo( On 2009/10/28 18:23:05, michaeln wrote: > Google-style generally frowns upon non-const references, this can just as easily > be a ptr type. > > And that may even have a semantic benefit... are they're any opportunities to > get an error when reading from the db in order to populate the 'info' struct... > if so... a return value of NULL could indicate that class of error condition. done. i don't think there's any "error condition" when populating the cache: if we can't get the list of databases for this origin from the DB table, we'll just create an "empty" record. http://codereview.chromium.org/334039/diff/7001/7003#newcode178 Line 178: return GetCachedOriginInfo(origin_identifier). On 2009/10/28 18:23:05, michaeln wrote: > wrapping on the . looks odd to my eye... maybe spring for a local variable? done. http://codereview.chromium.org/334039/diff/7001/7007 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/7001/7007#newcode100 Line 100: return delete_statement.Run(); On 2009/10/28 18:23:05, michaeln wrote: > similarly, maybe only return true if something was deleted? done. http://codereview.chromium.org/334039/diff/7001/7008 File webkit/database/databases_table.h (right): http://codereview.chromium.org/334039/diff/7001/7008#newcode26 Line 26: FilePath file_path; On 2009/10/28 18:23:05, michaeln wrote: > Maybe we don't need the 'file_path' column just yet. The c++ code more oftent > than not ignores the value stored in the details record and just re-computes the > path as needed. > > Eventually, i suspect we'll have a layer of indirection here (where the actually > file_path is not necesarilly computable from <origin/databaseName>)... but that > day is not today. i think we need file_path. maybe not in this structure, but i think we need it somewhere. let's talk about this. http://codereview.chromium.org/334039/diff/7001/7008#newcode38 Line 38: bool UpdateDatabaseDetails(const DatabaseDetails& details); On 2009/10/28 18:23:05, michaeln wrote: > as mentioned in the unittest.cc file, i think returning 'false' when the caller > tries to update a row that doesn't exist would be an improvement done. http://codereview.chromium.org/334039/diff/7001/7009 File webkit/database/databases_table_unittest.cc (right): http://codereview.chromium.org/334039/diff/7001/7009#newcode16 Line 16: const DatabaseDetails& d2) { On 2009/10/28 18:23:05, michaeln wrote: > indent is off fixed. http://codereview.chromium.org/334039/diff/7001/7009#newcode26 Line 26: SQL_FROM_HERE, "SELECT * FROM Databases;")); On 2009/10/28 18:23:05, michaeln wrote: > Maybe "SELECT Count(*) FROM Databases" for this query? As coded, i think some > other kinds of errors could cause a false positive. done. > nit: the trailing ';' is not needed within the sql string, here and throughout > including in the databases_table.cc file, i think omitting them makes the SQL > (when wrapped up in c++ constructs) more readable. done. http://codereview.chromium.org/334039/diff/7001/7009#newcode52 Line 52: EXPECT_TRUE(databases_table.UpdateDatabaseDetails(details_in1)); On 2009/10/28 18:23:05, michaeln wrote: > The return value of 'true' is strange for this method in this case. I understand > the update statement succeeded, but just that no rows matched. Maybe the return > value should AND in (db.GetLastChangeCount() == 1). Wdyt? changed to EXPECT_FALSE and changed UpdateDatabaseDetails() to return false if we couldn't find any record to update. http://codereview.chromium.org/334039/diff/7001/7009#newcode68 Line 68: // Insert details for another database with the same origin On 2009/10/28 18:23:05, michaeln wrote: > A test that tries (and fails) to insert a duplicate row would be good. done. http://codereview.chromium.org/334039/diff/7001/7009#newcode90 Line 90: EXPECT_EQ(details_out_origin3.size(), 0); On 2009/10/28 18:23:05, michaeln wrote: > gcc will complain about signed/unsigned comparisons here... one option... > size_t(0) > > also the EXPECT_EQ macro wants... > EXPECT_EQ(expected_value, actual_value) > ... so the test case output reads more accurately fixed. http://codereview.chromium.org/334039/diff/7001/7009#newcode100 Line 100: DatabaseDetailsAreEqual(details_out_origin1[1], details_in1))); On 2009/10/28 18:23:05, michaeln wrote: > too bad about the sort order... i wonder if you are guaranteed a particular > order based on the index your using here (origin,name)... another option would > be to ORDER BY so you get the results back in a known order... since this is how > they're indexed, i don't think there would be any performance penalty to ORDER > BY'ing? done. added a ORDER BY clause to GetAllDatabaseDetailsForOrigin(). http://codereview.chromium.org/334039/diff/7001/7009#newcode110 Line 110: db.Close(); On 2009/10/28 18:23:05, michaeln wrote: > If you rearrange the order of the locals in this method, you can avoid having to > call db.Close() here... > > ScopedTempDir temp_dir; > sql::Connection db; > > ... declared in that order, the ~Connection will run first when the stack > unwinds, and ~Connection will Close file connection automatically. done. i thought about the same, but wasn't sure the destruction order was guaranteed to be the inverse order in which the objects were created.
http://codereview.chromium.org/334039/diff/7001/7003 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/7001/7003#newcode169 Line 169: DatabaseTracker::CachedOriginInfo& DatabaseTracker::GetCachedOriginInfo( "i don't think there's any "error condition"" "if we can't get the list of databases for this origin" But, you just described an error condition? http://codereview.chromium.org/334039/diff/14001/15002#newcode138 Line 138: return FilePath::FromWStringHack(UTF16ToWide( Do you think this is more readable? void DatabaseTracker::InsertOrUpdateDatabaseDetails( const string16& origin_identifier, const string16& database_name, const string16& database_description, int64 estimated_size) { // Insert or update the details if we need to. DatabaseDetails details; if (!databases_table_.GetDatabaseDetails( origin_identifier, database_name, &details)) { details.origin_identifier = origin_identifier; details.database_name = database_name; details.description = database_description; details.estimated_size = estimated_size; databases_table_->InsertDatabaseDetails(details); } else if (details.estimated_size != estimated_size || details.description != description) { details.description = database_description; details.estimated_size = estimated_size; databases_table_->UpdateDatabaseDetails(details); } } I think that should look somewhat familiar to you? http://codereview.chromium.org/334039/diff/14001/15002#newcode147 Line 147: return db_file_size; since both of your callsites for GetDBFileSize look like... GetDBFileSize(GetDBFileName(origin, name)) maybe change this method signature to... int64 GetDBFileSize(origin, name) and have it compute the file_name and full_path internally That way, with the exception of GetDBFileSize, the c++ logic never refers to the file_name or full_file_path. It always uses the tuple <origin, database_name> to identify which database is being thought about. http://codereview.chromium.org/334039/diff/14001/15006 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/14001/15006#newcode46 Line 46: if (details) { I think 'details' can be a required parameter. DCHECK(details) up top. http://codereview.chromium.org/334039/diff/14001/15006#newcode74 Line 74: if (!GetDatabaseDetails(details.origin_identifier, oh... you don't actually want to run two statements for this operation... i think db.GetLastChangeCount, as mentioned before, will tell you if an update did anything or not? http://codereview.chromium.org/334039/diff/14001/15006#newcode95 Line 95: if (!GetDatabaseDetails(origin_identifier, database_name, NULL)) ditto about running two statements http://codereview.chromium.org/334039/diff/14001/15008 File webkit/database/databases_table_unittest.cc (right): http://codereview.chromium.org/334039/diff/14001/15008#newcode33 Line 33: SQL_FROM_HERE, "SELECT * FROM Databases")); I thought you were changing this to SELECT COUNT(*)? http://codereview.chromium.org/334039/diff/14001/15008#newcode43 Line 43: db.set_error_delegate(new TestErrorDelegate()); What is the significance of the error delegate? And where is it deleted? Do the tests not pass unless there is an error delegate? http://codereview.chromium.org/334039/diff/14001/15008#newcode102 Line 102: EXPECT_EQ(size_t(0), details_out_origin3.size()); could also use the bool empty() method here
http://codereview.chromium.org/334039/diff/7001/7003 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/7001/7003#newcode169 Line 169: DatabaseTracker::CachedOriginInfo& DatabaseTracker::GetCachedOriginInfo( On 2009/10/29 00:32:27, michaeln wrote: > "i don't think there's any "error condition"" > "if we can't get the list of databases for this origin" > > But, you just described an error condition? yeah, i guess we could return NULL in case of a SQL error -- we'd need to install a custom error handler for that though, so we should probably leave that for later, since we don't even know what errors we wanna catch and how to handle them. http://codereview.chromium.org/334039/diff/14001/15002#newcode138 Line 138: return FilePath::FromWStringHack(UTF16ToWide( On 2009/10/29 00:32:27, michaeln wrote: > Do you think this is more readable? > > void DatabaseTracker::InsertOrUpdateDatabaseDetails( > const string16& origin_identifier, > const string16& database_name, > const string16& database_description, > int64 estimated_size) { > // Insert or update the details if we need to. > DatabaseDetails details; > if (!databases_table_.GetDatabaseDetails( > origin_identifier, database_name, &details)) { > details.origin_identifier = origin_identifier; > details.database_name = database_name; > details.description = database_description; > details.estimated_size = estimated_size; > databases_table_->InsertDatabaseDetails(details); > } else if (details.estimated_size != estimated_size || > details.description != description) { > details.description = database_description; > details.estimated_size = estimated_size; > databases_table_->UpdateDatabaseDetails(details); > } > } > > I think that should look somewhat familiar to you? changed. http://codereview.chromium.org/334039/diff/14001/15002#newcode147 Line 147: return db_file_size; On 2009/10/29 00:32:27, michaeln wrote: > since both of your callsites for GetDBFileSize look like... > GetDBFileSize(GetDBFileName(origin, name)) > maybe change this method signature to... > int64 GetDBFileSize(origin, name) > and have it compute the file_name and full_path internally > > That way, with the exception of GetDBFileSize, the c++ logic never refers to > the file_name or full_file_path. It always uses the tuple <origin, > database_name> to identify which database is being thought about. > done. http://codereview.chromium.org/334039/diff/14001/15006 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/14001/15006#newcode46 Line 46: if (details) { On 2009/10/29 00:32:27, michaeln wrote: > I think 'details' can be a required parameter. DCHECK(details) up top. done. http://codereview.chromium.org/334039/diff/14001/15006#newcode74 Line 74: if (!GetDatabaseDetails(details.origin_identifier, On 2009/10/29 00:32:27, michaeln wrote: > oh... you don't actually want to run two statements for this operation... i > think db.GetLastChangeCount, as mentioned before, will tell you if an update did > anything or not? done. http://codereview.chromium.org/334039/diff/14001/15006#newcode95 Line 95: if (!GetDatabaseDetails(origin_identifier, database_name, NULL)) On 2009/10/29 00:32:27, michaeln wrote: > ditto about running two statements done. http://codereview.chromium.org/334039/diff/14001/15008 File webkit/database/databases_table_unittest.cc (right): http://codereview.chromium.org/334039/diff/14001/15008#newcode33 Line 33: SQL_FROM_HERE, "SELECT * FROM Databases")); On 2009/10/29 00:32:27, michaeln wrote: > I thought you were changing this to SELECT COUNT(*)? oops, done now. http://codereview.chromium.org/334039/diff/14001/15008#newcode43 Line 43: db.set_error_delegate(new TestErrorDelegate()); On 2009/10/29 00:32:27, michaeln wrote: > What is the significance of the error delegate? And where is it deleted? Do the > tests not pass unless there is an error delegate? we need to set an error delegate to not crash the test when we encounter a sql error (in debug mode). ErrorDelegate is refcounted, and the Connection class stores it in a scoped_refptr<>, so it should be ok as far as memory goes. made the changes we talked about. http://codereview.chromium.org/334039/diff/14001/15008#newcode102 Line 102: EXPECT_EQ(size_t(0), details_out_origin3.size()); On 2009/10/29 00:32:27, michaeln wrote: > could also use the bool empty() method here done.
Are you still planning on adding 'tracker' unittests? > install a custom error handler for that though Not really, see comment about statement.Succeeded() http://codereview.chromium.org/334039/diff/10007/10009 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/10007/10009#newcode83 Line 83: // when we remove a listener, we do not know which cached information When http://codereview.chromium.org/334039/diff/10007/10009#newcode85 Line 85: // clear all caches and re-populate them as needed as needed. http://codereview.chromium.org/334039/diff/10007/10009#newcode101 Line 101: // 4. the Databases table exists or is successfully created this comment doesn't really add any value, the code is easy enough to read http://codereview.chromium.org/334039/diff/10007/10010 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/10007/10010#newcode45 Line 45: virtual ~Observer() { } nit: no need for a space between the brackets in chrome-style http://codereview.chromium.org/334039/diff/10007/10010#newcode104 Line 104: const string16& database_name) const; indent is off http://codereview.chromium.org/334039/diff/10007/10010#newcode107 Line 107: int64 GetOriginSpaceAvailable(const string16& origin_identifier); nit: similar'ify names, or is there a compelling reason why one should be unlike the others? http://codereview.chromium.org/334039/diff/10007/10013 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/10007/10013#newcode116 Line 116: return true; You can return statement.Succeeded() here to distinguish between no results and an error when running the SELECT.
Please take a look at the lint errors too.
Definitely planning to add a unittest for database_tracker; hopefully it will happen today... http://codereview.chromium.org/334039/diff/10007/10009 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/10007/10009#newcode83 Line 83: // when we remove a listener, we do not know which cached information On 2009/10/29 01:59:01, michaeln wrote: > When done. http://codereview.chromium.org/334039/diff/10007/10009#newcode85 Line 85: // clear all caches and re-populate them as needed On 2009/10/29 01:59:01, michaeln wrote: > as needed. done. http://codereview.chromium.org/334039/diff/10007/10009#newcode101 Line 101: // 4. the Databases table exists or is successfully created On 2009/10/29 01:59:01, michaeln wrote: > this comment doesn't really add any value, the code is easy enough to read removed. http://codereview.chromium.org/334039/diff/10007/10010 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/10007/10010#newcode45 Line 45: virtual ~Observer() { } On 2009/10/29 01:59:01, michaeln wrote: > nit: no need for a space between the brackets in chrome-style removed. http://codereview.chromium.org/334039/diff/10007/10010#newcode104 Line 104: const string16& database_name) const; On 2009/10/29 01:59:01, michaeln wrote: > indent is off fixed. http://codereview.chromium.org/334039/diff/10007/10010#newcode107 Line 107: int64 GetOriginSpaceAvailable(const string16& origin_identifier); On 2009/10/29 01:59:01, michaeln wrote: > nit: similar'ify names, or is there a compelling reason why one should be unlike > the others? GetQuotaForOrigin --> GetOriginQuota. http://codereview.chromium.org/334039/diff/10007/10013 File webkit/database/databases_table.cc (right): http://codereview.chromium.org/334039/diff/10007/10013#newcode116 Line 116: return true; On 2009/10/29 01:59:01, michaeln wrote: > You can return statement.Succeeded() here to distinguish between no results and > an error when running the SELECT. done. and changed database_tracker to check for this result.
getting pretty close http://codereview.chromium.org/334039/diff/11003/11005 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/11003/11005#newcode142 Line 142: DatabaseTracker::CachedOriginInfo* DatabaseTracker::GetCachedOriginInfo( This method could return NULL if the info can't be read from the DB. There are three callsites for this method that would have to check for a NULL return value and do something appropiate. As coded, when the info for an origin can't be read, the system doesn't have any idea how much data the origin actually has on disk, and it will allow them to accrue more than they should be able to. http://codereview.chromium.org/334039/diff/11003/11005#newcode166 Line 166: DCHECK(origin_info); for now... maybe... if (!origin_info) return 0; http://codereview.chromium.org/334039/diff/11003/11005#newcode176 Line 176: DCHECK(origin_info); for now... maybe... if (!origin_info) return true; http://codereview.chromium.org/334039/diff/11003/11005#newcode182 Line 182: DCHECK(origin_info); for now... maybe... if (!origin_info) return kint32max; ... this would result in the system preventing new data from being written to disk when the tracker DB is busted
http://codereview.chromium.org/334039/diff/11003/11005 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/11003/11005#newcode142 Line 142: DatabaseTracker::CachedOriginInfo* DatabaseTracker::GetCachedOriginInfo( On 2009/10/30 01:08:25, michaeln wrote: > This method could return NULL if the info can't be read from the DB. There are > three callsites for this method that would have to check for a NULL return value > and do something appropiate. As coded, when the info for an origin can't be > read, the system doesn't have any idea how much data the origin actually has on > disk, and it will allow them to accrue more than they should be able to. returning NULL if databases_table_->GetAllDatabaseDetailsForOrigin() returns false. http://codereview.chromium.org/334039/diff/11003/11005#newcode166 Line 166: DCHECK(origin_info); On 2009/10/30 01:08:25, michaeln wrote: > for now... maybe... > > if (!origin_info) > return 0; done. http://codereview.chromium.org/334039/diff/11003/11005#newcode176 Line 176: DCHECK(origin_info); On 2009/10/30 01:08:25, michaeln wrote: > for now... maybe... > > if (!origin_info) > return true; this method now returns a int64 (the new file size), so it can return new_size even if the cache could not be updated. http://codereview.chromium.org/334039/diff/11003/11005#newcode182 Line 182: DCHECK(origin_info); On 2009/10/30 01:08:25, michaeln wrote: > for now... maybe... > > if (!origin_info) > return kint32max; > > ... this would result in the system preventing new data from being written to > disk when the tracker DB is busted returning the max positive integer that fits in a int64.
http://codereview.chromium.org/334039/diff/16010/11024 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/16010/11024#newcode15 Line 15: class TestObserver : public DatabaseTracker::Observer { would be good to put TestObserver in an anon namespace, this name could very easily collide. Additionally the local helper function, CheckNotificationReceived, should also go in that anon namespace. http://codereview.chromium.org/334039/diff/16010/11024#newcode19 Line 19: virtual void DatabaseSizeChanged(const string16& origin_identifier, ah... OnFoo naming for callbacks? http://codereview.chromium.org/334039/diff/8013/13006 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/8013/13006#newcode100 Line 100: ASCIIToUTF16(".db")))); now can we remove the .db extension? http://codereview.chromium.org/334039/diff/8013/13006#newcode155 Line 155: CachedOriginInfo& origin_info = origins_info_map_[origin_identifier]; almost... this line right here will insert an empty info struct, so on the next GetCacheOriginInfo call for this origin, it will return a ptr to the empty struct instead of null. please move this line down to line 161 (right before the for loop in which the info structure is poked at). http://codereview.chromium.org/334039/diff/8013/13006#newcode195 Line 195: return (int64(1) << 63) - 1; // max positive number that fits in an int64 almost... there is a constant for this value in basictypes.h... please use it here http://codereview.chromium.org/334039/diff/8013/13007 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/8013/13007#newcode68 Line 68: const int64 DefaultOriginQuota() const { return default_origin_quota_; } I think you've made this public solely for the unittests, see comments there about removing this new method. http://codereview.chromium.org/334039/diff/8013/13007#newcode80 Line 80: int64 SetCachedDatabaseSize(const string16& database_name, int64 new_size) { i don't see any callsites that depend on this 'old_size' return value... maybe this should be just void SetCachedDatabaseSize(...) http://codereview.chromium.org/334039/diff/8013/13007#newcode113 Line 113: const int64 default_origin_quota_; if the new method is removed... this too can be removed http://codereview.chromium.org/334039/diff/8013/13013 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/8013/13013#newcode62 Line 62: TEST(DatabaseTrackerTest, DatabaseTrackerTest) { nit: naming... TEST(DatabaseTrackerTest, TestIt) http://codereview.chromium.org/334039/diff/8013/13013#newcode67 Line 67: const int64 kDefaultQuota = tracker->DefaultOriginQuota(); Instead of inventing a new public method for this (with the unittest being the only use case), you can make this test a friend of the DatabaseTracker (using the FRIEND_TEST(DatabaseTrackerTest, TestIt) macro). Then you can call... const int64 kDefaultQuota = tracker->GetOriginQuota(kOrigin1); ... without having to introduce anything new in the class interface for this. http://codereview.chromium.org/334039/diff/8013/13013#newcode84 Line 84: string16 description = ASCIIToUTF16("database_description"); can origin1 thru description be const string16's? i don't think you need to alter them below?
http://codereview.chromium.org/334039/diff/16010/11024 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/16010/11024#newcode15 Line 15: class TestObserver : public DatabaseTracker::Observer { On 2009/10/30 04:36:56, michaeln wrote: > would be good to put TestObserver in an anon namespace, this name could very > easily collide. Additionally the local helper function, > CheckNotificationReceived, should also go in that anon namespace. > done. http://codereview.chromium.org/334039/diff/16010/11024#newcode19 Line 19: virtual void DatabaseSizeChanged(const string16& origin_identifier, On 2009/10/30 04:36:56, michaeln wrote: > ah... OnFoo naming for callbacks? that's the name of the method in DatabaseTracker::Observer. do we want to change it there too? http://codereview.chromium.org/334039/diff/8013/13006 File webkit/database/database_tracker.cc (right): http://codereview.chromium.org/334039/diff/8013/13006#newcode100 Line 100: ASCIIToUTF16(".db")))); On 2009/10/30 04:36:56, michaeln wrote: > now can we remove the .db extension? done. http://codereview.chromium.org/334039/diff/8013/13006#newcode155 Line 155: CachedOriginInfo& origin_info = origins_info_map_[origin_identifier]; On 2009/10/30 04:36:56, michaeln wrote: > almost... this line right here will insert an empty info struct, so on the next > GetCacheOriginInfo call for this origin, it will return a ptr to the empty > struct instead of null. > > please move this line down to line 161 (right before the for loop in which the > info structure is poked at). done. good catch. http://codereview.chromium.org/334039/diff/8013/13006#newcode195 Line 195: return (int64(1) << 63) - 1; // max positive number that fits in an int64 On 2009/10/30 04:36:56, michaeln wrote: > almost... there is a constant for this value in basictypes.h... please use it > here replaced. http://codereview.chromium.org/334039/diff/8013/13007 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/8013/13007#newcode68 Line 68: const int64 DefaultOriginQuota() const { return default_origin_quota_; } On 2009/10/30 04:36:56, michaeln wrote: > I think you've made this public solely for the unittests, see comments there > about removing this new method. yes. removed. http://codereview.chromium.org/334039/diff/8013/13007#newcode80 Line 80: int64 SetCachedDatabaseSize(const string16& database_name, int64 new_size) { On 2009/10/30 04:36:56, michaeln wrote: > i don't see any callsites that depend on this 'old_size' return value... maybe > this should be just > > void SetCachedDatabaseSize(...) done. i was thinking of void vs int64 and thought that returning the old value would be more in line with what some sets/maps methods do, but since we never use this value, void is probably more appropriate. http://codereview.chromium.org/334039/diff/8013/13007#newcode113 Line 113: const int64 default_origin_quota_; On 2009/10/30 04:36:56, michaeln wrote: > if the new method is removed... this too can be removed done. http://codereview.chromium.org/334039/diff/8013/13013 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/8013/13013#newcode62 Line 62: TEST(DatabaseTrackerTest, DatabaseTrackerTest) { On 2009/10/30 04:36:56, michaeln wrote: > nit: naming... TEST(DatabaseTrackerTest, TestIt) done here and in databases_table_unittest.cc. http://codereview.chromium.org/334039/diff/8013/13013#newcode67 Line 67: const int64 kDefaultQuota = tracker->DefaultOriginQuota(); On 2009/10/30 04:36:56, michaeln wrote: > Instead of inventing a new public method for this (with the unittest being the > only use case), you can make this test a friend of the DatabaseTracker (using > the FRIEND_TEST(DatabaseTrackerTest, TestIt) macro). > > Then you can call... > > const int64 kDefaultQuota = tracker->GetOriginQuota(kOrigin1); > > ... without having to introduce anything new in the class interface for this. done. http://codereview.chromium.org/334039/diff/8013/13013#newcode84 Line 84: string16 description = ASCIIToUTF16("database_description"); On 2009/10/30 04:36:56, michaeln wrote: > can origin1 thru description be const string16's? i don't think you need to > alter them below? done.
http://codereview.chromium.org/334039/diff/8019/17004 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/8019/17004#newcode50 Line 50: virtual ~DatabaseTracker(); shouldn't need to be virtual http://codereview.chromium.org/334039/diff/8019/17004#newcode73 Line 73: int64 GetOriginQuota(const string16& origin_identifier) const; please put this back with the other GetOriginFoo methods in the private section again, there's no need to muck with the class interface to put together your unit tests (other than to declare FRIEND_TESTs as needed)
http://codereview.chromium.org/334039/diff/8019/17004 File webkit/database/database_tracker.h (right): http://codereview.chromium.org/334039/diff/8019/17004#newcode50 Line 50: virtual ~DatabaseTracker(); On 2009/10/30 23:35:38, michaeln wrote: > shouldn't need to be virtual removed. http://codereview.chromium.org/334039/diff/8019/17004#newcode73 Line 73: int64 GetOriginQuota(const string16& origin_identifier) const; On 2009/10/30 23:35:38, michaeln wrote: > please put this back with the other GetOriginFoo methods in the private section > > again, there's no need to muck with the class interface to put together your > unit tests (other than to declare FRIEND_TESTs as needed) done. forgot to revert those changes.
http://codereview.chromium.org/334039/diff/11030/16021 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/11030/16021#newcode87 Line 87: const string16 origin3 = ASCIIToUTF16("origin1"); // same as origin1 why the duplicate constant value? http://codereview.chromium.org/334039/diff/11030/16021#newcode91 Line 91: const string16 description = ASCIIToUTF16("database_description"); all of these const string16 should be using the kNaming convention
http://codereview.chromium.org/334039/diff/11030/16021 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/11030/16021#newcode87 Line 87: const string16 origin3 = ASCIIToUTF16("origin1"); // same as origin1 On 2009/10/31 00:08:46, michaeln wrote: > why the duplicate constant value? it allows us to always pair dbi with origini (vs. db3 with origin1). http://codereview.chromium.org/334039/diff/11030/16021#newcode91 Line 91: const string16 description = ASCIIToUTF16("database_description"); On 2009/10/31 00:08:46, michaeln wrote: > all of these const string16 should be using the kNaming convention done.
modulo minor style issue, LGTM http://codereview.chromium.org/334039/diff/11030/16021 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/11030/16021#newcode87 Line 87: const string16 origin3 = ASCIIToUTF16("origin1"); // same as origin1 > > why the duplicate constant value? > it allows us to always pair dbi with origini (vs. db3 with origin1). nit: I see, fwiw, it doesn't lend clarity really... // origins kFruits = 'fruits'; kVegetables = 'vegetables'; // database names kApple = 'apple'; kSpinach = 'spinach'; kOrange = 'orange'; http://codereview.chromium.org/334039/diff/12030/6030#newcode30 Line 30: bool temp_new_notification_received_ = new_notification_received_; don't need the underbar on the local var
http://codereview.chromium.org/334039/diff/11030/16021 File webkit/database/database_tracker_unittest.cc (right): http://codereview.chromium.org/334039/diff/11030/16021#newcode87 Line 87: const string16 origin3 = ASCIIToUTF16("origin1"); // same as origin1 On 2009/10/31 00:52:01, michaeln wrote: > > > why the duplicate constant value? > > it allows us to always pair dbi with origini (vs. db3 with origin1). > > nit: I see, fwiw, it doesn't lend clarity really... > > // origins > kFruits = 'fruits'; > kVegetables = 'vegetables'; > > // database names > kApple = 'apple'; > kSpinach = 'spinach'; > kOrange = 'orange'; removed kOrigin3. http://codereview.chromium.org/334039/diff/12030/6030#newcode30 Line 30: bool temp_new_notification_received_ = new_notification_received_; On 2009/10/31 00:52:01, michaeln wrote: > don't need the underbar on the local var removed.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
