Bug 105449 – IndexedDB: Cursor and IndexWriter cleanup for refactor

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

IndexedDB: Cursor and IndexKey cleanup for refactor

[Alec Flett]

Created attachment 180197 [details]
Patch

[Alec Flett]

jsbell - a pretty easy one for future refactor work...

[Alec Flett]

Created attachment 180203 [details]
Patch

A few more tweaks... still ready for review

[Joshua Bell]

Comment on attachment 180203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180203&action=review

lgtm with nits

> Source/WebCore/ChangeLog:8
> +        This is a cleanup of some code to make the refactor that is coming in

Should mention that IndexWriter is exposed in the header file now since it's usage will move out of IDBObjectStoreBackendImpl soon.

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=102741 cleaner. Some assertions

Just assertions about cursor type, right?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:405
>                  keys.append(primaryKey);

Indentation here looks wrong now, but this might be the diff.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:409
> +        bool backingStoreSuccess = indexWriter->verifyIndexKeys(*backingStore,

Don't need to wrap here.

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h:104
> +        bool verifyIndexKeys(IDBBackingStore&, IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys, const IDBKey* primaryKey = 0, String* errorMessage = 0) WARN_UNUSED_RETURN;

Can this method be const?

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h:109
> +

Can delete this blank line.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-188
> -    ASSERT(m_cursorType == IDBCursorBackendInterface::InvalidCursorType);

I believe this could be replaced by ASSERT(!m_pendingCursor); - the creator of this IDBRequest should be calling this to "stamp" the request with the cursor details shortly after creation, therefore there should neither be a result cursor (which the m_readyState assertion handles) nor a pending cursor

[Joshua Bell]

Comment on attachment 180203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180203&action=review

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-291
> -    ASSERT(m_cursorType != IDBCursorBackendInterface::InvalidCursorType);

Ditto for this ASSERT. (Ugh, my review comments keep getting dropped.)

[Alec Flett]

Comment on attachment 180203 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180203&action=review

>> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.h:104
>> +        bool verifyIndexKeys(IDBBackingStore&, IDBBackingStore::Transaction*, int64_t databaseId, int64_t objectStoreId, int64_t indexId, bool& canAddKeys, const IDBKey* primaryKey = 0, String* errorMessage = 0) WARN_UNUSED_RETURN;
> 
> Can this method be const?

unfortunately no - because it triggers the code that may cleanup stale index entries in the backing store.

[Alec Flett]

Created attachment 180230 [details]
Patch

[Alec Flett]

tony@ - r?

[Alec Flett]

Created attachment 180241 [details]
Patch

[Tony Chang]

Comment on attachment 180241 [details]
Patch

Can we use an enum with 2 values instead of a bool for keyOnly?

[Build Bot]

Comment on attachment 180241 [details]
Patch

Attachment 180241 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15405839

New failing tests:
fast/frames/sandboxed-iframe-attribute-parsing.html

[Alec Flett]

Created attachment 180383 [details]
Patch

now with enums

[Alec Flett]

tony@- comments addressed. r?

[Tony Chang]

Comment on attachment 180383 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 180383 [details]
Patch

Clearing flags on attachment: 180383

Committed r138290: <http://trac.webkit.org/changeset/138290>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.