[webkit-reviews] review granted: [Bug 128316] IDB: storage/indexeddb/create-index-with-integer-keys.html fails : [Attachment 223429] Patch v2 - Fix style

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 7 10:17:12 PST 2014


Geoffrey Garen <ggaren at apple.com> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 128316: IDB: storage/indexeddb/create-index-with-integer-keys.html fails
https://bugs.webkit.org/show_bug.cgi?id=128316

Attachment 223429: Patch v2 - Fix style  
https://bugs.webkit.org/attachment.cgi?id=223429&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223429&action=review


r=me

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:208
> +	   for (auto i : keyDatas)

This should be "auto&" to avoid an apparent copy.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:209
> +	       keys.append(i.maybeCreateIDBKey());

Let's null check before append to avoid a nullptr dereference later.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:329
> +		   indexKeys.append(i.maybeCreateIDBKey());

Let's null check before append to avoid a nullptr dereference later.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:355
> +	   if (!indexKey->isValid())
> +	       return;

Maybe we should check isValid in both cases -- maybe up at the top, by the
nullptr check? It seems that an array key can be invalid, so I guess we should
check it.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:363
> +	   for (auto i : indexKey->array())

auto&?

>
Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQ
Lite.cpp:646
> +	   for (auto indexKey : indexKeys) {

auto&?

>
Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQ
Lite.h:117
> +    OwnPtr<JSC::JSGlobalObject> m_globalObject;

Please use JSC::Strong. It's like OwnPtr, but for GC instead of malloc.
Otherwise, crash-o.


More information about the webkit-reviews mailing list