[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 #26 from Marcus Bulach <bulach at chromium.org>  2010-08-05 02:40:57 PST ---
more renames went in, I'm merging this patch (again).. please wait for a new patch to review.

(In reply to comment #25)
> thanks for the detailed review, Jeremy!
> replies inline:
> 
> (In reply to comment #23)
> > (From update of attachment 62935 [details] [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.