Bug 69620 – IndexedDB: Remove SQLite backing store

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Remove SQLite backing store

[Hans Wennborg]

Created attachment 110133 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Joshua Bell]

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) ?

[Darin Fisher (:fishd, Google)]

Comment on attachment 110133 [details]
Patch

R- based on jsbell's feedback

[Hans Wennborg]

(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.

[Hans Wennborg]

Created attachment 110494 [details]
Patch

[Joshua Bell]

Comment on attachment 110494 [details]
Patch

LGTM

[David Grogan]

Should we plan on keeping the @1 in perpetuity?  If so we should abstract it out instead of leaving the magic constant.

[Joshua Bell]

(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.

[Hans Wennborg]

(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.

[Hans Wennborg]

Created attachment 110656 [details]
Patch

[David Grogan]

LGTM

[Hans Wennborg]

Darin: would you like to take another look?

[Darin Fisher (:fishd, Google)]

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.

[Hans Wennborg]

Committed r97453: <http://trac.webkit.org/changeset/97453>

[Donggwan Kim]

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.

[Hans Wennborg]

(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.