database_dispatcher_host changes.

Adding support for DatabaseTracker messages to
DatabaseDispatcherHost. This code will get called as soon as we switch
from WebCore's DatabaseTracker implementation to Chromium's. Also,
cleaned up the pre-existing support for VFS messages.

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31507

[michaeln, 2009-11-03 03:47:11]

I haven't looked closely yet, but here's one thing that caught my eye... jam
recently introduced an all-new-and-improved method of posting tasks across
well-known ChromeThreads...

   ChromeThread::PostTask(
      ChromeThread::IO, FROM_HERE,
      NewRunnableMethod(
          this, &ResourceMessageFilter::SendDelayedReply, reply_msg));

... which helps resolve some sticky bugs at shutdown time.

I think this new code should be written to use that new method.

[michaeln, 2009-11-03 03:53:08]

http://codereview.chromium.org/340067/diff/3013/3020
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/340067/diff/3013/3020#newcode95
Line 95: scoped_refptr<ResourceMessageFilter> resource_message_filter_;
Does this cause a reference cycle?

~RMF calls DDH->Shutdown(), but how can that ever happen if DDH holds a
reference to RMF?

[michaeln, 2009-11-05 04:09:05]

this is looking much better... i haven't looked at things real close yet but i
did see some superficial style things (indents are off in several places in
DDH.cc), and there are a few lint errors to look at too.

http://codereview.chromium.org/340067/diff/7001/8008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/7001/8008#newcode115
Line 115: const IPC::Message& message, bool* message_was_ok) {
indent is off

[michaeln, 2009-11-05 17:09:14]

http://codereview.chromium.org/340067/diff/7001/8008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/7001/8008#newcode42
Line 42: shutdown_(false) {
Some DCHECKs would be nice...
DCHECK(db_tracker_ && message_sender_);

http://codereview.chromium.org/340067/diff/7001/8008#newcode45
Line 45: NewRunnableMethod(this, &DatabaseDispatcherHost::AddObserver));
Maybe we should defer AddObserver until the first call OnDatabaseOpenFile. We'd
need a flag to remember if we're added or not, but that's cheap enough. Wdyt?

http://codereview.chromium.org/340067/diff/7001/8008#newcode49
Line 49: shutdown_ = true;
maybe clear out message_sender_ too so we don't have a dangling ptr

http://codereview.chromium.org/340067/diff/7001/8008#newcode67
Line 67: // 'origin_identifier/database_name'. Get the origin identifier and the
This comment is not entirely accurate. The 'file_name' can be either...

a) an <origin_indentifier/database_name> tuple, when referring to a 'main'
database file

b) an <origin_indentifier/database_name><sqlitespecific_extension>, when
referring to a 'journal file (and perhaps other anclillary files used by
sqlite?)

c) the emtpy string when attempting to open a temp file (so i think the empty
value is only valid for the OnOpenDatabaseFile processing, is that right?)

Most of my comments are around how this 'file_name' string token is interpreted
and how the code is structured to do that. It'll be easier to walk thru this
in-person rather than in line-by-line code review comments... lets do that
today.

