[Webkit-unassigned] [Bug 61000] Control Indexeddb backends from LayoutTestController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61000





--- Comment #12 from Greg Simon <gregsimon at chromium.org>  2011-05-23 11:13:53 PST ---
(In reply to comment #10)
> (From update of attachment 94368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94368&action=review
> 
> > LayoutTests/ChangeLog:7
> > +        can be implemented and tested.
> 
> I think this descriptive text is usually put after the bugzilla url below.
Fixed

> > Source/WebCore/ChangeLog:6
> > +        https://bugs.webkit.org/show_bug.cgi?id=61000
> 
> Should probably have a description here too
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:80
> > +        // Also check that backing store types match: this is important for migration
> 
> period.
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:82
> > +            && (backingStoreType == it->second->backingStore()->backingStoreType())) {
> 
> no need for newline
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96
> > +            bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir);
> 
> i think this should be named hasSQLBackend.. same for hasLevelDBBackend below
Fixed

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:97
> > +            bool hasLevelDbBackend = IDBLevelDBBackingStore::backingStoreExists(securityOrigin.get(), dataDir);
> 
> hmm, now we're doing LevelDB stuff outside of the #if ENABLE(LEVELDB) guard.. i'm not sure that's a good idea

Added ENABLE(LEVELDB) guards

> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:102
> > +            // Migration: if the database exists and is SQLite we want to migrate it to LevelDB
> 
> period.
Fixed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276
> > +    if (!makeAllDirectories(pathBase)) {
> 
> we probably don't need this?
Removed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1281
> > +    // FIXME: We should eventually use the same LevelDB database for all origins
> 
> period.
Fixed

> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1284
> > +    // FIXME: It would be more thorough to open the database here but also more expensive
> 
> yes, i think this is good enough. period after comment.
Fixed

> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> > +    if (!makeAllDirectories(pathBase)) {
> 
> we probably don't need this?
Removed

> > Source/WebKit/chromium/public/WebIDBFactory.h:65
> > +    // Used for DumpRenderTree tests
> 
> period.
> 
> > Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:81
> > +{
> 
> hmm, shouldn't this get passed on somewhere?
This path is not used for LayoutTests but I plumbed it through for chrome side of the world.

> > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163
> > +    bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore);
> 
> one line further down to keep it sorted i believe
The "addMockSpeecInputResult" is the item out of order so I moved it instead.

> > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233
> > +    delete m_tempFolder;
> 
> can we use an OwnPtr instead so we don't have to call delete explicitly?
Replaced with OwnPtr

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.