Created attachment 58744[details]
patch #1: implement DatabaseSync::openDatabaseSync()
All new code in DatabaseSync.cpp was copy-pasted from Database.cpp (with a couple of trivial changes, such as removing calls to DatabaseThread).
The plan is to:
1. Implement DatabaseSync.cpp one "feature" at a time (openDatabaseSync(), transaction(), clean up everything properly when the worker shuts down in the middle of a DB operation, etc.)
2. Copy-paste code from Database.cpp to DatabaseSync.cpp and modify it as needed, in order to make each feature in DatabaseSync.cpp work.
3. Once feature 'n' is landed in DatabaseSync.cpp, work on feature 'n + 1', and in parallel work on moving the duplicate code introduced in feature 'n' to AbstractDatabase.
I think the advantages of this approach are:
1. We _know_ what code is duplicated when moving it from Database/DatabaseSync to AbstractDatabase, instead of guessing.
2. Moving duplicate code to AbstractDatabase doesn't block the DatabaseSync implementation; instead, it can be done in parallel.
Attachment 58744[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/DatabaseSync.cpp:343: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> WebCore/storage/DatabaseSync.cpp:343: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
This code is copy-pasted from Database.cpp. I think it might be easier to review this patch if the new code in DatabaseSync matches exactly the existing one in Database. I can fix this style problem when moving this common code to AbstractDatabase.
Attachment 58972[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSExceptionBase.cpp:39: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 7 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 59039[details]
patch #2: Add the SQLException class
Same patch, converted line endings in JSExceptionBase.cpp from CRLF to LF.
The Chromium bot fails because apparently it misses a step that regenerates .h/.cpp files from .idl files. I tried the patch in a local Chromium client and had no issues with it.
Comment on attachment 59039[details]
patch #2: Add the SQLException class
WebCore/bindings/js/JSExceptionBase.cpp:65
+ return reinterpret_cast<ExceptionBase*>(pathException);
Why does this diff look so funny?
WebCore/page/DOMWindow.idl:733
+ attribute SQLExceptionConstructor SQLException;
Do you need to update the results of the tests that enumerate the global object for all the ports?
WebCore/storage/SQLException.h:7
+ * version 2 of the License, or (at your option) any later version.
2.0? I thought we used 2.1.. Google usually contributes under BSD.
(In reply to comment #9)
> (From update of attachment 59039[details])
> WebCore/bindings/js/JSExceptionBase.cpp:65
> + return reinterpret_cast<ExceptionBase*>(pathException);
> Why does this diff look so funny?
JSExceptionBase.cpp currently uses CRLF line endings, and I had to convert them all to LF to make the style checker happy.
Working on the other comments.
Created attachment 59095[details]
patch #2: Add the SQLException class
patch #2 should be ready for another look.
Fixed the copyright header.
Fixed the expectations of the failing tests on Mac + the expectations for those tests on all other ports that I could find.
The patch passes all tests on Mac; a whole bunch of tests fail in the Qt port, but none of them seems related to this patch (most of them are in css1/, editing/ and fast/dom/Geolocation/ -- maybe I didn't pass in some flags?).
Comment on attachment 59095[details]
patch #2: Add the SQLException class
Adding things to the global scope is a bit of a pain. Please watch the bots carefully to make sure you've got all the expectation updates.
WebCore/WebCore.xcodeproj/project.pbxproj:11037
+ 81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
Did you mean to add these files?
> WebCore/WebCore.xcodeproj/project.pbxproj:11037
> + 81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
> Did you mean to add these files?
No. Hmm, not sure how they got there. Thanks for catching this!
Created attachment 59320[details]
patch #1: implement DatabaseSync::openDatabaseSync()
Michael, I changed patch #1 as we discussed (I think...). Basically, I moved performOpenAndVerify() + some fields + simple getters for those fields from Database to AbstractDatabase. I also moved all guid-related methods to AbstractDatabase, but the plan is to move them to a separate class once this patch is landed. I also made a trivial change to DatabaseTracker, in order to remove the dependency on Database. I haven't changed the logic at all.
I can already see a few ways to clean up Database further, but I'll work on those in separate patches.
All tests in storage/ passed.
Attachment 59320[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/AbstractDatabase.cpp:334: Place brace on its own line for function definitions. [whitespace/braces] [4]
WebCore/storage/DatabaseAuthorizer.h:34: Alphabetical sorting problem. [build/include_order] [4]
WebCore/storage/Database.cpp:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 3 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/AbstractDatabase.cpp:334: Place brace on its own line for function definitions. [whitespace/braces] [4]
> WebCore/storage/DatabaseAuthorizer.h:34: Alphabetical sorting problem. [build/include_order] [4]
> WebCore/storage/Database.cpp:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
> Total errors found: 3 in 12 files
Fix all style problems.
> void DatabaseSync::markAsDeletedAndClose()
> {
> // FIXME: we probably need to do something else in here other than calling closeImmediately().
> closeImmediately();
> }
>
> void DatabaseSync::closeImmediately()
> {
> // FIXME: probably need to do more things in here
> closeDatabase();
> DatabaseTracker::tracker().removeOpenDatabase(this);
> }
These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.
Created attachment 59556[details]
patch #1: implement DatabaseSync::openDatabaseSync()
(In reply to comment #24)
> > void DatabaseSync::markAsDeletedAndClose()
> > {
> > // FIXME: we probably need to do something else in here other than calling closeImmediately().
> > closeImmediately();
> > }
> >
> > void DatabaseSync::closeImmediately()
> > {
> > // FIXME: probably need to do more things in here
> > closeDatabase();
> > DatabaseTracker::tracker().removeOpenDatabase(this);
> > }
>
> These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.
fixed.
This refactoring LGTM. Mostly moving code, only altering what needs altering to be shared with the sync case.
> Implementing DatabaseSync::openDatabaseSync().
The CL description could use some word smithing to briefly describe the changes made in support of implementing openDatabaseSync().
* Hoisted many methods involved in opening a web database from Database to AbstractDatabase for reuse in DatabaseSync.
* Virtualized performOpenAndVerify() to keep interactions with the DatabaseThread out of the base class as that only applies to the Database class and not DatabaseSync.
* Avoided storing a reference to the creation callback in the base class since its not needed beyond return from the static constructor method in the sync case.
Oooops... I spoke a little too some on the code part being ready. Couple of things to look at.
* In the usual case where a DatabaseSync is not closed forcibly thru closeImmediately() or markAsDeletedAndClose(), looks like we're leaving dangling pointers in the 'tracker' and cruft in the various GUID map goo maintained by the base class.
* The FIXME comments on closeImmediately() and markAsDeletedAndClose() are vague. Please be more clear about what has to happen or not.
I think you've got a good impl of closeImmediately() in place already and can simply remove the FIXME in that method.
I haven't understood the semantics of markAsDeletedAndClose() until about an hour ago. Similar semantics to closeImmediately() except its expected to be done in a sync fashion and there are additional side effects around the version() method (and around the callers of the public deleted() method). The impl in the most recent patch is not correct since it doesn't behave syncly. I think its OK to leave this undone for now, and just put in a FIXME for it.
Comment on attachment 59696[details]
patch #1: implement DatabaseSync::openDatabaseSync()
> +void DatabaseSync::closeImmediately()
> {
> - ASSERT(m_scriptExecutionContext->isContextThread());
> - return m_scriptExecutionContext.get();
> + if (!opened())
> + return;
I think you should move the call to opened() down so that the method is called on the context thread.
> +
> + if (!m_scriptExecutionContext->isContextThread()) {
> + m_scriptExecutionContext->postTask(CloseSyncDatabaseOnContextThreadTask::create(this));
> + return;
> + }
> +
> + DatabaseTracker::tracker().removeOpenDatabase(this);
> +
> + closeDatabase();
> }> DatabaseSync::~DatabaseSync()
> {
> ASSERT(m_scriptExecutionContext->isContextThread());
> + closeImmediately();
> }
Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
http://www.artima.com/cppsource/nevercall.html
A seperate, private, non-virtual DatabaseSync::closeAndRemoveFromTracker() method thats only be called on the context thread may help that can be called from the virtual closeImmediately and from the dtor.
Created attachment 59713[details]
patch #1: implement DatabaseSync::openDatabaseSync()
(In reply to comment #32)
> (From update of attachment 59696[details])
> > +void DatabaseSync::closeImmediately()
> > {
> > - ASSERT(m_scriptExecutionContext->isContextThread());
> > - return m_scriptExecutionContext.get();
> > + if (!opened())
> > + return;
>
> I think you should move the call to opened() down so that the method is called on the context thread.
done.
> > DatabaseSync::~DatabaseSync()
> > {
> > ASSERT(m_scriptExecutionContext->isContextThread());
> > + closeImmediately();
> > }
>
> Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
> http://www.artima.com/cppsource/nevercall.html
gotta love C++... i changed the call to closeImmediately() to:
if (opened()) {
DatabaseTracker.tracker().removeOpenDatabase(this);
closeDatabase();
}
Comment on attachment 59713[details]
patch #1: implement DatabaseSync::openDatabaseSync()
This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.
WebCore/storage/AbstractDatabase.cpp:137
+ if (origin.endsWith("/"))
I still don't understand how origin can ever end with a /
WebCore/storage/AbstractDatabase.cpp:390
+ DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
Does this mean that this table name is visible to web content? That would be unfortunate.
(In reply to comment #35)
> (From update of attachment 59713[details])
> This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.
>
> WebCore/storage/AbstractDatabase.cpp:137
> + if (origin.endsWith("/"))
> I still don't understand how origin can ever end with a /
Techinically, SecurityOrigin::toString() can return "file://". However, I don't think it matters: we just need a string that uniquely identifies the origin + DB name, and adding another '/' in case of "file://" doesn't change that. So I removed the check and changed this code to always use origin + '/' + name as a unique DB identifier.
> WebCore/storage/AbstractDatabase.cpp:390
> + DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
> Does this mean that this table name is visible to web content? That would be unfortunate.
It's not. DatabaseAuthorizer doesn't allow the web app to do anything on this table (not even read it).
Created attachment 59831[details]
patch #1: implement DatabaseSync::openDatabaseSync()
Same patch, with minor changes:
1. Made ScriptExecutionContext aware of sync DBs, because we need a place to close them before the destructor runs (calling DatabaseTracker::removeOpenDatabase() in the destructor leads to weird things in Chromium).
2. Because of #1, moved 2 sync DB-specific lines from the loop in ScriptExecutionContext::stopDatabases() to Database::stop().
3. Commented out some test cases in the layout test, because they fail in V8. The failure happens because unlike JSC, v8 suppresses the errors in toString(). I'm investigating why this is so, and what can be done to make JSC and V8 behave the same way.
Attachment 59831[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5]
> WebCore/ChangeLog:15: Line contains tab character. [whitespace/tab] [5]
> Total errors found: 2 in 16 files
Will fix these on landing or when uploading a new version of the patch.
Comment on attachment 59833[details]
patch #1: implement DatabaseSync::openDatabaseSync()
I see where ScriptExecutionContext's collection of open databases has been modified to accept DatabaseSync instances as well as Database instances, but I don't see where the DatabaseSync instances are ever added/removed to/from that collection. Maybe i'm missing something, could you walk me thru how that happens?
Comment on attachment 60066[details]
patch #1: implement DatabaseSync::openDatabaseSync()
lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)
> + Reviewed by NOBODY (OOPS!).
> +
> + Implementing DatabaseSync::openDatabaseSync().
> + https://bugs.webkit.org/show_bug.cgi?id=40607
> +
> + 1. Moved some common code from Database to AbstractDatabase.
> + 2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> + need to interact with DatabaseThread.
> + 3. Removed the m_creationCallback field, since it's only needed in
> + the openDatabase{Sync} methods.
> + 4. Made ScriptExecutionContext aware of sync DBs, so we can close
> + the sync DBs before the destructor is called.
> + 5. Moved a couple of async DB-specific lines from
> + ScriptExecutionContext::stopDatabases() to Database::stop().
> lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)
>
> > + Reviewed by NOBODY (OOPS!).
> > +
> > + Implementing DatabaseSync::openDatabaseSync().
> > + https://bugs.webkit.org/show_bug.cgi?id=40607
> > +
> > + 1. Moved some common code from Database to AbstractDatabase.
> > + 2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> > + need to interact with DatabaseThread.
> > + 3. Removed the m_creationCallback field, since it's only needed in
> > + the openDatabase{Sync} methods.
> > + 4. Made ScriptExecutionContext aware of sync DBs, so we can close
> > + the sync DBs before the destructor is called.
> > + 5. Moved a couple of async DB-specific lines from
> > + ScriptExecutionContext::stopDatabases() to Database::stop().
oops, will do on landing.
Attachment 60325[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 65 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
Fixed. Will wait for more comments before I upload a new patch.
Created attachment 60412[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
Same patch, should fix the style problem, and should build on Chromium.
Created attachment 60445[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
Addressed the problems Michael and I talked about offline:
1. An attempt to run a transaction inside another transaction should immediately result in an exception being thrown; added a test for that too.
2. Removed the SQLTransactionSyncClient class, and changed SQLTransactionClient to be usable by both sync and async transactions.
3. Changed SQLStatementSync.cpp to be a 'svn copy' of SQLStatement.cpp.
4. Replaced 'typedef int ExceptionCode' with '#include "ExceptionCode.h"' in all DB header files.
5. Changed the name of the SQLTransactionClient::didCommitTransaction() method to didCommitWriteTransaction().
I also changed '#include <wtf/Threading.h>' to '#include <wtf/ThreadSafeShared.h>' in a few callbacks, removed a few unnecessary #includes in SQLTransactionClientChromium.cpp, and moved the code that runs a transaction to a separate DatabaseSync::runTransaction() method, so it can be shared by DatabaseSync::transaction() and DatabaseSync::changeVersion().
Attachment 60445[details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/SQLTransactionSync.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 77 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/SQLTransactionSync.cpp:39: Alphabetical sorting problem. [build/include_order] [4]
> Total errors found: 1 in 77 files
Fixed, will wait for more comments before re-uploading the patch.
Comment on attachment 60445[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
This patch is huge. I only reviewed the first half of it and there's already some concerning stuff. :(
WebCore/storage/DatabaseSync.cpp:95
+ return new ChangeVersionPreflightStep(expectedVersion);
AdoptRef ??? This looks like a leak.
WebCore/storage/DatabaseSync.cpp:115
+ ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
explicit? Please make this more than one line.
WebCore/storage/DatabaseSync.cpp:123
+ return new ChangeVersionPostflightStep(newVersion);
adoptRef. If you haven't already, please read http://webkit.org/coding/RefPtr.html
WebCore/storage/DatabaseSync.cpp:138
+ ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
explicit, multiline.
WebCore/storage/DatabaseSync.cpp:162
+ if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
Can these return ec and also true? If not, that seems redundant.
WebCore/storage/SQLStatementSync.cpp:27
+ */
missing space
WebCore/storage/SQLStatementSync.cpp:59
+ bool SQLStatement::execute(Database* db)
This function is too complicated for me to understand at 4am.
WebCore/storage/SQLStatementSync.cpp:2
+ * Copyright (C) 2010 Google Inc. All rights reserved.
Woah there. You can't just change the copyright on a file from Apple to Google. What's going on here with the license block?
WebCore/storage/SQLStatementSync.cpp:75
+ for (unsigned int i = 0; i < m_arguments.size(); ++i) {
We usually just say "unsigned"
WebCore/storage/SQLStatementSync.h:54
+
Extra blank line.
WebCore/storage/SQLTransaction.h:
+ typedef int ExceptionCode;
Who review this line of code? It's nonsense. (Hope it wasn't me!)
WebCore/storage/SQLTransactionClient.h:44
+ class SQLTransactionClient : public Noncopyable {
If this is a client, why aren't the methods virtual?
WebCore/storage/SQLTransactionSync.cpp:80
+ PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
again, too big for my little brain.
Created attachment 60812[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
> This patch is huge.
the layout tests are probably more than half of it. another good chunk of it is changes to build files.
> I only reviewed the first half of it and there's already some concerning stuff. :(
happy to fix them. :)
> WebCore/storage/DatabaseSync.cpp:95
> + return new ChangeVersionPreflightStep(expectedVersion);
> AdoptRef ??? This looks like a leak.
fixed.
> WebCore/storage/DatabaseSync.cpp:115
> + ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
> explicit? Please make this more than one line.
done.
> WebCore/storage/DatabaseSync.cpp:123
> + return new ChangeVersionPostflightStep(newVersion);
> adoptRef. If you haven't already, please read http://webkit.org/coding/RefPtr.html
done. i thought PassRefPtr::PassRefPtr(T*) and adoptRef() did the same thing.
> WebCore/storage/DatabaseSync.cpp:138
> + ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
> explicit, multiline.
done.
> WebCore/storage/DatabaseSync.cpp:162
> + if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
> Can these return ec and also true? If not, that seems redundant.
you're right, they are redundant. fixed, made these functions return an ExceptionCode.
> WebCore/storage/SQLStatementSync.cpp:27
> + */
> missing space
not sure what you mean, didn't change anything.
> WebCore/storage/SQLStatementSync.cpp:59
> + bool SQLStatement::execute(Database* db)
> This function is too complicated for me to understand at 4am.
SQLStatement{Sync}::execute() do the following:
1. changes the authorizer mode to read-only, if it's a read-only transaction (to make sure write-statements are disallowed).
2. prepares the statement.
3. makes sure the number of arguments matches the number of '?'s in the statements.
4. bind all arguments.
5. step through the statement, and add data to the result set, one row at a time.
> WebCore/storage/SQLStatementSync.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> Woah there. You can't just change the copyright on a file from Apple to Google. What's going on here with the license block?
oops, reverted, and added a 1-line google copyright. also fixed the copyright notice in all other sync classes (they should all have the google copyright header).
> WebCore/storage/SQLStatementSync.cpp:75
> + for (unsigned int i = 0; i < m_arguments.size(); ++i) {
> We usually just say "unsigned"
fixed.
> WebCore/storage/SQLStatementSync.h:54
> +
> Extra blank line.
removed.
> WebCore/storage/SQLTransaction.h:
> + typedef int ExceptionCode;
> Who review this line of code? It's nonsense. (Hope it wasn't me!)
it was around since... forever. michael had the same comment, so i removed the 'typedef int ExceptionCode;' line from all DB header files and added '#include "ExceptionCode.h"'.
> WebCore/storage/SQLTransactionClient.h:44
> + class SQLTransactionClient : public Noncopyable {
> If this is a client, why aren't the methods virtual?
made them virtual, even though this is the only implementation we have (well, 1 implementation for chromium, and 1 for all other ports), and i think it's quite unlikely we'll ever have another one.
> WebCore/storage/SQLTransactionSync.cpp:80
> + PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
> again, too big for my little brain.
same code as SQLTransaction::executeSQL(), only instead of storing a SQLError, we set a SQLException.
"typedef int ExceptionCode" was historically the standard way to “forward-declare” the ExceptionCode type, so we didn’t have to include ExceptionCode.h in every single header.
The idea was that header contained the actual exception code constants, but if you only needed the WebCore::ExceptionCode type you could typedef it to int. Any files that included both your header and ExceptionCode.h would end up checking it for you. If the typedef didn't match the compiler would generate an error.
Still-older code simply used int, which I felt was less self-documenting.
It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.
> It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.
Thanks, I didn't know that. It seems suboptimal to duplicate that declaration everywhere. I'm glad we're moving away from that pattern.
Comment on attachment 60817[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
This is a big patch, I haven't looked at the test cases yet, but I am glad that you have put together a bunch of them to verify that this code works.
WebCore/storage/DatabaseSync.cpp:159
+ RefPtr<SQLTransactionSync> transaction = transactionPrP;
Do you need to local variable here? Seems like deref'ing the prp is all you need.
WebCore/storage/SQLStatementSync.cpp:122
+ return resultSet.release();
Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
WebCore/storage/SQLTransactionClient.h:49
+ virtual bool didExceedQuota(AbstractDatabase*);
These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..
WebCore/storage/SQLTransactionSync.cpp:142
+ ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?
Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.
void changeVersion(...) {
trans.begin();
read version from db and compare with expected;
trans.execute(); // invoke the user callback
write new version to db;
trans.commit();
}
wdyt?
WebCore/storage/SQLTransactionSync.cpp:208
+ return postflightExceptionCode;
Ditto shouldn't we be doing this in the transaction.
(In reply to comment #63)
> WebCore/storage/SQLStatementSync.cpp:122
> + return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
fwiw, the way it is written is the preferred usage.
"If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html
(In reply to comment #64)
> (In reply to comment #63)
> > WebCore/storage/SQLStatementSync.cpp:122
> > + return resultSet.release();
> > Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
>
> fwiw, the way it is written is the preferred usage.
>
> "If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html
Yes, the release() is preferred.
If you forget to do the release(), then the reference count gets incremented and then decremented. If you remember to do the release(), then the reference count isn't touched. So it's a bit more efficient if you do the release(). There’s no correctness problem either way.
> WebCore/storage/DatabaseSync.cpp:159
> + RefPtr<SQLTransactionSync> transaction = transactionPrP;
> Do you need to local variable here? Seems like deref'ing the prp is all you need.
removed the runTransaction() method, so this statement went away.
> WebCore/storage/SQLStatementSync.cpp:122
> + return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
as Dave and Darin have said, this should be the right way to do this.
> WebCore/storage/SQLTransactionClient.h:49
> + virtual bool didExceedQuota(AbstractDatabase*);
> These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..
reverted. i'll see if adam has any objection to this.
> WebCore/storage/SQLTransactionSync.cpp:142
> + ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
> This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?
i think the spec contradicts itself: http://dev.w3.org/html5/webdatabase/#dom-database-sync-changeversion. on one hand, it says that check version + invoke callback + update version should be done atomically. on the other hand, if you look at the step by step description, it looks to me like things should be done in this order:
1. begin transaction
2. check version
3. invoke callback
4. commit transaction
5. update version
i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we should probably make it more clear in the spec too).
> Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.
>
> void changeVersion(...) {
> trans.begin();
> read version from db and compare with expected;
> trans.execute(); // invoke the user callback
> write new version to db;
> trans.commit();
> }
>
> wdyt?
done. initially i thought that "optional transaction steps" might be cool, but now i'm thinking that it's quite unlikely that we'll ever need to use them for anything else.
> WebCore/storage/SQLTransactionSync.cpp:208
> + return postflightExceptionCode;
> Ditto shouldn't we be doing this in the transaction.
done.
Created attachment 61025[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
Same patch, should make gcc happy this time.
Comment on attachment 61025[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.
51 static SQLTransactionClient* transactionClient()
52 {
53 DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
54 return &transactionClient;
55 }
I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.
Created attachment 61134[details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
(In reply to comment #70)
> (From update of attachment 61025[details])
> I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.
>
> 51 static SQLTransactionClient* transactionClient()
> 52 {
> 53 DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
> 54 return &transactionClient;
> 55 }
>
> I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.
done.
Hi webkit reviewers that happen to be chromium developers too. This patch makes it possible to use sync db api in workers. Would be nice to get it into M6.
Ooops, it looks like the change actually committed is still setting the version outside of the transaction?
93 void DatabaseSync::changeVersion(const String& oldVersion, const String& newVersion,
...
125 if ((ec = transaction->commit()))
126 return;
127
128 setExpectedVersion(newVersion);
129 }
> > Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill
> > for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly
> > modified form of runTransaction() essentially.
> >
> > void changeVersion(...) {
> > trans.begin();
> > read version from db and compare with expected;
> > trans.execute(); // invoke the user callback
> > write new version to db;
> > trans.commit();
> > }
> >
> > wdyt?
>
> > WebCore/storage/SQLTransactionSync.cpp:208
> > + return postflightExceptionCode;
> > Ditto shouldn't we be doing this in the transaction.
>
> done.
>
> i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update
> version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we
> should probably make it more clear in the spec too).
(In reply to comment #78)
> Created an attachment (id=62275) [details]
> patch #4: interrupt all DB operations when the worker is shutting down
Can you move this patch to a separate bug? This could serve as an umbrella bug, but for the sake of committing/reviewing, each bug should have one patch.
Comment on attachment 62275[details]
patch #4: interrupt all DB operations when the worker is shutting down
moving patch #4 to bug 42843, and closing this bug.
2010-06-14 20:29 PDT, Dumitru Daniliuc
2010-06-17 03:05 PDT, Dumitru Daniliuc
2010-06-17 14:43 PDT, Dumitru Daniliuc
2010-06-18 04:30 PDT, Dumitru Daniliuc
dumi: commit-queue-
2010-06-21 18:04 PDT, Dumitru Daniliuc
2010-06-23 12:58 PDT, Dumitru Daniliuc
2010-06-23 15:17 PDT, Dumitru Daniliuc
2010-06-23 16:14 PDT, Dumitru Daniliuc
2010-06-24 13:49 PDT, Dumitru Daniliuc
2010-06-24 17:32 PDT, Dumitru Daniliuc
2010-06-26 04:56 PDT, Dumitru Daniliuc
2010-06-26 05:19 PDT, Dumitru Daniliuc
2010-06-29 16:03 PDT, Dumitru Daniliuc
dumi: commit-queue-
2010-07-01 18:51 PDT, Dumitru Daniliuc
2010-07-02 15:24 PDT, Dumitru Daniliuc
2010-07-03 02:54 PDT, Dumitru Daniliuc
dumi: commit-queue-
2010-07-07 17:34 PDT, Dumitru Daniliuc
2010-07-07 18:03 PDT, Dumitru Daniliuc
2010-07-09 01:34 PDT, Dumitru Daniliuc
2010-07-09 02:56 PDT, Dumitru Daniliuc
2010-07-09 18:21 PDT, Dumitru Daniliuc
dumi: commit-queue-
2010-07-22 01:47 PDT, Dumitru Daniliuc