Last Comment Bug 553413 - (async-IndexedDB) Implement Asynchronous parts of the Indexed Database API
(async-IndexedDB)
: Implement Asynchronous parts of the Indexed Database API
Status: RESOLVED FIXED
[evang-wanted]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla2.0
Assigned To: Shawn Wilsher :sdwilsh
:
Mentors:
Depends on: 571581 571599 574232 574257
Blocks: IndexedDB
  Show dependency treegraph
 
Reported: 2010-03-18 15:10 PDT by Shawn Wilsher :sdwilsh
Modified: 2011-02-17 07:26 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+


Attachments
v0.1 (903.59 KB, patch)
2010-06-10 17:35 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v0.1.1 (502.42 KB, patch)
2010-06-10 17:44 PDT, Shawn Wilsher :sdwilsh
jst: review+
Details | Diff | Splinter Review
v0.1.1 to v0.2 interdiff (166.91 KB, patch)
2010-06-18 11:15 PDT, Shawn Wilsher :sdwilsh
jst: review+
Details | Diff | Splinter Review
v0.2 to v0.3 interdiff (68.46 KB, patch)
2010-06-21 14:36 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2010-03-18 15:10:58 PDT

    
Comment 1 Shawn Wilsher :sdwilsh 2010-03-24 11:27:03 PDT
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 2010-06-10 17:35:52 PDT
Created attachment 450528 [details] [diff] [review]
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 2010-06-10 17:44:02 PDT
Created attachment 450537 [details] [diff] [review]
v0.1.1

Last one had two files marked as changed that shouldn't have been.  This fixes that.
Comment 4 Shawn Wilsher :sdwilsh 2010-06-11 11:29:43 PDT
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, jst@mozilla.com) 2010-06-16 20:01:50 PDT
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 2010-06-18 11:15:44 PDT
Created attachment 452305 [details] [diff] [review]
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, jst@mozilla.com) 2010-06-18 14:38:55 PDT
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!) 2010-06-18 15:59:59 PDT
(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!) 2010-06-21 14:36:54 PDT
Created attachment 452853 [details] [diff] [review]
v0.2 to v0.3 interdiff

Spec complete as of last friday.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2010-06-21 16:15:34 PDT
Comment on attachment 452853 [details] [diff] [review]
v0.2 to v0.3 interdiff

Looks good!
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-24 07:02:49 PDT
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!) 2010-06-24 08:36:36 PDT
http://hg.mozilla.org/mozilla-central/rev/e910fd948e5b

This landed yesterday afternoon, sorry.
Comment 13 Eric Shepherd [:sheppy] 2011-02-17 07:26:14 PST
Documentation:

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

Note You need to log in before you can comment on or make changes to this bug.