Bug 107475 - webdatabase: Split openDatabase() between front and back end work
Summary: webdatabase: Split openDatabase() between front and back end work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 107535 107784 108275 108278 108279 108431 108724 108759 108988 108995
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-21 13:52 PST by Mark Lam
Modified: 2013-02-06 13:54 PST (History)
11 users (show)

See Also:


Attachments
work in progress 1. (244.51 KB, patch)
2013-01-29 17:35 PST, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress 2 (112.18 KB, patch)
2013-02-01 11:45 PST, Mark Lam
no flags Details | Formatted Diff | Diff
work in progress 3: time to test on the EWS bots. (36.88 KB, patch)
2013-02-05 18:18 PST, Mark Lam
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
work in progress 4: let's check the ews bots again. (42.85 KB, patch)
2013-02-05 22:32 PST, Mark Lam
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
work in progress 5: let's check those ews bots again. =( (42.97 KB, patch)
2013-02-05 22:53 PST, Mark Lam
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
the patch. (43.92 KB, patch)
2013-02-05 23:42 PST, Mark Lam
no flags Details | Formatted Diff | Diff
the patch with comments fixed up. (46.72 KB, patch)
2013-02-06 01:46 PST, Mark Lam
no flags Details | Formatted Diff | Diff
No more gotos. (46.86 KB, patch)
2013-02-06 13:41 PST, Mark Lam
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-01-21 13:52:53 PST
1. Need to refactor the openDatabase() process to isolate the front-end script activity from back-end database activity.

Also, ...
2. Rename AbstractDatabase to DatabaseBackend.
    - Currently, this is just a rename operation.  In a subsequent task, we'll change Database and DatabaseSync to delegate to DatabaseBackend instead of inheriting from it.
3. Rename DBBackend::Server to DatabaseServer.
    - This simplifies things.  I realized that splitting out a DBBackend namespace to encapsulate all backend code isn't worth the effort yet.
4. Create a DatabaseBackendContext that the database backend uses.
    - Currently, it is only a wrapper which inherits from DatabaseContext.  Later on, we'll need to split the 2 apart.
5. Fix up add/removeOpenDatabase() so that they are called only when a database has been opened and closed respectively.
    - Previously, client code needs to call removeOpenDatabase() directly, and the Database constructor automatically calls addOpenDatabase() even when the database has not been opened yet.
    - Currently, addOpenDatabase() is only called if the database is successfully opened, and removeOpenDatabase() is only called when the database is closed.
    - In addition, we ensure that the database destructor will always close the database if it is still opened.
Comment 1 Mark Lam 2013-01-29 17:35:20 PST
Created attachment 185358 [details]
work in progress 1.

Uploading in case anyone wants to take a preliminary look at this.  This patch builds should build but still need to self-review (after the latest svn up which changed some logic) and re-test.  In it's current form, it shows how all the pieces come together for this refactor, but the patch as a whole is big.  I will break them out into smaller pieces in separate bugs.
Comment 2 Mark Lam 2013-02-01 11:45:42 PST
Created attachment 186093 [details]
work in progress 2
Comment 3 Mark Lam 2013-02-05 18:18:25 PST
Created attachment 186740 [details]
work in progress 3: time to test on the EWS bots.
Comment 4 WebKit Review Bot 2013-02-05 18:21:35 PST
Attachment 186740 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h', u'Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp', u'Source/WebCore/Modules/webdatabase/Database.cpp', u'Source/WebCore/Modules/webdatabase/Database.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackend.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendAsync.h', u'Source/WebCore/Modules/webdatabase/DatabaseBackendSync.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseBackendSync.h', u'Source/WebCore/Modules/webdatabase/DatabaseError.h', u'Source/WebCore/Modules/webdatabase/DatabaseManager.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseManager.h', u'Source/WebCore/Modules/webdatabase/DatabaseServer.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseServer.h', u'Source/WebCore/Modules/webdatabase/DatabaseSync.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseSync.h', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp', u'Source/WebCore/Modules/webdatabase/DatabaseTracker.h', u'Source/WebCore/Modules/webdatabase/WorkerContextWebDatabase.cpp', u'Source/WebCore/Modules/webdatabase/chromium/DatabaseTrackerChromium.cpp']" exit_code: 1
Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h:55:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 WebKit Review Bot 2013-02-05 18:25:30 PST
Comment on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.

Attachment 186740 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373911
Comment 6 Early Warning System Bot 2013-02-05 18:26:08 PST
Comment on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.

Attachment 186740 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16374975
Comment 7 kov's GTK+ EWS bot 2013-02-05 18:27:17 PST
Comment on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.

Attachment 186740 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16389044
Comment 8 Early Warning System Bot 2013-02-05 18:31:04 PST
Comment on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.

Attachment 186740 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16376970
Comment 9 Mark Lam 2013-02-05 18:38:15 PST
Comment on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.

Time to stop the EWS bleeding.  =(
Comment 10 Mark Lam 2013-02-05 22:32:57 PST
Created attachment 186759 [details]
work in progress 4: let's check the ews bots again.
Comment 11 WebKit Review Bot 2013-02-05 22:37:46 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16398005
Comment 12 Early Warning System Bot 2013-02-05 22:38:08 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16394066
Comment 13 WebKit Review Bot 2013-02-05 22:38:16 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16393077
Comment 14 EFL EWS Bot 2013-02-05 22:38:47 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16394068
Comment 15 Early Warning System Bot 2013-02-05 22:40:54 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16391122
Comment 16 Peter Beverloo (cr-android ews) 2013-02-05 22:41:39 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Attachment 186759 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16388154
Comment 17 Mark Lam 2013-02-05 22:44:52 PST
Comment on attachment 186759 [details]
work in progress 4: let's check the ews bots again.

Arggg... GCC!!!  Oh well.  Will upload another patch shortly with some unreachable code to placate GCC because it doesn't have enough smarts to infer that the end of the function DatabaseManager::exceptionCodeForDatabaseError() is unreachable.
Comment 18 Mark Lam 2013-02-05 22:53:56 PST
Created attachment 186761 [details]
work in progress 5: let's check those ews bots again. =(
Comment 19 WebKit Review Bot 2013-02-05 23:00:12 PST
Comment on attachment 186761 [details]
work in progress 5: let's check those ews bots again. =(

Attachment 186761 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16388161
Comment 20 Mark Lam 2013-02-05 23:42:16 PST
Created attachment 186767 [details]
the patch.
Comment 21 Mark Lam 2013-02-06 01:46:36 PST
Created attachment 186791 [details]
the patch with comments fixed up.
Comment 22 Anders Carlsson 2013-02-06 13:15:43 PST
Comment on attachment 186791 [details]
the patch with comments fixed up.

View in context: https://bugs.webkit.org/attachment.cgi?id=186791&action=review

> Source/WebCore/Modules/webdatabase/DatabaseManager.cpp:243
> +    if (!backend) {
> +        ASSERT(error != DatabaseError::None);
> +
> +        switch (error) {
> +        case DatabaseError::DatabaseIsBeingDeleted:
> +        case DatabaseError::DatabaseSizeOverflowed:
> +        case DatabaseError::GenericSecurityError:
> +            goto handleError;
> +
> +        case DatabaseError::InvalidDatabaseState:
> +            goto handleInvalidDatabaseState;

Please restructure the code so you can avoid these gotos.
Comment 23 Mark Lam 2013-02-06 13:41:44 PST
Created attachment 186911 [details]
No more gotos.
Comment 24 Mark Lam 2013-02-06 13:54:20 PST
Thanks for the review.  Landed in r142030: <http://trac.webkit.org/changeset/142030>.