[webkit-reviews] review denied: [Bug 83492] IndexedDB: Attributes for a cursor "run past the end" should be undefined. : [Attachment 136282] Patch

[bugzilla-daemon at webkit.org]

Joshua Bell <jsbell at chromium.org> has denied Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 83492: IndexedDB: Attributes for a cursor "run past the end" should be
undefined.
https://bugs.webkit.org/show_bug.cgi?id=83492

Attachment 136282: Patch
https://bugs.webkit.org/attachment.cgi?id=136282&action=review

------- Additional Comments from Joshua Bell <jsbell at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=136282&action=review


> Source/WebCore/ChangeLog:18
> +	   update and delete can be called before continue, but not afterwards.


Nit: For consistency, should be update() and delete() and reword to start the
sentence with a capital.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:87
> +    return IDBAny::create(m_currentValue.get());

As an aside, the spec includes "if this property returns an object, it returns
the same object instance every time it is inspected, until the cursor's value
is changed. This means that if the object is modified, those modifications will
be seen by anyone inspecting the value of the cursor." - do we implement that?
Would returning the same IDBAny instance on each call affect that behavior? My
guess is no and no. Implementing that part of the spec may be problematic.
(This change doesn't affect it either way.)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:100
> +	   ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

Spec says this should be InvalidStateError (DOM4-style) - can you make it
INVALID_STATE_ERR (not part of the IDBDatabaseException enum)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:123
> +	   ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

ditto

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:150
> +	   ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;

ditto

> Source/WebCore/Modules/indexeddb/IDBCursor.h:87
> +    RefPtr<IDBKey> m_currentKey;

Does "current" add anything here? Can it just be m_key etc?

> Source/WebCore/Modules/indexeddb/IDBCursor.h:89
> +    RefPtr<SerializedScriptValue> m_currentValue;

Although I believe this code is correct, I would have this be a RefPtr<IDBAny>
and re-use the IDBAny across calls so that we minimize the number of places IDB
objects hold SSV pointers. It shouldn't make any practical difference, though.

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:52
> +    result = evalAndLog("objectStore.add({'x': " + nextToAdd + " }, " +
nextToAdd + ")");

Multiple add calls can be done w/o waiting for the success callback of the
previous one. Just use a for a loop here.

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:148
> +	   // behavior against pre-caching. Make sure to use prime

Nit: "pre-fetching" (so readers know what to search on)

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:173
> +	   onTransactionComplete();

The transaction is not actually complete here, the cursor has just run to the
end...

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:179
> +    evalAndExpectException("cursor.continue()", "");

I'd move this inline since it's part of the consistency checks...

> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:180
> +    finishJSTest();

And either call this directly, or hang it off he actual transaction oncomplete
callback.