553413 - (async-IndexedDB) Implement Asynchronous parts of the Indexed Database API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Comment 1

[Shawn Wilsher :sdwilsh]

So, what I'm going to do here is spin off sub bugs to do the chunks of work, and every so often I'll land them to mozilla-central when it makes sense to do so (such as "we have opening and creating object stores done, let's land).

Comment 2

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

This is still rough, and I'll be going over it tomorrow more to see if I have things that I see need to be fixed (and some stuff needs to be pulled out into its own bug).

Getting it up so people can start looking it over though...

Generated with the following command:
diff -x .hg -x idb.diff -x build -x config -r -U8 -p -N ../clea
n-mozilla-central/ . > idb.diff

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v0.1.1

Last one had two files marked as changed that shouldn't have been.  This fixes that.

Comment 4

[Shawn Wilsher :sdwilsh]

jst: please don't review the stuff in storage/.  I'll be spinning that out into a new bug today.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 450537 [details] [diff] [review]
v0.1.1

This was one *huge* patch! I did make it through the whole patch (but skipped the storage changes) but only found a couple of problems. So apart from what's listed below this looks good to me!

- In dom/base/nsDOMClassInfo.cpp:

+  NS_DEFINE_CLASSINFO_DATA(IDBRequest, nsEventTargetSH,
+                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

XXX: Looking at nsEventTargetSH it seems to be specifically written to deal with nsXHREventTarget, is that really what we want here?

- In IDBCursorRequest::Update():

+      JSString* str = JSVAL_TO_STRING(prop.value());
+      size_t len = JS_GetStringLength(str);
+      if (!len) {
+        return NS_ERROR_INVALID_ARG;
+      }
+      const PRUnichar* chars =
+        reinterpret_cast<const PRUnichar*>(JS_GetStringChars(str));
+      if (key.StringValue() != nsDependentString(chars, len)) {

Instead typing out all of that you could use nsDependentJSString. Same in a few other place.

- In UpdateHelper::DoDatabaseWork():

+  // TODO update indexes if needed

Bug on file?

- In GetSuccessEvent::GetResult() in IDBEvents.cpp:

+  if (!mJSRuntime) {
+    JSContext* cx;
+    rv = cc->GetJSContext(&cx);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    JSAutoRequest ar(cx);
+
+    JSRuntime* rt = JS_GetRuntime(cx);
+
+    JSBool ok = JS_AddNamedRootRT(rt, &mCachedValue,
+                                  "GetSuccessEvent::mCachedValue");
+    NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
+
+    nsCOMPtr<nsIJSON> json(new nsJSON());
+    rv = json->DecodeToJSVal(mValue, cx, &mCachedValue);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    mJSRuntime = rt;

GetSuccessEvent::~GetSuccessEvent() only calls JS_RemoveRootRT() if mJSRuntime is set, so if the above JSON call fails we'll leak and/or crash later on.

r=jst with that looked into.

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v0.1.1 to v0.2 interdiff

Builds on v0.1.1 by implementing more things, addressing review requests, and adding more tests.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 452305 [details] [diff] [review]
v0.1.1 to v0.2 interdiff

- In dom/indexedDB/IDBObjectStoreRequest.cpp, GetKeyFromObject():

-    size_t len = JS_GetStringLength(str);
-    if (!len) {
-      return NS_ERROR_INVALID_ARG;
-    }
-    const PRUnichar* chars =
-      reinterpret_cast<const PRUnichar*>(JS_GetStringChars(str));
-    aKey = nsDependentString(chars, len);
+    aKey = nsDependentJSString(key);
     return NS_OK;

The !len check got dropped here, is that intentional?

r=jst with that looked into.

Comment 8

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

(In reply to comment #7)
> The !len check got dropped here, is that intentional?

Yeah, it's ok.

Comment 9

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

Attachment: v0.2 to v0.3 interdiff

Spec complete as of last friday.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 452853 [details] [diff] [review]
v0.2 to v0.3 interdiff

Looks good!

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

This is marked as a b1 blocker, and all the dependencies are complete - should this get checked in, marked as fixed ... just trying to determine status.

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/e910fd948e5b

This landed yesterday afternoon, sorry.

Comment 13

[Eric Shepherd [:sheppy]]

Documentation:

https://developer.mozilla.org/en/IndexedDB