LGTM
On Wed, Aug 27, 2008 at 6:29 PM, Chris Prince <[EMAIL PROTECTED]> wrote:
> On Wed, Aug 27, 2008 at 3:28 PM, Michael Nordman <[EMAIL PROTECTED]>
> wrote:
> > ------------------------------------
> > Line 319: // Hooray, it succeeded. Nothing more to do here.
> > maybe invert the test, LOG("something went wrong") if anything doesn't
> work
>
> On the flip side, it's nice that the current version uses parallel
> logic in Step 1 and Step 2. Feels like it could go either way. I'm
> leaning toward leaving as-is.
The logic is fine, in most cases with SQLite code we test for errors and do
an early return.
This method inverts that typical usage and it caught my eye is all. Either
way works.
>
>
> > ------------------------------------
> > Line 337: File::Delete(full_path.c_str()); // ignore return value
> > As files are really deleted, we may be able to actually delete the
> corresponding
> > row instead of leaving them marked as deleted. Not sure its worth it...
> just
> > throwing this out there.
>
> Yeah, not sure it's worth it. I also don't know the other SQL well
> enough to be sure it's 100% safe. Again, leaning toward leaving
> as-is.
Fine by me (nothing is 100% safe :)
Also we would lose some history about databases going corrupt that could be
useful for diagnostic purposes... actually may be a good reason to not prune
this table.
>
>
> > ========================================================================
> >
> http://mondrian.corp.google.com/file/8116546///depot/googleclient/gears/opensource/gears/base/common/permissions_db.cc?a=1
> > File
> //depot/googleclient/gears/opensource/gears/base/common/permissions_db.cc
> (snapshot 1)
> > ------------------------------------
> > Line 186: database_name_table_.DeleteOriginDatabases(origin);
> > maybe rename this to DeleteDatabasesForOrigin for consistency with the
> language
> > used below
>
> Done.
>
> > ------------------------------------
> > Line 192: }
> > At this point, we may be able to delete data directory for this origin
> all
> > together.
> >
> > There may be shortcut icons stored in here, otherwise Gears would have no
> other
> > data within this directory.
> >
> > Deleting the directory if its empty (or contains only empty directories)
> would
> > be safe.
>
> I think I'd have to write such a function, and test it well. It
> doesn't seem worthwhile, since this only affects empty directories.
Punt is fine by me. Perhaps a TODO.
