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

[bugzilla-daemon at webkit.org]

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





--- Comment #6 from Hans Wennborg <hans at chromium.org>  2011-05-18 10:25:28 PST ---
(From update of attachment 93872)
View in context: https://bugs.webkit.org/attachment.cgi?id=93872&action=review

> LayoutTests/ChangeLog:9
> +        to be used for IndexedDB so a unit test can be written

s/unit test/layout test/ throughout; we have unit tests too, e.g. in Source/WebKit/chromium/tests/

> LayoutTests/storage/indexeddb/migrate-basics.html:96
> +    } catch( ex ) {

i suppose you get an exception here because trans.objectStore("store") returns null, and then store.openCursor(keyRange) fails. I think it would be nicer to do an explicit check that "store" exists in the database. That would make it easier to see why it fails.

> LayoutTests/storage/indexeddb/migrate-basics.html:105
> +    if(!cursor) {

ultra nit: space between if and (

> LayoutTests/storage/indexeddb/migrate-basics.html:110
> +    if (cursor.value.text != "testing 1,2,3")

use the shouldBe() function that we use in the other layout tests

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:90
> +        if (backingStoreType == DefaultBackingStore

no need for a newline here

> Source/WebKit/chromium/public/WebIDBFactory.h:65
> +    // Used for DumpRenderTree tests

ultra nit: period at end of comment.

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:36
> +#include <base/memory/scoped_temp_dir.h>

can we really use that here? isn't this a header in Chromium?

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:49
> +static ScopedTempDir tempLevelDBDir;

ScopedTempDir is a non-POD type, so I don't think it's allowed as a static variable

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:64
> +        LOG_ERROR("Failed to clear LevelDB temporary folder.");

doesn't ScopedTempDir delete the dir on destruction?

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:83
> +            && backingStoreType == LevelDBBackingStore)

no need for newline

> Source/WebKit/chromium/src/WebIDBFactoryImpl.cpp:84
> +        {

curly brace goes on same line as the if

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:36
> +#include "WebIDBFactory.h"

alpha order

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

these statements look like they should be sorted

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