Closed Bug 755514 Opened 8 years ago Closed 8 years ago

Cursor.update support for keypath arrays

Categories

(Core :: Storage: IndexedDB, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: khuey, Assigned: sicking)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
We added the support already, just needs some tests.
Attachment #637190 - Flags: review?(bent.mozilla)
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.
Attachment #637190 - Flags: review?(bent.mozilla) → review-
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → khuey
Attachment #637190 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #637720 - Flags: review?(jonas)
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.
Attachment #637720 - Flags: review?(jonas) → review-
Ok, I'm convinced that I don't actually understand how this is supposed to work.
Assignee: khuey → jonas
Attached patch Tests + fixSplinter Review
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.
Attachment #637720 - Attachment is obsolete: true
Attachment #637766 - Flags: review?(khuey)
Attachment #637766 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/6b50a5c705c2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.