[webkit-reviews] review granted: [Bug 128282] IDB: storage/indexeddb/mozilla/clear.html fails : [Attachment 223313] Patch v1 - Enable the test, update it to current spec, and make it pass.

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 128282: IDB: storage/indexeddb/mozilla/clear.html fails
https://bugs.webkit.org/show_bug.cgi?id=128282

Attachment 223313: Patch v1 - Enable the test, update it to current spec, and
make it pass.
https://bugs.webkit.org/attachment.cgi?id=223313&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=223313&action=review


r=me, but cq- to consider the question about converting a RefPtr<IDBKey> to a
bool, and whether to use "!!" syntax for pointer conversion to bool.

> Source/WebCore/ChangeLog:10
> +	   Update the value deserializer to take into account whether or not
there was an IDBKey

Nit: Can haz period?

> Source/WebCore/ChangeLog:16
> +	   * Modules/indexeddb/IDBRequest.cpp:
> +	   (WebCore::IDBRequest::onSuccess): Call the new form of
deserializeIDBValueBuffer

Ditto.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:290
> -    Deprecated::ScriptValue value =
deserializeIDBValueBuffer(requestState(), backend->valueBuffer());
> +    Deprecated::ScriptValue value =
deserializeIDBValueBuffer(requestState(), backend->valueBuffer(), key);

Technically, shouldn't this be "key.get()" to match the similar call below? 
And would "!!key.get()" be clearer that you're converting a pointer into a
bool?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:421
> -    Deprecated::ScriptValue value =
deserializeIDBValueBuffer(requestState(), buffer);
> +    Deprecated::ScriptValue value =
deserializeIDBValueBuffer(requestState(), buffer, key.get());

Would it be clearer to use "!!key.get()" here instead to show you're converting
a pointer into a bool?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:311
> +    // If the key doesn't exist, then the value must be undefined (as
opposed to null)

Nit: Plz add period.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:313
> +	   // We either shouldn't have a buffer or it should be of size 0

Nit: Add period.

> Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp:104

> +    const char* modeString;

Should probably initialize this to NULL:

const char* modeString = NULL;

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:70
> +    // It's okay to not have a SQLite transaction or not have started it yet
because its okay for a WebProcess

Typo:  "its" => "it's"