Closed Bug 452897 Opened 12 years ago Closed 12 years ago

Automatically provide the wrapper for Javascript

Categories

(Toolkit :: Storage, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 4 obsolete files)

For JavaScript, we should automagically provide the mozIStorageStatementWrapper interface.  This will help make using storage APIs with JS much easier.
Whiteboard: [needs patch]
Blocks: 452899
Attached patch v0.1 (obsolete) — Splinter Review
This doesn't seem to work, but I'm not sure why yet.  Needs investigating.
Attached patch refactorSplinter Review
This just moves some code that is currently all in one file into three different ones, plus header files so I can actually re-use code.  Next patch will actually have changes that are reviewable.
Attachment #336290 - Attachment is obsolete: true
Attached patch v0.2 (obsolete) — Splinter Review
Almost there - doesn't quite work yet.  This only gives access to the row and params properties, but I think that's all that really matters.
Attached patch v1.0 (obsolete) — Splinter Review
Wow this took way longer than I ever expected.  Worth it though.  This requires the refactor patch to be applied first.
Attachment #336767 - Attachment is obsolete: true
Attachment #336922 - Flags: review?(vladimir)
Attachment #336922 - Flags: review?(mrbkap)
Whiteboard: [needs patch] → [has patch][needs review vlad][needs review mrbkap]
Attachment #336922 - Flags: review?(mrbkap)
Comment on attachment 336922 [details] [diff] [review]
v1.0

diff --git a/storage/src/mozStorageStatement.cpp b/storage/src/mozStorageStatement.cpp
+static mozStorageStatementClassInfo sStatementClassInfo;

I think that having file static things is a Bad Thing for some reason, but I'm not sure. Check on that?
>diff --git a/storage/src/mozStorageStatementJSHelper.cpp b/storage/src/mozStorageStatementJSHelper.cpp
>+  nsresult rv = XPConnect()->GetWrappedNativeOfJSObject(
>+    aCtx, JS_THIS_OBJECT(aCtx, _vp), getter_AddRefs(wrapper)
>+  );
>+  NS_ENSURE_SUCCESS(rv, JS_FALSE);

Need to report the exception to aCtx (I laugh at your silly names!).

>+//// nsIXPCScriptable

I pointed this out to you in person, but you can use the macros in js/src/xpconnect/public/xpc_map_end.h to help you avoid having to write some of this gunk.

>+NS_IMETHODIMP
>+mozStorageStatementJSHelper::GetProperty(nsIXPConnectWrappedNative *aWrapper,
>+                                         JSContext *aCtx, JSObject *aScope,

aScope -> aScopeObject at the very least, please.

>+                                         jsval aId, jsval *_result,
>+                                         PRBool *_retval)
>+{
>+  mozStorageStatement *stmt =
>+    static_cast<mozStorageStatement *>(aWrapper->Native());
>+
>+#ifdef DEBUG
>+  {
>+    nsCOMPtr<mozIStorageStatement> isStatement(do_QueryInterface(stmt));
>+    NS_ASSERTION(isStatement, "How is this not a statement?!");
>+  }
>+#endif
>+
>+  nsresult rv;
>+  const char *propName = JS_GetStringBytes(JS_ValueToString(aCtx, aId));

I'd rather see an early test for JSVAL_IS_STRING(aId) so this could become an infallible JSVAL_TO_STRING.

>+  if (strcmp(propName, "row") == 0) {
>+    rv = getRow(stmt, aCtx, aScope, _result);
>+    *_retval = PR_TRUE;

_retval is guaranteed to be PR_TRUE on entry. No need to set it.

>+    return NS_OK;
>+  }
>+  else if (strcmp(propName, "params") == 0) {
>+    rv = getParams(stmt, aCtx, aScope, _result);
>+    *_retval = PR_TRUE;

Ditto. (Also, else after return!).

>+    return NS_OK;

This return doesn't seem necessary.

>+  }
>+
>+  return NS_OK;
>+}

>+mozStorageStatementJSHelper::NewResolve(nsIXPConnectWrappedNative *aWrapper,
>+                                        JSContext *aCtx, JSObject *aScopeObj,
>+                                        jsval aId, PRUint32 aFlags,
>+                                        JSObject **_objp, PRBool *_retval)
>+{
...
>+  *_retval = PR_TRUE;

Don't need to do this.

>diff --git a/storage/src/mozStorageStatementRow.cpp b/storage/src/mozStorageStatementRow.cpp
> NS_IMETHODIMP
> mozStorageStatementRow::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
>                          JSObject * obj, jsval id, jsval * vp, PRBool *_retval)
> {
>     if (JSVAL_IS_STRING(id)) {
>-        nsDependentString jsid((PRUnichar *)::JS_GetStringChars(JSVAL_TO_STRING(id)),
>-                               ::JS_GetStringLength(JSVAL_TO_STRING(id)));
>+        nsDependentCString jsid(::JS_GetStringBytes(JSVAL_TO_STRING(id)));

This change actually makes this less efficient (since we're guaranteed to have the wide string in the JS engine but may have to lazily create the narrow version during JS_GetStringBytes).


> mozStorageStatementRow::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
>                                    JSObject * obj, jsval id, PRUint32 flags, JSObject * *objp, PRBool *_retval)
> {
>     if (JSVAL_IS_STRING(id)) {
>         JSString *str = JSVAL_TO_STRING(id);
>-        nsDependentString name((PRUnichar *)::JS_GetStringChars(str),
>-                               ::JS_GetStringLength(str));
>+        nsDependentCString name(::JS_GetStringBytes(str));

Ditto here.

I'll stamp the next patch.
(In reply to comment #5)
> This change actually makes this less efficient (since we're guaranteed to have
> the wide string in the JS engine but may have to lazily create the narrow
> version during JS_GetStringBytes).
I'll have to create the C string anyway because the storage API needs an nsACString.
Attached patch v1.1 (obsolete) — Splinter Review
Addresses review comments - short of the static stuff.  bsmedberg isn't around to ask that question.
Attachment #336922 - Attachment is obsolete: true
Attachment #336938 - Flags: review?(vladimir)
Attachment #336938 - Flags: review?(mrbkap)
Attachment #336922 - Flags: review?(vladimir)
Attached patch v1.1Splinter Review
uploaded the same file as last time...
Attachment #336938 - Attachment is obsolete: true
Attachment #336940 - Flags: review?(vladimir)
Attachment #336940 - Flags: review?(mrbkap)
Attachment #336938 - Flags: review?(vladimir)
Attachment #336938 - Flags: review?(mrbkap)
Attachment #336940 - Flags: review?(mrbkap) → review+
Whiteboard: [has patch][needs review vlad][needs review mrbkap] → [has patch][needs review mrbkap]
Whiteboard: [has patch][needs review mrbkap] → [has patch][needs review vlad]
(In reply to comment #5)
> I think that having file static things is a Bad Thing for some reason, but I'm
> not sure. Check on that?
<bsmedberg> sdwilsh: it's safe only if the object doesn't do any real work in the constructor/destructor and doesn't have any interesting members like comptrs or strings
memo to myself:
Update mozIStorageStatement idl comments to indicate that JS has these properties as well.
Blocks: 403376
No longer blocks: 403376
Comment on attachment 336940 [details] [diff] [review]
v1.1

Looks fine, blake already caught most of the same stuff that I had questions about :)
Whiteboard: [has patch][needs review vlad] → [needs new patch][has reviews]
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/66f60fcbdfad
http://hg.mozilla.org/mozilla-central/rev/37298927e98b
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [needs new patch][has reviews]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 457743, 464202
You need to log in before you can comment on or make changes to this bug.