Bug 43625 - Make SQLiteDatabase ref counted
Summary: Make SQLiteDatabase ref counted
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-06 09:38 PDT by Jeremy Orlow
Modified: 2010-08-09 04:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (39.92 KB, patch)
2010-08-06 09:45 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-08-06 09:38:23 PDT
Make SQLiteDatabase ref counted
Comment 1 Jeremy Orlow 2010-08-06 09:45:10 PDT
Created attachment 63732 [details]
Patch
Comment 2 Early Warning System Bot 2010-08-06 09:51:11 PDT
Attachment 63732 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3628515
Comment 3 Eric Seidel (no email) 2010-08-06 09:59:09 PDT
Attachment 63732 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3612635
Comment 4 Jeremy Orlow 2010-08-06 10:00:55 PDT
So far looks like simple breakage.  Will fix before landing + watch bots when I land.
Comment 5 Dumitru Daniliuc 2010-08-06 14:09:07 PDT
I'm not sure we should make SQLiteDatabase ref-counted. It's just a simple wrapper around sqlite3* + C++ interface to some sqlite functions, and I think it should stay that way. Also, don't you want to pass around some state associated with this DB, too? In that case, it seems to me that it might be better to have a ref-counted "wrapper" with additional state (like AbstractDatabase), and pass around instances of that class.

I think you also missed a couple of references to this class: WebCore/loader/icon/IconDatabase and WebCore/loader/appcache/ApplicationCacheLoader
Comment 6 WebKit Review Bot 2010-08-06 15:28:28 PDT
Attachment 63732 [details] did not build on win:
Build output: http://queues.webkit.org/results/3656253
Comment 7 Michael Nordman 2010-08-06 16:32:19 PDT
I'm with dumi on this one. Making that class refcounted adds complexity that would be nice to avoid. For the indexedDB use case, can you create a new refcounted class to hold the instance to be shared?
Comment 8 WebKit Review Bot 2010-08-06 21:48:25 PDT
Attachment 63732 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3566984
Comment 9 Jeremy Orlow 2010-08-08 15:06:32 PDT
The use case here is that IDBFactory needs to open the database and do some initial reads before creating an IDBDatabase class which will then take ownership.  So I can just heap allocate it and then pass it in to the IDBDatabase class and it'll work.

But we use ref counted objects in most other cases like this.  That's why ref counting is so heavily optimized: it's often used for cases where there aren't many links.

But I don't care that much, so I'll close this for now.  Personally I think the object probably should have been ref counted to begin wtih.
Comment 10 Jeremy Orlow 2010-08-09 04:39:14 PDT
On the other hand, the direction WebKit is headed is to never do a new outside of a create() factory.  This is going to force me to do that (and I believe it's been done in other parts of the code)?  Thus this really should be at a very have a private constructor and a factory that returns a PassOwnPtr.  Of course the other SQL wrappers should probably have the same thing.  But that'll make stack allocating stuff much less clean (syntax wise).  So maybe it's worth punting on this for now....but the trend is for code like this to go away.