692614 - IndexedDB: Support all spec'ed key types, including arrays

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We currently only support strings and integers. We should also support floats, dates and arrays.

Arrays can contain any key types, including other arrays.

The order goes:
Arrays
Strings
Dates
floats/integers

Note that the spec doesn't make any difference between integers and floats.

Comment 1

[Jan Varga [:janv]]

I've been working on it quite intensively, here's what I did:
- changed all key columns in the schema to the BLOB type
- implemented schema upgrade
- rewrote Key.h to work with plain buffers (strings, integers and arrays supported)
- fixed all callers of BindToStatement() to pass additional argument (auto-increment flag) 
- forked some string code into StringUtils.h/cpp

So it seems the approach is correct, all automatic tests passed on the try server.
There's still a lot of work, but it looks promising (to have it in FF 11).

The work is dependent on Ben's compressed structures clones and files in indexedDB

Comment 2

[Jan Varga [:janv]]

Attachment: initial patch (not for review)

Comment 3

[Jan Varga [:janv]]

Support for floats has been added. I'm now going to rebase using the patch in bug 701772.

Comment 4

[Jan Varga [:janv]]

I added also support for dates. So the patch should now cover all types.
I'll attach it in a minute.

Comment 5

[Jan Varga [:janv]]

Attachment: patch for feedback

Comment 6

[Jan Varga [:janv]]

