Bug 39041

Created attachment 55936 [details] patch #1: Add a base class for Database and DatabaseSync

Comment on attachment 55936 [details] patch #1: Add a base class for Database and DatabaseSync Ok. I think you're over reacting to bradee-oh, but whatever.

patch #1 landed as r59349.

(In reply to comment #2) > (From update of attachment 55936 [details]) > Ok. I think you're over reacting to bradee-oh, but whatever. Lol.

Created attachment 56032 [details] patch #2: clean up Cleaning up the DB classes to make it easier to move code out of them into new classes: removing unused #includes, replacing #includes in header files with forward class declarations as much as possible, removing 'friend' declarations, and adding getters for private fields where necessary.

Attachment 56032 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/storage/Database.cpp:50: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 56032 [details] patch #2: clean up Looks reasonable overall. I need to look at it more carefully later.

> WebCore/storage/Database.cpp:50: Alphabetical sorting problem. [build/include_order] [4] Will take care of this when landing/re-uploading.

Ping.

Comment on attachment 56032 [details] patch #2: clean up Looks great! Nice work. WebCore/storage/Database.h:55 + class SecurityOrigin; I don't think we need the NDEBUG conditional here. It's not a terrible thing to forward declare this class. WebCore/storage/DatabaseTask.cpp:  + MutexLocker locker(m_transaction->database()->m_transactionInProgressMutex); Not 100% sure why we lost this mutex here, but ok. WebCore/storage/Database.cpp:660 + MutexLocker locker(m_transactionInProgressMutex); Oh, I see. It went here.

> WebCore/storage/Database.h:55 > + class SecurityOrigin; > I don't think we need the NDEBUG conditional here. It's not a terrible thing to forward declare this class. done. > WebCore/storage/DatabaseTask.cpp:  > + MutexLocker locker(m_transaction->database()->m_transactionInProgressMutex); > Not 100% sure why we lost this mutex here, but ok. > > WebCore/storage/Database.cpp:660 > + MutexLocker locker(m_transactionInProgressMutex); > Oh, I see. It went here. i made this change to keep m_transactionInProgressMutex private to the Database class.

patch #2 landed as r60508.

Created attachment 57643 [details] patch #3: Move the reusable code from Database to AbstractDatabase patch #3 has mostly mechanical changes, with a couple of exceptions: 1. guidToDatabaseMap() was removed from Database.cpp; it wasn't used by anybody. 2. Database::close() was changed to be callable from any thread by moving the logic from WebKit/chromium/src/WebDatabase.cpp there. All storage/ and fast/workers/storage/ layout tests passed.

Attachment 57643 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp" WebCore/storage/AbstractDatabase.cpp:41: Alphabetical sorting problem. [build/include_order] [4] WebCore/storage/AbstractDatabase.cpp:122: 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] WebCore/storage/AbstractDatabase.h:36: Alphabetical sorting problem. [build/include_order] [4] WebKit/gtk/webkit/webkitwebview.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 4 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 57644 [details] patch #3: Move the reusable code from Database to AbstractDatabase Same patch, fixed the style errors.

Attachment 57643 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2763107

Attachment 57644 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2858004

Created attachment 57711 [details] patch #3: Move the reusable code from Database to AbstractDatabase The patch should build on all platforms now.

Comment on attachment 57711 [details] patch #3: Move the reusable code from Database to AbstractDatabase WebCore/storage/AbstractDatabase.cpp:51 + if (origin.endsWith("/")) This shouldn't ever happen. Origins don't end in "/". WebCore/storage/AbstractDatabase.cpp:48 + static int guidForOriginAndName(const String& origin, const String& name) You should use an anonymous namespace instead of "static". It doesn't matter much, but it's the C++ way. Oh, I guess this is all just moving code around. I thought I was supposed to review the correctness of the code (which is quite complicated). Let me try this again.

Comment on attachment 57711 [details] patch #3: Move the reusable code from Database to AbstractDatabase This is too complicated for me to review. The general approach seems reasonable. The perform* methods don't seem quite ideal... I've also never seen this code before, so it's all new to me. Is there someone more familiar with this stuff who can review?

Let's postpone patch #3, since we don't have a sync implementation yet and don't know what parts will be reused and which ones won't. We can do a refactoring (merge code from Database and DatabaseSync into AbstractDatabase) after DatabaseSync is implemented.

Yup... getting further along in the sync impl may help inform this refactoring effort. I'm not sure having promoted so much to the public interface is an improvement. The set of classes that make up the database system are a tightly couple bunch. The public interface being determined what that set of classes needs really makes it difficult for an innocent consumer of the Database class to know what methods they should be calling from the oustide looking in. ***** DatabaseThread.h Since this class only works with async Database instances, I don't think the unscheduleDatabaseTasks() method should accept an AbstractDatabase* as a parameter. ***** DatabaseThread.cpp It looks like you've renamed the existing non-virtual close(ClosePolicy) method to performClose(ClosePolicy)... and added a new virtual method for close(). I think this callsite can continue to call the renamed non-virtual performClose(ClosePolicy) method. If the class interface is not modified, the changes to SameDatabasePredicate class are not needed. Also, it might be nice to put the SameDatabasePredicate in an anon namespace. ***** ScriptExecutionContext.cpp Since the DBThread only works with async Databases, it seems like the call to unscheduleTasks for AbstactDatabases is out of place in stopDatabases. Hmmm... does the script execution context even need to keep SyncDatabases in its collection of open databases. It looks like the only reason it has the collection is to 'stop' anyhing in async process when the time comes to do so... but with SyncDatabases... there never is anything going on in the background to 'stop'. ***** WebKit\chromium\src\WebDatabase.cpp I'm still getting a handle on what has been virtualized and why so can't comment on this quite yet. ***** AbstractDatabse vs Database virtual void performCreationCallback(); Why virtualize this method, its only called for concrete Databases? virtual void notifyDatabaseThreadDatabaseOpen(); This method feels very out of place in this class. Maybe this is something the 'openTask' could do after having called performOpenAndVerify()? virtual void close(); virtual void stop(); I think you've virtualized this pair of methods to support the following callsites. - ScriptExcecutionContext::stopDatabases() - WebDatabase::closeDatabaseImmediately() The virtual stop() method on the abstract class feels a little out of place. What does 'stop' mean for a sync database class. I'm trying to track down the callsites for close(), it looks like WebDatabase::closeDatabaseImmediately the only usage of that method. Maybe we could use a virtualized method for that? virtual void closeImmediately();

Created attachment 58437 [details] patch #3: Get DatabaseTracker ready for sync DBs

Attachment 58437 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3214175

Created attachment 58438 [details] patch #3: Get DatabaseTracker ready for sync DBs

Comment on attachment 58438 [details] patch #3: Get DatabaseTracker ready for sync DBs This change is so much easier to grok! Fwiw... LGTM WebKit/chromium/src/WebDatabase.cpp:114 + PassRefPtr<SecurityOrigin> originPrp(WebSecurityOrigin::createFromDatabaseIdentifier(originIdentifier)); Maybe use SecurityOrigin::createFromDatabaseIdentifier() directly instead of the awkward local PassRefPtr variable?

> WebKit/chromium/src/WebDatabase.cpp:114 > + PassRefPtr<SecurityOrigin> originPrp(WebSecurityOrigin::createFromDatabaseIdentifier(originIdentifier)); > Maybe use SecurityOrigin::createFromDatabaseIdentifier() directly instead of the awkward local PassRefPtr variable? let's leave it as it is for now, since it's not directly related to this patch. i'll bring this up with darin (fisher) and if he agrees, i'll change this in a separate patch.

(In reply to comment #27) > > WebKit/chromium/src/WebDatabase.cpp:114 > > + PassRefPtr<SecurityOrigin> originPrp(WebSecurityOrigin::createFromDatabaseIdentifier(originIdentifier)); > > Maybe use SecurityOrigin::createFromDatabaseIdentifier() directly instead of the awkward local PassRefPtr variable? > > let's leave it as it is for now, since it's not directly related to this patch. i'll bring this up with darin (fisher) and if he agrees, i'll change this in a separate patch. fixed, after our discussion.

Comment on attachment 58438 [details] patch #3: Get DatabaseTracker ready for sync DBs LGTM. Thanks for making this easy to review.

patch #3 landed as r61154. This bug should stay open until we have a fully working sync implementation of WebSQLDatabases in workers. Then we can move the common code to AbstractDatabase and close this bug.

http://trac.webkit.org/changeset/61154 might have broken Chromium Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/61153 http://trac.webkit.org/changeset/61154 http://trac.webkit.org/changeset/61155

Created attachment 58767 [details] patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag Moved isAvailable() and setIsAvailable() from Database and DatabaseSync to AbstractDatabase. Also, made a cosmetic change to WorkerContext::openDatabaseSync() to make it match the style of WorkerContext::openDatabase().

Attachment 58767 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebsettings.cpp" WebKit/gtk/webkit/webkitwebview.cpp:43: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 58767 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3275191

Attachment 58767 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3340192

Created attachment 58768 [details] patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag Same patch, should fix the style and build problems.

Attachment 58768 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/3290181

Created attachment 58769 [details] patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag Forgot to patch WebCore.base.exp.

Comment on attachment 58769 [details] patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag Great! A couple minor comments. WebCore/storage/AbstractDatabase.cpp:36 + static bool isDatabaseAvailable = true; I forget how the webkit-dev thread turned out. g_isDatabaseAvailable? Not sure. WebCore/workers/WorkerContext.cpp:297 + ec = SECURITY_ERR; Security Error?

> WebCore/storage/AbstractDatabase.cpp:36 > + static bool isDatabaseAvailable = true; > I forget how the webkit-dev thread turned out. g_isDatabaseAvailable? Not sure. Took a look at that thread. Looks like Jeremy + Jon Mason proposed using the "g_" prefix for file-level static variables, Maciej objected, and that was the end of it. > WebCore/workers/WorkerContext.cpp:297 > + ec = SECURITY_ERR; > Security Error? From the DB spec: "The user agent may raise a SECURITY_ERR exception instead of returning a Database object if the request violates a policy decision (e.g. if the user agent is configured to not allow the page to open databases)."

patch #4 landed as r61388.

All common code was moved to AbstractDatabase. There's still some clean up to do in AbstractDatabase; I'll open separate bugs for that as I get to it.