Refactor the DB code to make all DB layout tests pass on test_shell....

Refactor the DB code to make it accessible from test_shell.

BUG=21442
TEST=none

[michaeln, 2009-09-15 21:53:46]

This is a nice refactoring. Maybe mention in the CL description that this allows
us to reuse the core datasebase specific code in chrome and the test_shell.

http://codereview.chromium.org/203074/diff/2003/1013
File chrome/browser/DEPS (right):

http://codereview.chromium.org/203074/diff/2003/1013#newcode11
Line 11: "+webkit/databases",
maybe database, no plural

http://codereview.chromium.org/203074/diff/2003/1008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/203074/diff/2003/1008#newcode118
Line 118: if (error_code == SQLITE_IOERR_DELETE) {
it looks like you may be able to simply function now that the helper is picking
things apart for you...

void Delete {
  if (too many retries) {
    error = SQLITE_IOERR_DELETE;
  } else {
    error = databases::DatabaseDeleteFile(...)
    if (SQLITE_IOERROR_DELETE) {
      Post retry task
      return;
    }
  }

  Post send completion task
}

http://codereview.chromium.org/203074/diff/2003/1012
File webkit/DEPS (right):

http://codereview.chromium.org/203074/diff/2003/1012#newcode14
Line 14: "+third_party/sqlite",   # HTML5 DBs in test_shell
appcache will soon have this dependency too, so this won't be specific to
databse for long, maybe drop the comment?

http://codereview.chromium.org/203074/diff/2003/1006
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/203074/diff/2003/1006#newcode426
Line 426: web_prefs_->databases_enabled = true;
Cool!

http://codereview.chromium.org/203074/diff/5001/5009
File webkit/databases/db_utils.h (right):

http://codereview.chromium.org/203074/diff/5001/5009#newcode14
Line 14: void DatabaseOpenFile(const FilePath& file_name,
Since these function and the backend impl of our VFS layer, maybe name the file
and functions with Vfs?

vfs_backend.h .cc

database::VfsOpen(...)
database::VfsDelete(...)
database::VfsGetFileAttributes(...)
database::VfsGetFileSize(...)

At callsites in chrome and test_shell not in the database namespace, this naming
would lend clarity to these calls.

wdyt?

http://codereview.chromium.org/203074/diff/5001/5003
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/5001/5003#newcode48
Line 48: static FilePath GetDBDir() {
The two static methods seem out of place in this file. And there is some logic
in here too (retry logic). It may be nice to put the test_shell database
specifics in a seperate file, along the lines of...

#include "webkit/tools/test_shell/mock_webclipboard_impl.h"
#include "webkit/tools/test_shell/simple_appcache_system.h"
#include "webkit/tools/test_shell/simple_resource_loader_bridge.h"

#include "webkit/tools/test_shell/simple_database_system.h"

http://codereview.chromium.org/203074/diff/5001/5003#newcode50
Line 50: file_util::GetCurrentDirectory(&current_directory);
May be nice to use a ScopedTempDir so on exit, everything gets deleted
automatically.

[dumi, 2009-09-16 19:51:15]

http://codereview.chromium.org/203074/diff/2003/1008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/203074/diff/2003/1008#newcode118
Line 118: if (error_code == SQLITE_IOERR_DELETE) {
On 2009/09/15 21:53:46, michaeln wrote:
> it looks like you may be able to simply function now that the helper is
picking
> things apart for you...
> 
> void Delete {
>   if (too many retries) {
>     error = SQLITE_IOERR_DELETE;
>   } else {
>     error = databases::DatabaseDeleteFile(...)
>     if (SQLITE_IOERROR_DELETE) {
>       Post retry task
>       return;
>     }
>   }
> 
>   Post send completion task
> }

done.

http://codereview.chromium.org/203074/diff/2003/1012
File webkit/DEPS (right):

http://codereview.chromium.org/203074/diff/2003/1012#newcode14
Line 14: "+third_party/sqlite",   # HTML5 DBs in test_shell
On 2009/09/15 21:53:46, michaeln wrote:
> appcache will soon have this dependency too, so this won't be specific to
> databse for long, maybe drop the comment?

removed.

http://codereview.chromium.org/203074/diff/5001/5009
File webkit/databases/db_utils.h (right):

http://codereview.chromium.org/203074/diff/5001/5009#newcode14
Line 14: void DatabaseOpenFile(const FilePath& file_name,
On 2009/09/15 21:53:46, michaeln wrote:
> Since these function and the backend impl of our VFS layer, maybe name the
file
> and functions with Vfs?
> 
> vfs_backend.h .cc
> 
> database::VfsOpen(...)
> database::VfsDelete(...)
> database::VfsGetFileAttributes(...)
> database::VfsGetFileSize(...)
> 
> At callsites in chrome and test_shell not in the database namespace, this
naming
> would lend clarity to these calls.
> 
> wdyt?

renamed the file to vfs_backend, but i'm not sure Vfs* are good names; to me,
they seem to imply that we're asking the VFS to do something. maybe
VfsBackend{OpenFile, DeleteFile, ...}?

http://codereview.chromium.org/203074/diff/5001/5003
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/5001/5003#newcode48
Line 48: static FilePath GetDBDir() {
On 2009/09/15 21:53:46, michaeln wrote:
> The two static methods seem out of place in this file. And there is some logic
> in here too (retry logic). It may be nice to put the test_shell database
> specifics in a seperate file, along the lines of...
> 
> #include "webkit/tools/test_shell/mock_webclipboard_impl.h"
> #include "webkit/tools/test_shell/simple_appcache_system.h"
> #include "webkit/tools/test_shell/simple_resource_loader_bridge.h"
> 
> #include "webkit/tools/test_shell/simple_database_system.h"
> 

done. added simple_database_system.{h|cc} files for this.

http://codereview.chromium.org/203074/diff/5001/5003#newcode50
Line 50: file_util::GetCurrentDirectory(&current_directory);
On 2009/09/15 21:53:46, michaeln wrote:
> May be nice to use a ScopedTempDir so on exit, everything gets deleted
> automatically.

done.

[michaeln, 2009-09-16 20:33:20]

just a couple of things

http://codereview.chromium.org/203074/diff/3004/4015
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/203074/diff/3004/4015#newcode20
Line 20: static bool OpenFileFlagsAreConsistent(const FilePath& file_name,
it would be good to svn copy database_dispatcher_host.cc to vfs_backend.cc so we
get the revision history and can see any minor mods made in transit

http://codereview.chromium.org/203074/diff/3004/4016
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/3004/4016#newcode13
Line 13: 
class VfsBackend {
  static OpenFile(...);
  ...
};

works very much for me

http://codereview.chromium.org/203074/diff/3004/4012
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/203074/diff/3004/4012#newcode971
Line 971: SimpleDatabaseSystem().ClearAllDatabases();
I don't think this is doing what you want it to do?

The call to clear all databases on the newly constructed SimpleDatabaseSystem,
simply deletes a newly created and empty temp dir. The 'real' instance created
elsewhere remains unaffected.

http://codereview.chromium.org/203074/diff/3004/4014
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/3004/4014#newcode22
Line 22: const int kNumDeleteRetries = 5;
can you put this constant in the DatabaseDeleteFile method body

http://codereview.chromium.org/203074/diff/3004/4014#newcode42
Line 42: while ((numRetries < kNumDeleteRetries) &&
maybe a comment that explains the reason for the retries, that being... the
default sqlite vfs has this behavior for some reason known to sqlite.

http://codereview.chromium.org/203074/diff/3004/4004
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/3004/4004#newcode27
Line 27: #include "webkit/database/vfs_backend.h"
alpha ordering

http://codereview.chromium.org/203074/diff/3004/4004#newcode98
Line 98: return true;
do you want this change... what other behaviors besides "to chromeVfs install or
not" is affected... just checking?

[michaeln, 2009-09-16 20:48:45]

ooops... one more thing...

http://codereview.chromium.org/203074/diff/3004/4014
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/3004/4014#newcode42
Line 42: while ((numRetries < kNumDeleteRetries) &&
Does sql's default vfs call Sleep(t) or enter an alertable wait state (or
something) in between these retries that our impl here needs to mimic? In
chrome, we return the the message loop for retry purposes, but for test_shell...
what do we need to put in between them?

We can't return the the message loop, because there likely is no
base::MessageLoop to return to, we're generally running under a WebCore::Thread
at this point.

[dumi, 2009-09-16 22:28:41]

http://codereview.chromium.org/203074/diff/3004/4015
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/203074/diff/3004/4015#newcode20
Line 20: static bool OpenFileFlagsAreConsistent(const FilePath& file_name,
On 2009/09/16 20:33:20, michaeln wrote:
> it would be good to svn copy database_dispatcher_host.cc to vfs_backend.cc so
we
> get the revision history and can see any minor mods made in transit

done.

http://codereview.chromium.org/203074/diff/3004/4016
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/3004/4016#newcode13
Line 13: 
On 2009/09/16 20:33:20, michaeln wrote:
> class VfsBackend {
>   static OpenFile(...);
>   ...
> };
> 
> works very much for me

i don't think we need a class here (we don't store any state), so i just renamed
all methods to VfsBackendDoSomething (from DatabaseDoSomething).

http://codereview.chromium.org/203074/diff/3004/4012
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/203074/diff/3004/4012#newcode971
Line 971: SimpleDatabaseSystem().ClearAllDatabases();
On 2009/09/16 20:33:20, michaeln wrote:
> I don't think this is doing what you want it to do?
> 
> The call to clear all databases on the newly constructed SimpleDatabaseSystem,
> simply deletes a newly created and empty temp dir. The 'real' instance created
> elsewhere remains unaffected.

oops, you're right. fixed.

http://codereview.chromium.org/203074/diff/3004/4014
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/3004/4014#newcode22
Line 22: const int kNumDeleteRetries = 5;
On 2009/09/16 20:33:20, michaeln wrote:
> can you put this constant in the DatabaseDeleteFile method body

done.

http://codereview.chromium.org/203074/diff/3004/4014#newcode42
Line 42: while ((numRetries < kNumDeleteRetries) &&
On 2009/09/16 20:33:20, michaeln wrote:
> maybe a comment that explains the reason for the retries, that being... the
> default sqlite vfs has this behavior for some reason known to sqlite.

done.

http://codereview.chromium.org/203074/diff/3004/4014#newcode42
Line 42: while ((numRetries < kNumDeleteRetries) &&
On 2009/09/16 20:48:45, michaeln wrote:
> Does sql's default vfs call Sleep(t) or enter an alertable wait state (or
> something) in between these retries that our impl here needs to mimic? In
> chrome, we return the the message loop for retry purposes, but for
test_shell...
> what do we need to put in between them?
> 
> We can't return the the message loop, because there likely is no
> base::MessageLoop to return to, we're generally running under a
WebCore::Thread
> at this point.

done.

http://codereview.chromium.org/203074/diff/3004/4004
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/3004/4004#newcode27
Line 27: #include "webkit/database/vfs_backend.h"
On 2009/09/16 20:33:20, michaeln wrote:
> alpha ordering

fixed. moved webkit/extensions/ up too.

http://codereview.chromium.org/203074/diff/3004/4004#newcode98
Line 98: return true;
On 2009/09/16 20:33:20, michaeln wrote:
> do you want this change... what other behaviors besides "to chromeVfs install
or
> not" is affected... just checking?

that's the only thing affected by this flag. and we should probably rename the
flag/method to inBrowserProcess(), or something similar -- sandboxEnabled() is
incorrect for a couple of reasons.

[michaeln, 2009-09-18 03:56:39]

http://codereview.chromium.org/203074/diff/11008/12016
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/11008/12016#newcode6
Line 6: #define WEBKIT_DATABASE_VFS_BACKEND_H_
no need to svn copy the .h file, there is no relevant history here like there is
in the .cc file

http://codereview.chromium.org/203074/diff/11008/12014
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/11008/12014#newcode19
Line 19: #include "webkit/api/public/WebString.h"
shouldn't need this anymore

http://codereview.chromium.org/203074/diff/11008/12014#newcode32
Line 32: hack_main_db_handle_ = base::kInvalidPlatformFileValue;
this would be better in an initializer list, i don't think you need to repeat
the HACK comment here

http://codereview.chromium.org/203074/diff/11008/12014#newcode40
Line 40: // HACK: see DatabaseOpenFile
remove comment

http://codereview.chromium.org/203074/diff/11008/12014#newcode66
Line 66: if (hack_main_db_handle_ == base::kInvalidPlatformFileValue) {
don't need the curly braces for a one-line block

http://codereview.chromium.org/203074/diff/11008/12014#newcode77
Line 77: // we sleep for 100ms between retries for the same reason
Whoa... 100ms, that seems like a mighty long time? I think we could dial down
that number in database_dispather_host.cc and the number of attempts too.

500ms is an eternity. Wow, thats what os_win.c has alright, although with fewer
attempts.

/*
** Close a file.
**
** It is reported that an attempt to close a handle might sometimes
** fail.  This is a very unreasonable result, but windows is notorious
** for being unreasonable so I do not doubt that it might happen.  If
** the close fails, we pause for 100 milliseconds and try again.  As
** many as MX_CLOSE_ATTEMPT attempts to close the handle are made before
** giving up and returning an error.
*/

  #define MX_CLOSE_ATTEMPT 3
  do{
    rc = CloseHandle(pFile->h);
  }while( rc==0 && cnt++ < MX_CLOSE_ATTEMPT && (Sleep(100), 1) );

http://codereview.chromium.org/203074/diff/11008/12014#newcode94
Line 94: FilePath(webkit_glue::WebStringToFilePathString(file_name)));
Why we don't we need a full file path here?
GetDBFileFullPath(file_name);

http://codereview.chromium.org/203074/diff/11008/12014#newcode100
Line 100: FilePath(webkit_glue::WebStringToFilePathString(file_name)));
ditto

http://codereview.chromium.org/203074/diff/11008/12014#newcode104
Line 104: file_util::Delete(GetDBDir(), true);
Is this a correct implementation of this method?

In other ports, it looks like the LayoutTestController's clearAllDatabases()
method is plumbed to DatabaseTracker::deleteAllDatabases(). A proper impl of
that may have to wait until we have a representation of (Web)DatabaseTracker in
our WebKit api layer.

If that's right, maybe the thing to do now is...

void SimpleDatabaseSystem::ClearAllDatabases() {
  // TODO(dumi): fill this in when we have WebDatabaseTracker shimware
  // DatabaseTracker::tracker().deleteAllDatabases();
}


Here's what a different port does...

void LayoutTestController::clearAllDatabases()
{
    COMPtr<IWebDatabaseManager> databaseManager;
    COMPtr<IWebDatabaseManager> tmpDatabaseManager;
    if (FAILED(WebKitCreateInstance(CLSID_WebDatabaseManager, 0,
                                    IID_IWebDatabaseManager,
                                    (void**)&tmpDatabaseManager)))
        return;
    if (FAILED(tmpDatabaseManager->sharedWebDatabaseManager(&databaseManager)))
        return;

    databaseManager->deleteAllDatabases();
}

HRESULT STDMETHODCALLTYPE WebDatabaseManager::deleteAllDatabases()
{
    if (this != s_sharedWebDatabaseManager)
        return E_FAIL;

    DatabaseTracker::tracker().deleteAllDatabases();

    return S_OK;
}

void DatabaseTracker::deleteAllDatabases()
{
    ASSERT(currentThread() == m_thread);

    Vector<RefPtr<SecurityOrigin> > originsCopy;
    origins(originsCopy);

    for (unsigned i = 0; i < originsCopy.size(); ++i)
        deleteOrigin(originsCopy[i].get());
}

http://codereview.chromium.org/203074/diff/11008/12014#newcode112
Line 112: const WebKit::WebString& file_name) {
indentation is off

http://codereview.chromium.org/203074/diff/11008/12011
File webkit/tools/test_shell/simple_database_system.h (right):

http://codereview.chromium.org/203074/diff/11008/12011#newcode10
Line 10: #include "webkit/api/public/WebString.h"
Now that this class is one step removed from TestShellWebKitInit and its impl of
WebKitClient, we should be able to drop usage of WebKit api'isms. I think you
can replace the WebString usages with FilePath here, and have
TestShellWebKitInit do the type translations.

http://codereview.chromium.org/203074/diff/11008/12011#newcode16
Line 16: virtual ~SimpleDatabaseSystem();
why virtual?

[dumi, 2009-09-18 22:18:41]

http://codereview.chromium.org/203074/diff/11008/12016
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/11008/12016#newcode6
Line 6: #define WEBKIT_DATABASE_VFS_BACKEND_H_
On 2009/09/18 03:56:40, michaeln wrote:
> no need to svn copy the .h file, there is no relevant history here like there
is
> in the .cc file

reverted, and added as a new file.

http://codereview.chromium.org/203074/diff/11008/12014
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/11008/12014#newcode19
Line 19: #include "webkit/api/public/WebString.h"
On 2009/09/18 03:56:40, michaeln wrote:
> shouldn't need this anymore

removed.

http://codereview.chromium.org/203074/diff/11008/12014#newcode32
Line 32: hack_main_db_handle_ = base::kInvalidPlatformFileValue;
On 2009/09/18 03:56:40, michaeln wrote:
> this would be better in an initializer list, i don't think you need to repeat
> the HACK comment here

done.

http://codereview.chromium.org/203074/diff/11008/12014#newcode66
Line 66: if (hack_main_db_handle_ == base::kInvalidPlatformFileValue) {
On 2009/09/18 03:56:40, michaeln wrote:
> don't need the curly braces for a one-line block

chromium's style guidelines don't say anything about this, so i'm gonna keep
them, because:
1. google's c++ guidelines requires them.
2. i see both styles used extensively in webkit/tools/test_shell.
3. personally, i think the bracers-around-all-blocks is "better".

but please let me know if you feel strongly about this, and i'll remove them.

http://codereview.chromium.org/203074/diff/11008/12014#newcode77
Line 77: // we sleep for 100ms between retries for the same reason
On 2009/09/18 03:56:40, michaeln wrote:
> Whoa... 100ms, that seems like a mighty long time? I think we could dial down
> that number in database_dispather_host.cc and the number of attempts too.
> 
> 500ms is an eternity. Wow, thats what os_win.c has alright, although with
fewer
> attempts.
> 
> /*
> ** Close a file.
> **
> ** It is reported that an attempt to close a handle might sometimes
> ** fail.  This is a very unreasonable result, but windows is notorious
> ** for being unreasonable so I do not doubt that it might happen.  If
> ** the close fails, we pause for 100 milliseconds and try again.  As
> ** many as MX_CLOSE_ATTEMPT attempts to close the handle are made before
> ** giving up and returning an error.
> */
> 
>   #define MX_CLOSE_ATTEMPT 3
>   do{
>     rc = CloseHandle(pFile->h);
>   }while( rc==0 && cnt++ < MX_CLOSE_ATTEMPT && (Sleep(100), 1) );
> 
> 
> 

lowered the number of retries to 3 (to match SQLite's implementation) and
decreased the sleep time in test_shell to 10ms. i left it at 100ms in
database_dispatcher_host.cc.

http://codereview.chromium.org/203074/diff/11008/12014#newcode94
Line 94: FilePath(webkit_glue::WebStringToFilePathString(file_name)));
On 2009/09/18 03:56:40, michaeln wrote:
> Why we don't we need a full file path here?
> GetDBFileFullPath(file_name);

bug. fixed.

http://codereview.chromium.org/203074/diff/11008/12014#newcode100
Line 100: FilePath(webkit_glue::WebStringToFilePathString(file_name)));
On 2009/09/18 03:56:40, michaeln wrote:
> ditto

fixed.

http://codereview.chromium.org/203074/diff/11008/12014#newcode104
Line 104: file_util::Delete(GetDBDir(), true);
On 2009/09/18 03:56:40, michaeln wrote:
> Is this a correct implementation of this method?
> 
> In other ports, it looks like the LayoutTestController's clearAllDatabases()
> method is plumbed to DatabaseTracker::deleteAllDatabases(). A proper impl of
> that may have to wait until we have a representation of (Web)DatabaseTracker
in
> our WebKit api layer.
> 
> If that's right, maybe the thing to do now is...
> 
> void SimpleDatabaseSystem::ClearAllDatabases() {
>   // TODO(dumi): fill this in when we have WebDatabaseTracker shimware
>   // DatabaseTracker::tracker().deleteAllDatabases();
> }
> 
> 
> Here's what a different port does...
> 
> void LayoutTestController::clearAllDatabases()
> {
>     COMPtr<IWebDatabaseManager> databaseManager;
>     COMPtr<IWebDatabaseManager> tmpDatabaseManager;
>     if (FAILED(WebKitCreateInstance(CLSID_WebDatabaseManager, 0,
>                                     IID_IWebDatabaseManager,
>                                     (void**)&tmpDatabaseManager)))
>         return;
>     if
(FAILED(tmpDatabaseManager->sharedWebDatabaseManager(&databaseManager)))
>         return;
> 
>     databaseManager->deleteAllDatabases();
> }
> 
> HRESULT STDMETHODCALLTYPE WebDatabaseManager::deleteAllDatabases()
> {
>     if (this != s_sharedWebDatabaseManager)
>         return E_FAIL;
> 
>     DatabaseTracker::tracker().deleteAllDatabases();
> 
>     return S_OK;
> }
> 
> void DatabaseTracker::deleteAllDatabases()
> {
>     ASSERT(currentThread() == m_thread);
> 
>     Vector<RefPtr<SecurityOrigin> > originsCopy;
>     origins(originsCopy);
> 
>     for (unsigned i = 0; i < originsCopy.size(); ++i)
>         deleteOrigin(originsCopy[i].get());
> }

commented out the implementation and added a short comment.

http://codereview.chromium.org/203074/diff/11008/12014#newcode112
Line 112: const WebKit::WebString& file_name) {
On 2009/09/18 03:56:40, michaeln wrote:
> indentation is off

fixed.

http://codereview.chromium.org/203074/diff/11008/12011
File webkit/tools/test_shell/simple_database_system.h (right):

http://codereview.chromium.org/203074/diff/11008/12011#newcode10
Line 10: #include "webkit/api/public/WebString.h"
On 2009/09/18 03:56:40, michaeln wrote:
> Now that this class is one step removed from TestShellWebKitInit and its impl
of
> WebKitClient, we should be able to drop usage of WebKit api'isms. I think you
> can replace the WebString usages with FilePath here, and have
> TestShellWebKitInit do the type translations.

done.

http://codereview.chromium.org/203074/diff/11008/12011#newcode16
Line 16: virtual ~SimpleDatabaseSystem();
On 2009/09/18 03:56:40, michaeln wrote:
> why virtual?

in case anybody wants to extend this class. seems unlikely though, so i removed
'virtual'.

[michaeln, 2009-09-20 16:48:23]

Aa few #includes and comments to look at, otherwise LGTM.

http://codereview.chromium.org/203074/diff/9018/9032
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/9018/9032#newcode9
Line 9: #include "base/platform_file.h"
I think you just need forward declarations for FilePath and PlatformFile Instead
of #include file_path and platform_file.

http://codereview.chromium.org/203074/diff/9018/9030
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/9018/9030#newcode13
Line 13: #include "base/file_path.h"
i think this is in the .h file and the #include can be nix'd here. ditto
platform_file and scoped_temp_dir... please check for other dups too.

http://codereview.chromium.org/203074/diff/9018/9030#newcode71
Line 71: // we try to delete the file 5 times, because that's what the default
VFS does
5 files --> multiple times

http://codereview.chromium.org/203074/diff/9018/9030#newcode73
Line 73: // we sleep for 100ms between retries for the same reason
100ms disagrees with the 10ms in the source

http://codereview.chromium.org/203074/diff/9018/9020
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/9018/9020#newcode11
Line 11: #include "base/process_util.h"
can these includes be nixed now that the new code is encapsulated elsewhere?

[dumi, 2009-09-21 19:08:14]

http://codereview.chromium.org/203074/diff/9018/9032
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/9018/9032#newcode9
Line 9: #include "base/platform_file.h"
On 2009/09/20 16:48:23, michaeln wrote:
> I think you just need forward declarations for FilePath and PlatformFile
Instead
> of #include file_path and platform_file.

PlatformFile cannot be forward-declared, because it's a typedef. FilePath can.

http://codereview.chromium.org/203074/diff/9018/9030
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/9018/9030#newcode13
Line 13: #include "base/file_path.h"
On 2009/09/20 16:48:23, michaeln wrote:
> i think this is in the .h file and the #include can be nix'd here. ditto
> platform_file and scoped_temp_dir... please check for other dups too.

done.

http://codereview.chromium.org/203074/diff/9018/9030#newcode71
Line 71: // we try to delete the file 5 times, because that's what the default
VFS does
On 2009/09/20 16:48:23, michaeln wrote:
> 5 files --> multiple times

changed.

http://codereview.chromium.org/203074/diff/9018/9030#newcode73
Line 73: // we sleep for 100ms between retries for the same reason
On 2009/09/20 16:48:23, michaeln wrote:
> 100ms disagrees with the 10ms in the source

fixed (to 10ms).

http://codereview.chromium.org/203074/diff/9018/9020
File webkit/tools/test_shell/test_shell_webkit_init.h (right):

http://codereview.chromium.org/203074/diff/9018/9020#newcode11
Line 11: #include "base/process_util.h"
On 2009/09/20 16:48:23, michaeln wrote:
> can these includes be nixed now that the new code is encapsulated elsewhere?

done.

[michaeln, 2009-09-21 22:28:26]

lgtm!

[darin (slow to review), 2009-09-21 23:47:31]

http://codereview.chromium.org/203074/diff/17022/18014
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/17022/18014#newcode13
Line 13: namespace database {
the namespace should be webkit_database, right?  i see that webkit/appcache has
the namespace just as "appcache".  that kind of works since appcache is a pretty
particular name, but "database" as a namespace name seems like it could conflict
with other modules.  also, the "glue" directory uses the namespace webkit_glue.

http://codereview.chromium.org/203074/diff/17022/18014#newcode15
Line 15: void VfsBackendOpenFile(const FilePath& file_name,
how about a class named VfsBackend that contains static methods?  that way
someone could do 'using webkit_database::VfsBackend'.

http://codereview.chromium.org/203074/diff/17022/18010
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/203074/diff/17022/18010#newcode972
Line 972: result->SetNull();
nit: use google style for indentation and brackets

http://codereview.chromium.org/203074/diff/17022/18012
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/17022/18012#newcode72
Line 72: int numRetries = 0;
nit: num_retries

http://codereview.chromium.org/203074/diff/17022/18009
File webkit/tools/test_shell/simple_database_system.h (right):

http://codereview.chromium.org/203074/diff/17022/18009#newcode18
Line 18: base::PlatformFile DatabaseOpenFile(
why bother prefixing these methods with "Database" when they are already
contained within a class that has "Database" in its name?

http://codereview.chromium.org/203074/diff/17022/18009#newcode34
Line 34: FilePath GetDBDir();
nit: please list private methods above private data members

[dumi, 2009-09-22 02:30:47]

http://codereview.chromium.org/203074/diff/17022/18014
File webkit/database/vfs_backend.h (right):

http://codereview.chromium.org/203074/diff/17022/18014#newcode13
Line 13: namespace database {
On 2009/09/21 23:47:31, darin wrote:
> the namespace should be webkit_database, right?  i see that webkit/appcache
has
> the namespace just as "appcache".  that kind of works since appcache is a
pretty
> particular name, but "database" as a namespace name seems like it could
conflict
> with other modules.  also, the "glue" directory uses the namespace
webkit_glue.

fixed.

http://codereview.chromium.org/203074/diff/17022/18014#newcode15
Line 15: void VfsBackendOpenFile(const FilePath& file_name,
On 2009/09/21 23:47:31, darin wrote:
> how about a class named VfsBackend that contains static methods?  that way
> someone could do 'using webkit_database::VfsBackend'.

done.

http://codereview.chromium.org/203074/diff/17022/18010
File webkit/tools/test_shell/layout_test_controller.cc (right):

http://codereview.chromium.org/203074/diff/17022/18010#newcode972
Line 972: result->SetNull();
On 2009/09/21 23:47:31, darin wrote:
> nit: use google style for indentation and brackets

done. fixed the bracket in the function above this one too.

http://codereview.chromium.org/203074/diff/17022/18012
File webkit/tools/test_shell/simple_database_system.cc (right):

http://codereview.chromium.org/203074/diff/17022/18012#newcode72
Line 72: int numRetries = 0;
On 2009/09/21 23:47:31, darin wrote:
> nit: num_retries

fixed.

http://codereview.chromium.org/203074/diff/17022/18009
File webkit/tools/test_shell/simple_database_system.h (right):

http://codereview.chromium.org/203074/diff/17022/18009#newcode18
Line 18: base::PlatformFile DatabaseOpenFile(
On 2009/09/21 23:47:31, darin wrote:
> why bother prefixing these methods with "Database" when they are already
> contained within a class that has "Database" in its name?

fixed.

http://codereview.chromium.org/203074/diff/17022/18009#newcode34
Line 34: FilePath GetDBDir();
On 2009/09/21 23:47:31, darin wrote:
> nit: please list private methods above private data members

fixed.

[darin (slow to review), 2009-09-22 04:03:54]

LGTM!