The items in the array should be stringified. When evaluating the "keypath" we should evaluate all parts of the array and concatenate the result into an array. This should work both on objectStore and index keypaths.

This is working but lacks fixing index.keyPath to return an array rather than a string when a index is created with an array of keypaths. I'll do a second patch for that but wanted to let you start reviewing this for now

Comment on attachment 582968 [details] [diff] [review] Patch to fix we discussed all this stuff on IRC, Jonas addressed all my comments r=janv

The one thing this interdiff doesn't contain is some review comments for Key::AppendArrayItem. Accidentally merged them into the base patch.

This one works!

Comment on attachment 583022 [details] [diff] [review] Fixes review comments and the index.keyPath property Ben, if you get to this one before Jan, feel free to just go for it

Comment on attachment 582968 [details] [diff] [review] Patch to fix Review of attachment 582968 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBFactory.cpp @@ +304,2 @@ > NS_ENSURE_SUCCESS(rv, rv); > + if (!keyPath.IsEmpty() && keyPath.First() == ',') { Sneaky! This could probably use a comment ;) ::: dom/indexedDB/IDBObjectStore.cpp @@ +1660,5 @@ > + return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; > + } > + > + if (!length) { > + return NS_ERROR_DOM_INVALID_ACCESS_ERR; Wait, what? According to the spec I think you want SYNTAX_ERR? ::: dom/indexedDB/Key.h @@ +152,5 @@ > EncodeNumber(double(aInt), eFloat); > TrimBuffer(); > } > > + nsresult SetFromJSVal(JSContext* aCx, const jsval aVal) Nit: Please revert. @@ +189,5 @@ > > return NS_OK; > } > > + nsresult AppendArrayItem(JSContext* aCx, const jsval aVal, bool aFirst) Nit: args on their own line. @@ +194,5 @@ > + { > + NS_ASSERTION(aFirst == mBuffer.IsEmpty(), > + "Wrong aFirst or we didn't encode properly"); > + > + nsresult rv = EncodeJSVal(aCx, aVal, aFirst ? eMaxType : 0); Wasn't there an eArray value? Do you want that instead? ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +1821,5 @@ > else if (schemaVersion == MakeSchemaVersion(11, 0)) { > rv = UpgradeSchemaFrom11_0To12_0(connection); > } > + else if (schemaVersion == MakeSchemaVersion(12, 0)) { > + rv = connection->SetSchemaVersion(MakeSchemaVersion(13, 0)); Comment here why you're bumping the schema but not upgrading anything (backwards-incompatible change)

Comment on attachment 583022 [details] [diff] [review] Fixes review comments and the index.keyPath property Review of attachment 583022 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBIndex.cpp @@ +382,5 @@ > return mObjectStore->GetName(aStoreName); > } > > NS_IMETHODIMP > +IDBIndex::GetKeyPath(JSContext* aCx, jsval* aVal) Nit: Sorta dumb (I wish I could undo this...) but each param gets its own line. @@ +387,5 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + if (UsesKeyPathArray()) { > + JSObject* array = JS_NewArrayObject(aCx, 0, nsnull); Er, you should give the correct length here. You know what it is, right? @@ +408,5 @@ > + > + *aVal = OBJECT_TO_JSVAL(array); > + } > + else { > + if (!xpc_qsStringToJsval(aCx, mKeyPath, aVal)) { Nit: else if ::: dom/indexedDB/IDBObjectStore.cpp @@ +577,5 @@ > jsval key; > nsresult rv = GetJSValFromKeyPath(aCx, aObject, aKeyPathArray[i], key); > NS_ENSURE_SUCCESS(rv, rv); > > + if (NS_FAILED(value.AppendArrayItem(i == 0, aCx, key))) { This looks wrong... What's going on here? Your first patch has cx going first, and there are no changes in this patch that seem relevant...

> ::: dom/indexedDB/IDBObjectStore.cpp > @@ +1660,5 @@ > > + return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR; > > + } > > + > > + if (!length) { > > + return NS_ERROR_DOM_INVALID_ACCESS_ERR; > > Wait, what? According to the spec I think you want SYNTAX_ERR? Spec doesn't actually define that this should throw an exception at all, but I think that's a bug. Happy to change to syntax_err though to reduce number of errors people have to check for. > ::: dom/indexedDB/OpenDatabaseHelper.cpp > @@ +1821,5 @@ > > else if (schemaVersion == MakeSchemaVersion(11, 0)) { > > rv = UpgradeSchemaFrom11_0To12_0(connection); > > } > > + else if (schemaVersion == MakeSchemaVersion(12, 0)) { > > + rv = connection->SetSchemaVersion(MakeSchemaVersion(13, 0)); > > Comment here why you're bumping the schema but not upgrading anything > (backwards-incompatible change) I might actually remove this if this lands at the same time as arrays-as-keys. Otherwise I'll add a comment.

Jan correctly reminded me that this should work on objectstores too!

Comment on attachment 583098 [details] [diff] [review] Add support on objectStores too r=janv

Checked in! https://hg.mozilla.org/mozilla-central/rev/0ad9c268e932 Thanks everyone for late night reviews!

It missed the Firefox 11 release for a few hours.

I was wrong about the Firefox 11 target because this changeset is between: * The latest changeset in Nightly 11.0a1/20111220: http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b. * The first changeset in 11.0a2/20111220: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654

I have updated the relevant pages with this detail https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase/createObjectStore https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/createIndex And added a note to the relevant release notes: https://developer.mozilla.org/en-US/Firefox/Releases/11#IndexedDB