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

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Hans Wennborg <hans at chromium.org>  2011-05-23 08:51:07 PST ---
(From update of attachment 94368)
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.

> Source/WebCore/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=61000

Should probably have a description here too

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

period.

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

no need for newline

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:96
> +            bool hasSqlBackend = IDBSQLiteBackingStore::backingStoreExists(securityOrigin.get(), dataDir);

i think this should be named hasSQLBackend.. same for hasLevelDBBackend below

> 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

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

period.

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:1276
> +    if (!makeAllDirectories(pathBase)) {

we probably don't need this?

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

period.

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

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:996
> +    if (!makeAllDirectories(pathBase)) {

we probably don't need this?

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

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:163
> +    bindMethod("setOverrideIndexedDBBackingStore", &LayoutTestController::setOverrideIndexedDBBackingStore);

one line further down to keep it sorted i believe

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:233
> +    delete m_tempFolder;

can we use an OwnPtr instead so we don't have to call delete explicitly?

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