| Summary: | IndexedDB: Use LevelDB also for in-memory databases | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hans Wennborg <hans> | ||||||
| Component: | New Bugs | Assignee: | Hans Wennborg <hans> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dgrogan, steveblock, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Hans Wennborg
2011-09-27 07:46:58 PDT
Created attachment 108850 [details]
Patch
Comment on attachment 108850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review LGTM I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation. Though I suppose our browser tests would pick up some of those. > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113 > + const String name = path.isNull() ? "/in-mem-db" : path; "in-mem-db" isn't magic, right? Nothing in leveldb looks for that string, it's just a dummy? > Source/WebCore/platform/leveldb/LevelDBDatabase.h:66 > + OwnPtr<leveldb::Comparator> m_comparatorAdapter; Did you move this to match the order that the destructor deletes these? (In reply to comment #2) > (From update of attachment 108850 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review > > LGTM > > I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation. Though I suppose our browser tests would pick up some of those. I'm not too concerned about that. Like you said, the browser tests should cover it. We should look into running leveldb's own test suite as part of chromium's tests, but that's out-of-scope for this bug. > > > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113 > > + const String name = path.isNull() ? "/in-mem-db" : path; > > "in-mem-db" isn't magic, right? Nothing in leveldb looks for that string, it's just a dummy? No magic, just a dummy name. > > > Source/WebCore/platform/leveldb/LevelDBDatabase.h:66 > > + OwnPtr<leveldb::Comparator> m_comparatorAdapter; > > Did you move this to match the order that the destructor deletes these? Yes, even though it doesn't matter because we clear them in the right order in the destructor, it bothered me to see them in the wrong order here. Comment on attachment 108850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110 > + if (env) Do you need this check? What's the default value of Options::env ? >>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113 >>> + const String name = path.isNull() ? "/in-mem-db" : path; >> >> "in-mem-db" isn't magic, right? Nothing in leveldb looks for that string, it's just a dummy? > > No magic, just a dummy name. This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory. On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment. > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150 > + OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase); Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think. > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015 > + String path = ":memory:"; Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment. (In reply to comment #4) > > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110 > > + if (env) > > Do you need this check? What's the default value of Options::env ? The default of Options::env is Env::Default(). I guess I could just pass that in explicitly. > >>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113 > >>> + const String name = path.isNull() ? "/in-mem-db" : path; > >> > >> "in-mem-db" isn't magic, right? Nothing in leveldb looks for that string, it's just a dummy? > > > > No magic, just a dummy name. > > This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory. You're right, that is weird. I think we can just scrap this line actually. > > On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment. Technically, it's not really ignored (it will be used as a path in the in-memory filesystem that the Env implements), but passing in an empty path works, so let's do that. > > > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150 > > + OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase); > > Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think. After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and sticking things in OwnPtr was the way to do things? > > > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015 > > + String path = ":memory:"; > > Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment. Yes, that's a magic token for SQLite. I'll add a comment. Created attachment 109018 [details]
Patch
Steve, would you like to take another look? Comment on attachment 109018 [details] Patch > After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and > sticking things in OwnPtr was the way to do things? Yes, you're right. Comment on attachment 109018 [details] Patch Clearing flags on attachment: 109018 Committed r96322: <http://trac.webkit.org/changeset/96322> All reviewed patches have been landed. Closing bug. |