273050 - Review storage module

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Darin Fisher]

Review storage module

Comment 1

[Darin Fisher]

Attachment: review of interfaces

Comment 2

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

I also had some comments on the interfaces in bug 261861; but I don't know in
how far those are still relevant.

Comment 3

[Darin Fisher]

Vlad:

Poke, poke... any movement on this bug?  More and more consumers are popping up for moz storage, which is really great, but that also means that it will become increasingly difficult to make changes to its interfaces.

My review comments are now almost a year old... what's the deal? ;-)

Comment 4

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

I know, I know :(  I've started going over it yesterday and am continuing now.. I'll post a patch here with updates/changes for more feedback.

Comment 5

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

Comment on attachment 167776 [details]
review of interfaces

>mozIStorageService
>
>  How about adding #defines in a %{C++ section for the valid
>  values for aStorageKey?
Done.

>  Threading it going to be important if we want to implement
>  the shared cert database.

Not sure if this is relevant any more; I've removed the "cert" bit from here as well, since it sounds like we'll want to just use the NSS interfaces.

>  Add an overall description for this interface.  what is it?
>  why would someone use it?  etc.
Done.

>  Do the functions throw any interesting exceptions worth
>  documenting?
Done.

>mozIStorageConnection
>
>  Is databaseName a leafname or a full path?

I've changed this to be a nsIFile for the actual database file, null if it's an in-memory database.

>  What does lastError really mean?  "Last error code" of what?
>  Should it be a nsresult value?  Was this error code previously
>  returned by some function?  What are the possible values?

It's the last SQLite error code; I should probably put in some enums in here that correspond to the sqlite error codes.  lastErrorString is a string representation of that error code.  Error handling in sqlite is a little weird, since errors can get propagated upwards from low-level statement errors to the connection/etc. in strange ways, though I think that only occurs with SQLITE_SCHEMA.

>  Is lastInsertRowID really a signed 64-bit integer?

It really is.

>  createStatement says:
>    "The expression may use ?0 ?1 etc. to indicate numbered arguments."
>  What about the other mechanisms supported by SQLite?  are those
>  verboten?

At one point all other mechanisms other than ? were broken in sqlite; I think this has since been fixed.
I've updated this comment.

>  beginTransaction, commitTransaction, rollbackTransaction talk about
>  possible exceptions but they do not explain what those exceptions
>  mean.  why might NS_ERROR_STORAGE_INVALID_OPERATION be thrown?
>  under what conditions might beginTransaction be an invalid operation?

This has been mostly covered with brett's patch.

>  createTable's documentation seems incomplete or wrong.

Updated.

>  In the disk cache, I wrote a trigger like this:
>    "CREATE TEMP TRIGGER cache_on_delete AFTER DELETE"
>    " ON moz_cache FOR EACH ROW BEGIN SELECT"
>    " cache_eviction_observer("
>    "  OLD.clientID, OLD.key, OLD.generation);"
>    " END;"
>  The idea being to get a callback when a row is deleted so that I can
>  delete the corresponding file on disk.  It would seem that
>  createTrigger does not support this kind of trigger.  Instead, I 
>  assume that I need to just call executeSimpleSQL.  Is that right?

Yeah, you'd do that with executeSimpleSQL.  The trigger stuff here is a holdover from when we wanted to do IPC proxying of trigger events and the like.. not sure if it's still relevant or if I should just delete the trigger APIs and just use the normal execute SQL.

>  Add an overall description for this interface.
Done.

>mozIStorageFunction
>
>  Add an overall description for this interface.
Done.

>mozIStorageValueArray
>
>  Add a comment stating what the VALUE_TYPE_XXX constants are for.
Done.

>  Document getTypeOfIndex.
Done.

>  Should numColumns be numEntries instead?  An "array" has entries
>  not columns.  I understand that this is meant to correspond to
>  a row in a table, but I think numEntries might still be better.

Hmm.. yeah, ok, I'll buy that.  I'll need to update a bunch of other code, but shouldn't be too bad.


>  -/* Due to SQLite's type conversion rules, any of these is valid */
>  +/* Due to SQLite's type conversion rules, any of these are valid */ 
>  
>  Do you really want to allow Mozilla code to depend on SQLite's
>  type mashing?  Shouldn't we try to abstract SQLite?  Or, are we
>  happy to be tightly coupled to SQLite?  I don't think Mozilla
>  needs to depend on this type mashing.

I think we're ok to depend on the type mashing; it's possible to do the same conversions for any other database, except that the database might not do it yourself.  We're going to be tightly coupled with SQLite in other ways, we may as well not fight it here.

>  getAsCString should probably return ACString to be consistent
>  with getAsString and getAsUTF8String.  However, why do we even
>  need getAsCString?  I thought SQLite used either UTF-8 or UTF-16.
>  Are you saying that you can store strings with random charsets?

SQLite will always store strings as either UTF8 or UTF16, depending on the given database encoding.  SQLite will convert either one to whatever is requested, and I think this is convenient for us since we have such a mix of UTF8 and UTF16 in various other parts of Mozilla.

>  Also, why does asSharedBlob return |const void **| instead of
>  |const PRUint8 **|?

For some reason I couldn't get PRUint8* as a return type; I'm not sure why I ended up using void**.  This method really should end up with a 'const PRUint8* AsSharedBlob(int col, int* bytelen);' signature.  (Still TODO)

>  asSharedCString and asSharedWString could be named better.  Perhaps
>  they should have UTF8 and UTF16 in their names?

asSharedUTF8String and asSharedUTF16String?  I'd buy that.  Maybe just asSharedUTF8String and asSharedString? (Still TODO)

>  I'm not sure why we need asInt32 and getAsInt32.  Same applies
>  to the Int64 and Double cases.

To be able to use them in native code like: |long val =  dbc->AsInt32(col1) - dbc->AsInt32(col2);|,
instead of having to use temporary variables everywhere so that we can pass the pointers in to get the values.

>  I understand the reason for providing shared accessors for the
>  string values and blob values, but perhaps those should still
>  return nsresult.

I'd rather go the other way and change the signature of the blob getter to return
the blob const PRUint8* from the method. (still TODO)

>    [noscript] void getUTF8StringConst(in unsigned long aIndex,
>                                       out unsigned long aLength,
>                                       [shared, retval] out string aValue);
>
>  And, document the fact that aLength may be null.
Done
>  Why do we need a way to get a const |wstring| value?  I thought we
>  were only going to store UTF-8 valued strings?  Does SQLite allow
>  you to get the UTF-16 value of a UTF-8 string entry?

It does; it'll do the conversion internally and then cache it until the statement is released.

>  Probably don't need getIsNull and isNull.

Same reason as the int32 case.. |if (dbc->IsNull(col1)) { ... }| as opposed to |PRBool nullval; if (dbc->GetIsNull(&nullval) && nullval) { ... }|

>  /* XXX handle blobs! */   <-- stale comment?
Yup.

>mozIStorageStatement
>
>  clone needs to be documented.
Done.

>  here you have bindDataParameter, but to read you call it a "blob"
>  perhaps you should use "data" or "blob" in both places?
Yeah, Data->Blob

>  for the string parameter binding functions, you should use the same
>  name "CString", "String", "WString", etc. that are used for the
>  corresponding getters on mozIStorageValueArray.
I agree.. let me figure out what to do about the UTF8/UTF16 stuff (still TODO)

>  bindParameters is commented out.  should it be removed?

Yeah; I wanted to provide a C++ varargs interface for statements also, but it ended up
being not possible with XPIDL.  We have the JS wrapper now, so this isn't necessary any more.

>
>mozIStorageDataSet
>
>  why not just replace this interface with nsIArray?
>  afterall, it has a method to return a nsISimpleEnumerator.

Heh.. executeDataSet() isn't implemented and there's only a stub implementation of mozStorageDataSet.  I'm wondering how useful this is; should I just get rid of it?  It would probably be more useful as a JS wrapper function than anything (return a JS array of arrays). (still TODO)


Patch for this stuff so far coming in a sec, minus the things marked as (still TODO).

Comment 6

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

Attachment: storage-interface-cleanup-1.patch

Review comments incorporated according to previous comment.

Comment 7

[Darin Fisher]

> >  Threading it going to be important if we want to implement
> >  the shared cert database.
> 
> Not sure if this is relevant any more; I've removed the "cert" bit from here 
> as well, since it sounds like we'll want to just use the NSS interfaces.

Yeah, I understood that NSS now provides its own solution for this or at least that the Redhat guys implemented something already.  Is that what you are referring to?  I'm not sure what you mean by "just use the NSS interfaces."


> >  What does lastError really mean?  "Last error code" of what?
> >  Should it be a nsresult value?  Was this error code previously
> >  returned by some function?  What are the possible values?
> 
> It's the last SQLite error code; I should probably put in some enums in here
> that correspond to the sqlite error codes.  lastErrorString is a string
> representation of that error code.  Error handling in sqlite is a little 
> weird, since errors can get propagated upwards from low-level statement errors 
> to the connection/etc. in strange ways, though I think that only occurs with
> SQLITE_SCHEMA.

It might make sense to implement a mapping from SQLite error codes to nsresult values.  Then, we could use the string bundle service to map the nsresult error codes to localized text.


> Yeah, you'd do that with executeSimpleSQL.  The trigger stuff here is a
> holdover from when we wanted to do IPC proxying of trigger events and the
> like.. not sure if it's still relevant or if I should just delete the trigger
> APIs and just use the normal execute SQL.

Yeah, I figured.  Perhaps it would be wise to kill the current trigger stuff if no one is using it.


> >  getAsCString should probably return ACString to be consistent
> >  with getAsString and getAsUTF8String.  However, why do we even
> >  need getAsCString?  I thought SQLite used either UTF-8 or UTF-16.
> >  Are you saying that you can store strings with random charsets?
> 
> SQLite will always store strings as either UTF8 or UTF16, depending on the
> given database encoding.  SQLite will convert either one to whatever is
> requested, and I think this is convenient for us since we have such a mix of
> UTF8 and UTF16 in various other parts of Mozilla.

OK, but this doesn't answer my question :)  Why do we need getAsCString?
And, if we need it, then why doesn't it return an ACString?  We should avoid
use of |string| and |wstring|.  By the way, ACString could also be used to
return a blob.  From Javascript or C++, ACString supports null characters and non-ASCII bytes.  The JS String object is also a more efficient 'byte-array'
container than a JS Array of integer objects (or so I'm told).


> >  Also, why does asSharedBlob return |const void **| instead of
> >  |const PRUint8 **|?
> 
> For some reason I couldn't get PRUint8* as a return type; I'm not sure why I
> ended up using void**.  This method really should end up with a 'const PRUint8*
> AsSharedBlob(int col, int* bytelen);' signature.  (Still TODO)

OK, please make this change before we resolve this bug.


> >  asSharedCString and asSharedWString could be named better.  Perhaps
> >  they should have UTF8 and UTF16 in their names?
> 
> asSharedUTF8String and asSharedUTF16String?  I'd buy that.  Maybe just
> asSharedUTF8String and asSharedString? (Still TODO)

The names should parallel whatever we do for the "getAs" versions.  So, that would mean "asSharedUTF8String" and "asSharedString".


> >  I'm not sure why we need asInt32 and getAsInt32.  Same applies
> >  to the Int64 and Double cases.
> 
> To be able to use them in native code like: |long val =  dbc->AsInt32(col1) -
> dbc->AsInt32(col2);|,
> instead of having to use temporary variables everywhere so that we can pass 
> the pointers in to get the values.

Why not solve that with %{C++ blocks that define inline helper functions?  See for example nsIAtom.


> >  I understand the reason for providing shared accessors for the
> >  string values and blob values, but perhaps those should still
> >  return nsresult.
> 
> I'd rather go the other way and change the signature of the blob getter to
> return the blob const PRUint8* from the method. (still TODO)

Yeah, I agree.  Since the underlying SQLite API cannot return an error from
the value getters, we don't need to invent error codes at that level.


> >  Probably don't need getIsNull and isNull.
> 
> Same reason as the int32 case.. |if (dbc->IsNull(col1)) { ... }| as opposed to
> |PRBool nullval; if (dbc->GetIsNull(&nullval) && nullval) { ... }|

Same suggestion as above.  Convert it into an inline helper.  Minimize the vtable :)


> >mozIStorageDataSet
> >
> >  why not just replace this interface with nsIArray?
> >  afterall, it has a method to return a nsISimpleEnumerator.
> 
> Heh.. executeDataSet() isn't implemented and there's only a stub implementation
> of mozStorageDataSet.  I'm wondering how useful this is; should I just get rid
> of it?  It would probably be more useful as a JS wrapper function than anything
> (return a JS array of arrays). (still TODO)

Agreed.  Don't make it too easy for someone to shoot themselves in the foot ;-)

Comment 8

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

(In reply to comment #7)
> > >  Threading it going to be important if we want to implement
> > >  the shared cert database.
> > 
> > Not sure if this is relevant any more; I've removed the "cert" bit from here 
> > as well, since it sounds like we'll want to just use the NSS interfaces.
> 
> Yeah, I understood that NSS now provides its own solution for this or at least
> that the Redhat guys implemented something already.  Is that what you are
> referring to?  I'm not sure what you mean by "just use the NSS interfaces."

Yep, I meant the Redhat implementation.  The NSS stuff will be independent of the mozStorage stuff.

> > >  What does lastError really mean?  "Last error code" of what?
> > >  Should it be a nsresult value?  Was this error code previously
> > >  returned by some function?  What are the possible values?
> > 
> > It's the last SQLite error code; I should probably put in some enums in here
> > that correspond to the sqlite error codes.  lastErrorString is a string
> > representation of that error code.  Error handling in sqlite is a little 
> > weird, since errors can get propagated upwards from low-level statement errors 
> > to the connection/etc. in strange ways, though I think that only occurs with
> > SQLITE_SCHEMA.
> 
> It might make sense to implement a mapping from SQLite error codes to nsresult
> values.  Then, we could use the string bundle service to map the nsresult error
> codes to localized text.

I don't think we'll be able to do this; there are only a handful of error codes that are useful without the associated error string, and the error string returned isn't constant -- for example, for a parse error, it'll include information about where in the statement the parse error occurred etc.

A separate function call (or service) to translate error codes to generic localized strings may be useful, but I think we can add that later.

> > Yeah, you'd do that with executeSimpleSQL.  The trigger stuff here is a
> > holdover from when we wanted to do IPC proxying of trigger events and the
> > like.. not sure if it's still relevant or if I should just delete the trigger
> > APIs and just use the normal execute SQL.
> 
> Yeah, I figured.  Perhaps it would be wise to kill the current trigger stuff if
> no one is using it.

Ok, removed.

> > >  getAsCString should probably return ACString to be consistent
> > >  with getAsString and getAsUTF8String.  However, why do we even
> > >  need getAsCString?  I thought SQLite used either UTF-8 or UTF-16.
> > >  Are you saying that you can store strings with random charsets?
> > 
> > SQLite will always store strings as either UTF8 or UTF16, depending on the
> > given database encoding.  SQLite will convert either one to whatever is
> > requested, and I think this is convenient for us since we have such a mix of
> > UTF8 and UTF16 in various other parts of Mozilla.
> 
> OK, but this doesn't answer my question :)  Why do we need getAsCString?
> And, if we need it, then why doesn't it return an ACString?  We should avoid
> use of |string| and |wstring|.  By the way, ACString could also be used to
> return a blob.  From Javascript or C++, ACString supports null characters and
> non-ASCII bytes.  The JS String object is also a more efficient 'byte-array'
> container than a JS Array of integer objects (or so I'm told).

It can't return ACString because those can't be return types in IDL, last time I tried it.  

> > >  Also, why does asSharedBlob return |const void **| instead of
> > >  |const PRUint8 **|?
> > 
> > For some reason I couldn't get PRUint8* as a return type; I'm not sure why I
> > ended up using void**.  This method really should end up with a 'const PRUint8*
> > AsSharedBlob(int col, int* bytelen);' signature.  (Still TODO)
> 
> OK, please make this change before we resolve this bug.

fixed.

> 
> > >  asSharedCString and asSharedWString could be named better.  Perhaps
> > >  they should have UTF8 and UTF16 in their names?
> > 
> > asSharedUTF8String and asSharedUTF16String?  I'd buy that.  Maybe just
> > asSharedUTF8String and asSharedString? (Still TODO)
> 
> The names should parallel whatever we do for the "getAs" versions.  So, that
> would mean "asSharedUTF8String" and "asSharedString".

Changed.

> > >  I'm not sure why we need asInt32 and getAsInt32.  Same applies
> > >  to the Int64 and Double cases.
> > 
> > To be able to use them in native code like: |long val =  dbc->AsInt32(col1) -
> > dbc->AsInt32(col2);|,
> > instead of having to use temporary variables everywhere so that we can pass 
> > the pointers in to get the values.
> 
> Why not solve that with %{C++ blocks that define inline helper functions?  See
> for example nsIAtom.

Hmm, that's a great idea.  What do you think of renaming, say, |getAsInt32| -> |getInt32|, with a C++ inline helper |PRInt32 int32(PRUint32 column)|?

> > >  I understand the reason for providing shared accessors for the
> > >  string values and blob values, but perhaps those should still
> > >  return nsresult.
> > 
> > I'd rather go the other way and change the signature of the blob getter to
> > return the blob const PRUint8* from the method. (still TODO)
> 
> Yeah, I agree.  Since the underlying SQLite API cannot return an error from
> the value getters, we don't need to invent error codes at that level.

Done.

> > >mozIStorageDataSet
> > >
> > >  why not just replace this interface with nsIArray?
> > >  afterall, it has a method to return a nsISimpleEnumerator.
> > 
> > Heh.. executeDataSet() isn't implemented and there's only a stub implementation
> > of mozStorageDataSet.  I'm wondering how useful this is; should I just get rid
> > of it?  It would probably be more useful as a JS wrapper function than anything
> > (return a JS array of arrays). (still TODO)
> 
> Agreed.  Don't make it too easy for someone to shoot themselves in the foot ;-)

Done.

Comment 9

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

The names I ended up with:

XPCOM getters, and their inline C++ helpers:

getInt32 -> asInt32
getInt64 -> asInt64
getDouble -> asDouble
getUTF8String (nsACString) -> none
getString (nsAString) -> none
getBlob (PRUint8 array) -> none
getIsNull -> isNull

[noscript] shared getters:
getSharedUTF8String (string, char*) -> asSharedUTF8String
getSharedString (wstring, PRUnichar*) -> asSharedString
getSharedBlob (PRUint8*) -> asSharedBlob

What think you?

Comment 10

[Darin Fisher]

> It can't return ACString because those can't be return types in IDL, last time
> I tried it.  

Does something like this work?

  void getBlah([retval] out ACString value);


> Hmm, that's a great idea.  What do you think of renaming, say, |getAsInt32| ->
> |getInt32|, with a C++ inline helper |PRInt32 int32(PRUint32 column)|?

"int32" looks like something that might be used as a type name.  I think I'd rather we name it differently.  Not sure what though.

Comment 11

[Darin Fisher]

(In reply to comment #9)
...
> What think you?

Looks good to me!

Comment 12

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

Attachment: storage-interface-cleanup-2.patch

This should incorporate all the comments and fixes.

Comment 13

[Darin Fisher]

Comment on attachment 203029 [details] [diff] [review]
storage-interface-cleanup-2.patch

Just a bunch of nits... please make these revisions before commiting the patch.


>+++ storage/public/mozIStorageConnection.idl	905299b5170a9f0af3ad8cbe0db7f2ae9fce45ae

>+/**
>+ * mozIStorageConnection represents a database connection attached
>+ * to a specific file or to the in-memory data storage.
>+ * It is the primary interface for interacting with a database,
>+ * including creating prepared statements, executing SQL, and examining
>+ * database errors.

nit: how about wrapping the comments to 80 chars for readability?  ("gqap" in vim)
same applies to all of the comments :)


>+   * the current database nsIFile.  Null if the database
>+   * connection refers to an in-memory database.

nit: s/the/The/


>+   * lastError returns the last error SQLite error code

     * @returns The last SQLite error code for this connection.

nit: Use "@returns" or "Returns"
Same for other readonly attributes?


>   /**
>    * Create the table with the given name and schema.  If the table
>+   * already exists with the given schema, NS_ERROR_FAILURE is thrown.
>+   * (XXX should be NS_STORAGE_TABLE_EXISTS)
>    *
>+   * (If the table exists but with a different schema,
>+   * NS_ERROR_STORAGE_TABLE_CONFLICT (?) is thrown. -- Not yet
>+   * implemented)

nit: Best to document error codes using the @throws doxygen rule.

What's the plan for defining that error code?


>    * @param aTableName the table name to be created, consisting of [A-Za-z0-9_], and beginning with a letter.
>    *
>    * @param aTableSchema the schema of the table; what would normally go between the parens in a CREATE TABLE
>+   *                     statement: e.g., "foo INTEGER, bar STRING".
>+   *
>+   * @throws NS_ERROR_FAILURE if the table already exists or could not be created for some other reason
>+   *

nit: Please wrap long lines to 80 chars.


>+++ storage/public/mozIStorageService.idl	0ef33fab516c26aa84c446a7fffdd0fc2c209fbb

>   mozIStorageConnection getProfileStorage(in string aStorageKey);
...
>   mozIStorageConnection openDatabase(in nsIFile aDatabaseFile);

nit: How about some symmetry between these two method names?  Some
(lame) suggestions:

  getProfileStorage and getFileStorage?
  getProfileStorage and getStorage?
  getSpecialStorage and getStorage?
  getNamedStorage and getStorage?
  openProfileDatabase and openDatabase
  openSpecialDatabase and openDatabase
  openNamedDatabase and openDatabase


>+++ storage/public/mozIStorageStatement.idl	dbfdda0c1dfefa8fba0e93d37062065490a26c0d

>+  /**
>+   * Create a clone of this statement, by initializing a new statement
>+   * with the same connection and same SQL statement as this one.
>+   */
>   mozIStorageStatement clone ();

nit: Would be nice if storage IDL was consistent about the whitespace
usage between method names and opening parenthesis.

Are there any constraints on when this clone method may be called?
Can it be called on a statement that is being "stepped"?


>   void bindUTF8StringParameter (in unsigned long aParamIndex, in AUTF8String aValue);
>   void bindStringParameter (in unsigned long aParamIndex, in AString aValue);
>   void bindDoubleParameter (in unsigned long aParamIndex, in double aValue);
>   void bindInt32Parameter (in unsigned long aParamIndex, in long aValue);
>   void bindInt64Parameter (in unsigned long aParamIndex, in long long aValue);
> 
>   void bindNullParameter (in unsigned long aParamIndex);

nit: documentation?


>+  void bindBlobParameter (in unsigned long aParamIndex, [array,const,size_is(aValueSize)] in octet aValue, in unsigned long aValueSize);

nit: wrap to 80 chars.  Please fix up the rest of the code too.


>+++ storage/public/mozIStorageValueArray.idl	08e37e4995220b080efd4bd5abb187c9c3a14bca

> [scriptable, uuid(44fc1d3b-dc91-4d17-8bc5-2069d8fd3cca)]
> interface mozIStorageValueArray : nsISupports {

nit: This interface needs a comment block above it.


>+  inline PRInt32 asInt32(PRUint32 idx) {

I think this should be AsInt32 instead.  camelCase is not the
preferred style for C++ method names in Mozilla.


>+++ storage/src/mozStorageStatement.cpp

>+NS_IMETHODIMP
>+mozStorageStatement::GetSharedString(PRUint32 aIndex, PRUint32 *aLength, const PRUnichar **_retval)
> {
>     if (aLength) {
>         int slen = sqlite3_column_bytes16 (mDBStatement, aIndex);

nit: Please be consistent with usage of whitespace between function name
and opening paren.


>+mozStorageStatementRowValueArray::GetIsNull(PRUint32 aIndex, PRBool *_retval)
> {
>+    PRInt32 t;
>+    nsresult rv = GetTypeOfIndex (aIndex, &t);
>+    if (NS_FAILED(rv)) return rv;

nit: Please return on a newline so it is possible to set a breakpoint
on the return statement in the debugger.

Comment 14

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

(In reply to comment #13)
> (From update of attachment 203029 [details] [diff] [review] [edit])
> Just a bunch of nits... please make these revisions before commiting the patch.

Fixed 'em all, will commit shortly.

> >   /**
> >    * Create the table with the given name and schema.  If the table
> >+   * already exists with the given schema, NS_ERROR_FAILURE is thrown.
> >+   * (XXX should be NS_STORAGE_TABLE_EXISTS)
> >    *
> >+   * (If the table exists but with a different schema,
> >+   * NS_ERROR_STORAGE_TABLE_CONFLICT (?) is thrown. -- Not yet
> >+   * implemented)
> 
> nit: Best to document error codes using the @throws doxygen rule.
> 
> What's the plan for defining that error code?

I ran into some problems implementing that, so just left it out for now (and removed the comment).  The function will throw if a table with the given name already exists.

> >   mozIStorageConnection getProfileStorage(in string aStorageKey);
> ...
> >   mozIStorageConnection openDatabase(in nsIFile aDatabaseFile);
> 
> nit: How about some symmetry between these two method names?

I went with openSpecialDatabase() and openDatabase()
> >+  /**
> >+   * Create a clone of this statement, by initializing a new statement
> >+   * with the same connection and same SQL statement as this one.
> >+   */
> >   mozIStorageStatement clone ();
> 
> nit: Would be nice if storage IDL was consistent about the whitespace
> usage between method names and opening parenthesis.

I fixed that in the IDL.. not much chance of getting me to fix it in the C++ though, I'm horribly inconsistent with that, but I'm usually consistent in that I'll throw in a space where it looks like it'll improve readability (especially with a long parameter list) ;)

> Are there any constraints on when this clone method may be called?
> Can it be called on a statement that is being "stepped"?

No constraints; it just creates a new statement with the same SQL.  It does not preserve the statement's state.. I updated the comment to reflect this.

Comment 15

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

Attachment: patch that landed for posterity

Checked in on trunk.  Hope I didn't break anything...

Comment 16

[David G King]

I think it did break something. This on a Fedora Core 4 Linux box :-

storage1.cpp
c++ -o storage1.o -c -I../../dist/include/system_wrappers -include /home/mozilla/source/mozilla/config/gcc_hidden.h -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6.14-1\" -DOSARCH=\"Linux\" -DBUILD_ID=0000000000  -I../../dist/include/xpcom -I../../dist/include/string -I../../dist/include/storage -I../../dist/include/storagecomps -I../../dist/include -I../../dist/include -I../../dist/include/nspr    -I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O  -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../mozilla-config.h -Wp,-MD,.deps/storage1.pp /home/mozilla/source/mozilla/storage/test/storage1.cpp
/home/mozilla/source/mozilla/storage/test/storage1.cpp: In function ‘int main(int, char**)’:
/home/mozilla/source/mozilla/storage/test/storage1.cpp:112: error: ‘class nsDerivedSafe<mozIStorageValueArray>’ has no member named ‘GetNumColumns’
/home/mozilla/source/mozilla/storage/test/storage1.cpp:113: error: ‘class nsDerivedSafe<mozIStorageValueArray>’ has no member named ‘AsSharedCString’
/home/mozilla/source/mozilla/storage/test/storage1.cpp:117: warning: format ‘%p’ expects type ‘void*’, but argument 5 has type ‘nsDerivedSafe<mozIStorageValueArray>*’
gmake[5]: *** [storage1.o] Error 1

Comment 17

[Darin Fisher]

By the way, since some interface methods were modified or removed, we should really update the UUID properties of the interfaces.  I realize that storage isn't on by default, so this shouldn't be much of an issue, but when it comes to changing interfaces, I think we're always better off safe than sorry.

Comment 18

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

Hmm, I thought I fixed the test, though I may have built with --disable-tests.. oops.  I'll fix that and update UUIDs in a sec.