[webkit-reviews] review denied: [Bug 41888] Initial bindings and plumbing for IDBCursor. : [Attachment 60928] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Marcus Bulach
<bulach at chromium.org>'s request for review:
Bug 41888: Initial bindings and plumbing for IDBCursor.
https://bugs.webkit.org/show_bug.cgi?id=41888

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:88
 +  
remove extra newline

WebKit/chromium/src/WebIDBKeyRange.cpp:39
 +  
get rid of this extra newline

WebKit/chromium/src/WebIDBKeyRange.cpp:42
 +  #if WEBKIT_IMPLEMENTATION
This is always defined in chromium/src

WebKit/chromium/src/WebIDBKeyRange.cpp:64
 +  void WebIDBKeyRange::reset()
Do you actually need this?

Where's the WebIDBKeyRange.h?  Why isn't WebIDBKeyRange designed like
WebIDBDatabaseError (and such)...i.e. a thin wrapper around a WebCore type?  We
only do this stuff with Impls and proxies when chromium needs to implement
something as well.

WebKit/chromium/src/WebIDBCursorImpl.cpp:32
 +  #if ENABLE(INDEXED_DATABASE)
We don't use guards in WebKit code usually...we just assume it's on.

WebKit/chromium/src/WebIDBCallbacksImpl.cpp:67
 +  void WebIDBCallbacksImpl::onSuccess(WebKit::WebIDBCursor* webKitInstance)
This should probably follow the model of database errors...


WebCore/storage/IndexedDatabaseRequest.cpp:66
 +	RefPtr<IDBAny> db = IDBAny::create();
Should we add ::create() functions for each type so we can do this inline when
creating the requests rather than needing 3 lines to do it?

WebCore/storage/IDBObjectStoreRequest.h:70
 +	PassRefPtr<IDBRequest> openCursor(ScriptExecutionContext*,
PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short
direction = IDBCursor::NEXT);
= 0 is the same thing

WebCore/storage/IDBObjectStoreRequest.cpp:106
 +	RefPtr<IDBAny> db = IDBAny::create();
Hm...these changes shoudl probably all be done in their own cl.

WebCore/storage/IDBObjectStoreImpl.h:53
 +	void openCursor(PassRefPtr<IDBCallbacks>, PassRefPtr<IDBKeyRange> range
= PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT);
don't need default params

WebCore/storage/IDBObjectStore.h:55
 +	virtual void openCursor(PassRefPtr<IDBCallbacks>,
PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short
direction = IDBCursor::NEXT) = 0;
don't need default params

WebCore/storage/IDBKeyRange.h:61
 +  protected:
Why protected?

WebCore/storage/IDBCursorRequest.idl:37
 +	    void continue(in [Optional] IDBAny key);
I believe this should be an IDBKey

WebCore/storage/IDBCursorRequest.idl:36
 +	    void update(in IDBAny value);
This should probably be a serializedScriptValue

IDBAny is only a return type...and perhaps not very well named?

WebCore/storage/IDBCursorRequest.cpp:79
 +	// FIXME: Implement.
ASSERT_NOT_IMPLEMENTED();

and above

WebCore/storage/IDBCursor.h:53
 +	virtual unsigned short direction() const { return m_direction; }
virtual?  If so, the ~ should be virtual as well probably.

WebCore/storage/IDBCursor.h:37
 +  // FIXME: Make this class pure virtual.
What's the hold up?



These comments are not exhaustive...shoudl get you started tho.