[Webkit-unassigned] [Bug 44700] IDBObjectStore::get should run in a transaction.

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Jeremy Orlow <jorlow at chromium.org>  2010-09-20 12:28:11 PST ---
(From update of attachment 68067)
View in context: https://bugs.webkit.org/attachment.cgi?id=68067&action=review

> LayoutTests/storage/indexeddb/objectstore-basics-expected.txt:116
> +db.transaction()

hu?   does this work?  Don't you need to save it to a variable?  ideally use db.transaction() too?

> LayoutTests/storage/indexeddb/objectstore-removeobjectstore.html:73
> +function indexAdded()

Half the layout tests are named for what just happened and the other half are what should happen next.  In this place, it's the latter, so name it something like commitTransaction

> LayoutTests/storage/indexeddb/transaction-get.html:18
> +        debug("Accessing a committed transaction should throw");

This should be a failure, not a debug

It's ok to check in a failing result if that's what it should be.

> LayoutTests/storage/indexeddb/transaction-get.html:22
> +        shouldBe('exc.code', '9');

use the constant

> LayoutTests/storage/indexeddb/transaction-get.html:27
> +function nonExistingKey() {

all functions should put { on the next line

> LayoutTests/storage/indexeddb/transaction-get.html:41
> +    transaction.onabort = unexpectedErrorCallback;

newline after this maybe?

> LayoutTests/storage/indexeddb/transaction-get.html:44
> +    var result = store.get('myKey');

put in evalAndLog

> LayoutTests/storage/indexeddb/transaction-get.html:49
> +    var emptyResult = store.get('nonExistingKey');

put at the end of gotValue (yes, this is supposed to work, but that'll make it easier to read)

> LayoutTests/storage/indexeddb/transaction-get.html:80
> +function test() {

>From now on, please write new tests from top down in terms of execution and function names saying what just happened, please.

> WebCore/storage/IDBTransactionBackendImpl.cpp:115
> +void IDBTransactionBackendImpl::runNextTasks()

I don't like this function name.  execute tasks or something? Or just move it into the timer function?

> WebCore/storage/IDBTransactionBackendImpl.cpp:121
> +    // pump the task queue dry

Use complete sentences.  Of course this comment doesn't seem necessary.

> WebCore/storage/IDBTransactionBackendImpl.cpp:123
> +    queue.swap(m_taskQueue);

Is there a reason we need to do this?  Even if stuff is appended, we can keep running them, right?

> WebCore/storage/IDBTransactionBackendImpl.cpp:125
> +        OwnPtr<ScriptExecutionContext::Task> task(queue.first().release());

personally I prefer = style initialization, but if you like this better it's fine

> WebCore/storage/IDBTransactionBackendImpl.h:72
> +    bool m_started;

Please use enums to make this a proper state machine.

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