http://codereview.chromium.org/340067/diff/7001/8008#newcode95
Line 95: return db_tracker_->GetFullDBFilePath(
Since we're calling the 'tracker' here, I'd rather see this method only used on
the FILE thread. That way if(when) we add a layer if indirection between the
webapp developer provided database_name and the actual file name on disk, the
tracker will only have to do the lookups on the background thread.

I think that may actually make some of the other methods smaller too.

http://codereview.chromium.org/340067/diff/7001/8008#newcode117
Line 117: if (shutdown_)
Can this actually get called after shutdown? If not, maybe...
  DCHECK(!shutdown_);

http://codereview.chromium.org/340067/diff/7001/8008#newcode154
Line 154: FilePath db_file_name = GetDBFileFullPath(file_name);
make this call on the FILE thread in DatabaseOpenFile instead

http://codereview.chromium.org/340067/diff/7001/8008#newcode160
Line 160: #elif defined(OS_POSIX)
for readablility, consider packaging the platform specifics into a helper, the
#ifdef'd blocks are a large fraction of these otherwise tiny method bodies...

static void SetOpenFileResponseParams(
    ViewMsg_DatabaseOpenFileResposne_Params* params,
    base::PlatformFile file_handle,
    base::PlatformFile directory_handle) {
#if defined(OS_WIN)
  ...
#elif defined(OS_POSIX) 
  ...
#endif
}

http://codereview.chromium.org/340067/diff/7001/8008#newcode211
Line 211: const FilePath& file_name, const bool& sync_dir, int32 message_id) {
indent

http://codereview.chromium.org/340067/diff/7001/8008#newcode212
Line 212: FilePath db_file_name = GetDBFileFullPath(file_name);
make this call on the FILE thread in DatabaseDeleteFile instead

http://codereview.chromium.org/340067/diff/7001/8008#newcode278
Line 278: const FilePath& file_name, int32 message_id) {
indent

http://codereview.chromium.org/340067/diff/7001/8008#newcode279
Line 279: FilePath db_file_name = GetDBFileFullPath(file_name);
make this call on the FILE thread in DatabaseGetFileAttributes instead

http://codereview.chromium.org/340067/diff/7001/8008#newcode311
Line 311: const FilePath& file_name, int32 message_id) {
indent

http://codereview.chromium.org/340067/diff/7001/8008#newcode312
Line 312: FilePath db_file_name = GetDBFileFullPath(file_name);
make this call on the FILE thread in DatabaseGetFileSize instead

[michaeln, 2009-11-05 17:44:38]

Also, can you refresh my memory... are database_names case insensitive or case
sensitive per the WebDatabases spec?

[dumi, 2009-11-05 23:59:16]

the DB names are case-sensitive, i double-checked.

haven't looked at the lint errors yet -- i'll fix them all in the next upload.

http://codereview.chromium.org/340067/diff/7001/8008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/7001/8008#newcode42
Line 42: shutdown_(false) {
On 2009/11/05 17:09:14, michaeln wrote:
> Some DCHECKs would be nice...
> DCHECK(db_tracker_ && message_sender_);

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode45
Line 45: NewRunnableMethod(this, &DatabaseDispatcherHost::AddObserver));
On 2009/11/05 17:09:14, michaeln wrote:
> Maybe we should defer AddObserver until the first call OnDatabaseOpenFile.
We'd
> need a flag to remember if we're added or not, but that's cheap enough. Wdyt?

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode49
Line 49: shutdown_ = true;
On 2009/11/05 17:09:14, michaeln wrote:
> maybe clear out message_sender_ too so we don't have a dangling ptr

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode67
Line 67: // 'origin_identifier/database_name'. Get the origin identifier and the
On 2009/11/05 17:09:14, michaeln wrote:
> This comment is not entirely accurate. The 'file_name' can be either...
> 
> a) an <origin_indentifier/database_name> tuple, when referring to a 'main'
> database file
> 
> b) an <origin_indentifier/database_name><sqlitespecific_extension>, when
> referring to a 'journal file (and perhaps other anclillary files used by
> sqlite?)
> 
> c) the emtpy string when attempting to open a temp file (so i think the empty
> value is only valid for the OnOpenDatabaseFile processing, is that right?)
> 
> Most of my comments are around how this 'file_name' string token is
interpreted
> and how the code is structured to do that. It'll be easier to walk thru this
> in-person rather than in line-by-line code review comments... lets do that
> today.

ok.

