Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9159020:
Create a class to represent a DOM Storage Database. (Closed)
|
Created:
8 years, 11 months ago by benm (inactive) Modified:
8 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dpranke-watch+content_chromium.org, marja Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionCreate a class to represent a Dom Storage Database.
Add a class that can read/write a local storage database in the same format as WebCore's StorageArea.
Also add a method to sql::Statement to query the declared type of a column and a method to return a string16 from a Blob.
BUG=106763
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121442
Patch Set 1 #
Total comments: 13
Patch Set 2 : Add DomStorageDatabase class and unit tests. #Patch Set 3 : Add DOM Storage database class. #
Total comments: 12
Patch Set 4 : DomStorageDatabase for backing local storage to disk. #
Total comments: 20
Patch Set 5 : Update API to a single function for committing changes #Patch Set 6 : Remove some unused includes. #
Total comments: 34
Patch Set 7 : Address comments and add a new test for failed DB upgrade. #Patch Set 8 : Add bug # to description, address some try bot complaints #Patch Set 9 : Disable test that uses the webcore localstorage file pending the test data getting checked in. #
Total comments: 37
Patch Set 10 : Address comments and add handling of a corrupt database file in LazyOpen #
Total comments: 23
Patch Set 11 : Address review comments, streamline tests and allow them to operate on in-memory databases. #
Total comments: 1
Patch Set 12 : Addressed a couple of things I noticed :) #
Total comments: 1
Patch Set 13 : Rebase gypi files. #Patch Set 14 : Tidy up. #
Total comments: 15
Patch Set 15 : Restructure LazyOpen and add DetectSchemaVersion. #Patch Set 16 : Prune some comments. #Patch Set 17 : Disable the WebCore DB test to keep try bots happy while the binary test data is landed #
Total comments: 16
Patch Set 18 : Address review comments #Patch Set 19 : Add missing binary file from r121442 #Messages
Total messages: 37 (0 generated)
http://codereview.chromium.org/9159020/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9159020/diff/1/chrome/chrome_tests.gypi#newcod... chrome/chrome_tests.gypi:2102: '../webkit/dom_storage/local_storage_database_unittest.cc', we should put tests in test_shell_tests, see webkit/tools/test_shell/test_shell.gypi http://codereview.chromium.org/9159020/diff/1/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/9159020/diff/1/content/content_browser.gypi#ne... content/content_browser.gypi:312: '../webkit/dom_storage/local_storage_database.cc', we should create a new dom_storage library for this, i've got a CL up that does that, see http://codereview.chromium.org/9146025/ http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:31: bool is_open = db_->Open(file_path_); WebCore uses sqlite3_open16 where as the sql::Connection class uses sqlite3_open. That difference changes the default encoding used by the database. http://www.sqlite.org/c3ref/open.html If we're to do this first round w/o changing data formats (that would be my vote), we want to avoid switching the default encoding for newly created db files. The encoding can be set explicitly with a PRAGMA statement after sqlite3_open has been called. I think PRAGMA encoding = "UTF-16" would match webcore is currently creating these files. http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:36: meta_table_.Init(db_.get(), 1, 1); ... similarly, since this isn't in the current schema of the existing files, i think we should avoid adding the 'meta' table. http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:120: bool LocalStorageDatabase::UpgradeVersion1To2IfNeeded() { Code to utilize the new schema has been in used for a long time now (landed on July 15 2011). We might be able to skip upgrading existing data and start afresh if we encounter on old file? http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.h (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.h:29: bool WriteAllValues(const std::map<string16, string16>& values); We should figure out the read/write access patterns better prior to nailing this interface down. I think ReadAll() is all we'll want on the read side of things, but not so sure about the WriteAll()?
Thanks for taking a look Michael! I've got an update in progress, will upload soon. Also been working on a DomStorageNamespace that will use one of these to try and help with the API this class exposes. http://codereview.chromium.org/9159020/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9159020/diff/1/chrome/chrome_tests.gypi#newcod... chrome/chrome_tests.gypi:2102: '../webkit/dom_storage/local_storage_database_unittest.cc', sg, thanks http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:31: bool is_open = db_->Open(file_path_); On 2012/01/24 00:18:16, michaeln wrote: > WebCore uses sqlite3_open16 where as the sql::Connection class uses > sqlite3_open. That difference changes the default encoding used by the database. > http://www.sqlite.org/c3ref/open.html > Good spot! > If we're to do this first round w/o changing data formats (that would be my > vote), Agreed. we want to avoid switching the default encoding for newly created db > files. The encoding can be set explicitly with a PRAGMA statement after > sqlite3_open has been called. I think PRAGMA encoding = "UTF-16" would match > webcore is currently creating these files. Cool, will do. So far the unit tests just verify the class in isolation, I'd like to get hold of a WebCore style database for a test and make sure that this class is happy with it. http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:36: meta_table_.Init(db_.get(), 1, 1); On 2012/01/24 00:18:16, michaeln wrote: > ... similarly, since this isn't in the current schema of the existing files, i > think we should avoid adding the 'meta' table. Fair enough, but this won't stop us using a WebCore style database I don't think...? http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.cc:120: bool LocalStorageDatabase::UpgradeVersion1To2IfNeeded() { On 2012/01/24 00:18:16, michaeln wrote: > Code to utilize the new schema has been in used for a long time now (landed on > July 15 2011). We might be able to skip upgrading existing data and start afresh > if we encounter on old file? I wondered about that too, but the upgrade process is quite straightforward so I thought I'd just do it. Not really fussed either way about keeping it. http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.h (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.h:29: bool WriteAllValues(const std::map<string16, string16>& values); Yeah, I thought about this when I started drafting a storage namespace to actually use this class. For now, I've removed the "All" from the function name and updated the sql to use REPLACE rather than INSERT. Maybe it'd be worth adding an API to write one item, not sure yet.
http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.h (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.h:19: class LocalStorageDatabase { class DOMStorageDatabase... for consistency with other class names in this library and also since we'll also be using this for 'session' storage in some cases... wdyt?
http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... File webkit/dom_storage/local_storage_database.h (right): http://codereview.chromium.org/9159020/diff/1/webkit/dom_storage/local_storag... webkit/dom_storage/local_storage_database.h:19: class LocalStorageDatabase { sgtm
New snapshot based on our discussions the other day.
http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:113: string16 value(reinterpret_cast<const char16*>(stmt.ColumnBlob(1))); Can these values have embedded NULLs in them and is there any guarantee that the value is NULL terminated? Seems like we need a ColumnBlobAsString16 helper somewhere. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:133: stmt.BindBlob(1, value.c_str(), -1); do we have to be explicit about the length here? bool BindBlob(int col, const void* value, int value_len); http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:145: for ( ; it != keys.end(); ++it) { We'll want to put multiple write statements in a transactions. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.h (right): http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:36: virtual bool Init(); In other 'db' classes we create and open underlying database files very lazily to avoid creating files unless we really need to. In those classes, there usually isn't a public Open() method at all, and instead a private a LazyOpen(bool create_if_needed) method gets called internally as needed. I'd vote to follow that pattern here too. Take a look at quota_database.cc &| appcache_database.cc for examples of this pattern. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:40: virtual ValuesMap ReadAllValues() const; do you know if all of our compilers are smart enough to not copy the collection upon return? the old-schooler in me likes to see ptr args to non-const data for methods that compute large amount values http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:44: virtual bool WriteValues(const ValuesMap& values); do the methods of this class need to be virtual?
http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:113: string16 value(reinterpret_cast<const char16*>(stmt.ColumnBlob(1))); I'm writing out using c_str(), so I think it should be ok to assume null terminated. But embedded nulls would be a problem if they were present in the original input as it would have only written up to the first null (the -1 in the BindBlob call). http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:133: stmt.BindBlob(1, value.c_str(), -1); -1 says to write up to the first null, so not as long as the blob is null terminated and there are no embedded nulls. Null terminated is OK as I'm using c_str(), but embedded nulls... not so sure. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:145: for ( ; it != keys.end(); ++it) { Done (and for Write above). http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.h (right): http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:36: virtual bool Init(); sgtm. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:40: virtual ValuesMap ReadAllValues() const; I would've thought so, but don't know for certain. Happy to change to output ptr param. http://codereview.chromium.org/9159020/diff/10001/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:44: virtual bool WriteValues(const ValuesMap& values); I think the virtuals are left over from an older iteration where I had this class deriving from an interface, so I don't think they are necessary any more.
http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:38: Close(); Since sql::Connection dtor will close automatically for you, maybe we don't have a use case for an explicit close method just yet. (although i expect we will have a use case when we get to supporting context->PurgeMemory) http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:41: bool DomStorageDatabase::LazyOpen() { can you reorder the methods in the .cc file to match the order in which they're declared in the .h file http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:47: db_->set_error_delegate(GetErrorHandlerForDomStorageDatabase()); If the db->Open() method can invoke the error delegate (i'm not sure if it can), we probably want to attach it prior to calling Open(). And if not, no reason to attach it if Open() failes. Is the is_open local needed? http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:70: return CreateTable(); style nit: since the if { block } has brackets, the else { block } should too, also there's some nesting here that maybe could get flattened out given early returns http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:74: if (!LazyOpen()) at this point, there's no reason to open if not already open http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:135: return true; return transaction.Commit(); http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:186: { why the extra scope http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:192: migration.Commit(); return migration.Commit()? http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.h (right): http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:35: bool WriteValues(const ValuesMap& values); With the current public interface, there is no way to update a value and delete another in the same transaction. My model for how changes gets committed is that changes accrue in memory until some amount of time has past, and then those changes get committed atomically so what results in the db exactly matches an instance of what was in memory at some point in time. With that in mind, wdyt about a public api that looks like this... typedef std::map<string16, NullableString16> ValuesMap; // Reads all values from the database and populates the // |all_values| map with non-null NullableString16 results. // The |all_values| collection is expected to be initially empty. If // the backing database file does not exist upon entry, none will not // be created on disk. bool ReadAll(ValueMap* all_values); // Inserts, updates, or deletes the values in the |changed_values| // collection, null NullableString16 values are deleted from the database, // non-null NullableString16 values are inserted or updated. // If |clear_all_first| is true, any existing values in the // database file are deleted prior to applying the inserts and updates. // If the backing database file does not exist, one will be created only // if |changed_values| is non-empty. bool CommitChanges(bool clear_all_first, const ValueMap& changed_values); DomStorageArea would keep two collections in memory, all_values_ and changed_values_, the latter representing inserts/updates/deletes. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:57: bool LazyOpen(); If we can avoid creating files on disk, all the better. Wdyt about adding a 'create_if_needed' flag to this method. For ReadAll() it would pass false, and for CommitChanges it would pass true if changed_values is non-emtpy.
thanks, new patch soon! http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:38: Close(); Close is private and useful in the unit tests. I could move it into the test fixture I guess for now, but given as you say we'll likely need it in the future, shall I just leave it here? I guess that we don't need to call it from the dtor here though. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:41: bool DomStorageDatabase::LazyOpen() { On 2012/01/31 22:45:32, michaeln wrote: > can you reorder the methods in the .cc file to match the order in which they're > declared in the .h file Done. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:47: db_->set_error_delegate(GetErrorHandlerForDomStorageDatabase()); The docs in sql::Connection actually say that it should be called before Open, so I imagine Open can invoke the ErrorDelegate. Will move. is_open is left over from a previous version when I was using it for other things... can remove now. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:70: return CreateTable(); gah, webkit style on the brain :) http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:74: if (!LazyOpen()) Good spot, think I was a little too generous with search/replace :) http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:135: return true; On 2012/01/31 22:45:32, michaeln wrote: > return transaction.Commit(); Done. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:186: { hmm, I think I used to have some extra stuff after the transaction was done but before the function returned, so the extra scope was to make sure it got cleaned up asap. No need for it now though http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:192: migration.Commit(); On 2012/01/31 22:45:32, michaeln wrote: > return migration.Commit()? Done. http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.h (right): http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:35: bool WriteValues(const ValuesMap& values); Like it! http://codereview.chromium.org/9159020/diff/15002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:57: bool LazyOpen(); OK, I guess we need to check if the file exists too and go and ahead and open the database regardless of the create_if_needed flag.
looking pretty good, can u run some try jobs and update the CL to include the bug number 106763 http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc File sql/statement.cc (right): http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc#newcode273 sql/statement.cc:273: bool Statement::ColumnBlobAsString16(int col, string16* val) const { it be good to more closely match the impl of ColumnBlobAsString http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc#newcode280 sql/statement.cc:280: val->resize(len); len/2 since resize(arg) is in characters whereas len is in bytes? http://codereview.chromium.org/9159020/diff/18002/sql/statement.h File sql/statement.h (right): http://codereview.chromium.org/9159020/diff/18002/sql/statement.h#newcode147 sql/statement.h:147: bool ColumnBlobAsString16(int col, string16* val) const; can u move this up so its under the other String variant, also in the .cc file http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:42: if (!stmt) i'm looking at http://codereview.chromium.org/9249025/ which is about adjusting sqlite usage patterns to be consistent, including removing checks for is_valid() and early returns in light of that i think these s/b removed http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:68: sql::Statement stmt; can this be moved into the body of the for loop? also style nit: statement instead of stmt http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:80: string16 value_str = value.string(); can the local be removed to avoid making a copy http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: LOG(WARNING) << "Unable to open DOM storage database at " what should we do at this point? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:127: return false; upon return, will IsOpen() report 'true' since theres an open db connection... but the schema will not be setup? in these failed cases, maybe .reset() the db_ connection altogether? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:132: return CreateTableV2(); doesn't look like we really need two separate methods here? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:136: if (!LazyOpen(false)) should this method DCHECK that the db_ connection is open instead? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:155: sql::ColType valueColumnType = stmt.DeclaredColumnType(1); nit: nix camelCase variable naming http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:160: // Something is messed up. This is not a V1 database. what should we do at this point? delete it and start over? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:173: if (migration.Begin()) { prefer early returns if (!begin()) return false http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:226: // Verify the db now has V2 structure. a few of the test verify the schema, might be worth a helper method to do that given an open sql::Connection?
http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc File sql/statement.cc (right): http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc#newcode273 sql/statement.cc:273: bool Statement::ColumnBlobAsString16(int col, string16* val) const { On 2012/02/01 21:03:11, michaeln wrote: > it be good to more closely match the impl of ColumnBlobAsString Done. http://codereview.chromium.org/9159020/diff/18002/sql/statement.cc#newcode280 sql/statement.cc:280: val->resize(len); On 2012/02/01 21:03:11, michaeln wrote: > len/2 since resize(arg) is in characters whereas len is in bytes? Done. http://codereview.chromium.org/9159020/diff/18002/sql/statement.h File sql/statement.h (right): http://codereview.chromium.org/9159020/diff/18002/sql/statement.h#newcode147 sql/statement.h:147: bool ColumnBlobAsString16(int col, string16* val) const; On 2012/02/01 21:03:11, michaeln wrote: > can u move this up so its under the other String variant, also in the .cc file Done. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:42: if (!stmt) Removed the early returns but kept DCHECK, as I think they're useful for debugging. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:68: sql::Statement stmt; On 2012/02/01 21:03:11, michaeln wrote: > can this be moved into the body of the for loop? > also style nit: statement instead of stmt Done, updated stmt everywhere http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:80: string16 value_str = value.string(); On 2012/02/01 21:03:11, michaeln wrote: > can the local be removed to avoid making a copy Done. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: LOG(WARNING) << "Unable to open DOM storage database at " Good question :) My thinking was that it's ok to return false from this function and let the caller deal with it. I suppose we could create an in-memory database and try opening on disk again when we come to commit changes. Wdyt, probably not for the initial checkin but I could add a TODO? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:127: return false; good call http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:132: return CreateTableV2(); My thinking was that it's useful to have a specific V2 function for the upgrade process, and a generic function that will always create a table at the current version. i.e. if there was a version 3, CreateTable would be updated to return CreateTableV3(). http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:136: if (!LazyOpen(false)) sgtm http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:155: sql::ColType valueColumnType = stmt.DeclaredColumnType(1); On 2012/02/01 21:03:11, michaeln wrote: > nit: nix camelCase variable naming Done. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:160: // Something is messed up. This is not a V1 database. Yeah, that is taken care of by the LazyOpen function. If the upgrade method returns false, it assumes the upgrade failed and drops the db and creates a fresh table at the right version. Will add a note about return value in the header http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:173: if (migration.Begin()) { On 2012/02/01 21:03:11, michaeln wrote: > prefer early returns if (!begin()) return false Done. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:226: // Verify the db now has V2 structure. On 2012/02/01 21:03:11, michaeln wrote: > a few of the test verify the schema, might be worth a helper method to do that > given an open sql::Connection? Done.
Hey Michael, I've addressed your last set of comments. The try servers seem to be having trouble locating the test database file that is also part of this patch, and that's why the one unit test is failing. Still trying to work out what I did wrong there...
It seems that the try-bot isn't creating the file in the first place (consensus seems to be patch not liking binary files), so I plan to land the test data in this change but leave that specific test disabled. That way I can (hopefully!) get a clean try-bot run and then send a quick follow up CL to turn the test on again.
mac and linux try bots are green and windows is red but it appears not due to my changes.
The DomStorageDatabase class is looking pretty good, please take a look at my comments about the tests, overall i think the tests could be simplified and stripped down somewhat. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: LOG(WARNING) << "Unable to open DOM storage database at " On 2012/02/02 12:14:49, benm wrote: > Good question :) My thinking was that it's ok to return false from this function > and let the caller deal with it. The caller is every method that may want to read or write. I think we have to deal with this kind of error condition more centrally, not needed for this initial checkin but prior to shipping. > I suppose we could create an in-memory database and try opening on disk again > when we come to commit changes. Wdyt, probably not for the initial checkin but I > could add a TODO? That usage of in-memory dbs scares me :) What if the file on disk contains values, and whaddaya-know it can be opened later? A page just ran seeing an empty area and wrote a bunch of values with that emptiness in mind. Hmmm... unittests could run much faster if they operated on in-memory dbs, it might make sense to allow this class to treat empty file path as meaning use an in-memory db and to massage the tests to take advantage of that where possible? http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:132: return CreateTableV2(); On 2012/02/02 12:14:49, benm wrote: > My thinking was that it's useful to have a specific V2 function for the upgrade > process, and a generic function that will always create a table at the current > version. i.e. if there was a version 3, CreateTable would be updated to return > CreateTableV3(). Still don't see what value the indirection adds. There's only code to create the current version ever, so why two methods? My thinking is less is more. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:160: // Something is messed up. This is not a V1 database. On 2012/02/02 12:14:49, benm wrote: > Yeah, that is taken care of by the LazyOpen function. If the upgrade method > returns false, it assumes the upgrade failed and drops the db and creates a > fresh table at the right version. Will add a note about return value in the > header LazyOpen continues to try to work with the existing file, but if its botched to the point of being unopenable as a sqlite db, or that calling DROP TABLE fails... we'll be wedged. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:79: "INSERT INTO ItemTable VALUES (?,?)")); style nit: indent two more spaces http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: if (!db_->Open(file_path_)) { please put a todo here about dealing with this sort error condition, if the file is corrupt we don't want to try to repeatedly open on every access http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:131: if (db_->Execute("DROP TABLE ItemTable")) { The DROP TABLE statement and the following CREATE TABLE statement probably should be in the same transaction. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:200: db_.reset(NULL); could this be simplified to just db_.reset(NULL) as the whole method body? http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:35: // Create a table with the value type as FLOAT - this is "invalid" a more interesting test case would be trying to open a file that isn't a database file at all, i think the invalid table case and that non-a-database case could/should be handled in the same way? http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:58: void InsertDataV2(sql::Connection* db, Would it make sense for callsites to directly use the DomStorageDatabase::CommitChanges method instead of replicating the logic here? Having knowlege of the schema here makes it more difficult to change the schema later (w/o having to revisit this helper). I think these tests need to have explicit knowlege of the v2 schema in VerifySchema(), but otherwise, mutating or accessing the data can be accomplished by calling ReadValues or CommitChanges directly. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:69: DomStorageDatabase::ValuesMap ReadAllRows(sql::Connection* db) { similarly, would it make sense to directly use the DomStorageDatabase method instead of replicating it here http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:107: TEST_F(DomStorageDatabaseTest, SimpleOpenAndClose) { this test could use an in-memory db http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:113: EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value")); Would it make sense to put these three statements be in VerifySchema too? 110 EXPECT_TRUE(db.db_->DoesTableExist("ItemTable")); 111 // Ensure that we've got the colums we expect. 112 EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "key")); 113 EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value")); http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:136: TEST_F(DomStorageDatabaseTest, TestUpgradesV1TableToV2) { this one maybe could be done out of an in-mem db by setting up a connection which a v1 schema and calling the upgrade method directly similar to the test below http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:161: TEST_F(DomStorageDatabaseTest, TestIsOpen) { could be done with an in-mem db http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:185: TEST_F(DomStorageDatabaseTest, SimpleWrite) { Since SimpleWrite actually also reads, maybe call it SimpleReadWrite and delete SimpleRead? http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:223: storage[kCannedKeys[i]] = kCannedValues[i]; this is replicated in the previous test too, feels like a helper function would be good http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:242: TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2NoData) { there are multiple upgrade tests, can any of them be merged, how is this any different than TestUpgradesV1TableToV2? http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:249: "SELECT value from ItemTable LIMIT 1")); can this statement be removed, what purpose does it serve? http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:283: // Now open the db again and check that the data is consistent. provided other tests ensure that files containing data are actually opened by LazyOpen(), not sure you need this block in this test. And then, the upgrade logic could be tested in-memory (much faster).
Seems like the v1 to v2 upgrade code path is adding a lot of complexity, especially for testing. Consider stripping that out for the initial checkin and get the basics (LazyOpen, ReadAllValues, CommitChanges) in first?
Thanks again Michael.. new patch coming soon http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: LOG(WARNING) << "Unable to open DOM storage database at " > The caller is every method that may want to read or write. I think we have to > deal with this kind of error condition more centrally, not needed for this > initial checkin but prior to shipping. Good point, added a TODO to handle this failure to open the file better. I suppose what we should really do is delete the path on disk (if it exists) and try again. If that still fails or the file wasn't there in the first place, then we can set the flag and LocalStorage will effectively be unavailable. Unless of course we did try and implement some sort of in-memory db and treat localstorage as being effectively session storage... it wouldn't matter if the file on disk had values already as we have nuked it anyway or it wasn't there to begin with... might be a good fallback but perhaps not entirely in line with the spec (i.e. local storage should only be expired by the user or for security reasons), but the spec naturally doesn't specify what to do in this kind of exceptional circumstance ... I guess if we were using an in memory db we could try one final time to sync it to disk when the storage area is closed something like that... > Hmmm... unittests could run much faster if they operated on in-memory dbs, it > might make sense to allow this class to treat empty file path as meaning use an > in-memory db and to massage the tests to take advantage of that where possible? That's a nice idea, I guess we'd still want a test or two writing the file to disk to ensure that code path stays working but the rest could be in memory. I'll add a todo in the test file. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:132: return CreateTableV2(); My thinking is that if we introduce a V3 database (which would probably be something we could consider doing to improve performance, or what not) then we'd write an UpgradeV2ToV3 function that would use CreateTableV3, and change CreateTable so that it would call CreateTableV3. But we'd still want CreateTableV2 so it can be used in UpgradeV1ToV2. http://codereview.chromium.org/9159020/diff/18002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:160: // Something is messed up. This is not a V1 database. Good point. However I don't think that we can get to this point if the database was unopenable - we would have dropped out of LazyOpen before we got to the point that we try and upgrade. I've added a TODO to handle failing to open the file more gracefully in LazyOpen. I can also add a TODO to handle the failure of the DROP in LazyOpen better. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:79: "INSERT INTO ItemTable VALUES (?,?)")); On 2012/02/03 04:45:15, michaeln wrote: > style nit: indent two more spaces Done. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:106: if (!db_->Open(file_path_)) { On 2012/02/03 04:45:15, michaeln wrote: > please put a todo here about dealing with this sort error condition, if the file > is corrupt we don't want to try to repeatedly open on every access Done. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:131: if (db_->Execute("DROP TABLE ItemTable")) { On 2012/02/03 04:45:15, michaeln wrote: > The DROP TABLE statement and the following CREATE TABLE statement probably > should be in the same transaction. Done. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:200: db_.reset(NULL); lgtm! :) http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:35: // Create a table with the value type as FLOAT - this is "invalid" Yes, I think so. An file that isn't a database would be a good test. I can check in a blob to use for that, but given that the trybots aren't happy with adding files for testing, I'll leave the test DISABLED until the next CL, like the one that verifies we can read a WebCore DB. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:58: void InsertDataV2(sql::Connection* db, I added the helpers up here so that we can test the read and commit methods independently, i.e. we test that values were committed correctly by pulling them out of the DB directly with SQL without regard to what ReadAllValues does. wdyt? Maybe this method would be better named just InsertData, and then in the future we would just update it to match the new schema if it changes, and then add InsertDataV2 to test the upgrade from v2->v3. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:69: DomStorageDatabase::ValuesMap ReadAllRows(sql::Connection* db) { See above. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:107: TEST_F(DomStorageDatabaseTest, SimpleOpenAndClose) { Agreed, I've added a TODO about this but as it will need more work in the DomStorageClass to implement, I'd like to leave the actual implementation out for now. I'll go over all the tests here and replace with an in-mem db where appropriate when fixing the TODO. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:113: EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value")); On 2012/02/03 04:45:15, michaeln wrote: > Would it make sense to put these three statements be in VerifySchema too? > > 110 EXPECT_TRUE(db.db_->DoesTableExist("ItemTable")); > 111 // Ensure that we've got the colums we expect. > 112 EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "key")); > 113 EXPECT_TRUE(db.db_->DoesColumnExist("ItemTable", "value")); Done. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:185: TEST_F(DomStorageDatabaseTest, SimpleWrite) { So the idea here was that we're only testing the CommitChanges method of DomStorageDatabase - that's why I've got the helpers up top to pull out of the underlying DB with SQL, regardless of what the ReadAllValues method on the DomStorageDatabase does. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:223: storage[kCannedKeys[i]] = kCannedValues[i]; On 2012/02/03 04:45:15, michaeln wrote: > this is replicated in the previous test too, feels like a helper function would > be good Done. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:242: TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2NoData) { I think what I was thinking is that this test verifies that db.UpgradeVersion1To2IfNeeded() works, and TestUpgradesV1TableToV2 verifies that LazyOpen triggers the upgrade logic. I can rename the other test to be more explicit about that. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:249: "SELECT value from ItemTable LIMIT 1")); I think you need a valid SQL statement to be able to call DeclaredColoumType below. I believe it can be any statement you like, I picked a SELECT :) http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:283: // Now open the db again and check that the data is consistent. Yes, it should be covered by the next test. will remove
http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:35: // Create a table with the value type as FLOAT - this is "invalid" Writing this test exposed some interesting behavior with regard to sqlite's Open function call. It will quite happily open a connection to any file that you like and return success, but later dies when you try and do something with that connection. So I've added another layer of checks inside LazyOpen to cover this. And the test data file I have is ASCII not binary, so patch should be happy and I needn't check it in DISABLED. \o/
http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:185: TEST_F(DomStorageDatabaseTest, SimpleWrite) { On 2012/02/03 11:46:40, benm wrote: > ...that's why I've got the helpers up top to pull out of the > underlying DB with SQL... I see that, but my point is that it makes the tests less resilient to changes to the DomStorageDatabase class. Eventually a very similar class (maybe the same class name) will utilize levelDB under the covers (with bits and pieces of the existing impl to help with migration). It'd be nice to be able to throw the very similar tests at it for the basic functionality. The more the tests stick to utilizing class interfaces to perform those tests, the easier it is to migrate the impl. http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:242: TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2NoData) { On 2012/02/03 11:46:40, benm wrote: > ... verifies that LazyOpen triggers the upgrade logic... Got it. Since that test also verifies the resulting schema in the new db (given an empty v1 schema as input), its essentially performing this test as a part of that test. So can we remove this one since its already covered by that one? I'm a big believer in less is more ;) http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:249: "SELECT value from ItemTable LIMIT 1")); sorry, i meant what purpose does assertion part serve... see earlier comment about testing the results test setup in the test body. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:125: if (db_->ExecuteAndReturnErrorCode("PRAGMA quick_check(1)") != SQLITE_OK) { oh... this is probably a good idea, but i wonder how many existing localstorage db files would actually fail this test and become suddenly unusable. w/o knowing that, i'm kind of wary of acting heavy handed based on the results of this check. it could be a significant change in behavior. might be good to gather UMA stats on the quick_check(1)? maybe we can use a variant of VerifySchema() in this open sequence to help determine what needs to happen in here, and that method could also be used in the unittests to verify things... enum SchemaVersion { INVALID, V1, V2 } SchemaVersion GetSchemaVersion(); if (!db_.open()) { // oh no!!!! return database_exists ? DeleteAndRecreate() : false; } if (!database_exists) { // brand new file, blaze away and create the schema return CreateSchema(); } version = GetSchemaVersion(); if (version == V2) { return true; // whoohoo! } if (version == V1) { // give the upgrade path a whirl return Upgrade() || DeleteAndRecreate(); } if (version == INVALID) { // deal with a botched existing file return DeleteAndRecreate(); } http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:133: ignore_result(db_->Execute("PRAGMA encoding=\"UTF-16\"")); does this pragma need to be applied prior to the previous one to get it to stick? http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:18: // run much faster if they were operating on in-memory databases. faster and more reliably too, i think it'd be worth doing this prior to the initial checkin, probably pay for itself in time time saved not having to respond to test hiccups that will come along with actually fileio http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:94: EXPECT_EQ((expected.find(key)->second).string(), value.string()); what about is_null()? if lhs is_null() with an empty string, it will equate with a rhs thats !is_null() but also with a empty string http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:119: NullableString16(ASCIIToUTF16("18-01-2012"), false) Maybe add a empty string as a value to ensure that's not confused with 'null' on a round trip in and out of the DB. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:163: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); Not sure this block adds value? Its testing the results of the test setup (CreateV1Table) and not the upgrade method. Not having it in the unittest would enhance readability. It might make more sense to put this in CreateV1Table directly, but i'm not sure its really needed in this file? Looks like its testing the stmt.DeclaredColumnType() method moreso than anything else, maybe put tests for that method in statement_unittest.cc instead? http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:251: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); ditto http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:272: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); ditto http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:354: PathService::Get(base::DIR_SOURCE_ROOT, ¬_a_database); would be more hermetic to not rely on an external data file for this one, should we create the file in the scoped temp dir instead, file_util::WriteFile would work i think (makes sense to include the sqlite3 db file for the test above because to create that hermetically would mean dragging all of sqlite3 into the unittest) also a similar test where the input is a directory instead of a file would be good http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:366: EXPECT_FALSE(db.CommitChanges(true, values)); we want the behavior to be such that this commit succeeds after deleting the bad file and creating a new, right?
http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:185: TEST_F(DomStorageDatabaseTest, SimpleWrite) { OK, done http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:242: TEST_F(DomStorageDatabaseTest, UpgradeFromV1ToV2NoData) { sounds good. It's nice to have such a powerful testing framework, I want to write ALL the tests!! :P http://codereview.chromium.org/9159020/diff/24013/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:249: "SELECT value from ItemTable LIMIT 1")); Got it - removed now. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:125: if (db_->ExecuteAndReturnErrorCode("PRAGMA quick_check(1)") != SQLITE_OK) { On 2012/02/04 00:21:34, michaeln wrote: > oh... this is probably a good idea, but i wonder how many existing localstorage > db files would actually fail this test and become suddenly unusable. w/o knowing > that, i'm kind of wary of acting heavy handed based on the results of this > check. it could be a significant change in behavior. > As it stands at the moment, we're executing the quick check to only verify the database connection can execute a query - it doesn't really need to be quick_check, but I needed something that will fail fast when we haven't actually opened a database, but not be too slow in the common "good file" case. We will continue regardless of the returned results of the query (it returns a table where each row is a detected error or a single row "ok") as long as it could be executed. It would be nice to try and recover from those errors, but that's a lot more work because there isn't an API on sql::Connection to get a statement you can step that doesn't DCHECK with an invalid database connection (maybe it'd be good to add one, but I think that's best done as a separate patch). Perhaps it'd be a good idea to change Connection::Open to fail if the file opened is not a database. But that's a bit scary and would need thorough testing. > might be good to gather UMA stats on the quick_check(1)? > > maybe we can use a variant of VerifySchema() in this open sequence to help > determine what needs to happen in here, and that method could also be used in > the unittests to verify things... > > enum SchemaVersion { > INVALID, > V1, > V2 > } > SchemaVersion GetSchemaVersion(); > > > if (!db_.open()) { > // oh no!!!! > return database_exists ? DeleteAndRecreate() : false; > } > > if (!database_exists) { > // brand new file, blaze away and create the schema > return CreateSchema(); > } > > version = GetSchemaVersion(); > if (version == V2) { > return true; // whoohoo! > } > if (version == V1) { > // give the upgrade path a whirl > return Upgrade() || DeleteAndRecreate(); > } > if (version == INVALID) { > // deal with a botched existing file > return DeleteAndRecreate(); > } That's quite a nice plan, but the tricky part is detecting that you've got an INVALID version with the current sql API, as if you try and run queries then the code will DCHECK. I'll have a think about this today. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:133: ignore_result(db_->Execute("PRAGMA encoding=\"UTF-16\"")); Mm, good question. The SQLite docs aren't very clear on this. I tried some experiments locally, and it seems that the encoding pragma sticks as long as you don't execute a mutating statement like a CREATE, so I think it's OK. Sadly this pragma doesn't fail on an invalid database connection, so it can't be used in place of the quick_check above. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:18: // run much faster if they were operating on in-memory databases. Is there a runtime flag or an #ifdef I can use to determine when we're running/building tests? Would be nice to be sure I'm only creating an in memory database when we're under test and not in real life? http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:94: EXPECT_EQ((expected.find(key)->second).string(), value.string()); On 2012/02/04 00:21:34, michaeln wrote: > what about is_null()? > > if lhs is_null() with an empty string, it will equate with a rhs thats > !is_null() but also with a empty string Done. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:119: NullableString16(ASCIIToUTF16("18-01-2012"), false) On 2012/02/04 00:21:34, michaeln wrote: > Maybe add a empty string as a value to ensure that's not confused with 'null' on > a round trip in and out of the DB. Done. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:163: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); Good point. I put it as an ASSERT as there's no point in continuing if we weren't able to create the table at V1. But we already ASSERT the creation statements inside CreateTableV1... so I guess it's OK to remove this. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:251: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); On 2012/02/04 00:21:34, michaeln wrote: > ditto Done. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:272: ASSERT_EQ(sql::COLUMN_TYPE_TEXT, statement.DeclaredColumnType(0)); On 2012/02/04 00:21:34, michaeln wrote: > ditto Done. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:354: PathService::Get(base::DIR_SOURCE_ROOT, ¬_a_database); On 2012/02/04 00:21:34, michaeln wrote: > would be more hermetic to not rely on an external data file for this one, should > we create the file in the scoped temp dir instead, file_util::WriteFile would > work i think > > (makes sense to include the sqlite3 db file for the test above because to create > that hermetically would mean dragging all of sqlite3 into the unittest) > Done. > also a similar test where the input is a directory instead of a file would be > good > The directory test causes the call to Connection::Open to ASSERT, as we hit the NOTREACHED() in DiagnosticErrorDelegate::OnSqlError (as the open actually fails on a directory). I think that means then that there isn't anyone out there that is testing this sort of thing (as otherwise they'd be ASSERTing in the test)? I can demote the ASSERT in DiagnosticErrorDelegate to a log message, or not test this... wdyt? http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:366: EXPECT_FALSE(db.CommitChanges(true, values)); Right, but don't have the code to do the delete and recreate yet :) Makes the directory test case even more useful, because we can't create anything in that case.
http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:18: // run much faster if they were operating on in-memory databases. I found #ifdef UNIT_TEST. I think that should do the trick.
New snapshot with all these changes on the way. http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:18: // run much faster if they were operating on in-memory databases. Done - the variance in time to run is quite remarkable between tests that hit the disk and those that stay in memory.
http://codereview.chromium.org/9159020/diff/29001/sql/diagnostic_error_delega... File sql/diagnostic_error_delegate.h (right): http://codereview.chromium.org/9159020/diff/29001/sql/diagnostic_error_delega... sql/diagnostic_error_delegate.h:34: */ RecordErrorInHistogram(error); d'oh, didn't mean to leave this in. This NOTREACHED() causes us to fail in the test that tries to open the directory as that case actually causes the Connection::Open call to die. As I understsand it, this only fails in debug builds? Should this NOTREACHED() be demoted to a LOG(ERROR), or should I remove the test for opening a directory? Or is there a way to tell unit tests to ignore/not crash on NOTREACHED() ? Any other ways around this?
Hi Michael, I think the latest version of the patch should address the latest round of comments. Please take a look! The issue I'm most concerned about is changing the NOTREACHED() to LOG(ERROR) in ErrorDiagnostic. wdyt? Thanks!
>> if (!db_->Open()) {
>> // oh no!!!!
>> return database_exists ? DeleteAndRecreate() : false;
>> }
>>
>> if (!database_exists) {
>> // brand new file, blaze away and create the schema
>> return CreateSchema();
>> }
>>
>> version = DetectSchemaVersion(db_.get());
>> if (version == V2) {
>> return true; // whoohoo!
>> }
>> if (version == V1) {
>> // give the upgrade path a whirl
>> return UpgradeSchema() || DeleteAndRecreate();
>> }
>> if (version == INVALID) {
>> // deal with a botched existing file
>> return DeleteAndRecreate();
>> }
>
> That's quite a nice plan, but the tricky part is detecting that you've got an
> INVALID version with the current sql API, as if you try and run queries then
the
> code will DCHECK. I'll have a think about this today.
I don't understand the comment about DCHECKS?
SchemaVersion DomStorageDatabase::DetectSchemaVersion(connection) {
if (!connection->DoesTableExist("ItemTable") ||
!connection->DoesColumnExist("ItemTable", "key") ||
!connection->DoesColumnExist("ItemTable", "value"))
return INVALID;
sql::Statement statement("SELECT key, value from ItemTable");
if (statement.DeclaredColumnType(0) != TEXT)
return INVALID;
switch (statement.DeclaredColumnType(1)) {
case BLOB:
return V2;
case TEXT:
return V1;
default:
return INVALID;
}
}
http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database_unittest.cc (right):
http://codereview.chromium.org/9159020/diff/23014/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:18: // run much faster if
they were operating on in-memory databases.
On 2012/02/06 17:03:21, benm wrote:
> Done - the variance in time to run is quite remarkable between tests that hit
> the disk and those that stay in memory.
yes, how many human lifetimes will be saved in try runs :)
http://codereview.chromium.org/9159020/diff/29002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database_unittest.cc (right):
http://codereview.chromium.org/9159020/diff/29002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:24: // tests to use
databases in memory.
this comment seems useful since the ctor here is somewhat sneaky, consider
retaining it
http://codereview.chromium.org/9159020/diff/36002/sql/diagnostic_error_delega...
File sql/diagnostic_error_delegate.h (right):
http://codereview.chromium.org/9159020/diff/36002/sql/diagnostic_error_delega...
sql/diagnostic_error_delegate.h:31: LOG(ERROR) << "sqlite error " << error
Switching to LOG(ERROR) here sgtm.
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database.cc (right):
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.cc:123: LOG(ERROR) << "Unable to open
DOM storage database in memory.";
this could probably be a NOTREACHED() instead
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.cc:179: return CreateTableV2();
i still don't see the reason for two CreateTable methods and think that one of
them should be removed
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database.h (right):
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.h:97: // DCHECK when running the unit
tests.
ah... i see the comment moved to here!
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database_unittest.cc (right):
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:26:
ASSERT_TRUE(db->Execute("DROP TABLE IF EXISTS ItemTable"));
since it's a brand new in memory db, no need to drop table
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:38:
ASSERT_TRUE(db->Execute("DROP TABLE IF EXISTS ItemTable"));
ditto
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:136: db.Close();
wait a minute... close will delete the in-mem database, so the call to lazyOpen
will create a new one... and not test the upgrade path.
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:149: db.Close();
ditto the call below not testing the path its trying to test
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database_unittest.cc:158:
TEST(DomStorageDatabaseTest, TestIsOpen) {
can we merge this with SimpleOpenAndClose() which goes thru the exact same
steps... and delete this one
hi ben, i keep coming back to the lazy open logic (with auto upgrade) and test complications. i do think that if the open logic was restructured, it might be more amenable to unit testing and maybe be easier to follow the open sequence these methods can be pretty directly tested independent of one another DetectSchemaVersion(connection) UpgradeSchema(connection) // only expects valid v1 stuff as input DeleteAndRecreate() // will delete whatever and create a new given good coverage of those constituent parts, we probably dont need explicit coverage for all possible LazyOpen() inputs
On 2012/02/08 06:11:33, michaeln wrote:
> I don't understand the comment about DCHECKS?
>
> SchemaVersion DomStorageDatabase::DetectSchemaVersion(connection) {
> if (!connection->DoesTableExist("ItemTable") ||
> !connection->DoesColumnExist("ItemTable", "key") ||
> !connection->DoesColumnExist("ItemTable", "value"))
> return INVALID;
In the case of a file that isn't a database, these will invoke the DLOG(FATAL)
in Connection::GetUniqueStatement() and Statement::CheckValid()
> sql::Statement statement("SELECT key, value from ItemTable");
I don't think you can create a statement with an SQL string, you need to invoke
Connection::GetCached[Unique]Statement and so again this will DLOG(FATAL) as
above in the case that the file isn't a database.
> if (statement.DeclaredColumnType(0) != TEXT)
> return INVALID;
>
> switch (statement.DeclaredColumnType(1)) {
> case BLOB:
> return V2;
> case TEXT:
> return V1;
> default:
> return INVALID;
> }
> }
Having said that, I think we can still make use of something like the
quick_check(1) query I have at the moment inside the DetectSchema function.
http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:123: LOG(ERROR) << "Unable to open DOM storage database in memory."; On 2012/02/08 06:11:33, michaeln wrote: > this could probably be a NOTREACHED() instead Done. http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:179: return CreateTableV2(); OK, done. http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:26: ASSERT_TRUE(db->Execute("DROP TABLE IF EXISTS ItemTable")); I've changed this to work on an already-open database. That way the tests below that use it can create the db on disk to test LazyOpen, but others can use an in memory db. Given that, I've left the DROP in. http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:136: db.Close(); good spot. I've made this test use a disk backed db. http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:149: db.Close(); This test is now deleted as the code it exercised should now be covered by TestCanOpenFileThatIsNotADatabase. http://codereview.chromium.org/9159020/diff/36002/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:158: TEST(DomStorageDatabaseTest, TestIsOpen) { On 2012/02/08 06:11:33, michaeln wrote: > can we merge this with SimpleOpenAndClose() which goes thru the exact same > steps... and delete this one Done.
New snapshot uploaded and trybots are running. Please take a look! Thanks, Ben
getting real close, no real substantive comments but a few things to consider https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... File webkit/dom_storage/dom_storage_database.cc (right): https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database.cc:145: if (CreateTableV2()) Do we want to DeleteAndRecreate() in this case of having just created a brand new empty db file and failing, or would it suffice to Close() and return false if create table fails here? depending, some nesting could be avoided by rearranging a little... if (exists) { if (!CreateTableV2()) Close(); return IsOpen(); } https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database.cc:177: if (db_->ExecuteAndReturnErrorCode("PRAGMA quick_check(1)") != SQLITE_OK) Thnx for the explanation about statements yacking if the file is not a db An alternative to quick_check might be vacuum? In a previous project, greg simon used vacuum over integrity_check/quick_check for this purpose... i think because vacuum would handle rolling back uncommitted xactions given the presense of an unlocked .journal file at open time. We'll need to verify whether quick_check works in that case, but that can come later. https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database.cc:223: // If it's not a directory and we can delete the file, try and open it again. why are we careful not to avoid deleting if the input path is a directory instead of a file... just curious https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database.cc:257: return migration.Commit(); style: not sure, would this be more readable? Transaction transaction; return transaction.Begin() && db_->Execute("DROP TABLE ItemTable") && CreateTableV2() && CommitChanges(false, values) && transaction.Commit(); https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... File webkit/dom_storage/dom_storage_database.h (right): https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database.h:75: // Creates the database table at V1. Used when upgrading the database comment is confusing https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... File webkit/dom_storage/dom_storage_database_unittest.cc (right): https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database_unittest.cc:140: EXPECT_TRUE(db.IsOpen()); With minor mods, this test could serve to verify what TestOpenCloseDataPreserved is meant to verify. Maybe worth it to perform less actual file io in test runs. Instead of calling db.ReadAllValues() directly, a call to CheckValuesMatch() would kick the lazy open case and verify that the expected values are preserved (and the other test could be removed). https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database_unittest.cc:173: EXPECT_EQ(DomStorageDatabase::V2, db.DetectSchemaVersion()); looks like much of the boiler plate around each case could be eliminated since each of the create table helpers drops any old table. DomStorageDatabase db; db.db_.reset(new sql::Connection()); ASSERT_TRUE(db.db_->OpenInMemory()); EXPECT_EQ(DomStorageDatabase::INVALID, db.DetectSchemaVersion()); CreateInvalidValueColumnTable(db.db_.get()); EXPECT_EQ(DomStorageDatabase::INVALID, db.DetectSchemaVersion()); CreateV1Table(db.db_.get()); EXPECT_EQ(DomStorageDatabase::V1, db.DetectSchemaVersion()); ..etc... https://chromiumcodereview.appspot.com/9159020/diff/49012/webkit/dom_storage/... webkit/dom_storage/dom_storage_database_unittest.cc:202: // Test write. this comment and the one below probably can be removed, i think they're stating the obvious
Thanks Michael... new snapshot on the way http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.cc (right): http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:145: if (CreateTableV2()) My thinking was that it doesn't hurt to have another go in the error case, perhaps the moons were out of alignment ... ;) http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:177: if (db_->ExecuteAndReturnErrorCode("PRAGMA quick_check(1)") != SQLITE_OK) I just tried VACUUM and it seems to work. But I'm slightly concerned about the cost of a VACUUM on a large database (e.g. a full 5MB quota will need up to 10MB of space on open) which isn't very desirable on a mobile device. But you're right that dealing with the rollback is a plus (which it seems from the docs that quick_check won't do). It seems like querying the current auto_vacuum status will also suffice for detecting a corrupt database so is probably preferable to quick_check in terms of speed. http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:223: // If it's not a directory and we can delete the file, try and open it again. I was a bit scared about accidentally deleting entire directories given the comments in file_util.h. Maybe a bit paranoid but just playing it safe. Also if the path was a directory then I guess we're never going to have much success opening it as a database, so no need to try again... http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.cc:257: return migration.Commit(); I like it. http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database.h (right): http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database.h:75: // Creates the database table at V1. Used when upgrading the database It is! Fixed. http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... File webkit/dom_storage/dom_storage_database_unittest.cc (right): http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:140: EXPECT_TRUE(db.IsOpen()); On 2012/02/08 22:12:08, michaeln wrote: > With minor mods, this test could serve to verify what TestOpenCloseDataPreserved > is meant to verify. Maybe worth it to perform less actual file io in test runs. > > Instead of calling db.ReadAllValues() directly, a call to CheckValuesMatch() > would kick the lazy open case and verify that the expected values are preserved > (and the other test could be removed). Done. http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:173: EXPECT_EQ(DomStorageDatabase::V2, db.DetectSchemaVersion()); Thanks, done! http://codereview.chromium.org/9159020/diff/49012/webkit/dom_storage/dom_stor... webkit/dom_storage/dom_storage_database_unittest.cc:202: // Test write. On 2012/02/08 22:12:08, michaeln wrote: > this comment and the one below probably can be removed, i think they're stating > the obvious Done.
Linux and Mac green, Windows red but apparently not due to my change. Please take a look at the latest code!
lgtm Not real clear what's best LazyOpen litmus test check (reading auto_vacuum vs quick_check vs vacuum), but we can edit to taste going forward.
Darin, Tony, can I get OWNERS approval for changing th gypi and adding the test data? Thanks!
webkit/ LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benm@chromium.org/9159020/51001
Change committed as 121442 |
