Bug 69131 – Return null for the value of IDB key cursors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

Return null for the value of IDB key cursors

[David Grogan]

Created attachment 109259 [details]
Patch

[David Grogan]

This is the webkit patch in support of http://codereview.chromium.org/7834006

Michael and/or Hans, could you take a look?

[Hans Wennborg]

Comment on attachment 109259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109259&action=review

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:-76
> -    ASSERT(m_cursorType != IndexKeyCursor);

i'm not sure I'm following... if the cursor is an IndexKeyCursor, this shouldn't be called, because such cursors don't have a value

[Michael Nordman]

lgtm (fwiw)

You have to look at the chrome CL to fully understand this webkit patch. When a cursor is created or stepped, the value is retrieved and sent to the renderer w/o regard for the cursorType. The branch to return null for IndexKeyCursor type has to be somewhere. IDBCursorBackendImpl.cpp is a fine place for that branch.

http://codereview.chromium.org/7834006/diff/13001/content/browser/in_process_webkit/indexed_db_callbacks.cc

+  dispatcher_host()->Send(new IndexedDBMsg_CallbacksSuccessIDBCursor(
+      response_id(), object_id, IndexedDBKey(idb_object->key()),
+      IndexedDBKey(idb_object->primaryKey()),
+      SerializedScriptValue(idb_object->value())));


Notice there is no 'type' getter on WebIDBCursor. In the current design (where chrome doesn't have much insight into these datatypes) there doesn't need to be.

class WebIDBCursor {
public:
    virtual ~WebIDBCursor() { }

    virtual unsigned short direction() const
    {
        WEBKIT_ASSERT_NOT_REACHED();
        return 0;
    }
    virtual WebIDBKey key() const
    {
        WEBKIT_ASSERT_NOT_REACHED();
        return WebIDBKey::createInvalid();
    }
    virtual WebIDBKey primaryKey() const
    {
        WEBKIT_ASSERT_NOT_REACHED();
        return WebIDBKey::createInvalid();
    }
    virtual WebSerializedScriptValue value() const
    {
        WEBKIT_ASSERT_NOT_REACHED();
        return WebSerializedScriptValue();
    }

    virtual void update(const WebSerializedScriptValue&, WebIDBCallbacks*, WebExceptionCode&) { WEBKIT_ASSERT_NOT_REACHED(); }
    virtual void continueFunction(const WebIDBKey&, WebIDBCallbacks*, WebExceptionCode&) { WEBKIT_ASSERT_NOT_REACHED(); }
    virtual void deleteFunction(WebIDBCallbacks* callbacks, WebExceptionCode& ec) { WEBKIT_ASSERT_NOT_REACHED(); }

protected:
    WebIDBCursor() { }
};

[David Grogan]

Tony, could you review this two-line patch?

[David Grogan]

ping tc

Sorry for the flood of IDB patches lately.  Let me/us know if you need a break from them.

[Tony Chang]

Comment on attachment 109259 [details]
Patch

Can we add a layout test for this?

[David Grogan]

(In reply to comment #7)
> (From update of attachment 109259 [details])
> Can we add a layout test for this?

I don't think so.  DRT won't be able to exercise this code path - there's no javascript API to get the value of a key cursor.  It's only called by WebIDBCursorImpl (which is called by IndexedDBCallbacks in content/browser/in_process_webkit/indexed_db_callbacks.cc)

[WebKit Review Bot]

Comment on attachment 109259 [details]
Patch

Clearing flags on attachment: 109259

Committed r96877: <http://trac.webkit.org/changeset/96877>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.