Please report any other irregularities here.
Useful for all of our consumers.
Created attachment 270360 [details] [diff] [review] v1.0
1) from http://www.sqlite.org/pragma.html "The pragmas schema_version and user_version are used to set or get the value of the schema-version and user-version, respectively. Both the schema-version and the user-version are 32-bit signed integers stored in the database header." So, in the idl: attribute unsigned long schemaVersion; that should not be unsigned, right? 2) + *version = 0; + PRBool hasResult; + if (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) + *version = stmt->AsInt64(0); Should that be AsInt32(), not AsInt64() 3) for your tests, should verify that you can set the version to -1, too? perhaps also test setting the version if it is already set (go from 1 to 2).
11 years ago
(In reply to comment #2) > that should not be unsigned, right? ah, yes. Good catch. > Should that be AsInt32(), not AsInt64() Now, yes, but before we needed AsInt64 to get the full range of a PRUint32 (or almost) > for your tests, should verify that you can set the version to -1, too? perhaps > also test setting the version if it is already set (go from 1 to 2). I can do that
Created attachment 270365 [details] [diff] [review] v1.1
should we be returning an error when the schema is not set? see http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#523 and cool, you test both the negative schema and the modification of a set schema (going from 1 to -1) with just one additional test.
(In reply to comment #5) > should we be returning an error when the schema is not set? Well, in the idl I say that we return 0 if it isn't set, which is what happens anyway in my experience using SQLite Database Browser (and makes sense based on their docs saying that is always stored with the database). Consumers can always do an NS_ENSURE_TRUE on the result.
Comment on attachment 270365 [details] [diff] [review] v1.1 r=sspitzer, thanks for answering my questions. I'm getting 0 back when the schema is not set as well, but also, I like return 0 in either case (not set or 0.)
Attachment #270365 - Flags: review?(sspitzer) → review+
storage/public/mozIStorageConnection.idl 1.9 storage/src/mozStorageConnection.cpp 1.22 storage/test/unit/test_storage_connection.js 1.2
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Removing doc needed keyword in favor of a new bug that covers the fact that in general we actually need proper documentation for Storage; that bug (bug 399535) is set as blocked by this one.
You need to log in before you can comment on or make changes to this bug.