http://codereview.chromium.org/340067/diff/7001/8008#newcode115
Line 115: const IPC::Message& message, bool* message_was_ok) {
On 2009/11/05 04:09:05, michaeln wrote:
> indent is off

fixed.

http://codereview.chromium.org/340067/diff/7001/8008#newcode117
Line 117: if (shutdown_)
On 2009/11/05 17:09:14, michaeln wrote:
> Can this actually get called after shutdown? If not, maybe...
>   DCHECK(!shutdown_);

hmm, probably not. replaced with a DCHECK.

http://codereview.chromium.org/340067/diff/7001/8008#newcode154
Line 154: FilePath db_file_name = GetDBFileFullPath(file_name);
On 2009/11/05 17:09:14, michaeln wrote:
> make this call on the FILE thread in DatabaseOpenFile instead

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode160
Line 160: #elif defined(OS_POSIX)
On 2009/11/05 17:09:14, michaeln wrote:
> for readablility, consider packaging the platform specifics into a helper, the
> #ifdef'd blocks are a large fraction of these otherwise tiny method bodies...
> 
> static void SetOpenFileResponseParams(
>     ViewMsg_DatabaseOpenFileResposne_Params* params,
>     base::PlatformFile file_handle,
>     base::PlatformFile directory_handle) {
> #if defined(OS_WIN)
>   ...
> #elif defined(OS_POSIX) 
>   ...
> #endif
> }

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode211
Line 211: const FilePath& file_name, const bool& sync_dir, int32 message_id) {
On 2009/11/05 17:09:14, michaeln wrote:
> indent

fixed.

http://codereview.chromium.org/340067/diff/7001/8008#newcode212
Line 212: FilePath db_file_name = GetDBFileFullPath(file_name);
On 2009/11/05 17:09:14, michaeln wrote:
> make this call on the FILE thread in DatabaseDeleteFile instead

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode278
Line 278: const FilePath& file_name, int32 message_id) {
On 2009/11/05 17:09:14, michaeln wrote:
> indent

fixed.

http://codereview.chromium.org/340067/diff/7001/8008#newcode279
Line 279: FilePath db_file_name = GetDBFileFullPath(file_name);
On 2009/11/05 17:09:14, michaeln wrote:
> make this call on the FILE thread in DatabaseGetFileAttributes instead

done.

http://codereview.chromium.org/340067/diff/7001/8008#newcode311
Line 311: const FilePath& file_name, int32 message_id) {
On 2009/11/05 17:09:14, michaeln wrote:
> indent

fixed.

http://codereview.chromium.org/340067/diff/7001/8008#newcode312
Line 312: FilePath db_file_name = GetDBFileFullPath(file_name);
On 2009/11/05 17:09:14, michaeln wrote:
> make this call on the FILE thread in DatabaseGetFileSize instead

done.

[michaeln, 2009-11-06 02:37:01]

http://codereview.chromium.org/340067/diff/12001/13008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/12001/13008#newcode224
Line 224: FilePath db_file_name = GetDBFileFullPath(file_name);
if db_file_name.empty() we can return immediately w/o retrying i think

http://codereview.chromium.org/340067/diff/12001/13008#newcode270
Line 270: int32 attributes = -1;
is there a meaningful constant value we can use for the -1 here?

http://codereview.chromium.org/340067/diff/13016/12006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/340067/diff/13016/12006#newcode202
Line 202: db_tracker_(new webkit_database::DatabaseTracker(GetPath())) {
Would it make sense to defer constructuction until GetDatabaseTracker() is
called?

Also, i'd be more comfortable with passing in an empty FilePath() instead of the
path to the 'real' profile data.

http://codereview.chromium.org/340067/diff/13016/12006#newcode583
Line 583: db_tracker_(new webkit_database::DatabaseTracker(GetPath())) {
Ditto deferred construction?

http://codereview.chromium.org/340067/diff/13016/12010
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/13016/12010#newcode8
Line 8: #include <windows.h>
do you still really need this?

http://codereview.chromium.org/340067/diff/13016/12010#newcode50
Line 50: message_sender_ = NULL;
i think you only need to post the task 

if (observer_added_) {
}

... i'm not sure if ObserverList<T> will complain about removing an observe that
wasn't added, but its easy enough to not do it here.

http://codereview.chromium.org/340067/diff/13016/12010#newcode66
Line 66: FilePath DatabaseDispatcherHost::GetDBFileFullPath(const FilePath&
file_name) {
vfs_file_name (i think)

also, comments about the format of that value are good, its just that the
comments that were here were not entirely accurate

http://codereview.chromium.org/340067/diff/13016/12011
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/340067/diff/13016/12011#newcode26
Line 26: bool OnMessageReceived(const IPC::Message& message, bool*
message_was_ok);
Like we talked about, comments to indicate which methods are from which side of
the world...

// VFS related IPC message handlers.

http://codereview.chromium.org/340067/diff/13016/12011#newcode27
Line 27: void OnDatabaseOpenFile(const FilePath& file_name,
file_name --> vfs_file_name

http://codereview.chromium.org/340067/diff/13016/12011#newcode30
Line 30: void OnDatabaseDeleteFile(const FilePath& file_name,
vfs_file_name

http://codereview.chromium.org/340067/diff/13016/12011#newcode33
Line 33: void OnDatabaseGetFileAttributes(const FilePath& file_name,
vfs_file_name

http://codereview.chromium.org/340067/diff/13016/12011#newcode35
Line 35: void OnDatabaseGetFileSize(const FilePath& file_name,
vfs_file_name

http://codereview.chromium.org/340067/diff/13016/12011#newcode36
Line 36: int32 message_id);
// Tracker related IPC message handlers.

http://codereview.chromium.org/340067/diff/13016/12011#newcode44
Line 44: const string16& database_name);
// DatabaseTracker::Observer implementation

http://codereview.chromium.org/340067/diff/13016/12011#newcode60
Line 60: void DatabaseOpenFile(const FilePath& file_name,
ditto vfs_file_name convention where applicable in the private section

[michaeln, 2009-11-06 02:46:19]

http://codereview.chromium.org/340067/diff/14013/15011
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/340067/diff/14013/15011#newcode86
Line 86: db_tracker_ = new webkit_database::DatabaseTracker(path_);
maybe construct it after the delete/create sequence below, or deferred?

[dumi, 2009-11-06 19:51:05]

http://codereview.chromium.org/340067/diff/12001/13008
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/12001/13008#newcode224
Line 224: FilePath db_file_name = GetDBFileFullPath(file_name);
On 2009/11/06 02:37:01, michaeln wrote:
> if db_file_name.empty() we can return immediately w/o retrying i think

done.

http://codereview.chromium.org/340067/diff/12001/13008#newcode270
Line 270: int32 attributes = -1;
On 2009/11/06 02:37:01, michaeln wrote:
> is there a meaningful constant value we can use for the -1 here?

there's INVALID_FILE_ATTRIBUTES on windows, but i couldn't find anything on
linux/mac. also, i couldn't find anything platform-independent in base/*; in
fact, file_util_win.cc uses INVALID_FILE_ATTRIBUTES, which makes me think we
don't have such a constant defined anywhere.

http://codereview.chromium.org/340067/diff/13016/12006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/340067/diff/13016/12006#newcode202
Line 202: db_tracker_(new webkit_database::DatabaseTracker(GetPath())) {
On 2009/11/06 02:37:01, michaeln wrote:
> Would it make sense to defer constructuction until GetDatabaseTracker() is
> called?

done.

> Also, i'd be more comfortable with passing in an empty FilePath() instead of
the
> path to the 'real' profile data.

done.

http://codereview.chromium.org/340067/diff/13016/12006#newcode583
Line 583: db_tracker_(new webkit_database::DatabaseTracker(GetPath())) {
On 2009/11/06 02:37:01, michaeln wrote:
> Ditto deferred construction?

done.

http://codereview.chromium.org/340067/diff/13016/12010
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/13016/12010#newcode8
Line 8: #include <windows.h>
On 2009/11/06 02:37:01, michaeln wrote:
> do you still really need this?

removed.

http://codereview.chromium.org/340067/diff/13016/12010#newcode50
Line 50: message_sender_ = NULL;
On 2009/11/06 02:37:01, michaeln wrote:
> i think you only need to post the task 
> 
> if (observer_added_) {
> }
> 
> ... i'm not sure if ObserverList<T> will complain about removing an observe
that
> wasn't added, but its easy enough to not do it here.

done.

http://codereview.chromium.org/340067/diff/13016/12010#newcode66
Line 66: FilePath DatabaseDispatcherHost::GetDBFileFullPath(const FilePath&
file_name) {
On 2009/11/06 02:37:01, michaeln wrote:
> vfs_file_name (i think)
> 
> also, comments about the format of that value are good, its just that the
> comments that were here were not entirely accurate

done.

http://codereview.chromium.org/340067/diff/13016/12011
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/340067/diff/13016/12011#newcode26
Line 26: bool OnMessageReceived(const IPC::Message& message, bool*
message_was_ok);
On 2009/11/06 02:37:01, michaeln wrote:
> Like we talked about, comments to indicate which methods are from which side
of
> the world...
> 
> // VFS related IPC message handlers.

added.

http://codereview.chromium.org/340067/diff/13016/12011#newcode27
Line 27: void OnDatabaseOpenFile(const FilePath& file_name,
On 2009/11/06 02:37:01, michaeln wrote:
> file_name --> vfs_file_name

done.

http://codereview.chromium.org/340067/diff/13016/12011#newcode30
Line 30: void OnDatabaseDeleteFile(const FilePath& file_name,
On 2009/11/06 02:37:01, michaeln wrote:
> vfs_file_name

done.

http://codereview.chromium.org/340067/diff/13016/12011#newcode33
Line 33: void OnDatabaseGetFileAttributes(const FilePath& file_name,
On 2009/11/06 02:37:01, michaeln wrote:
> vfs_file_name

done.

http://codereview.chromium.org/340067/diff/13016/12011#newcode35
Line 35: void OnDatabaseGetFileSize(const FilePath& file_name,
On 2009/11/06 02:37:01, michaeln wrote:
> vfs_file_name

done.

http://codereview.chromium.org/340067/diff/13016/12011#newcode36
Line 36: int32 message_id);
On 2009/11/06 02:37:01, michaeln wrote:
> // Tracker related IPC message handlers.

done.

http://codereview.chromium.org/340067/diff/13016/12011#newcode60
Line 60: void DatabaseOpenFile(const FilePath& file_name,
On 2009/11/06 02:37:01, michaeln wrote:
> ditto vfs_file_name convention where applicable in the private section

done.

http://codereview.chromium.org/340067/diff/14013/15011
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/340067/diff/14013/15011#newcode86
Line 86: db_tracker_ = new webkit_database::DatabaseTracker(path_);
On 2009/11/06 02:46:20, michaeln wrote:
> maybe construct it after the delete/create sequence below, or deferred?

done.

[michaeln, 2009-11-06 20:11:31]

almost...

http://codereview.chromium.org/340067/diff/14021/15035
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/340067/diff/14021/15035#newcode203
Line 203: db_tracker_(NULL) {
The default ctor will do this for you. I think you can remove this from the
initializer list.

http://codereview.chromium.org/340067/diff/14021/15035#newcode590
Line 590: db_tracker_(NULL) {
Ditto default ctor

http://codereview.chromium.org/340067/diff/15053/14032
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/340067/diff/15053/14032#newcode23
Line 23: ~DatabaseDispatcherHost() {}
Looks like you can remove this empty dtor.

http://codereview.chromium.org/340067/diff/15053/14024
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/340067/diff/15053/14024#newcode86
Line 86: db_tracker_ = NULL;
if this is a scoped_refptr, the default ctor does this

http://codereview.chromium.org/340067/diff/15053/14025
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/340067/diff/15053/14025#newcode81
Line 81: return db_tracker_;
is this what you intended?

[dumi, 2009-11-06 20:27:27]

http://codereview.chromium.org/340067/diff/14021/15035
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/340067/diff/14021/15035#newcode203
Line 203: db_tracker_(NULL) {
On 2009/11/06 20:11:31, michaeln wrote:
> The default ctor will do this for you. I think you can remove this from the
> initializer list.

removed.

http://codereview.chromium.org/340067/diff/14021/15035#newcode590
Line 590: db_tracker_(NULL) {
On 2009/11/06 20:11:31, michaeln wrote:
> Ditto default ctor

removed.

http://codereview.chromium.org/340067/diff/15053/14032
File chrome/browser/renderer_host/database_dispatcher_host.h (right):

http://codereview.chromium.org/340067/diff/15053/14032#newcode23
Line 23: ~DatabaseDispatcherHost() {}
On 2009/11/06 20:11:31, michaeln wrote:
> Looks like you can remove this empty dtor.

done.

http://codereview.chromium.org/340067/diff/15053/14024
File chrome/test/testing_profile.cc (right):

http://codereview.chromium.org/340067/diff/15053/14024#newcode86
Line 86: db_tracker_ = NULL;
On 2009/11/06 20:11:31, michaeln wrote:
> if this is a scoped_refptr, the default ctor does this

removed.

http://codereview.chromium.org/340067/diff/15053/14025
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/340067/diff/15053/14025#newcode81
Line 81: return db_tracker_;
On 2009/11/06 20:11:31, michaeln wrote:
> is this what you intended?

no, accidentally removed a relevant line. fixed.

[michaeln, 2009-11-07 17:55:12]

http://codereview.chromium.org/340067/diff/13023/12034
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/340067/diff/13023/12034#newcode545
Line 545: (!profile()->IsOffTheRecord()))) {
This change to disable databases for OTR could go in a seperate CL. Testing for
for equality in the loop is kind of ugly. Some other way to structure this
code/data would be good.

// Propagate the following switches to the renderer command line (along
// with any associated values) if present in the browser command line.
static const char* const switch_names[] = {
  ...
};

// Additional switches not propagated to OTR renderers.
static const char* const not_otr_switch_names[] = {
  switches::kEnableDatabases,
};


for (size_t i = 0; i < arraysize(switch_names); ++i) {
  if (browser_cmd.HasSwitch(switch_names[i])) {
  }
}

if (!profile()->IsOffTheRecord()) {
  for (size_t i = 0; i < arraysize(not_otr_switch_names); ++i) {
    if (browser_cmd.HasSwitch(not_otr_switch_names[i])) {
    }
  }
}

http://codereview.chromium.org/340067/diff/13023/12029
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/13023/12029#newcode42
Line 42: OnDatabaseOpened(ASCIIToUTF16("blah"), ASCIIToUTF16("blah"),
ASCIIToUTF16("blah"), 0);
intentional or accidental?

http://codereview.chromium.org/340067/diff/13023/12029#newcode231
Line 231: VfsBackend::DeleteFile(db_file_name, db_dir, sync_dir);
this looks like it might fit on one line?

[michaeln, 2009-11-07 19:19:54]

Also please update the CL description to indicate...

* adding support for DatabaseTracker messages, but still not reached at runtime
since the webkit patch(es) to allow chrome to replace webcore's DatabaseTracker
are not yet committed

* cleaning up pre-existing support for VFS messages

[dumi, 2009-11-09 20:08:52]

changed the CL description.

http://codereview.chromium.org/340067/diff/13023/12034
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/340067/diff/13023/12034#newcode545
Line 545: (!profile()->IsOffTheRecord()))) {
On 2009/11/07 17:55:12, michaeln wrote:
> This change to disable databases for OTR could go in a seperate CL. Testing
for
> for equality in the loop is kind of ugly. Some other way to structure this
> code/data would be good.
> 
> // Propagate the following switches to the renderer command line (along
> // with any associated values) if present in the browser command line.
> static const char* const switch_names[] = {
>   ...
> };
> 
> // Additional switches not propagated to OTR renderers.
> static const char* const not_otr_switch_names[] = {
>   switches::kEnableDatabases,
> };
> 
> 
> for (size_t i = 0; i < arraysize(switch_names); ++i) {
>   if (browser_cmd.HasSwitch(switch_names[i])) {
>   }
> }
> 
> if (!profile()->IsOffTheRecord()) {
>   for (size_t i = 0; i < arraysize(not_otr_switch_names); ++i) {
>     if (browser_cmd.HasSwitch(not_otr_switch_names[i])) {
>     }
>   }
> }

done. i was looking for something like that, but john said it was fine to do
what i did. still, i think your solution is better.

http://codereview.chromium.org/340067/diff/13023/12029
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/340067/diff/13023/12029#newcode42
Line 42: OnDatabaseOpened(ASCIIToUTF16("blah"), ASCIIToUTF16("blah"),
ASCIIToUTF16("blah"), 0);
On 2009/11/07 17:55:12, michaeln wrote:
> intentional or accidental?

accidental. added it here to test some code. removed.

http://codereview.chromium.org/340067/diff/13023/12029#newcode231
Line 231: VfsBackend::DeleteFile(db_file_name, db_dir, sync_dir);
On 2009/11/07 17:55:12, michaeln wrote:
> this looks like it might fit on one line?

done.

[michaeln, 2009-11-09 20:38:59]

LGTM