the patch depends on files in idb and autoincrement patch

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 582039 [details] [diff] [review]
patch for feedback

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1908,3 @@
>               mKey.ToInteger() >= mObjectStore->Info()->nextAutoIncrementId) {
>        // XXX Once we support floats, we should use floor(mKey.ToFloat()) here
>        autoIncrementNum = mKey.ToInteger();

See the XXX comment here.

This should be:

else if (mKey.IsFloat() &&
         mKey.ToFloat() >= (float)...->nextAutoIncrementId) {
  autoIncrementNum = floor(mKey.ToFloat());

::: dom/indexedDB/Key.cpp
@@ +51,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (JSVAL_IS_INT(aVal)) {
> +    *aType = AppendFromInteger(JSVAL_TO_INT(aVal));

Why not simply use AppendFromFloat((jsdouble)JSVAL_TO_INT(aVal))?

@@ +90,5 @@
> +
> +      *aType = 4;
> +      return NS_OK;
> +    } else if (JS_ObjectIsDate(aCx, obj)) {
> +      printf("date!\n");

Remove the printf

::: dom/indexedDB/Key.h
@@ +176,5 @@
> +
> +    char type;
> +    nsresult rv = AppendFromJSVal(aCx, aVal, &type);
> +// XXXvarga cleanup this mess!
> +//    NS_ENSURE_SUCCESS(rv, rv);

What's going on here?

@@ +205,4 @@
>      return NS_OK;
>    }
>  
>    PRInt64 ToInteger() const

I don't think we need this function any more

@@ +212,3 @@
>    }
>  
> +  void ToString(nsString& aString) const

Or this.

@@ +270,5 @@
> +
> +  static
> +  PRUint64 DoFlipBits(PRUint64 u)
> +  {
> +    PRUint64 mask = -PRInt64(u >> 63) | PR_UINT64(0x8000000000000000);

It's actually not safe to right-shift a signed value. The C++ spec doesn't define if sign extension will happen or not but here you are relying on sign extension not happening.

Additionally, this won't do the right thing for -0. I think it will compare less than 0 whereas it should compare equal to it.

I think simply doing

return u & PRUINT64(0x8000000000000000) ?
       -u :
       u | PRUINT64(0x8000000000000000);

will work. But it's possible that there's a way to do it without the branch.

@@ +281,5 @@
> +    PRUint64 mask = ((u >> 63) - 1) | PR_UINT64(0x8000000000000000);
> +    return u ^ mask;
> +  }
> +
> +  void PrintBuffer()

At least put this in #ifdef DEBUG.

@@ +299,5 @@
> +
> +    AppendUCS2toUTF8LT(aString, mBuffer);
> +    if (mBuffer.Last() != '\0') {
> +      mBuffer.Append('\0');
> +    }

I don't understand this branch? Why shouldn't we always end with an extra \0?

@@ +301,5 @@
> +    if (mBuffer.Last() != '\0') {
> +      mBuffer.Append('\0');
> +    }
> +
> +    return 3;

It seems silly to have a function that always returns the same value. Why not make AppendFromJSVal take a bool* instead and make that function explicitly set it to true in all places except when parsing void/null.

(FWIW, we should make AppendFromJSVal not deal with void/null, that should be the job of the callers or a separate explicit Key::FromJSValSupportNullAndVoid function (but with a better name))

@@ +309,5 @@
> +  {
> +    mBuffer.Append('\2');
> +
> +    PRUint64 number = *reinterpret_cast<PRUint64*>(&aFloat);
> +    number = SwapBytes(DoFlipBits(number));

I don't think calling SwapBytes here is correct for both low and high endiannness.

@@ +321,5 @@
> +  {
> +    mBuffer.Append('\1');
> +
> +    PRUint64 number = *reinterpret_cast<PRUint64*>(&aFloat);
> +    number = SwapBytes(DoFlipBits(number));

Same here.

Also, I don't think this will do the right thing for negative zero. Positive and negative zero should compare as the same value which I don't think will happen here.

@@ +386,5 @@
> +
> +    return result;
> +  }
> +
> +  PRInt64 ToInteger(PRUint32* aPos) const

I don't think we need this function any more.

::: dom/indexedDB/KeyUtils.cpp
@@ +39,5 @@
> +#include "nsAString.h"
> +#include "KeyUtils.h"
> +
> +void
> +AppendUCS2toUTF8LT( const nsAString& aSource, nsACString& aDest )

Make these two functions live as static helper functions on Key instead. That way we only need one .cpp file. And it'll be clear that they are only there for key encoding/decoding.

::: dom/indexedDB/test/test_put_get_values.html
@@ +22,5 @@
> +//      let testString = { key: String.fromCharCode(0xF3, 0xF4, 0xF6), value: "testString" };
> +//      let testInt = { key: 1, value: 1002 };
> +//      let testInt = { key: ["abc\0"], value: 1002 };
> +      let testInt = { key: new Date(1), value: 1002 };
> +//      let testInt = { key: ["abc"], value: 1002 };

The changes to this file look in general wonky

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Err.. nevermind about the shift safetyness. You are right-shifting a unsigned value which is totally safe.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fixes on top of Jans patch

This mostly changes the encoding/decoding code. It also fixes most (but not all) of my review comments.

The two remaining comments is that we need to fix swapbytes to only swap on little-endian systems, and the test-changes in test_put_get_values.html still need to be looked at. I'm happy to fix both these things tomorrow or monday.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Jan's plus my changes.

This is the total set of changes made by the two patches together. For review ease if needed.

Comment 11

[Jan Varga [:janv]]

test_keys.html is missing in your patches

Comment 12

[Jan Varga [:janv]]

actually, the endianness is properly handled in nsIStreamBufferAccess.idl

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: On top of my patch

This is on top of my patch (which is on top of Jan's patch). Includes fixes to the endian-issue and the test issue so this should be ready-to-go.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Total changes

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 582765 [details] [diff] [review]
Total changes

>--- a/dom/indexedDB/IDBObjectStore.cpp
>+++ b/dom/indexedDB/IDBObjectStore.cpp
>@@ -1903,34 +1903,34 @@ AddHelper::DoDatabaseWork(mozIStorageCon
>       // XXX Once we support floats, we should use floor(mKey.ToFloat()) here
>-      autoIncrementNum = mKey.ToInteger();
>+      autoIncrementNum = floor(mKey.ToFloat());

Remove comment.

>--- /dev/null
>+++ b/dom/indexedDB/Key.cpp
>@@ -0,0 +1,378 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Indexed Database.
>+ *
>+ * The Initial Developer of the Original Code is The Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ben Turner <[e-mail]>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#include "jsdate.h"
>+#include "jsnum.h"
>+#include "Key.h"
>+
>+USING_INDEXEDDB_NAMESPACE
>+
>+/*
>+ ["foo"]       7 s s s 0 0
>+ [["foo"]]    11 s s s 0 0 0
>+ [[["foo"]]]  12 3 s s s 0 0 0 0
>+ [[[]]]       12 0 0 0
>+ [[]]         8 0
>+ [[], "foo"]  8 3 s s s 0 0
>+ []           4
>+*/

Clarify what this comment actually means, please.

>+
>+const int MaxArrayCollapse = 3;
>+
>+nsresult
>+Key::EncodeJSVal(JSContext* aCx, const jsval aVal, PRUint8 aTypeOffset)
>+{
>+  PR_STATIC_ASSERT(eMaxType * MaxArrayCollapse < 256);
>+
>+  if (JSVAL_IS_STRING(aVal)) {
>+    jsval tempRoot = JSVAL_VOID;
>+    EncodeString(xpc_qsAString(aCx, aVal, &tempRoot), aTypeOffset);

Note that I fixed this in bug 709977.

>+    return NS_OK;
>+  }
>+
>+  if (JSVAL_IS_INT(aVal)) {
>+    EncodeNumber((PRFloat64)JSVAL_TO_INT(aVal), eFloat + aTypeOffset);

s/PRFloat64/double/

>+  if (!JSVAL_IS_PRIMITIVE(aVal)) {
>+    JSObject* obj = JSVAL_TO_OBJECT(aVal);
>+    if (JS_IsArrayObject(aCx, obj)) {
>+      return NS_OK;
>+    } else if (JS_ObjectIsDate(aCx, obj)) {

No else after a return.

>+// static
>+nsresult
>+Key::DecodeJSVal(const unsigned char*& aPos, const unsigned char* aEnd,

'*&'?

>+                 JSContext* aCx, PRUint8 aTypeOffset, jsval* aVal)
>+{
>+    while (aPos < aEnd && *aPos - aTypeOffset != eTerminator) {
>+      if (!JS_SetElement(aCx, array, index++, &val)) {

Incrementing here is ugly, IMO.

>+  } else if (*aPos - aTypeOffset == eDate) {
>+    jsdouble msec = static_cast<jsdouble>(DecodeNumber(aPos, aEnd));

Just use double.

This would look nicer if you returned at the end of each branch.

>+Key::EncodeString(const nsAString& aString, PRUint8 aTypeOffset)
>+{
>+  *(buffer++) = eString + aTypeOffset;

This is also '*buffer++', but I appreciate the parens.

>+  // Encode string
>+  for (const PRUnichar* iter = start; iter < end; ++iter) {
>+    }
>+    else if (*iter <= TWO_BYTE_LIMIT) {

Cuddle else-if.

>+      *(buffer++) = (char)(c >> 16);
>+      *(buffer++) = (char)(c >> 8);
>+      *(buffer++) = (char)c;

static_cast

>+Key::DecodeString(const unsigned char*& aPos, const unsigned char* aEnd,
>+                  nsString& aString)
>+{

Same here.

>+Key::EncodeNumber(PRFloat64 aFloat, PRUint8 aType)

double here too.

>+PRFloat64

And here

>--- a/dom/indexedDB/Key.h
>+++ b/dom/indexedDB/Key.h
>+  const unsigned char* BufferStart() const
>+  {
>+    return (const unsigned char*)mBuffer.BeginReading();

static_cast

>--- a/dom/indexedDB/Makefile.in
>+++ b/dom/indexedDB/Makefile.in
> LOCAL_INCLUDES = \
>   -I$(topsrcdir)/db/sqlite3/src \
>   -I$(topsrcdir)/xpcom/build \
>   -I$(topsrcdir)/dom/base \
>   -I$(topsrcdir)/dom/src/storage \
>   -I$(topsrcdir)/content/base/src \
>   -I$(topsrcdir)/content/events/src \
>   -I$(topsrcdir)/js/xpconnect/src \
>+  -I$(topsrcdir)/js/src \

r-. Seriously. You don't do this.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Clarify what this comment actually means, please.

Yeah, the intent was always to write a longer comment. Jan asked me to attach before I spent the time doing that though.

> >+    return NS_OK;
> >+  }
> >+
> >+  if (JSVAL_IS_INT(aVal)) {
> >+    EncodeNumber((PRFloat64)JSVAL_TO_INT(aVal), eFloat + aTypeOffset);
> 
> s/PRFloat64/double/

Is that guaranteed to be 64bit? Please provide pointers.

> >+// static
> >+nsresult
> >+Key::DecodeJSVal(const unsigned char*& aPos, const unsigned char* aEnd,
> 
> '*&'?

Yes. It's an in-out parameter.

> >+  } else if (*aPos - aTypeOffset == eDate) {
> >+    jsdouble msec = static_cast<jsdouble>(DecodeNumber(aPos, aEnd));
> 
> Just use double.
> 
> This would look nicer if you returned at the end of each branch.

Why?

> >+Key::EncodeString(const nsAString& aString, PRUint8 aTypeOffset)
> >+{
> >+  *(buffer++) = eString + aTypeOffset;
> 
> This is also '*buffer++', but I appreciate the parens.

Yeah, if precedence is ambigious enough that I feel the need to look it up, I'd rather not rely on it and force others to look it up too.

> >+  // Encode string
> >+  for (const PRUnichar* iter = start; iter < end; ++iter) {
> >+    }
> >+    else if (*iter <= TWO_BYTE_LIMIT) {
> 
> Cuddle else-if.

I made things consistent with the other style instead.

> >+      *(buffer++) = (char)(c >> 16);
> >+      *(buffer++) = (char)(c >> 8);
> >+      *(buffer++) = (char)c;
> 
> static_cast

I don't actually think that makes things more readable in bit-twiddling operations like this.

> >--- a/dom/indexedDB/Makefile.in
> >+++ b/dom/indexedDB/Makefile.in
> > LOCAL_INCLUDES = \
> >   -I$(topsrcdir)/db/sqlite3/src \
> >   -I$(topsrcdir)/xpcom/build \
> >   -I$(topsrcdir)/dom/base \
> >   -I$(topsrcdir)/dom/src/storage \
> >   -I$(topsrcdir)/content/base/src \
> >   -I$(topsrcdir)/content/events/src \
> >   -I$(topsrcdir)/js/xpconnect/src \
> >+  -I$(topsrcdir)/js/src \
> 
> r-. Seriously. You don't do this.

It's not an exported header, so there's no other option right now. I'll file a follow-up.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

If you're touching non-installed JS headers, you shouldn't land. Period. You can easily add some of the double bit twiddling into mfbt or nsMathUtils.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need this for the js_DateGetMsecSinceEpoch function. It does more than bit-twiddling unfortunately. The right fix is simply to move it into a public JS-API per discussion with jorendorff

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

jsdate.h is still exported:

http://hg.mozilla.org/mozilla-central/file/d75ebb37080e/js/src/Makefile.in#l198

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oooh, neato, I for some reason assumed it wasn't. Things compile fine with that -I removed.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, my misstake, we needed this for the JSDOUBLE_IS_NaN macro in jsnum.h. That is simple bit-twiddling which we can do elsewhere.

Suggestions for where?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Latest interdiff. Goes on top of the others

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Total changes

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'm pretty sure we rely on the size of 'double' in a lot of place.

How about mfbt/Double.h for the bit twiddling?

Comment 25

[Jan Varga [:janv]]

Comment on attachment 582788 [details] [diff] [review]
Total changes

this looks really good, there are only some nits and most of them have been already fixed

+#include "jsdate.h"
+#include "nsContentUtils.h"
+#include "Key.h"
+#include "nsJSUtils.h"
+#include "nsIStreamBufferAccess.h"
+#include "xpcprivate.h"
+#include "XPCQuickStubs.h"

the rules for headers are:
1. IndexedDatabase.h
2. Interfaces
3. Other moz headers
4. Other indexeddb class headers
5. Forward declarations of other classes.

and alphabetize them

>+  if (JSVAL_IS_DOUBLE(aVal) && !DOUBLE_IS_NaN(JSVAL_TO_DOUBLE(aVal))) {

This doesn't compile on windows
Jonas says, he already fixed it

The Double.h can be done in a followup IMO

>+static inline
>+PRUint64 DoFlipBits(PRUint64 u)
>+{
>+  return u & PR_UINT64(0x8000000000000000) ?
>+         -u :
>+         (u | PR_UINT64(0x8000000000000000));
>+}
>+
>+static inline
>+PRUint64 UndoFlipBits(PRUint64 u)
>+{
>+  return u & PR_UINT64(0x8000000000000000) ?
>+         (u & ~PR_UINT64(0x8000000000000000)) :
>+         -u;
>+}
>+

these operations can be done directly in EncodeNumber/DecodeNumber (per discussion on IRC)

>+protected:
>+

no new line needed

>+  const unsigned char* BufferStart() const

>-        is(event.target.result, key, "correct returned key in " + test);
>+        is(JSON.stringify(event.target.result), JSON.stringify(key),
>+           "correct returned key in " + test);

there's a function called isDeeply(), but as we discussed JSON.strigify() provides better debugging info

r=janv

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Total patch. Fixes review comments

This is the whole shebang with Jan's review comments fixed

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 582898 [details] [diff] [review]
Total patch. Fixes review comments

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

::: dom/indexedDB/Key.cpp
@@ +19,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2010
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Ben Turner <[e-mail]>

Hey, I had nothing to do with this!

@@ +158,5 @@
> +
> +  if (!JSVAL_IS_PRIMITIVE(aVal)) {
> +    JSObject* obj = JSVAL_TO_OBJECT(aVal);
> +    if (JS_IsArrayObject(aCx, obj)) {
> +      aTypeOffset += eMaxType;

I really think you mean eArray here, right?

@@ +179,5 @@
> +        if (!JS_GetElement(aCx, obj, index, &val)) {
> +          return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +        }
> +
> +        nsresult rv = EncodeJSVal(aCx, val, aTypeOffset);

Hm... This is recursive and based on unsanitized input so someone could craft a key that will crash us. You need some recursion protection.

@@ +221,5 @@
> +
> +    jsuint index = 0;
> +    while (aPos < aEnd && *aPos - aTypeOffset != eTerminator) {
> +      jsval val;
> +      nsresult rv = DecodeJSVal(aPos, aEnd, aCx, aTypeOffset, &val);

Here too.

::: dom/indexedDB/Key.h
@@ +72,3 @@
>                   "Don't compare unset keys!");
>  
> +    return mBuffer.Equals(aOther.mBuffer);

Just out of curiosity why didn't you do |Compare(a, b) == 0| to match all the others?

@@ +134,5 @@
> +  {
> +    NS_ASSERTION(IsFloat(), "Why'd you call this?");
> +    const unsigned char* pos = BufferStart();
> +    double res = DecodeNumber(pos, BufferEnd());
> +    NS_ASSERTION(pos >= BufferEnd(), "Should consume whole buffer");

Should be == right?

@@ +184,5 @@
> +    const unsigned char* pos = BufferStart();
> +    nsresult rv = DecodeJSVal(pos, BufferEnd(), aCx, 0, aVal);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    NS_ASSERTION(pos >= BufferEnd(),

== again

@@ +237,3 @@
>    }
>  
> +protected:

Er? We don't expect any subclasses, do we? I think we can leave this private.

@@ +252,5 @@
> +    eFloat = 1,
> +    eDate = 2,
> +    eString = 3,
> +    eArray = 4,
> +    eMaxType = eArray

So I don't quite understand why you use eMaxType all over the place. Seems to me you really want eArray, and you just want to make sure that eArray is always greater than the other basic types.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1110,5 @@
> +    if (type == mozIStorageStatement::VALUE_TYPE_INTEGER) {
> +      PRInt64 intKey;
> +      aArguments->GetInt64(0, &intKey);
> +      key.SetFromInteger(intKey);
> +    } else {

No cuddling!

And assert that type == VALUE_TYPE_TEXT?

@@ +1111,5 @@
> +      PRInt64 intKey;
> +      aArguments->GetInt64(0, &intKey);
> +      key.SetFromInteger(intKey);
> +    } else {
> +      nsAutoString stringKey;

nsString here.

@@ +1146,5 @@
> +      "id INTEGER PRIMARY KEY, "
> +      "object_store_id, "
> +      "key_value, "
> +      "data, "
> +      "file_ids "

Shit, how'd I miss this earlier? Since you're doing this, can you reorder the column so that |file_ids| comes before |data|?

@@ +1259,5 @@
> +  ));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "INSERT OR IGNORE INTO index_data "

At this point we don't need the OR IGNORE right?

@@ +1271,5 @@
> +  ));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE INDEX index_data_object_data_id_index "

Wait, you never dropped the old index. Have you tested this?

@@ +1327,5 @@
> +  ));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "CREATE INDEX unique_index_data_object_data_id_index "

Here too.

Comment 28

[Jan Varga [:janv]]

(In reply to ben turner [:bent] from comment #27)
> @@ +1271,5 @@
> > +  ));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> > +    "CREATE INDEX index_data_object_data_id_index "
> 
> Wait, you never dropped the old index. Have you tested this?
> 
> @@ +1327,5 @@
> > +  ));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> > +    "CREATE INDEX unique_index_data_object_data_id_index "
> 
> Here too.

triggers, indexes, etc drop with the table automatically

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in!

https://hg.mozilla.org/mozilla-central/rev/214af12ee22d

Comment 30

[Scoobidiver (away)]

It missed the Firefox 11 release for a few hours.

Comment 31

[Jan Varga [:janv]]

(In reply to Scoobidiver from comment #30)
> It missed the Firefox 11 release for a few hours.

are you sure ?

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It made 11.

Comment 34

[Jean-Yves Perrier [:teoli]]

Added a sentence in:
https://developer.mozilla.org/en/IndexedDB/Basic_Concepts_Behind_IndexedDB#section_6

and a note to:
https://developer.mozilla.org/en/Firefox_11_for_developers#IndexedDB