[webkit-reviews] review denied: [Bug 41250] Implement IDBObjectStore.get/set/remove : [Attachment 59989] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 41250: Implement IDBObjectStore.get/set/remove
https://bugs.webkit.org/show_bug.cgi?id=41250

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:15
 +	debug("openSuccess():");
nit: indentation

LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:30
 +	debug("createSuccess():");
nit: indentation

LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:46
 +	debug("addSuccess():");
ditto

WebCore/storage/IDBObjectStoreImpl.cpp:77
 +	       
callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR,
"A key was supplied for an objectStore that has a keyPath.")); 
it might be nice to define a helper method named notifyError that takes the
callbacks, exception type, and string.

WebKit/chromium/public/WebIDBKey.h:42
 +	WEBKIT_API static WebIDBKey createInvalid();
nit: please add a new line after this line to help readability

WebKit/chromium/public/WebIDBKey.h:62
 +	    /* Types not in WebCore::IDBKey */
nit: use C++ style comment

WebKit/chromium/public/WebIDBKey.h:67
 +	WEBKIT_API WebString string() const; // Only valid for StringType.
nit: maybe these methods should be named toString and toNumber or asString and
asNumber?

WebKit/chromium/public/WebIDBObjectStore.h:61
 +	//	  we do with the data in Chromium (for now) is pass it through,
it's not really worth the effort at the moment.
hmm... it would be better to make the API have enums.  in the future,
the embedder might have need to interpret these values, and when that
happens people often do not bother to go back and cleanup the API to
expose a proper enum type.

WebKit/chromium/src/WebIDBKey.cpp:39
 +  COMPILE_ASSERT(int(WebIDBKey::NullType) == int(IDBKey::NullType),
dummyNullType);
please move these to AssertMatchingEnums.cpp