Last Comment Bug 755514 - Cursor.update support for keypath arrays
: Cursor.update support for keypath arrays
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks: 726378
  Show dependency treegraph
 
Reported: 2012-05-15 14:29 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-06-29 11:55 PDT (History)
3 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.28 KB, patch)
2012-06-27 11:14 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review-
Details | Diff | Review
Patch (1.39 KB, patch)
2012-06-28 16:04 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jonas: review-
Details | Diff | Review
Tests + fix (3.66 KB, patch)
2012-06-28 19:27 PDT, Jonas Sicking (:sicking) PTO Until July 5th
khuey: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-15 14:29:40 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-27 11:14:52 PDT
Created attachment 637190 [details] [diff] [review]
Patch

We added the support already, just needs some tests.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-28 06:29:56 PDT
Comment on attachment 637190 [details] [diff] [review]
Patch

Review of attachment 637190 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/test/unit/test_complex_keyPaths.js
@@ +175,5 @@
> +    let cursor = e.target.result;
> +    if (cursor) {
> +      request = cursor.update(info.value);
> +      request.onerror = errorHandler;
> +      request.onsuccess = grabEventAndContinueHandler;

This won't actually test what we want to test. In the object store used here we don't use inline keys at all. So you'll want to move this code up to the "Test creating object stores and inserting data" part of the test.

Also, you only need to put the test in the part of the code run when we have a "key" property. That way you won't need the if(cursor) tests.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-28 16:04:17 PDT
Created attachment 637720 [details] [diff] [review]
Patch
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-28 18:49:44 PDT
Comment on attachment 637720 [details] [diff] [review]
Patch

Review of attachment 637720 [details] [diff] [review]:
-----------------------------------------------------------------

Err.. info.key is always true there, otherwise we would have called 'continue' earlier. I'll write something up.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-28 18:55:22 PDT
Ok, I'm convinced that I don't actually understand how this is supposed to work.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-06-28 19:27:20 PDT
Created attachment 637766 [details] [diff] [review]
Tests + fix

Basically you were doing the right thing, except that you had inverted the initial if-branch-check, causing none of your code to actually run. You were also using an index cursor for some reason even though that wasn't needed.


This patch both adds tests which makes sure that calling

cursor.update(<existing value>)

always works (which is what the previous versions of the patch was trying to do I believe).

It also tests that

cursor.update(<value with key changed>)

always throws.

This actually surfaced a case where we were doing the wrong thing. Calling cursor.update(<existing value>) on an object store with an empty keyPath throws. The fix is to remove a bogus check.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-29 11:55:10 PDT
https://hg.mozilla.org/mozilla-central/rev/6b50a5c705c2

Note You need to log in before you can comment on or make changes to this bug.