[Webkit-unassigned] [Bug 41888] Initial bindings and plumbing for IDBCursor.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=41888





--- Comment #25 from Marcus Bulach <bulach at chromium.org>  2010-08-04 10:24:03 PST ---
thanks for the detailed review, Jeremy!
replies inline:

(In reply to comment #23)
> (From update of attachment 62935 [details])
> Please redo the names in this patch to match the latest flavor of the week.
done.. this is now inline with the new spec, but everything has changed since the last patch. please, take a deep look at it.
andrei, could you also double check the naming conventions match the spec?

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

> 
> 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.
done.

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

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

> 
> WebCore/storage/IDBCursor.h:43
>  +      // Keep in sync with what's in the .idl file.
> Remove comment and use your automated mechanism.
uh, the automated mechanism just adds compile-time asserts, it's still necessary to keep in sync..

> 
> 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.)

Done.

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

Done.

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

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

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

> 
> WebCore/storage/IDBCursorImpl.h:60
>  +      int m_count;
> long long?
count has been removed.

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

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

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

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

> 
> 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.
yeah, as we talked, this is the bare minimum necessary to exercise the API and the plumbing pipeline, so it works only with a single key.
continue, update, remove aren't implemented yet, I guess it's better to wait for the new actual storage before implementing them.


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

> 
> WebCore/storage/IDBObjectStoreRequest.cpp:66
>  +      return request.release();
> Maybe do in another patch?  (And look at the other files too?)
I'll split this in a separate patch soon.

> 
> WebKit/chromium/public/WebIDBCursor.h:44
>  +          WEBKIT_ASSERT_NOT_REACHED();
> Should be easy to implement in this CL.
as above, not yet implemented.

> 
> 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.
thanks for the explanation, fixed.


> 
> WebKit/chromium/src/IDBCursorProxy.h:56
>  +      PassOwnPtr<WebKit::WebIDBCursor> m_idbCursor;
> RefPtr
OwnPtr, actually :)
done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.