Shawn Wilsher :sdwilsh Assignee
	Description • 16 years ago

For JavaScript, we should automagically provide the mozIStorageStatementWrapper interface.  This will help make using storage APIs with JS much easier.

	Shawn Wilsher :sdwilsh Assignee
	Comment 1 • 16 years ago

This doesn't seem to work, but I'm not sure why yet.  Needs investigating.

	Shawn Wilsher :sdwilsh Assignee
	Comment 2 • 16 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 3 • 16 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 4 • 16 years ago

Wow this took way longer than I ever expected.  Worth it though.  This requires the refactor patch to be applied first.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 5 • 16 years ago

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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 6 • 16 years ago

(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.

	Shawn Wilsher :sdwilsh Assignee
	Comment 7 • 16 years ago

Addresses review comments - short of the static stuff.  bsmedberg isn't around to ask that question.

	Shawn Wilsher :sdwilsh Assignee
	Comment 8 • 16 years ago

uploaded the same file as last time...

	Shawn Wilsher :sdwilsh Assignee
	Comment 9 • 16 years ago

(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

	Shawn Wilsher :sdwilsh Assignee
	Comment 10 • 16 years ago

memo to myself:
Update mozIStorageStatement idl comments to indicate that JS has these properties as well.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 11 • 16 years ago

Comment on attachment 336940 [details] [diff] [review]
v1.1

Looks fine, blake already caught most of the same stuff that I had questions about :)

	Shawn Wilsher :sdwilsh Assignee
	Comment 12 • 16 years ago

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/66f60fcbdfad
http://hg.mozilla.org/mozilla-central/rev/37298927e98b