Bug 39041 - Move the code that should be shared by sync and async DBs to a separate class
Summary: Move the code that should be shared by sync and async DBs to a separate class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks: 34990
  Show dependency treegraph
 
Reported: 2010-05-12 20:41 PDT by Dumitru Daniliuc
Modified: 2010-06-30 04:46 PDT (History)
8 users (show)

See Also:


Attachments
patch #1: Add a base class for Database and DatabaseSync (11.26 KB, patch)
2010-05-12 21:20 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #2: clean up (24.42 KB, patch)
2010-05-13 16:26 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: Move the reusable code from Database to AbstractDatabase (74.06 KB, patch)
2010-06-02 04:23 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: Move the reusable code from Database to AbstractDatabase (74.03 KB, patch)
2010-06-02 04:31 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: Move the reusable code from Database to AbstractDatabase (82.02 KB, patch)
2010-06-02 16:05 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: Get DatabaseTracker ready for sync DBs (20.87 KB, patch)
2010-06-10 19:44 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: Get DatabaseTracker ready for sync DBs (21.10 KB, patch)
2010-06-10 19:59 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag (19.27 KB, patch)
2010-06-15 04:11 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag (19.69 KB, patch)
2010-06-15 04:28 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #4: sync and async DB APIs should be enabled/disabled at runtime by the same flag (20.68 KB, patch)
2010-06-15 04:38 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-05-12 20:41:49 PDT
Some code in Database.cpp can be reused by DatabaseSync.cpp. We should move it to a common parent class. A parent class is probably better than a member variable, because then DatabaseTracker could handle both types of DBs in the same way.
Comment 1 Dumitru Daniliuc 2010-05-12 21:20:12 PDT
Created attachment 55936 [details]
patch #1: Add a base class for Database and DatabaseSync
Comment 2 Adam Barth 2010-05-12 22:22:56 PDT
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.
Comment 3 Dumitru Daniliuc 2010-05-12 23:49:41 PDT
patch #1 landed as r59349.
Comment 4 Brady Eidson 2010-05-13 09:39:28 PDT
(In reply to comment #2)
> (From update of attachment 55936 [details])
> Ok.  I think you're over reacting to bradee-oh, but whatever.

Lol.
Comment 5 Dumitru Daniliuc 2010-05-13 16:26:02 PDT
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.
Comment 6 WebKit Review Bot 2010-05-13 16:28:35 PDT
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 7 Adam Barth 2010-05-13 17:00:33 PDT
Comment on attachment 56032 [details]
patch #2: clean up

Looks reasonable overall.  I need to look at it more carefully later.
Comment 8 Dumitru Daniliuc 2010-05-13 17:07:06 PDT
> WebCore/storage/Database.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]

Will take care of this when landing/re-uploading.
Comment 9 Dumitru Daniliuc 2010-05-25 13:33:54 PDT
Ping.
Comment 10 Adam Barth 2010-06-01 11:47:38 PDT
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.
Comment 11 Dumitru Daniliuc 2010-06-01 12:56:15 PDT
> 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.
Comment 12 Dumitru Daniliuc 2010-06-01 14:40:59 PDT
patch #2 landed as r60508.
Comment 13 Dumitru Daniliuc 2010-06-02 04:23:23 PDT
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.
Comment 14 WebKit Review Bot 2010-06-02 04:27:29 PDT
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.
Comment 15 Dumitru Daniliuc 2010-06-02 04:31:50 PDT
Created attachment 57644 [details]
patch #3: Move the reusable code from Database to AbstractDatabase

Same patch, fixed the style errors.
Comment 16 WebKit Review Bot 2010-06-02 05:36:08 PDT
Attachment 57643 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2763107
Comment 17 WebKit Review Bot 2010-06-02 06:29:26 PDT
Attachment 57644 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2858004
Comment 18 Dumitru Daniliuc 2010-06-02 16:05:23 PDT
Created attachment 57711 [details]
patch #3: Move the reusable code from Database to AbstractDatabase

The patch should build on all platforms now.
Comment 19 Adam Barth 2010-06-09 12:12:27 PDT
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 20 Adam Barth 2010-06-09 12:16:58 PDT
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?
Comment 21 Dumitru Daniliuc 2010-06-10 14:46:28 PDT
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.
Comment 22 Michael Nordman 2010-06-10 15:05:48 PDT
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();
Comment 23 Dumitru Daniliuc 2010-06-10 19:44:08 PDT
Created attachment 58437 [details]
patch #3: Get DatabaseTracker ready for sync DBs
Comment 24 WebKit Review Bot 2010-06-10 19:56:49 PDT
Attachment 58437 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3214175
Comment 25 Dumitru Daniliuc 2010-06-10 19:59:38 PDT
Created attachment 58438 [details]
patch #3: Get DatabaseTracker ready for sync DBs
Comment 26 Michael Nordman 2010-06-11 12:30:01 PDT
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?
Comment 27 Dumitru Daniliuc 2010-06-11 12:50:41 PDT
> 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.
Comment 28 Dumitru Daniliuc 2010-06-11 13:12:33 PDT
(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 29 Adam Barth 2010-06-14 13:38:17 PDT
Comment on attachment 58438 [details]
patch #3: Get DatabaseTracker ready for sync DBs

LGTM.  Thanks for making this easy to review.
Comment 30 Dumitru Daniliuc 2010-06-14 15:09:37 PDT
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.
Comment 31 WebKit Review Bot 2010-06-14 15:23:52 PDT
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
Comment 32 Dumitru Daniliuc 2010-06-15 04:11:30 PDT
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().
Comment 33 WebKit Review Bot 2010-06-15 04:16:49 PDT
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.
Comment 34 Eric Seidel (no email) 2010-06-15 04:19:47 PDT
Attachment 58767 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3275191
Comment 35 Early Warning System Bot 2010-06-15 04:23:05 PDT
Attachment 58767 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3340192
Comment 36 Dumitru Daniliuc 2010-06-15 04:28:38 PDT
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.
Comment 37 Eric Seidel (no email) 2010-06-15 04:33:45 PDT
Attachment 58768 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3290181
Comment 38 Dumitru Daniliuc 2010-06-15 04:38:10 PDT
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 39 Adam Barth 2010-06-17 17:02:39 PDT
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?
Comment 40 Dumitru Daniliuc 2010-06-17 17:16:08 PDT
> 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)."
Comment 41 Dumitru Daniliuc 2010-06-18 02:16:53 PDT
patch #4 landed as r61388.
Comment 42 Dumitru Daniliuc 2010-06-30 04:46:57 PDT
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.