RESOLVED FIXED 41404
DB clean up: Make all DatabaseTasks internal Database classes 
https://bugs.webkit.org/show_bug.cgi?id=41404
Summary DB clean up: Make all DatabaseTasks internal Database classes
Dumitru Daniliuc
Reported 2010-06-30 04:51:03 PDT
All DatabaseTasks are used only by the async DBs, so we should "scope" them somehow to the Database class. If we make them internal Database classes, then we can also make some methods that are currently public (like performOpenAndVerify() and close()) private.
Attachments
patch (25.96 KB, patch)
2010-06-30 16:01 PDT, Dumitru Daniliuc 		dumi: commit-queue-
DetailsFormatted DiffDiff
patch (25.99 KB, patch)
2010-06-30 20:04 PDT, Dumitru Daniliuc 		no flags
DetailsFormatted DiffDiff
patch (25.25 KB, patch)
2010-07-02 00:40 PDT, Dumitru Daniliuc 		fishd: review+
dumi: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dumitru Daniliuc
Comment 1 2010-06-30 16:01:49 PDT
Created attachment 60160 [details] patch
Michael Nordman
Comment 2 2010-06-30 18:35:21 PDT
Gotta love patches that delete more code than they add ;) Really glad to see this cleanup. Like we talked about offline, i'm not sure having the db thread call the virtual closeImmediately() method is an improvement over the simpler more direct close() method.
Dumitru Daniliuc
Comment 3 2010-06-30 20:04:32 PDT
Created attachment 60187 [details] patch (In reply to comment #2) > Like we talked about offline, i'm not sure having the db thread call the virtual closeImmediately() method is an improvement over the simpler more direct close() method. done.
Michael Nordman
Comment 4 2010-07-01 12:54:37 PDT
This LGreatTM but what's going on with the trybots?
Darin Fisher (:fishd, Google)
Comment 5 2010-07-01 15:57:36 PDT
Comment on attachment 60187 [details] patch rs=me, deferring to michael's review.
Dumitru Daniliuc
Comment 6 2010-07-01 16:34:08 PDT
Landed: r62321.
WebKit Review Bot
Comment 7 2010-07-01 16:45:49 PDT
http://trac.webkit.org/changeset/62321 might have broken Chromium Linux Release
Dumitru Daniliuc
Comment 8 2010-07-01 17:24:49 PDT
Reopening, patch broke the Mac build, so I had to revert it.
Dumitru Daniliuc
Comment 9 2010-07-02 00:40:49 PDT
Created attachment 60344 [details] patch Same patch, with minor syntax changes. The Windows, Mac, Qt and Chromium/Win ports have built successfully with this patch applied.
Eric Seidel (no email)
Comment 10 2010-07-02 03:17:30 PDT
Comment on attachment 60187 [details] patch Cleared Darin Fisher's review+ from obsolete attachment 60187 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Darin Fisher (:fishd, Google)
Comment 11 2010-07-02 14:32:48 PDT
Comment on attachment 60344 [details] patch R=me, but you don't need any of the Database:: prefixes inside the class definitions.
Dumitru Daniliuc
Comment 12 2010-07-02 14:42:33 PDT
(In reply to comment #11) > (From update of attachment 60344 [details]) > R=me, but you don't need any of the Database:: prefixes inside the class definitions. Removed.
Dumitru Daniliuc
Comment 13 2010-07-02 15:51:14 PDT
re-landed: r62411.
Note You need to log in before you can comment on or make changes to this bug.