Bug 553413 (async-IndexedDB)

Implement Asynchronous parts of the Indexed Database API

RESOLVED FIXED in mozilla2.0

Status

()

defect
P1
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: [evang-wanted])

Attachments

(3 attachments, 1 obsolete attachment)

No description provided.
Alias: async-IndexedDB
Whiteboard: [evang-wanted]
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).
Depends on: 554693
Depends on: 555316
blocking2.0: ? → beta1+
No longer depends on: 554693
No longer depends on: 555316
Posted patch v0.1 (obsolete) — Splinter Review
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
Attachment #450528 - Flags: review?(jst)
Posted patch v0.1.1Splinter Review
Last one had two files marked as changed that shouldn't have been.  This fixes that.
Attachment #450528 - Attachment is obsolete: true
Attachment #450537 - Flags: review?(jst)
Attachment #450528 - Flags: review?(jst)
jst: please don't review the stuff in storage/.  I'll be spinning that out into a new bug today.
Depends on: 571581
Depends on: 571599
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.
Attachment #450537 - Flags: review?(jst) → review+
Builds on v0.1.1 by implementing more things, addressing review requests, and adding more tests.
Attachment #452305 - Flags: review?(jst)
Depends on: 568898
No longer depends on: 568898
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.
Attachment #452305 - Flags: review?(jst) → review+
(In reply to comment #7)
> The !len check got dropped here, is that intentional?

Yeah, it's ok.
Spec complete as of last friday.
Attachment #452853 - Flags: review?(jst)
Comment on attachment 452853 [details] [diff] [review]
v0.2 to v0.3 interdiff

Looks good!
Attachment #452853 - Flags: review?(jst) → review+
Depends on: 574232
Depends on: 574257
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.
http://hg.mozilla.org/mozilla-central/rev/e910fd948e5b

This landed yesterday afternoon, sorry.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3 → mozilla2.0
You need to log in before you can comment on or make changes to this bug.