69131 2011-09-29 23:58:24 -0700 Return null for the value of IDB key cursors 2011-10-06 16:57:22 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 dgrogan dgrogan hans michaeln tony webkit.review.bot oldest_to_newest 475936 0 dgrogan 2011-09-29 23:58:24 -0700 Return null for the value of IDB key cursors 475941 1 109259 dgrogan 2011-09-30 00:00:54 -0700 Created attachment 109259 Patch 475947 2 dgrogan 2011-09-30 00:04:51 -0700 This is the webkit patch in support of http://codereview.chromium.org/7834006 Michael and/or Hans, could you take a look? 476874 3 109259 hans 2011-10-03 04:00:35 -0700 Comment on attachment 109259 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 477036 4 michaeln 2011-10-03 09:40:11 -0700 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() { } }; 477224 5 dgrogan 2011-10-03 13:55:39 -0700 Tony, could you review this two-line patch? 478671 6 dgrogan 2011-10-05 14:09:26 -0700 ping tc Sorry for the flood of IDB patches lately. Let me/us know if you need a break from them. 478675 7 109259 tony 2011-10-05 14:12:38 -0700 Comment on attachment 109259 Patch Can we add a layout test for this? 479529 8 dgrogan 2011-10-06 14:58:53 -0700 (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) 479630 9 109259 webkit.review.bot 2011-10-06 16:57:18 -0700 Comment on attachment 109259 Patch Clearing flags on attachment: 109259 Committed r96877: <http://trac.webkit.org/changeset/96877> 479631 10 webkit.review.bot 2011-10-06 16:57:22 -0700 All reviewed patches have been landed. Closing bug. 109259 2011-09-30 00:00:54 -0700 2011-10-06 16:57:18 -0700 Patch bug-69131-20110930000053.patch text/plain 1528 dgrogan U3VidmVyc2lvbiBSZXZpc2lvbjogOTYzNzgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBmNjUxNTUxYTBjNjVkZWIx ZGE2NGJmOGNkMDRlZWZiMzA1Nzk2YTkwLi4yMTY1YmYyMjJjNTE4ZWEzN2M5OGM5ZTIxODNlNjhl N2E1NDU2ODkyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTEtMDktMjkgIERhdmlk IEdyb2dhbiAgPGRncm9nYW5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldHVybiBudWxsIGZv ciB0aGUgdmFsdWUgb2YgSURCIGtleSBjdXJzb3JzIGluc3RlYWQgb2YgQVNTRVJUaW5nCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02OTEzMQorICAgICAg ICAKKyAgICAgICAgVGhpcyBpcyBpbiBzdXBwb3J0IG9mIHRoZSBJbmRleGVkREIgcGVyZm9ybWFu Y2UgaW1wcm92ZW1lbnQgaGVyZToKKyAgICAgICAgaHR0cDovL2NvZGVyZXZpZXcuY2hyb21pdW0u b3JnLzc4MzQwMDYvCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgKiBzdG9yYWdlL0lEQkN1cnNvckJhY2tlbmRJbXBsLmNwcDoKKyAgICAgICAgKFdlYkNv cmU6OklEQkN1cnNvckJhY2tlbmRJbXBsOjp2YWx1ZSk6CisKIDIwMTEtMDktMjkgIE1hcnRpbiBS b2JpbnNvbiAgPG1yb2JpbnNvbkBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtGcmVldHlwZV0gU29t ZSB0ZXh0IGluIFBsYW5ldCBHTk9NRSByZW5kZXJzIGluIHRoZSB3cm9uZyBwbGFjZQpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvc3RvcmFnZS9JREJDdXJzb3JCYWNrZW5kSW1wbC5jcHAgYi9T b3VyY2UvV2ViQ29yZS9zdG9yYWdlL0lEQkN1cnNvckJhY2tlbmRJbXBsLmNwcAppbmRleCBiOGJj MWUzMjUzOTM3OWRhYTdkNzNhMmQyYTM5MDMyOTE4Yzc1NTJhLi45OWVlM2U2ODViMWZjZDdjN2Nm ZWE0YmM1ODZhMzNiMDdhOTM2ZmVmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9zdG9yYWdl L0lEQkN1cnNvckJhY2tlbmRJbXBsLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9zdG9yYWdlL0lE QkN1cnNvckJhY2tlbmRJbXBsLmNwcApAQCAtNzMsNyArNzMsOCBAQCBQYXNzUmVmUHRyPElEQktl eT4gSURCQ3Vyc29yQmFja2VuZEltcGw6OnByaW1hcnlLZXkoKSBjb25zdAogCiBQYXNzUmVmUHRy PFNlcmlhbGl6ZWRTY3JpcHRWYWx1ZT4gSURCQ3Vyc29yQmFja2VuZEltcGw6OnZhbHVlKCkgY29u c3QKIHsKLSAgICBBU1NFUlQobV9jdXJzb3JUeXBlICE9IEluZGV4S2V5Q3Vyc29yKTsKKyAgICBp ZiAobV9jdXJzb3JUeXBlID09IEluZGV4S2V5Q3Vyc29yKQorICAgICAgcmV0dXJuIFNlcmlhbGl6 ZWRTY3JpcHRWYWx1ZTo6bnVsbFZhbHVlKCk7CiAgICAgcmV0dXJuIFNlcmlhbGl6ZWRTY3JpcHRW YWx1ZTo6Y3JlYXRlRnJvbVdpcmUobV9jdXJzb3ItPnZhbHVlKCkpOwogfQogCg==