Closed Bug 767186 Opened 12 years ago Closed 12 years ago

Always return the same JSObject for IDB[ObjectStore|Index].keyPath

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
      No description provided.
Attachment #635513 - Flags: review?(bent.mozilla)
Comment on attachment 635513 [details] [diff] [review]
Patch

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

All the comments I made for IDBIndex apply to IDBObjectStore.

Need some answers below.

::: dom/indexedDB/IDBIndex.cpp
@@ +410,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(!mActorParent, "Actor parent owns us, how can we be dying?!");
> +
> +  if (mRooted) {
> +    NS_DROP_JS_OBJECTS(this, IDBIndex);

Wait... Didn't we just have a bug about calling NS_DROP_JSOBJECTS on idb objects?

@@ +665,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBIndex)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: remove extra line

@@ +673,5 @@
> +
> +  tmp->mCachedKeyPath = JSVAL_VOID;
> +
> +  if (tmp->mRooted) {
> +    NS_DROP_JS_OBJECTS(tmp, IDBIndex);

Hm... Weren't we planning on moving this to IDBWrapperCache?

@@ +717,5 @@
> +    *aVal = mCachedKeyPath;
> +    return NS_OK;
> +  }
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);

First assert PreservingWrapper()

@@ +720,5 @@
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {

JSVAL_IS_GCTHING will be better.

@@ +722,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {
> +    mRooted = true;
> +    NS_HOLD_JS_OBJECTS(this, IDBIndex);

Nit: Put mRooted second here.

::: dom/indexedDB/IDBIndex.h
@@ +148,5 @@
>  
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;

I can't decide if I like this... We should file a followup to replace all jsval with JS::Value I guess.

@@ +149,5 @@
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;
> +  bool mRooted;

Nit: Stick this below with the other bools so that they'll pack.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1763,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBObjectStore)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: extra line...
IDBObjectStore/IDBIndex don't inherit from IDBWrapperCache.
Comment on attachment 635513 [details] [diff] [review]
Patch

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

All the comments I made for IDBIndex apply to IDBObjectStore.

Need some answers below.

::: dom/indexedDB/IDBIndex.cpp
@@ +410,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(!mActorParent, "Actor parent owns us, how can we be dying?!");
> +
> +  if (mRooted) {
> +    NS_DROP_JS_OBJECTS(this, IDBIndex);

Wait... Didn't we just have a bug about calling NS_DROP_JSOBJECTS on idb objects?

@@ +665,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBIndex)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: remove extra line

@@ +673,5 @@
> +
> +  tmp->mCachedKeyPath = JSVAL_VOID;
> +
> +  if (tmp->mRooted) {
> +    NS_DROP_JS_OBJECTS(tmp, IDBIndex);

Hm... Weren't we planning on moving this to IDBWrapperCache?

@@ +717,5 @@
> +    *aVal = mCachedKeyPath;
> +    return NS_OK;
> +  }
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);

First assert PreservingWrapper()

@@ +720,5 @@
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {

JSVAL_IS_GCTHING will be better.

@@ +722,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {
> +    mRooted = true;
> +    NS_HOLD_JS_OBJECTS(this, IDBIndex);

Nit: Put mRooted second here.

::: dom/indexedDB/IDBIndex.h
@@ +148,5 @@
>  
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;

I can't decide if I like this... We should file a followup to replace all jsval with JS::Value I guess.

@@ +149,5 @@
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;
> +  bool mRooted;

Nit: Stick this below with the other bools so that they'll pack.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1763,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBObjectStore)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: extra line...
Attachment #635513 - Flags: review?(bent.mozilla) → review+
Uh... Splinter freaked out, sorry.

r=me!
https://hg.mozilla.org/mozilla-central/rev/4cbc78fd6eef
Assignee: nobody → khuey
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.