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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 30 04:37:04 PDT 2010


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 62935: Patch
https://bugs.webkit.org/attachment.cgi?id=62935&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
Please redo the names in this patch to match the latest flavor of the week.

LayoutTests/storage/indexeddb/script-tests/open-cursor.js:29
 +  function openCursor(objectStore)
Maybe put above dataAddedSuccess so everything's in revert chronological order?


LayoutTests/storage/indexeddb/script-tests/open-cursor.js:16
 +  function dataAddedSuccess()
Not needed...just have the success call open cursor directly or pull that logic
into this.

There's no inheritance of IDLs in the latest spec.  So please merge the 2
files.

WebCore/GNUmakefile.am:2854
 +    WebCore/storage/IDBCursor.cpp \
spacing issue

WebCore/storage/IDBCursor.h:43
 +	// Keep in sync with what's in the .idl file.
Remove comment and use your automated mechanism.

WebCore/storage/IDBCursor.h:41
 +  class IDBCursor : public RefCounted<IDBCursor> {
The interfaces we've been doing threadsafe ref counted since they could be used
on a worker or the main thread once we add it to workers.  (If I were doing it
again, I would have waited until we actually needed it to be threadsafe to do
this, but it's better to be consistent at this point I think.)

WebCore/storage/IDBCursorImpl.cpp:29
 +  #include "IDBAny.h"
put inside guard...here an elsewhere...from now on

WebCore/storage/IDBCursorImpl.cpp:68
 +  long long IDBCursorImpl::count() const
Could just be inline...but doesn't matter much either way.

WebCore/storage/IDBCursorImpl.cpp:74
 +  {
ASSERT_NOT_REACHED()

+ FIXME

WebCore/storage/IDBCursorImpl.h:57
 +	unsigned short m_direction;
This should be an enum, right?

WebCore/storage/IDBCursorImpl.h:60
 +	int m_count;
long long?

WebCore/storage/IDBCursorImpl.h:43
 +	IDBCursorImpl(PassRefPtr<IDBObjectStoreImpl>, PassRefPtr<IDBKeyRange>,
unsigned short direction, PassRefPtr<IDBKey>,
PassRefPtr<SerializedScriptValue>, int count);
make private, add ::create method.

WebCore/storage/IDBCursorRequest.cpp:62
 +	return m_cursor->count();
Could be inline...but I don't care much.

WebCore/storage/IDBObjectStoreImpl.cpp:139
 +	int count = value ? 1 : 0;
we should move count all together since we're not doing sync cursors.

WebCore/storage/IDBObjectStoreImpl.cpp:141
 +	  value = SerializedScriptValue::create();
Aren't we supposed to return a null cursor in this case?

WebCore/storage/IDBObjectStoreImpl.cpp:138
 +	RefPtr<SerializedScriptValue> value = m_tree->get(key.get());
This isn't right...what if the value doesn't exist but we can find something
higher/lower (depending on cursor type) than that?  m_tree will need some new
methods I guess.

WebCore/storage/IDBObjectStoreImpl.cpp:137
 +	RefPtr<IDBKey> key = range->left();
This doesn't handle direction.

WebCore/storage/IDBObjectStoreRequest.cpp:66
 +	return request.release();
Maybe do in another patch?  (And look at the other files too?)

WebKit/chromium/public/WebIDBCursor.h:44
 +	    WEBKIT_ASSERT_NOT_REACHED();
Should be easy to implement in this CL.

WebKit/chromium/public/WebIDBKeyRange.h:43
 +	WEBKIT_API WebIDBKeyRange(const WebIDBKeyRange& keyRange) { m_private =
keyRange.m_private; }
This won't work.  You need to make an assign method that does this and just
call assign.

Similarly ~ should be inline and call reset().

All methods that aren't inline and aren't virtual should have a WEBKIT_API
prefix.

WebKit/chromium/src/IDBCursorProxy.h:56
 +	PassOwnPtr<WebKit::WebIDBCursor> m_idbCursor;
RefPtr


More information about the webkit-reviews mailing list