333848 - add full-featured user-defined functions and progress handlers to storage

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement)

Description

[Lev Serebryakov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

(1) mozIStorageFunction returns nsIVariant, which passed to SQLite3
engine. I can not see any usage for current mozIStorageFunction
definition: without return value and without access to sqlite3's
rsult-setting functions.

(2) Functions can be added many times for different names (aliases).

(3) mozIStorageAggreagteFunction & helpers are added, which allows to
define user-defined aggregate functions. Aggreagte functions can be
registered only once (no aliases!)

(4) mozIStorageProgressHandler & helpers are added. This is VERY useful
when THREADS_20060307_BRANCH will be merged into trunk, because it
allows to pump events and don't freeze UI when long queries are in
progress. I've tested it with this branch and it works very well for me.
  Yes, I know, that 'Places' doesn't need such feature, but I'm looking
at XULRunenr as at generic platform, and many desktop applications will
run not-so-short queries, and freezed UI is very ugly. 

Reproducible: Always

Comment 1

[Lev Serebryakov]

Attachment: Patch to changed files.

New IDL files are in separate attaches.

Comment 2

[Lev Serebryakov]

Attachment: Aggreaget function interface description.

Comment 3

[Lev Serebryakov]

Attachment: Progress handler interface description.

Comment 4

[Nickolay_Ponomarev]

Please attach an output of cvs diff -u8pN (see http://developer.mozilla.org/en/docs/Creating_a_patch for a tip on how to convince CVS to include a new file in the diff) and request review (I guess from [e-mail]) by editing the attachment and setting an appropriate flag.

Comment 5

[Lev Serebryakov]

Attachment: Patch to add descirbed functionality.

Comment 6

[Lev Serebryakov]

(In reply to comment #4)
> Please attach an output of cvs diff -u8pN (see
> http://developer.mozilla.org/en/docs/Creating_a_patch for a tip on how to
> convince CVS to include a new file in the diff) and request review (I guess
> from [e-mail]) by editing the attachment and setting an appropriate
> flag.
Thank you for suggestions. Done.

Comment 7

[Martijn Wargers (dead)]

Vlad, any chance you can review this patch in the nearby future?

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 218296 [details] [diff] [review]
Patch to add descirbed functionality.

This looks great; a few spelling/formatting nits below, but other than that r+.  If brettw has time, it would be good to get him to look it over quickly as well.

>+++ public/mozIStorageAggregateFunction.idl	13 Apr 2006 15:43:49 -0000

>+  /**
>+   * onFinal is called when all rows in group has been
>+   * processed and engine need aggreagte function value.

typo; "aggregate"

>+   * @returns aggregte result as Variant.

same

>Index: src/mozStorageConnection.cpp
>===================================================================
>+++ src/mozStorageConnection.cpp	13 Apr 2006 15:43:50 -0000



>+    switch( dt )
>+    {
>+    case nsIDataType::VTYPE_INT8: 
>+    case nsIDataType::VTYPE_INT16:
>+    case nsIDataType::VTYPE_INT32:
>+    case nsIDataType::VTYPE_UINT8:
>+    case nsIDataType::VTYPE_UINT16:
>+    case nsIDataType::VTYPE_UINT32:
>+        {
>+            PRInt32 v;
>+            rv = var->GetAsInt32 (&v);
>+            if (NS_FAILED(rv)) {
>+                return rv;
>+            }
>+            sqlite3_result_int (ctx, v);
>+            break;
>+        }

Pull these break's out of the braces

>+    case nsIDataType::VTYPE_INT64:
>+    case nsIDataType::VTYPE_UINT64:
>+        {
>+            PRInt64 v;
>+            rv = var->GetAsInt64 (&v);
>+            if (NS_FAILED(rv)) {
>+                return rv;
>+            }
>+            sqlite3_result_int64 (ctx, v);
>+            break;

extraneous break;

>+        }
>+        break;
>+    case nsIDataType::VTYPE_FLOAT:
>+    case nsIDataType::VTYPE_DOUBLE:
>+        {
>+            double v;
>+            rv = var->GetAsDouble (&v);
>+            if (NS_FAILED(rv)) {
>+                return rv;
>+            }
>+            sqlite3_result_double (ctx, v);
>+            break;

same

>+        }
>+        break;
>+    case nsIDataType::VTYPE_BOOL:
>+    case nsIDataType::VTYPE_CHAR:
>+    case nsIDataType::VTYPE_WCHAR:
>+    case nsIDataType::VTYPE_DOMSTRING:
>+    case nsIDataType::VTYPE_CHAR_STR:
>+    case nsIDataType::VTYPE_WCHAR_STR:
>+    case nsIDataType::VTYPE_STRING_SIZE_IS:
>+    case nsIDataType::VTYPE_WSTRING_SIZE_IS:
>+    case nsIDataType::VTYPE_UTF8STRING:
>+    case nsIDataType::VTYPE_CSTRING:
>+    case nsIDataType::VTYPE_ASTRING:
>+        {
>+            nsAutoString v;
>+            rv = var->GetAsAString (v);
>+            if (NS_FAILED(rv)) {
>+                return rv;
>+            }
>+            sqlite3_result_text16 (ctx, 
>+                                   nsPromiseFlatString(v).get(),
>+                                   v.Length() * 2,
>+                                   SQLITE_TRANSIENT);
>+            break;

pull this one out as well

>+        }
>+    case nsIDataType::VTYPE_VOID:
>+    case nsIDataType::VTYPE_EMPTY:
>+        sqlite3_result_null (ctx);
>+        break;
>+    case nsIDataType::VTYPE_ID:
>+    case nsIDataType::VTYPE_INTERFACE:
>+    case nsIDataType::VTYPE_INTERFACE_IS:
>+    case nsIDataType::VTYPE_ARRAY:
>+    case nsIDataType::VTYPE_EMPTY_ARRAY:
>+    default:
>+      return NS_ERROR_FAILURE;
>+    }
>+    return NS_OK;
>+}

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 218296 [details] [diff] [review]
Patch to add descirbed functionality.

Brett, do you have time to glance at this?  It looks fine to me, just wondering if you see anything glaring that I might have missed.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Also, do you have a test case for the new features that we could add to the tree?

Comment 11

[Lev Serebryakov]

(In reply to comment #10)

I'll fix all your notes tomorrow or day later.

> Also, do you have a test case for the new features that we could add to the
> tree?
  In which format and for which environment? I have real code in my XULRunner application, which uses these features. It is JS code. Of course, I can extract simple test cases from this code. But in which environment will they be run? HTML? XUL? jsshell? Can they create database for test statements?
  Is here any documents, describing such tests and test environment?

Comment 12

[Nickolay_Ponomarev]

(In reply to comment #11)
> > Also, do you have a test case for the new features that we could add to the
> > tree?
>   In which format and for which environment? I have real code in my XULRunner
> application, which uses these features. It is JS code. Of course, I can extract
> simple test cases from this code. But in which environment will they be run?
> HTML? XUL? jsshell? Can they create database for test statements?
>   Is here any documents, describing such tests and test environment?
> 
As far as I understand, your tests should be able to run in xpcshell. There's little documentation about writing tests; I was pointed to http://wiki.mozilla.org/SoftwareTesting:Tools:Simple_xpcshell_test_harness for xpcshell unit tests. You can ask questions in irc.mozilla.org/qa and dev-quality.

Comment 13

[Brett Wilson]

Comment on attachment 218296 [details] [diff] [review]
Patch to add descirbed functionality.

You added a file that says the original author is Oracle. If you don't work for Oracle, you should put your name or your company's name there.

How do I clear a progress handler I've installed. If I can't, is there a reason why? If so, can you document this? Otherwise, I think we should add a way.

I'd like to see a little more documentation for createAggregateFunction in the IDL file. I don't know what an aggregate function is or what I should use it for. It would be great it you could put the same thing at the top of the aggregate function IDL file. I think the storage component's interfaces aren't documented quite enough, so it would be good if all new patches improved the situation.

I noticed you are converting a UInt32 to a sqlite int32, a problem I ran into while writing the variant functions for the annotation service. I'm not sure what to do about this.

Also, it would be nice (not in this patch, just generally) if we could expose variant->sqlite and sqlite->variant functions. The annotation service could use this.

I didn't look too closely at the functional logic.

Comment 14

[Lev Serebryakov]

Attachment: Path to add described functionality v2. Changes, requested by vlad and test.

Comment 15

[Lev Serebryakov]

Attachment: Patch to add described functionality v2.1. Changes, requested by vlad and test. Remove bogus mention about oracle.

Comment 16

[Lev Serebryakov]

(In reply to comment #13)
> You added a file that says the original author is Oracle. If you don't work for
> Oracle, you should put your name or your company's name there.
Oops. My fault.
> How do I clear a progress handler I've installed. If I can't, is there a reason
> why? If so, can you document this? Otherwise, I think we should add a way.
Comment to IDL file added.

> I'd like to see a little more documentation for createAggregateFunction in the
> IDL file. I don't know what an aggregate function is or what I should use it
> for. It would be great it you could put the same thing at the top of the
> aggregate function IDL file.
Added.

> I noticed you are converting a UInt32 to a sqlite int32, a problem I ran into
> while writing the variant functions for the annotation service. I'm not sure
> what to do about this.
Convert int32 to int64 now. But here is problem with UInt64 in any case :(

> Also, it would be nice (not in this patch, just generally) if we could expose
> variant->sqlite and sqlite->variant functions. The annotation service could use
> this.
  sqlite3 doesn't have any "variant-like" type, so I don't see how we can have generic converters :(

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 232331 [details] [diff] [review]
Patch to add described functionality v2.1. Changes, requested by vlad and test. Remove bogus mention about oracle.

Looks great, r=me

Comment 18

[Jeff Walden [:Waldo]]

Should this test be runnable using 'make check'?  CCing davel under the assumption that it should be...

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #18)
> Should this test be runnable using 'make check'?  CCing davel under the
> assumption that it should be...

No, it shouldn't.  The old test code wasn't runnable with 'make checkc', and I bet Lev has better things to do othan trawl through our build system to fix the test infrastructure.

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> No, it shouldn't.  The old test code wasn't runnable with 'make checkc'

Just because that's the way it is doesn't mean that's the way it should be.

> I bet Lev has better things to do othan trawl through our build system to fix
> the test infrastructure.

Agreed, and that's why we have davel -- I figure he'd be interested in hooking this up.  :-)  Until there's good, easily discoverable documentation on how to hook up automated tests for the build system, I don't think requiring test hookup in patches is a good idea.

Comment 21

[Dave Liebreich [:davel]]

Please wait a few days, then read the guidelines (to be published) for adding "make check" tests.  A pointer to the guidelines will be sent to mozilla.dev.general.

BTW, the responsibility for adding new tests falls on the programmers writing the code, due to scaling issues.

Comment 22

[Dave Liebreich [:davel]]

It's been more than a few days, and I've not posted that doc.  I apologize.

Here are the simple guidelines for adding a build-time test:

0) for now, enclose your test-related code with ifdef ENABLE_TESTS

1) run the test program from the "check" target in the makefile.  http://lxr.mozilla.org/mozilla/source/netwerk/test/Makefile.in has such an example (for the TestCookie program)

2) if the test fails, exit with a non-zero status and/or print the string "FAIL" to stdout

3) if the test passes, exit with a zero status and don't print the string "FAIL"

write the test so that you expect it to pass on all platforms, since if the test fails, the tree will go orange*

* it will once we've set this up - see bug 352240 for status

Comment 23

[Lev Serebryakov]

Attachment: Patch to add described functionality v2.2. Changes, requested by vlad and test. Remove bogus mention about oracle. Add "check::" target to tests and fix test to return 1 on errors.

Please, review it twice and commit at least :)

Comment 24

[Lev Serebryakov]

Attachment: Patch to add described functionality v2.2. Changes, requested by vlad and test. Remove bogus mention about oracle. Add "check::" target to tests and fix test to return 1 on errors.

Please, review it twice and commit at least :)

Comment 25

[Brett Wilson]

I don't have time to review a patch of this magnitude right now. Nice job writing tests, though!

Comment 26

[Lev Serebryakov]

(In reply to comment #25)
> I don't have time to review a patch of this magnitude right now. Nice job
  I hope, this one will be integrated before gecko 1.9 will be branched/frozen. I'm using this functionality in my XULRunner application for long time, and when application will be released, it will be good to work on "stock" XULRunner without degradation.
  Maybe, someone else can grant second review? It seems, that Vladimir grants first onw without problems -- version 2.2 is different from 2.1 only in test app and test Makefile.
> writing tests, though!
  I want to replace this C++ test program with full suite of JS-based unit-tests... IMHO, it is much better. But "I don't habe time to do this right now" :(

Comment 27

[Steffen Wilberg]

You don't need a second review in toolkit code unless the reviewer asks for it.
http://www.mozilla.org/projects/toolkit/review.html
If you only made trivial changes to a patch (the interdiff looks like it), the vlad's review+ still stands and you just need somebody for check in.
If you do want a second review, Darin Fisher reviews storage code as well.

Comment 28

[Brett Wilson]

Comment on attachment 243896 [details] [diff] [review]
Patch to add described functionality v2.2. Changes, requested by vlad and test. Remove bogus mention about oracle. Add "check::" target to tests and fix test to return 1 on errors.

I won't have time to do a review of this magnitude any time soon. Sorry.

Comment 29

[Shawn Wilsher :sdwilsh]

Sorry for the delays on this.  I'll do a in-depth review of this tomorrow.  I haven't looked at how extensive your tests are either, but we now also have an automated xpcshell test framework.  mozStorage already has a large number of tests that have landed, and you can see them here:
http://mxr.mozilla.org/seamonkey/source/storage/test/unit/

Comment 30

[Lev Serebryakov]

(0) I'm going on vacations right now for one week. so, all needed changes will be availiable week + n days later.
(1) Patch is obsolete now, after last commits to storage. I'll merge changes.
(2) I'll look at this unit tests and add appropriate to framework.

Comment 31

[Shawn Wilsher :sdwilsh]

Comment on attachment 243896 [details] [diff] [review]
Patch to add described functionality v2.2. Changes, requested by vlad and test. Remove bogus mention about oracle. Add "check::" target to tests and fix test to return 1 on errors.

I apologize that this has taken so long, but once these concerns are addressed, I think this is good to land :)

general nit:  line wrapping at 80 characters.  I know it's not well enforced in the code already, but it's global standard for the code base.  With that said, it's OK to be over a few characters on occasion.

general nit:  when passing a null value to sqlite, please use NULL instead of nsnull.

>+ * The Original Code is Lev Serebryakov code.
nit: Usually this is the module it is in, unless it's a company.  Something like mozStorage code would do fine by me if you are wiling to change this.

>+/**
>+ * mozIStorageAggregateFunction represents aggregate SQL function.
>+ * Common examples of aggregate functions are SUM() and COUNT().
>+ * Aggreagte function calculates one result for set of data.
nit: Aggregate

>+  /**
>+   * onFinal is called when all rows in group has been
>+   * processed and engine need aggreagte function value.
>+   *
>+   * @returns aggregte result as Variant.
>+   */
nit: aggregate x2

>   /**
>+   * Create a new SQLite aggregate function
>+   */
please specify the parameters and what they do.  Even if they seem self explanatory, we have docs that are auto-generated from comments.

>+  /**
>+   * Delete custom SQLite function (simple or aggregate one)
>+   */
same with here

>+  /**
>+   * Set progress handler.
>+   * Pass zero as aGranularity or NULL as aHandler to remove handler.
>+   *
>+   * @return previous handler.
>+   */
and here

>+  mozIStorageProgressHandler setProgressHandler(in PRInt32 aGranularity, in mozIStorageProgressHandler aHandler);
Would we want the ability to have more than one progress handler?  I suppose I don't quite understand what the use case is for this...

>+ * The Original Code is Lev Serebryakov code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ *  Oracle Corporation
Oops?

>+#include "nsISupports.idl"
>+
>+interface mozIStorageConnection;
>+
>+/**
>+ * mozIStorageFunction is to be implemented by storage consumers that
>+ * wish to define custom storage functions, through
>+ * mozIStorageConnection's createFunction method.
>+ */
This comment doesn't go along with the file at all.  Please fix :)

>+        if (mProgressHandler) {
>+          sqlite3_progress_handler(mDBConn, 0, NULL, NULL);
>+        }
nit: no need for braces with one line if statement

>+static nsresult mozStorageVarintToSQLite3Result (sqlite3_context *ctx,
>+                                             nsIVariant *var)
Variant not Varint

>+{
>+    nsresult rv;
>+    PRUint16 dt;
>+    rv = var->GetDataType( &dt );
>+    if (NS_FAILED(rv)) {
>+        return rv;
>+    }
This always returns NS_OK, so I'd prefer this:
(void)var->GetDataType(&dt);

>+    switch( dt )
>+    {
>+    case nsIDataType::VTYPE_INT8:
nit: this doesn't match the coding style in the rest of these files.  It should be like:
switch (dt) {
    case nsIDataType::VTYPE_INT8:

>+            rv = var->GetAsInt32 (&v);
>+            if (NS_FAILED(rv)) {
>+                return rv;
>+            }
nit: just do |if (NS_FAILED(rv) return rv;| please (and in all following places as well)

>+    case nsIDataType::VTYPE_UINT64: // Data loss possible, but there is no unsigned types in SQLite
nit: line wrapping @ 80 chars
Ug...nsIVarient should check for overflow on this, but it sadly doesn't.  We should probably file a bug on that...

>+    case nsIDataType::VTYPE_BOOL:
for JS, we treat nsIDataType::VTYPE_BOOL as either a 1 or a 0.  I think we should probably do the same here.

>+    case nsIDataType::VTYPE_ID:
>+    case nsIDataType::VTYPE_INTERFACE:
>+    case nsIDataType::VTYPE_INTERFACE_IS:
>+    case nsIDataType::VTYPE_ARRAY:
>+    case nsIDataType::VTYPE_EMPTY_ARRAY:
>+    default:
>+      return NS_ERROR_FAILURE;
Let's use NS_ERROR_CANNOT_CONVERT_DATA instead.

> NS_IMETHODIMP
> mozStorageConnection::CreateFunction(const char *aFunctionName,
>                                      PRInt32 aNumArguments,
>                                      mozIStorageFunction *aFunction)
> {
>+    NS_ASSERTION(mDBConn, "connection not initialized");
Let's return NS_ERROR_FAILURE here too if there is no database connection.

>+    int srv = sqlite3_create_function (mDBConn,
>+                                       aFunctionName,
>+                                       aNumArguments,
>+                                       SQLITE_ANY,
>+                                       aFunction,
>+                                       mozStorageSqlFuncHelper,
>+                                       nsnull,
>+                                       nsnull);
>+    if (srv != SQLITE_OK) {
>+        HandleSqliteError(nsnull);
>+        return NS_ERROR_FAILURE;
>+    }
There is now a ConvertResultCode function that converts the sqlite result code to an applicable nsresult code which you can then check.

>+    rv = mFunctions.Put (fn);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return NS_OK;
just |return mFunctions.Put (fn);| here

>+static void
>+mozStorageSqlAggrFuncStepHelper (sqlite3_context *ctx,
>+                                 int argc,
>+                                 sqlite3_value **argv)
>+{
>+    void *userData = sqlite3_user_data (ctx);
>+    // We don't want to QI here, because this will be called a -lot-
>+    mozIStorageAggregateFunction *userFunction = NS_STATIC_CAST(mozIStorageAggregateFunction *, userData);
>+
>+    nsCOMPtr<mozStorageArgvValueArray> ava = new mozStorageArgvValueArray (argc, argv);
>+    nsresult rv = userFunction->OnStep(ava);
Does this handle null OK?  If new doesn't work, this will be null.

>+NS_IMETHODIMP
>+mozStorageConnection::CreateAggregateFunction(const char *aFunctionName,
>+                                     PRInt32 aNumArguments,
>+                                     mozIStorageAggregateFunction *aFunction)
>+{
>+    NS_ASSERTION(mDBConn, "connection not initialized");
return a failure as well please

>+    // Check for name.
>+    if (mFunctions.Contains (fn)) {
>+        // already exists
>+        return NS_ERROR_FAILURE;
>+    }
NS_ENSURE_FALSE(mFunctions.Contains(fn), NS_ERROR_FAILURE);

>     int srv = sqlite3_create_function (mDBConn,
>                                        aFunctionName,
>                                        aNumArguments,
>                                        SQLITE_ANY,
>                                        aFunction,
>-                                       mozStorageSqlFuncHelper,
>+                                       nsnull,
>+                                       mozStorageSqlAggrFuncStepHelper,
>+                                       mozStorageSqlAggrFuncFinalHelper);
>+    if (srv != SQLITE_OK) {
>+        HandleSqliteError(nsnull);
>+        return NS_ERROR_FAILURE;
>+    }
rv = ConvertResultCode(srv);
NS_ENSURE_SUCCESS(rv, rv);

>+    // do we already have this function defined?
>+    // Check for name.
>+    if (!mFunctions.Contains (fn)) {
>+        // already exists
>+        return NS_ERROR_FAILURE;
>+    }
NS_ENSURE_TRUE(mFunctions.Contains(fn), NS_ERROR_FAILURE);

>     if (srv != SQLITE_OK) {
>         HandleSqliteError(nsnull);
>         return ConvertResultCode(srv);
>     }
I know you didn't touch this, but since you are in this function already, could you please clean this bit up with this:
rv = ConvertResultCode(srv);
NS_ENSURE_SUCCESS(rv, rv);

>+    mFunctions.Remove (fn);
>+    // If it was aggregate function, remove it from
>+    // set of checked functions.
Perhaps I'm missing something, but I don't see you removing anything from mAggregateFunctions

>+NS_IMETHODIMP
>+mozStorageConnection::SetProgressHandler(PRInt32 aGranularity,
>+                                     mozIStorageProgressHandler *aHandler,
>+                                     mozIStorageProgressHandler **aOldHandler)
>+{
>+    NS_ASSERTION(mDBConn, "connection not initialized");
return NS_ERROR_FAILURE if no connection please


>+    if(NULL == aHandler || aGranularity <= 0) {
>+      aHandler     = NULL;
here you'll want to use nsnull when assigning to aHandler

>+int
>+mozStorageConnection::ProgressHandler()
>+{
>+    nsresult rv;
Please declare this when you first use it.

>+    if(NULL != mProgressHandler) {
if (mProgressHandler) {

>+      rv = mProgressHandler->OnProgress(this, &res);
>+      if (NS_FAILED(rv)) return 0; // Don't break request
>+      return res ? 1 : 0;
>+    } else {
>+      return 0;
>+    }
Don't bother with the else, just return 0 afterwards please.

>+    // Progress handler
>+    int ProgressHandler();
I'm not so sure what this function is for still, could you explain it some more?

>-    nsCOMPtr<nsIMutableArray> mFunctions;
>+    nsStringHashSet mFunctions;
>+    nsCOMPtr<nsIMutableArray> mAggregateFunctions;
So, I know that we used an nsIMutableArray in the past, but I think an nsCOMArray<mozIStorageAggregateFunction> would be better now.

If you want to write xpcshell tests, that'd be awesome!  Our test suite currently doesn't even test functions (mostly because I don't understand what their use case it yet)

Comment 32

[Jeff Walden [:Waldo]]

(In reply to comment #31)
> > NS_IMETHODIMP
> > mozStorageConnection::CreateFunction(const char *aFunctionName,
> >                                      PRInt32 aNumArguments,
> >                                      mozIStorageFunction *aFunction)
> > {
> >+    NS_ASSERTION(mDBConn, "connection not initialized");
> Let's return NS_ERROR_FAILURE here too if there is no database connection.

NS_ERROR_NOT_INITIALIZED, I believe.


> >+    int srv = sqlite3_create_function (mDBConn,
> >+                                       aFunctionName,
> >+                                       aNumArguments,
> >+                                       SQLITE_ANY,
> >+                                       aFunction,
> >+                                       mozStorageSqlFuncHelper,
> >+                                       nsnull,
> >+                                       nsnull);
> >+    if (srv != SQLITE_OK) {
> >+        HandleSqliteError(nsnull);
> >+        return NS_ERROR_FAILURE;
> >+    }
> There is now a ConvertResultCode function that converts the sqlite result code
> to an applicable nsresult code which you can then check.

You mean |return ConvertResultCode(srv);|, then?  I don't think you do, but that seems better than always converting and then checking.


> >+    rv = mFunctions.Put (fn);
> >+    if (NS_FAILED(rv)) return rv;
> >+
> >+    return NS_OK;
> just |return mFunctions.Put (fn);| here

Space before function call parens doesn't conform to prevailing style.


> >+static void
> >+mozStorageSqlAggrFuncStepHelper (sqlite3_context *ctx,
> >+                                 int argc,
> >+                                 sqlite3_value **argv)
> >+{
> >+    void *userData = sqlite3_user_data (ctx);
> >+    // We don't want to QI here, because this will be called a -lot-
> >+    mozIStorageAggregateFunction *userFunction = NS_STATIC_CAST(mozIStorageAggregateFunction *, userData);
> >+
> >+    nsCOMPtr<mozStorageArgvValueArray> ava = new mozStorageArgvValueArray (argc, argv);
> >+    nsresult rv = userFunction->OnStep(ava);
> Does this handle null OK?  If new doesn't work, this will be null.

Not 100% sure, but I think this wants to be an nsRefPtr<...>; I think they have less overhead and are "better" for concrete classes, in ways I don't particularly remember right now.


> >+NS_IMETHODIMP
> >+mozStorageConnection::CreateAggregateFunction(const char *aFunctionName,
> >+                                     PRInt32 aNumArguments,
> >+                                     mozIStorageAggregateFunction *aFunction)
> >+{
> >+    NS_ASSERTION(mDBConn, "connection not initialized");
> return a failure as well please

Same comment as before.


> >+    // Check for name.
> >+    if (mFunctions.Contains (fn)) {
> >+        // already exists
> >+        return NS_ERROR_FAILURE;
> >+    }
> NS_ENSURE_FALSE(mFunctions.Contains(fn), NS_ERROR_FAILURE);

Surely there's a better error here?  If not, I strongly suggest making one, possibly one specific to storage.


> >+    if (srv != SQLITE_OK) {
> >+        HandleSqliteError(nsnull);
> >+        return NS_ERROR_FAILURE;
> >+    }
> rv = ConvertResultCode(srv);
> NS_ENSURE_SUCCESS(rv, rv);

As before.


> >+    if (!mFunctions.Contains (fn)) {
> >+        // already exists
> >+        return NS_ERROR_FAILURE;
> >+    }
> NS_ENSURE_TRUE(mFunctions.Contains(fn), NS_ERROR_FAILURE);

As before.


> 
> >     if (srv != SQLITE_OK) {
> >         HandleSqliteError(nsnull);
> >         return ConvertResultCode(srv);
> >     }

As before.


> >+NS_IMETHODIMP
> >+mozStorageConnection::SetProgressHandler(PRInt32 aGranularity,
> >+                                     mozIStorageProgressHandler *aHandler,
> >+                                     mozIStorageProgressHandler **aOldHandler)
> >+{
> >+    NS_ASSERTION(mDBConn, "connection not initialized");
> return NS_ERROR_FAILURE if no connection please

As before.


> (mostly because I don't understand what their use case it yet)

In that case, perhaps it would be a good idea to describe a use case or two in the IDL docs.

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> NS_ERROR_NOT_INITIALIZED, I believe.
Yup, that'd be better.

> You mean |return ConvertResultCode(srv);|, then?  I don't think you do, but
> that seems better than always converting and then checking.
I suppose that that does make more sense in this case.  However, there are other codes that result in success:
http://mxr.mozilla.org/seamonkey/source/storage/src/mozStorage.h#54

> Space before function call parens doesn't conform to prevailing style.
That style is all over the place in these files, so I'm rather indifferent.

> Not 100% sure, but I think this wants to be an nsRefPtr<...>; I think they have
> less overhead and are "better" for concrete classes, in ways I don't
> particularly remember right now.
That is correct actually, nsRefPtr for this instead of nsCOMPtr.

> In that case, perhaps it would be a good idea to describe a use case or two in
> the IDL docs.
Yes, a comment in the IDL would be great!

Comment 34

[Shawn Wilsher :sdwilsh]

We have until July 18th to land this.  I'm willing to drop whatever it is that I'm working on to get the review done to make sure it gets in (or loose sleep, which ever is less important)

Comment 35

[Lev Serebryakov]

Attachment: Version 3.0. All sdwilsh's nits are fixed. Unit tests are added.

This patch needs next one in netwerk, because netwerk uses mozIStoragefunction and it was changed.

All nits (and some bugs!) are fixed, tests are added.

Also, I unified all checks for mozIStorageConnection::mDBConn

Comment 36

[Lev Serebryakov]

Attachment: Small changes in netwerk to use new mozIStorageFunction

Small changes in netwerk to use new mozIStorageFunction

Comment 37

[Shawn Wilsher :sdwilsh]

Could you please generate your diffs with -u8p?  It's really hard to review without any context :)

Comment 38

[Lev Serebryakov]

Attachment: Previous patch with proper diff options

Comment 39

[Lev Serebryakov]

Attachment: netwerk patch with proper diff options

Comment 40

[Shawn Wilsher :sdwilsh]

Comment on attachment 270924 [details] [diff] [review]
Previous patch with proper diff options

>Index: public/mozIStorageAggregateFunction.idl
>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Corporation.
nit: put your name (unless you do work for MoCo)
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Lev Serebryakov <[e-mail]> (Original Author)
nit: don't need this with above fix

>+/**
>+ * mozIStorageAggregateFunction represents aggregate SQL function.
>+ * Common examples of aggregate functions are SUM() and COUNT().
>+ *
>+ * Aggregate function calculates one result for set of data.
>+ * Set of data is group of rows in SQL. Here can be one group
>+ * per request or many of them, if GROUP BY clause is used.
>+ */
nit: wording; let's try this instead:
/**
 * mozIStorageAggregateFunction represents an aggregate SQL function.
 * Common examples of an aggregate function are SUM() and COUNT().
 *
 * An aggregate function calculates one result for a given set of data, where
 * a set of data is a group of tuples. There can be one group
 * per request or many of them, if GROUP BY clause is used or not.
 */
(you'll have to fix line wrapping as it isn't obvious where 80 chars is in bugzilla)

>+  /**
>+   * onStep is called when next value should be passed to
>+   * a custom function.  There are no return values.
nit: remove "there are no return values"

>+  /**
>+   * onFinal is called when all rows in group has been
>+   * processed and engine need aggreagte function value.
nit: wording; let's try this instead:
/**
 * Called when all tuples in a group have been processed and the engine
 * needs the aggregate function's value.

>Index: public/mozIStorageConnection.idl
you forgot to change the uuid for the interface in this file

>+   * @param aFunction      The instance of mozIStorageFunction, which implements
>+   *                       function in question.
nit: wording: which implements the function in question

>+   * Create a new SQLite aggregate function
nit: s/Create/Creates/

>+   * @param aNumArguments  The number of arguments function takes.
nit: "The number of arguments the function takes."
>+   * @param aFunction      The instance of mozIStorageAggreagteFunction,
>+   *                       which implements function in question.

>+  /**
>+   * Set progress handler.
>+   * Only one handler can be registered at one time. If you need multiple
>+   * handlers, you need chain them yourself.
>+   * Pass zero as aGranularity or NULL as aHandler to remove handler.
nit: wording; let's try this instead:
/**
 * Sets a progress handler.  Only one handler can be registered at a time.  If
 * you need more than one, you need to chain them yourself.
I'm also wondering if we should have a method to just remove the handler instead of the bits about passing in 0/NULL.

>+  mozIStorageProgressHandler setProgressHandler(in PRInt32 aGranularity, in mozIStorageProgressHandler aHandler);
nit: line wrapping after comma please

> /**
>  * mozIStorageFunction is to be implemented by storage consumers that
>- * wish to define custom storage functions, through
>- * mozIStorageConnection's createFunction method.
>+ * wish to receive callbacks in process of request execution.
nit: "during the request execution."

>+ * This interface allows user to implement it owns, problem-
>+ * specific functions.
nit: wording; let's try this instead:
 * This interface allows consumers to implement their own,
 * problem-specific functions.

> [scriptable, uuid(898d4189-7012-4ae9-a2af-435491cfa114)]
you need to rev the uuid since you changed the interface

> interface mozIStorageFunction : nsISupports {
>   /**
>    * onFunctionCall is called when execution of a custom
>-   * function should occur.  There are no return values.
>+   * function should occur.
>    * 
>    * @param aNumArguments         The number of arguments
>    * @param aFunctionArguments    The arguments passed in to the function
>+   *
>+   * @returns any value as Variant type.
>    */
> 
>-  void onFunctionCall(in mozIStorageValueArray aFunctionArguments);
>+  nsIVariant onFunctionCall(in mozIStorageValueArray aFunctionArguments);
hmm, this fixed Bug 329812 in the process, now doesn't it?

>+/**
>+ * mozIProgressHandler is to be implemented by storage consumers that
>+ * wish to receive callbacks in process of request execution.
nit: "wish to receive callbacks during the request execution."

> mozStorageConnection::~mozStorageConnection()
> {
>     if (mDBConn) {
>+        if (mProgressHandler) {
>+          sqlite3_progress_handler(mDBConn, 0, NULL, NULL);
>+        }
nit: no bracing for one line ifs please

>+
>+        // Release all functions
>+        mFunctions.EnumerateRead(s_ReleaseFuncEnum, NULL);
 
>+PLDHashOperator mozStorageConnection::s_FindFuncEnum (const nsAString &aKey,
>+                                         nsISupports* aData, void* userArg)
nit: should be:
PLDHashOperator
mozStorageConnection::s_FindFuncEnum(const nsAString &aKey,
                                     nsISupports *aData,
                                     void *userArg)
>+{
>+    FindFuncEnumArgs *args = NS_STATIC_CAST (FindFuncEnumArgs *, userArg);
NS_STATIC_CAST is depreciated.  Use static_cast<FindFuncEnumArgs *> please

>+PLDHashOperator mozStorageConnection::s_ReleaseFuncEnum (const nsAString &aKey,
>+                                         nsISupports* aData, void* userArg)
nit: should be:
PLDHashOperator
mozStorageConnection::s_ReleaseFuncEnum(const nsAString &aKey,
                                        nsISupports *aData,
                                        void *userArg)

>+PRBool mozStorageConnection::FindFunctionByInstance (nsISupports *aInstance)
nit: return type on new line

>+static nsresult mozStorageVariantToSQLite3Result (sqlite3_context *ctx,
ditto

> NS_IMETHODIMP
> mozStorageConnection::CreateFunction(const char *aFunctionName,
>                                      PRInt32 aNumArguments,
>                                      mozIStorageFunction *aFunction)
> {
>+    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>+
>+    const nsAutoString &fn = NS_ConvertUTF8toUTF16 (nsDependentCString (aFunctionName));
nit: line wrapping

>+    int srv = sqlite3_create_function (mDBConn,
>+                                       aFunctionName,
>+                                       aNumArguments,
>+                                       SQLITE_ANY,
>+                                       aFunction,
>+                                       mozStorageSqlFuncHelper,
>+                                       nsnull,
>+                                       nsnull);
>+    nsresult rv = ConvertResultCode(srv);
>+    NS_ENSURE_SUCCESS(rv, rv);
let's do this like Waldo suggested and only convert the result code only if srv != SQLITE_OK

>+
>+    // We hold function object -- add ref to it
>+    NS_ADDREF(aFunction);
>+
>+    return mFunctions.Put (fn, aFunction);
mFunctions.Put returns a PRBool, not nsresult (both are typedef'd to ints I think though).  This line should be like this:
return mFunctions.Put(fn, aFunction) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
(see http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/glue/nsBaseHashtable.h&rev=3.13#137)

>+    mozIStorageAggregateFunction *userFunction = NS_STATIC_CAST(mozIStorageAggregateFunction *, userData);
nit: static_cast and line wrapping

>+static void
>+mozStorageSqlAggrFuncFinalHelper (sqlite3_context *ctx)
>+{
>+    void *userData = sqlite3_user_data (ctx);
>+    // We don't want to QI here, because this will be called a -lot-
>+    mozIStorageAggregateFunction *userFunction = NS_STATIC_CAST(mozIStorageAggregateFunction *, userData);
nit: static_cast and line wrapping

>+    nsRefPtr<nsIVariant> retval;
this one should be a nsCOMPtr

>+NS_IMETHODIMP
>+mozStorageConnection::CreateAggregateFunction(const char *aFunctionName,
>+                                     PRInt32 aNumArguments,
>+                                     mozIStorageAggregateFunction *aFunction)
nit: line up the args after the (, even if it goes over 80 chars

> 
>     int srv = sqlite3_create_function (mDBConn,
>                                        aFunctionName,
>                                        aNumArguments,
>                                        SQLITE_ANY,
>                                        aFunction,
>-                                       mozStorageSqlFuncHelper,
>+                                       nsnull,
>+                                       mozStorageSqlAggrFuncStepHelper,
>+                                       mozStorageSqlAggrFuncFinalHelper);
>+    nsresult rv = ConvertResultCode(srv);
check for SQLITE_OK (see previous comments

>+    return mFunctions.Put (fn, aFunction);
see previous comments

>+NS_IMETHODIMP
>+mozStorageConnection::RemoveFunction(const char *aFunctionName)
>+{
>+    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>+
>+    const nsAString &fn = NS_ConvertUTF8toUTF16 (nsDependentCString (aFunctionName));
>+    
>+    nsISupports *func;
>+
>+    NS_ENSURE_TRUE(mFunctions.Get (fn, &func), NS_ERROR_FAILURE);
>+	
>+    int srv = sqlite3_create_function (mDBConn,
>+                                       aFunctionName,
>+                                       0,
>+                                       SQLITE_ANY,
>+                                       nsnull,
>+                                       nsnull,
>                                        nsnull,
>                                        nsnull);
calls to SQLite functions should use NULL, not nsnull

>+static int
>+mozStorageSqlProgressHelper(void *arg)
>+{
>+  mozStorageConnection *_this = NS_STATIC_CAST(mozStorageConnection *, arg);
static_cast

>+NS_IMETHODIMP
>+mozStorageConnection::SetProgressHandler(PRInt32 aGranularity,
>+                                     mozIStorageProgressHandler *aHandler,
>+                                     mozIStorageProgressHandler **aOldHandler)
nit: line up args with (

>+    
>+    // Generic progress handler
>+    // Dispatch call to registered progress handler,
>+    // if here is one. Do nothing in other cases.
nit: s/here/there/
>+    int ProgressHandler();
Does/should this really be public?

>+    static PLDHashOperator s_FindFuncEnum (const nsAString &aKey,
>+                                      nsISupports* aData, void* userArg);
>+    static PLDHashOperator s_ReleaseFuncEnum (const nsAString &aKey,
>+                                          nsISupports* aData, void* userArg);
nit: line up args with ( please

>-#ifdef XP_UNIX
>-#define TEST_DB NS_LITERAL_CSTRING("/tmp/foo.sdb")
>-#else
> #define TEST_DB NS_LITERAL_CSTRING("foo.sdb")
>-#endif
why did you change this by chance?

Otherwise, this patch looks much much better. :)  Thanks for doing this!

Comment 41

[Lev Serebryakov]

> hmm, this fixed Bug 329812 in the process, now doesn't it?
Yes, it seems so. I've searched for "similar" bugs when submitted this one, but didn't found Bug 329812.

> why did you change this by chance?
OOOPS! Merge problems. Sorry.

Comment 42

[Lev Serebryakov]

>>+    int ProgressHandler();
> Does/should this really be public?
Do you mean static? :)

And one more comment before patch 3.5 :)

Maybe, we need to export all SQLite error codes through static constants in mozIStorageConnection? This interface has lastError field, and I use it in unit tests (to ensure, that request can be aborted by progress handler), but user need look at SQLite headers to understand these numbers right now...

Comment 43

[Lev Serebryakov]

>>+    int ProgressHandler();
> Does/should this really be public?
Not static, of course. But why it should be public? It is transition from static helper function to connection method (from C to C++), it should not be called "by hands"

Comment 44

[Lev Serebryakov]

>>+    int ProgressHandler();
> Does/should this really be public?
Oh. I'm dumb at mornings.
It should not be public, if helper will be static class function. Changed.

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #42)
> Maybe, we need to export all SQLite error codes through static constants in
> mozIStorageConnection? This interface has lastError field, and I use it in unit
> tests (to ensure, that request can be aborted by progress handler), but user
> need look at SQLite headers to understand these numbers right now...
Maybe, but let's not worry about that in this bug :)

Comment 46

[Lev Serebryakov]

Attachment: All latest nits seem to be fixed.

>>-#ifdef XP_UNIX
>>-#define TEST_DB NS_LITERAL_CSTRING("/tmp/foo.sdb")
>>-#else
>> #define TEST_DB NS_LITERAL_CSTRING("foo.sdb")
>>-#endif
> why did you change this by chance?
I don't remember, it was about year ago! But I definitely had some problems with "database in current directory" on windows. Maybe, "clear" target doesn't work?  I don't remember :)

Comment 47

[Lev Serebryakov]

Attachment: Version 3.5 with restored unit tests

New unit tests was lost by accident in previous patch (3.5)

Comment 48

[Shawn Wilsher :sdwilsh]

Comment on attachment 271048 [details] [diff] [review]
Version 3.5 with restored unit tests

>+  void createAggregateFunction (in string aFunctionName,
>+                                in long aNumArguments,
>+                                in mozIStorageAggregateFunction aFunction);
nit: no space after function name / before (

>+  void removeFunction (in string aFunctionName);
ditto

>+ * Also, these functions can be called from triggers.
>+ *
nit: extra line not needed
Also, do we support triggers?


>+PLDHashOperator
>+mozStorageConnection::s_FindFuncEnum (const nsAString &aKey,
>+                                      nsISupports* aData,
>+                                      void* userArg)
nit: no space after function name (this is global, and I think you may have misinterpreted one of my past comments - not a big deal :) )

>+PLDHashOperator
>+mozStorageConnection::s_ReleaseFuncEnum (const nsAString &aKey,
>+                                         nsISupports* aData,
>+                                         void* userArg)
>+{
>+    NS_RELEASE(aData);
>+    return PL_DHASH_NEXT;
>+}
>+
>+ 
nit: extra newline (you do this in a few places - there should only be one newline between functions)

>+        case nsIDataType::VTYPE_BOOL:
>+            {
>+                PRBool v;
>+                rv = var->GetAsBool(&v);
>+                if (NS_FAILED(rv)) return rv;
>+                sqlite3_result_int (ctx, v?1:0);
nit: "v ? 1 : 0"

>+            }
>+            break;
>+        case nsIDataType::VTYPE_CHAR:
>+        case nsIDataType::VTYPE_WCHAR:
>+        case nsIDataType::VTYPE_DOMSTRING:
>+        case nsIDataType::VTYPE_CHAR_STR:
>+        case nsIDataType::VTYPE_WCHAR_STR:
>+        case nsIDataType::VTYPE_STRING_SIZE_IS:
>+        case nsIDataType::VTYPE_WSTRING_SIZE_IS:
>+        case nsIDataType::VTYPE_UTF8STRING:
>+        case nsIDataType::VTYPE_CSTRING:
>+        case nsIDataType::VTYPE_ASTRING:
closer inspection here makes me wonder if we should be trying to convert all of these.  Also, you should really be using GetAsWString for WStrings, GetAsACString for CStings, etc. since they all can have different encodings.

>+        case nsIDataType::VTYPE_ID:
>+        case nsIDataType::VTYPE_INTERFACE:
>+        case nsIDataType::VTYPE_INTERFACE_IS:
>+        case nsIDataType::VTYPE_ARRAY:
>+        case nsIDataType::VTYPE_EMPTY_ARRAY:
>+        default:
I actually wouldn't even bother with the types we don't convert - just let it fall to default.

>+          return NS_ERROR_CANNOT_CONVERT_DATA;
nit: whitespace (add two more spaces)

>+    nsRefPtr<mozStorageArgvValueArray> ava = new mozStorageArgvValueArray (argc, argv);
>+    if (!ava) {
>+        NS_WARNING("mozIStorageConnection: Out of memory!\n");
>+        return;
>+    }
I've been informed that warning about out of memory is not liked, so you can just return and not warn here :)
Fix this in the other places you do this as well please

>+    // We hold function object -- add ref to it
>+    NS_ADDREF(aFunction);
>+
>+    return mFunctions.Put (fn, aFunction) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
actually, we leak this if it fails - d'oh.  Something like this will work:
if (mFunctions.Put(fn, aFunction)) {
    NS_ADDREF(aFunction);
    return NS_OK;
}
return NS_ERROR_OUT_OF_MEMORY;
This needs to be fixed in a few other places too.

>+static void
>+mozStorageSqlAggrFuncStepHelper (sqlite3_context *ctx,
>+                                 int argc,
>+                                 sqlite3_value **argv)
>+{
>+    void *userData = sqlite3_user_data (ctx);
>+    // We don't want to QI here, because this will be called a -lot-
>+    mozIStorageAggregateFunction *userFunction =
>+                        static_cast<mozIStorageAggregateFunction *>(userData);
nit: just do four spaces to indent on a new line (only for after =)

>+    nsRefPtr<mozStorageArgvValueArray> ava =
>+                                    new mozStorageArgvValueArray (argc, argv);
ditto


>+static void
>+mozStorageSqlAggrFuncFinalHelper (sqlite3_context *ctx)
>+{
>+    void *userData = sqlite3_user_data (ctx);
>+    // We don't want to QI here, because this will be called a -lot-
>+    mozIStorageAggregateFunction *userFunction =
>+                         static_cast<mozIStorageAggregateFunction *>(userData);
ditto

>+NS_IMETHODIMP
>+mozStorageConnection::CreateAggregateFunction(const char *aFunctionName,
>+                                     PRInt32 aNumArguments,
>+                                     mozIStorageAggregateFunction *aFunction)
nit: line up args with ( like so (global nit):
mozStorageConnection::CreateAggregateFunction(const char *aFunctionName,
                                              PRInt32 aNumArguments,
                                              mozIStorageAggregateFunction *aFunction)
>     if (srv != SQLITE_OK) {
>-        HandleSqliteError(nsnull);
>-        return ConvertResultCode(srv);
>+        nsresult rv = ConvertResultCode(srv);
>+        NS_ENSURE_SUCCESS(rv, rv);
can this return other OK codes?  if not, the old code is best.

>+    return mFunctions.Put (fn, aFunction);
mFunctions.Put returns PRBool, not nsresult

>+NS_IMETHODIMP
>+mozStorageConnection::RemoveFunction(const char *aFunctionName)
>+{
>+    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>+
>+    const nsAString &fn = NS_ConvertUTF8toUTF16 (nsDependentCString (aFunctionName));
this should probably be an nsAutoString - and actually, based on your idl, you should have this as your function header:
mozStorageConnection::RemoveFunction(const nsAString &aFunctionName)
which should make your life a whole lot easier anyway :)

>+    if (srv != SQLITE_OK) {
>+        nsresult rv = ConvertResultCode(srv);
>+        NS_ENSURE_SUCCESS(rv, rv);
ditto to previous statement about if this returns any other OK things

>+    // We don't hold function object anymore -- remove ref to it
>+    NS_RELEASE(func);
>+
>+    mFunctions.Remove (fn);
We should probably release after we remove it, just to be safe.


>+NS_IMETHODIMP
>+mozStorageConnection::SetProgressHandler(PRInt32 aGranularity,
>+                                         mozIStorageProgressHandler *aHandler,
>+                                         mozIStorageProgressHandler **aOldHandler)
>+{
>+    if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
>+
>+    // Return previous one
>+    NS_IF_ADDREF(*aOldHandler = mProgressHandler);
>+    
>+    if(nsnull == aHandler || aGranularity <= 0) {
nit: space after if, and just do !aHandler

>     // fetch the native handle
>     sqlite3 *GetNativeConnection() { return mDBConn; }
>-
>+    
oops :)

>+    static PLDHashOperator s_FindFuncEnum (const nsAString &aKey,
>+                                           nsISupports* aData, void* userArg);
>+    static PLDHashOperator s_ReleaseFuncEnum (const nsAString &aKey,
>+                                              nsISupports* aData, void* userArg);
nit: no space after function name x2

>+TestFunc::OnFunctionCall (mozIStorageValueArray *sva, nsIVariant **_retval)
>+    *_retval = outVar;
>+    NS_IF_ADDREF(*_retval);
just do |NS_ADDREF(*_retval = outVar);|

>+NS_IMETHODIMP
>+TestAggregateFunc::OnFinal (nsIVariant **_retval)
>+{
>+    nsCOMPtr<nsIWritableVariant> outVar;
>+
>+    fprintf (stderr, "* aggregate result %d!\n", - mCalls * mCalls);
>+
>+    outVar = do_CreateInstance(NS_VARIANT_CONTRACTID);
>+    if(!outVar)
>+        return NS_ERROR_FAILURE;
>+    outVar->SetAsInt32(-mCalls * mCalls);
>+    *_retval = outVar;
>+    NS_IF_ADDREF(*_retval);
ditto

> function getOpenedDatabase()
> {
>-  return gDBConn ? gDBConn : getService().openDatabase(getTestDB());
>+  return gDBConn ? gDBConn : gDBConn = getService().openDatabase(getTestDB());
whoops - that was my bad.  Thanks for fixing that!

>Index: test/unit/test_storage_aggregates.js
Thanks for adding xpcshell tests too!

>+ * The Initial Developer of the Original Code is
>+ *   Mozilla Corporation.
this again? :p

>+function cleanup()
does redefining this actually work correctly?

>+var testSquareAndSumFunction = {
This only seems like it actually tests sum

>+function cleanup()
>+{
>+  var msc = getOpenedDatabase();
>+  try {
>+    msc.executeSimpleSQL("DROP TABLE function_tests");
>+  } catch (e) {}
>+  try {
>+    msc.removeFunction("test_square");
>+  } catch (e) {}
>+  try {
>+    msc.removeFunction("test_square2");
>+  } catch (e) {}
>+}
actually, you don't (shouldn't) need to worry about this as we remove the whole database file at shutdown and startup.


>+function setup()
>+{
>+  var msc = getOpenedDatabase();
>+  msc.createTable("handler_tests", "id INTEGER PRIMARY KEY, num INTEGER");
>+  msc.beginTransaction();
>+  
>+  var stmt = createStatement("INSERT INTO handler_tests (id, num) VALUES(?1, ?2)");
>+  for(var i = 0; i < 100; ++i) {
>+    stmt.bindInt32Parameter(0, i);
>+    stmt.bindInt32Parameter(1, Math.floor(Math.random()*1000));
>+    stmt.execute();
>+  }
>+  stmt.reset();
>+  msc.commitTransaction();
>+}
>+
>+function cleanup()
>+{
>+  var msc = getOpenedDatabase();
>+  try {
>+    msc.executeSimpleSQL("DROP TABLE handler_tests");
>+  } catch (e) {}
>+  try {
>+    msc.setProgressHandler(0, null);
>+  } catch (e) {}
>+}
don't need this either (in fact, you don't need any of your cleanup code :))

Nearly there, and thank you sooo much for xpcshell tests.  It makes it a lot easier for our dev-doc writers to see example code on how to use this stuff!  I'll bug dcamp to get your review for the netwerk patch for you.

Comment 49

[Dave Camp (:dcamp)]

Comment on attachment 270925 [details] [diff] [review]
netwerk patch with proper diff options

>+  // It is VOID function
>+  nsCOMPtr<nsIWritableVariant> wrretval = do_CreateInstance(NS_VARIANT_CONTRACTID);
>+  if(!wrretval)
>+    return NS_ERROR_FAILURE;
>+  wrretval->SetAsVoid();
>+  *_retval = wrretval;
>+  NS_IF_ADDREF(*_retval);
>+

Would it be possible to create an empty variant once and just return a reference to it every time?

Comment 50

[Lev Serebryakov]

(In reply to comment #49)
> (From update of attachment 270925 [details] [diff] [review])
> >+  // It is VOID function
> >+  nsCOMPtr<nsIWritableVariant> wrretval = do_CreateInstance(NS_VARIANT_CONTRACTID);
> >+  if(!wrretval)
> >+    return NS_ERROR_FAILURE;
> >+  wrretval->SetAsVoid();
> >+  *_retval = wrretval;
> >+  NS_IF_ADDREF(*_retval);
> >+
> 
> Would it be possible to create an empty variant once and just return a
> reference to it every time?
Why not? And even more: I write code to interpret NULL (*_reatval = nsnull) as nsIVariant with "VTYPE_VOID" type (and not a error), but I don't know is it Ok to return pure null instead of component according to coding style?

Comment 51

[Lev Serebryakov]

(In reply to comment #48)
> Also, do we support triggers?
  SQLite does, and we support executing any SQL code (and triggers are SQL code, not external one, like functions). SQLCache from netwerk uses triggers.

> closer inspection here makes me wonder if we should be trying to convert all
> of these.  Also, you should really be using GetAsWString for WStrings,
> GetAsACString for CStings, etc. since they all can have different encodings.
 And CStrings are utf-8 or ASCII? I've lost in mozilla/xpcom string types :(

> this should probably be an nsAutoString - and actually, based on your idl, you
> should have this as your function header:
> mozStorageConnection::RemoveFunction(const nsAString &aFunctionName)
> which should make your life a whole lot easier anyway :)
  But "const char*" is generated by xpidl! So, or IDL should bexhanged to AUTF8String or we need "const char *" here. Some interfaces use just "string", some "AUTF8String"... Again, I've lost in mozilla/xpcom string types, there are too many of them :(

> >+ * The Initial Developer of the Original Code is
> >+ *   Mozilla Corporation.
> this again? :p
  :)

> >+function cleanup()
> does redefining this actually work correctly?
  Ooops, copy'n'paste from wrong file. It seems to work, but uou are right, I don't need local celanup()s at all.

Comment 52

[Lev Serebryakov]

(In reply to comment #48)
> >-
> >+    
> oops :)
BTW, there are such lines (whitespace-only) in almost all storage code, not only in files which I change... Should I clean up them all?

Comment 53

[Jeff Walden [:Waldo]]

(In reply to comment #52)
> Should I clean up them all?

Certainly not here, please.  :-)  This bug doesn't need anything keeping it from dying.

Comment 54

[Shawn Wilsher :sdwilsh]

Regarding strings - have you seen this?
http://developer.mozilla.org/en/docs/XPCOM_string_guide

Comment 55

[Shawn Wilsher :sdwilsh]

I missed something in your idl files - you should use AString instead of string for your string parameters.  Then you'll actually get the nsASting as the type for strings

Comment 56

[Lev Serebryakov]

(In reply to comment #54)
> Regarding strings - have you seen this?
> http://developer.mozilla.org/en/docs/XPCOM_string_guide
Yep. Some time ago :) And it seems, that at this time there was not section about IDL... Or I've missed it... Ok, I'll change IDL and code to AString

Comment 57

[Lev Serebryakov]

Attachment: Patch version 4.0: new bunch of nits :)

I've selected AUTF8String, not AString for function names due two reasons:
(1) No conversion from ASCII from native C code.
(2) It is uniform with requests strings.

Comment 58

[Lev Serebryakov]

Attachment: netwerk patch 2.0: new function name type, simple NULL instead of full-featured nsIVariant.

Changes are trivial

Comment 59

[Shawn Wilsher :sdwilsh]

Comment on attachment 271200 [details] [diff] [review]
Patch version 4.0: new bunch of nits :)

>+                // GetAsAString does proper conversion to UCS2
>+                // from all string-like types. It can be used
>+                // universally without problems.
really?  Sweet!

>+static void
>+mozStorageSqlAggrFuncStepHelper (sqlite3_context *ctx,
>+                                 int argc,
>+                                 sqlite3_value **argv)
>+{
>+    void *userData = sqlite3_user_data (ctx);
>+    // We don't want to QI here, because this will be called a -lot-
>+    mozIStorageAggregateFunction *userFunction =
>+        static_cast<mozIStorageAggregateFunction *>(userData);
>+
>+    nsRefPtr<mozStorageArgvValueArray> ava =
>+                                    new mozStorageArgvValueArray (argc, argv);
nit: only a four space indent on second line please

>+    // Aggreagte functions are stateful, so we can not have
>+    // aliases for them.
nit: cannot

>+    // Enumerate all functions and find, if here is this
>+    // implementation already.
nit: wording, let's try:
Enumerate all functions and determine if this one is already implemented

>+var testSquareAndSumFunction = {
>+  calls: 0,
>+  _sum: 0,
>+
>+  reset: function() {
>+    this.calls = 0;
>+    this._sum  = 0;
>+  },
>+
>+  onStep: function(val) {
>+    ++this.calls;
>+    this._sum += val.getInt32(0) * val.getInt32(0);
now you are squaring, not summing :)
please fix the variable names to match what you are doing

I'm gonna see if I can get someone else to look at the back-end stuff once more too just to make sure I didn't miss anything.

Comment 60

[Shawn Wilsher :sdwilsh]

Comment on attachment 271201 [details] [diff] [review]
netwerk patch 2.0: new function name type, simple NULL instead of full-featured nsIVariant.

biesi, can you r+sr this?

Comment 61

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 271201 [details] [diff] [review]
netwerk patch 2.0: new function name type, simple NULL instead of full-featured nsIVariant.

+  // It is VOID function, and it is allowed

"This is a VOID function"

Comment 62

[Christian :Biesinger (don't email me, ping me on IRC)]

though I'm not sure that the comment is really necessary

Comment 63

[Lev Serebryakov]

(In reply to comment #59)
> (From update of attachment 271200 [details] [diff] [review])
> >+                // GetAsAString does proper conversion to UCS2
> >+                // from all string-like types. It can be used
> >+                // universally without problems.
> really?  Sweet!
  Yep. nsIVariant has switch() around type inside it too, with all conversions.


> now you are squaring, not summing :)
  summing squares :)

Comment 64

[Lev Serebryakov]

(In reply to comment #62)
> though I'm not sure that the comment is really necessary
I don't know is it normal in Mozilla to return NULLs instead of nsIVariant :)

Comment 65

[Christian :Biesinger (don't email me, ping me on IRC)]

it is not uncommon to return null from functions. that it's legal here is sort of obvious, because otherwise you wouldn't do it.

Comment 66

[Lev Serebryakov]

Attachment: Very last nits seem to be fixed.

Ta-Da!

Comment 67

[Lev Serebryakov]

Attachment: Netwerk patch 2.1

Changes are trivial, again.

Comment 68

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 271356 [details] [diff] [review]
Netwerk patch 2.1

if I mark review+, and you just implement my review comments, there's no need to ask me again for review :)

Comment 69

[Shawn Wilsher :sdwilsh]

Comment on attachment 271355 [details] [diff] [review]
Very last nits seem to be fixed.

ditto :)

I'll probably check this in on Monday.

Comment 70

[Shawn Wilsher :sdwilsh]

Thanks for your work!

Checking in cache/src/nsDiskCacheDeviceSQL.cpp;
new revision: 1.11; previous revision: 1.10
Checking in public/Makefile.in;
new revision: 1.4; previous revision: 1.3
Checking in public/mozIStorageAggregateFunction.idl;
initial revision: 1.1
Checking in public/mozIStorageConnection.idl;
new revision: 1.11; previous revision: 1.10
Checking in public/mozIStorageFunction.idl;
new revision: 1.3; previous revision: 1.2
Checking in public/mozIStorageProgressHandler.idl;
initial revision: 1.1
Checking in src/mozStorageConnection.cpp;
new revision: 1.24; previous revision: 1.23
Checking in src/mozStorageConnection.h;
new revision: 1.7; previous revision: 1.6
Checking in test/storage1.cpp;
new revision: 1.9; previous revision: 1.8
Checking in test/unit/head_storage.js;
new revision: 1.4; previous revision: 1.3
Checking in test/unit/test_storage_aggregates.js;
initial revision: 1.1
Checking in test/unit/test_storage_function.js;
initial revision: 1.1
Checking in test/unit/test_storage_progresshandler.js;
initial revision: 1.1

Comment 71

[Eric Shepherd [:sheppy]]

Removing doc needed tag in favor of bug 399535.