| Summary: | webdatabase: Split openDatabase() between front and back end work | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Mark Lam <mark.lam> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | andersca, beidson, dglazkov, ggaren, gtk-ews, michaeln, peter+ews, sam, webkit-ews, webkit.review.bot, xan.lopez | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 107535, 107784, 108275, 108278, 108279, 108431, 108724, 108759, 108988, 108995 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
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.
Created attachment 186093 [details]
work in progress 2
Created attachment 186740 [details]
work in progress 3: time to test on the EWS bots.
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 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 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 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 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 on attachment 186740 [details]
work in progress 3: time to test on the EWS bots.
Time to stop the EWS bleeding. =(
Created attachment 186759 [details]
work in progress 4: let's check the ews bots again.
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 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 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 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 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 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 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.
Created attachment 186761 [details]
work in progress 5: let's check those ews bots again. =(
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 Created attachment 186767 [details]
the patch.
Created attachment 186791 [details]
the patch with comments fixed up.
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. Created attachment 186911 [details]
No more gotos.
Thanks for the review. Landed in r142030: <http://trac.webkit.org/changeset/142030>. |
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.