[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 #9 from Marcus Bulach <bulach at chromium.org>  2010-07-13 10:45:54 PST ---
(In reply to comment #5)
> (From update of attachment 60928 [details])
> WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:88
>  +  
> remove extra newline
> 
> WebKit/chromium/src/WebIDBKeyRange.cpp:39
>  +  
> get rid of this extra newline
done

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

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

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

WebIDBKeyRange.h is (was?) in the patch. anyways, made a thin wrapper, removed the proxy. thanks for the clarification!

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

> 
> WebKit/chromium/src/WebIDBCallbacksImpl.cpp:67
>  +  void WebIDBCallbacksImpl::onSuccess(WebKit::WebIDBCursor* webKitInstance)
> This should probably follow the model of database errors...
not sure I understood this bit?

> 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?
done in:
 https://bugs.webkit.org/show_bug.cgi?id=42161
landed, merged in the latest patch.

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

> 
> WebCore/storage/IDBObjectStoreRequest.cpp:106
>  +      RefPtr<IDBAny> db = IDBAny::create();
> Hm...these changes shoudl probably all be done in their own cl.
done in:
 https://bugs.webkit.org/show_bug.cgi?id=42161

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

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

> 
> WebCore/storage/IDBKeyRange.h:61
>  +  protected:
> Why protected?
reverted (this was needed for the proxy, now that it's a thin wrapper it's no longer necessary).

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

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

> 
> IDBAny is only a return type...and perhaps not very well named?
> 
> WebCore/storage/IDBCursorRequest.cpp:79
>  +      // FIXME: Implement.
> ASSERT_NOT_IMPLEMENTED();
done (nit: notImplemented())

> 
> 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.
made it a pure virtual interface.

> 
> WebCore/storage/IDBCursor.h:37
>  +  // FIXME: Make this class pure virtual.
> What's the hold up?
I should've done before...

> 
> 
> 
> These comments are not exhaustive...shoudl get you started tho.
thanks! I uploaded another patch. I'd appreciate if you could please take another *quick* :) look at it, as it's not yet final.
(it depends on your add / serializedscriptvalue landing..)

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