Database2Versions table, get/set version operations implemented

Permissions database components, necessary for implementing Database2 API versioning (http://code.google.com/p/google-gears/wiki/Database2API#Versioning) added.

I attempted to follow existing Permissions DB conventions/structure as closely as possible.

[Aaron, 2008-05-19 15:33:49]

http://codereview.appspot.com/800/diff/1/5
File gears/base/common/database2_versions_table.cc (right):

http://codereview.appspot.com/800/diff/1/5#newcode117
Line 117: bool Database2VersionsTable::MaybeCreateTableLatestVersion() {
Let's just call this CreateTableVersion8() and not have the IF NOT EXISTS. We
should fail if we are somehow trying to upgrade to version 8 and this table
exists.

http://codereview.appspot.com/800/diff/1/4
File gears/base/common/database2_versions_table.h (right):

http://codereview.appspot.com/800/diff/1/4#newcode31
Line 31: class Database2VersionsTable {
I'd like to rename this class Database2MetaData. We'll rename the existing
DatabaseNameTable to match (Database1MetaData).

http://codereview.appspot.com/800/diff/1/8
File gears/base/common/permissions_db.cc (right):

http://codereview.appspot.com/800/diff/1/8#newcode314
Line 314: bool PermissionsDB::GetDatabase2Version(const SecurityOrigin &origin,
I think we can eliminate this extra indirection by having a
PermissionsDB::GetDatabase2MetaData() method. That way we don't have to
duplicate all its methods here.

[Dimitri, 2008-05-20 03:07:07]

Updated, ready for another look.

http://codereview.appspot.com/800/diff/1/5
File gears/base/common/database2_versions_table.cc (right):

http://codereview.appspot.com/800/diff/1/5#newcode117
Line 117: bool Database2VersionsTable::MaybeCreateTableLatestVersion() {
On 2008/05/19 15:33:50, Aaron wrote:
> Let's just call this CreateTableVersion8() and not have the IF NOT EXISTS. We
> should fail if we are somehow trying to upgrade to version 8 and this table
> exists.

Done.

http://codereview.appspot.com/800/diff/1/4
File gears/base/common/database2_versions_table.h (right):

http://codereview.appspot.com/800/diff/1/4#newcode31
Line 31: class Database2VersionsTable {
On 2008/05/19 15:33:50, Aaron wrote:
> I'd like to rename this class Database2MetaData. We'll rename the existing
> DatabaseNameTable to match (Database1MetaData).

Done. One thought to consider: the DatabaseNameTable should probably still be
used in Database2 because of the nice corruption/version rollover logic, and
because it appears to be modeled as a cross-module, central metadata store for
tables.

http://codereview.appspot.com/800/diff/1/8
File gears/base/common/permissions_db.cc (right):

http://codereview.appspot.com/800/diff/1/8#newcode314
Line 314: bool PermissionsDB::GetDatabase2Version(const SecurityOrigin &origin,
On 2008/05/19 15:33:50, Aaron wrote:
> I think we can eliminate this extra indirection by having a
> PermissionsDB::GetDatabase2MetaData() method. That way we don't have to
> duplicate all its methods here.

Yes! I was wondering why the convention was to duplicate the methods, but didn't
dare stray. Done.

[Scott.Hess, 2008-05-20 20:40:56]

I'm not totally in agreement with Aaron about exposing the Database2Metadata
object to PermissionsDB clients.  To my mind, that binds them to internal
implementation details that they really don't need to know about.

http://codereview.appspot.com/800/diff/83/107
File gears/base/common/database2_metadata.cc (right):

http://codereview.appspot.com/800/diff/83/107#newcode69
Line 69: *version = statement.column_text16_safe(0);
Suggest returning an error if statement.step()!=SQLITE_DONE at this point.

http://codereview.appspot.com/800/diff/83/107#newcode126
Line 126: LOG(("Database2VersionsTabel::MaybeCreateTableLatestVersion create"
s/Database2VersionsTabel/Database2Metadata/;
s/MaybeCreateTableLatestVersion/CreateTableVersion8/;

[Dimitri, 2008-05-25 11:34:17]

This issue is now part of http://codereview.appspot.com/1145, closing this one.