Bug 69620 - IndexedDB: Remove SQLite backing store
Summary: IndexedDB: Remove SQLite backing store
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-07 06:03 PDT by Hans Wennborg
Modified: 2011-11-11 04:19 PST (History)
5 users (show)

See Also:


Attachments
Patch (74.42 KB, patch)
2011-10-07 06:07 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (74.28 KB, patch)
2011-10-11 03:58 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (74.38 KB, patch)
2011-10-12 02:34 PDT, Hans Wennborg
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-10-07 06:03:21 PDT
IndexedDB: Remove SQLite backing store
Comment 1 Hans Wennborg 2011-10-07 06:07:04 PDT
Created attachment 110133 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-07 06:09:49 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Joshua Bell 2011-10-10 16:31:32 PDT
Comment on attachment 110133 [details]
Patch

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

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:47
> +    return securityOrigin->databaseIdentifier();

Won't this mean the user will lose existing LevelDB-backed IndexedDBs, since the file identifier will change? (i.e. should we use a "@1" suffix until we intentionally break LevelDB store compatibility?)

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102
> +    const String fileIdentifier = computeFileIdentifier(securityOrigin.get());

This instance of fileIdentifier is unused and can be removed. (Leftover from a previous refactor, my fault)

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
>  #if USE(LEVELDB)

Planning on a follow-up CL for eliminating USE(LEVELDB) ?
Comment 4 Darin Fisher (:fishd, Google) 2011-10-10 21:52:57 PDT
Comment on attachment 110133 [details]
Patch

R- based on jsbell's feedback
Comment 5 Hans Wennborg 2011-10-11 03:55:08 PDT
(In reply to comment #3)
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:47
> > +    return securityOrigin->databaseIdentifier();
> 
> Won't this mean the user will lose existing LevelDB-backed IndexedDBs, since the file identifier will change? (i.e. should we use a "@1" suffix until we intentionally break LevelDB store compatibility?)

You're right! I wasn't thinking. Good catch.

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102
> > +    const String fileIdentifier = computeFileIdentifier(securityOrigin.get());
> 
> This instance of fileIdentifier is unused and can be removed. (Leftover from a previous refactor, my fault)

Removed.

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:132
> >  #if USE(LEVELDB)
> 
> Planning on a follow-up CL for eliminating USE(LEVELDB) ?

Yes. Not decided on the details yet, though.
Comment 6 Hans Wennborg 2011-10-11 03:58:06 PDT
Created attachment 110494 [details]
Patch
Comment 7 Joshua Bell 2011-10-11 10:01:09 PDT
Comment on attachment 110494 [details]
Patch

LGTM
Comment 8 David Grogan 2011-10-11 13:12:43 PDT
Should we plan on keeping the @1 in perpetuity?  If so we should abstract it out instead of leaving the magic constant.
Comment 9 Joshua Bell 2011-10-11 13:13:40 PDT
(In reply to comment #8)
> Should we plan on keeping the @1 in perpetuity?  If so we should abstract it out instead of leaving the magic constant.

+1 - At the very least, a comment explaining what it means and when to touch it.
Comment 10 Hans Wennborg 2011-10-12 02:33:17 PDT
(In reply to comment #8)
> Should we plan on keeping the @1 in perpetuity?  If so we should abstract it out instead of leaving the magic constant.

Right. Sticking it in a carefully named variable.
Comment 11 Hans Wennborg 2011-10-12 02:34:56 PDT
Created attachment 110656 [details]
Patch
Comment 12 David Grogan 2011-10-12 10:00:13 PDT
LGTM
Comment 13 Hans Wennborg 2011-10-13 01:45:05 PDT
Darin: would you like to take another look?
Comment 14 Darin Fisher (:fishd, Google) 2011-10-14 01:36:07 PDT
Comment on attachment 110656 [details]
Patch

R=me

Apologies for the delay!  Please feel free to send me direct email in the future when you are stuck waiting on me.
Comment 15 Hans Wennborg 2011-10-14 02:14:48 PDT
Committed r97453: <http://trac.webkit.org/changeset/97453>
Comment 16 Donggwan Kim 2011-11-10 18:40:42 PST
I have a question about this patch. I'm developing IndexedDB implementation for JSC using SQLiteBackingStore. is there any reason for removing SQLiteBackingStore?
is that discussed in webkit dev community? If so, i've missed it. please let me know about this patch history.
Comment 17 Hans Wennborg 2011-11-11 04:19:41 PST
(In reply to comment #16)
> I have a question about this patch. I'm developing IndexedDB implementation for JSC using SQLiteBackingStore. is there any reason for removing SQLiteBackingStore?

It was removed because it was unused. We did not want to maintain an unused back-end, and there were features in IndexedDB (such as compound keys), for which we did not have a plan for how to implement using SQLite.

Can you switch to using the LevelDB backing store instead?

> is that discussed in webkit dev community? If so, i've missed it. please let me know about this patch history.

I apologize for not raising this on the mailing list. We don't generally hear much about the IndexedDB implementation, so it was my impression that no-one was dependent on this code.