Refactored app cache clear on exit code to happen in the object owning the files.

Refactored app cache clear on exit code to happen in the object owning the files.

In an effort to remove the static functions that used to be called from the
BrowserProcessImpl very late in the shutdown process and move the code to
immediately after the files get freed from their respective users. Which
will help to parallelize the shutdown sequence better and ensure files are
deleted when they are not used anymore.

BUG=65076
TEST=TBA

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

[pastarmovj, 2010-12-22 17:23:23]

@* :
This the change proposed to move the code deleting the appcache files as soon as
the AppCacheService stops existing. 

@michaeln :
This CL deals with the AppCacheDatabase object of the AppCacheStore class only.
I have a second CL - http://codereview.chromium.org/6011003/ which extends this
to the AppCacheDiskCache too. However I am not sure I fully understand if I need
to explicitly care for this one and how it is used and when? 
Can you please have a look at this CL as well as the one I linked to tell me if
the Database delete code is enough or the other one should be committed too? It
will be great also if you could give me a short overview of what the disk cache
exactly does? :)

[pastarmovj, 2011-01-04 13:38:40]

Moved the clean up code to ChromeAppCacheService.

[jochen (gone - plz use gerrit), 2011-01-04 14:32:53]

please add unit tests

http://codereview.chromium.org/6077005/diff/3001/chrome/browser/appcache/chro...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/3001/chrome/browser/appcache/chro...
chrome/browser/appcache/chrome_appcache_service.cc:51:
file_util::Delete(cache_path_, true);
this should happen on the FILE thread

http://codereview.chromium.org/6077005/diff/3001/chrome/browser/appcache/chro...
File chrome/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/6077005/diff/3001/chrome/browser/appcache/chro...
chrome/browser/appcache/chrome_appcache_service.h:68: bool
clear_local_state_on_exit_;
this is accessed on both IO and UI threads, so you'll need a lock

[michaeln, 2011-01-04 23:23:14]

> @michaeln :
> This CL deals with the AppCacheDatabase object of the AppCacheStore class
only.
> I have a second CL - http://codereview.chromium.org/6011003/ which extends
this
> to the AppCacheDiskCache too. However I am not sure I fully understand if I
need
> to explicitly care for this one and how it is used and when? 
> Can you please have a look at this CL as well as the one I linked to tell me
if
> the Database delete code is enough or the other one should be committed too?
It
> will be great also if you could give me a short overview of what the disk
cache
> exactly does? :)

This CL looks like it will delete the directory containing all the bits on disk,
the sqlite database file and the disk cache files alike, so I don't think the
another CL should be needed. The structures in the file system looks like
this...
<profiledir>/Application Cache/
   Index   // this is the sqlite database file
   Cache   // this is a directory containing the disk cache files
The sqlite database contains structured representations of 'manifest' files. The
disk cache contains the http responses (headers and bodies).

Your CL is deleting the "Application Cache" directory which should nuke it all.

The trick will be to call file_util::Delete sometime after the files within that
directory have been closed. I don't think this CL guarantees that as currently
written since the task is being posted in the derived class dtor prior to the
base class dtor even running.

[michaeln, 2011-01-05 00:59:13]

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:53:
NewRunnableFunction(file_util::Delete, cache_path_, true));
I think if this task was posted to the DB thread from the base class's dtor
(after having reset the storage_ data member to NULL), then you'd be guaranteed
that all of the files within the directory had been closed.

AppCacheStorageImpl holds ptrs to the AppCacheDiskCache and the
AppCacheDatabase. The disk cache closes all file prior to return from it's dtor.
The database deletion occurs on the DB thread some time after
AppCacheStorageImpl has been deleted (DeleteSoon is used on the AppCacheDatabase
instance).

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:70: AutoLock lock(lock_);
Instead of a lock, it might be better to post a task to set the flag on the IO
thread. As currently coded, you have a race condition around this flag. What if
the pref is changed after the 'service' was constructed but before
InitializeOnIOThread has been run.

This is a fairly common pattern to avoid locks.

void ChromeAppCacheService::SetClearLocalStateOnExit(bool clear_local_state) {
  if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
    PostTask(SetClearLocalStateOnExit, clear_local_state);
    return;
  }
  clear_local_state_on_exit_ = clear_local_state;
}

[pastarmovj, 2011-01-05 10:47:06]

Addressed the comments.
Linux/Linux_chromeos bots are red because of some problem with the compilation
in general and not because of the CL itself.

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:53:
NewRunnableFunction(file_util::Delete, cache_path_, true));
On 2011/01/05 00:59:14, michaeln wrote:
> I think if this task was posted to the DB thread from the base class's dtor
> (after having reset the storage_ data member to NULL), then you'd be
guaranteed
> that all of the files within the directory had been closed.
> 
> AppCacheStorageImpl holds ptrs to the AppCacheDiskCache and the
> AppCacheDatabase. The disk cache closes all file prior to return from it's
dtor.
> The database deletion occurs on the DB thread some time after
> AppCacheStorageImpl has been deleted (DeleteSoon is used on the
AppCacheDatabase
> instance).
> 

After discussing this with Jochen, we decided that it is still better to keep
this clean up code in the ChromeAppCacheService class as it is Chrome specific.
However to ensure the code is only run after the destructor of the base class
has been run we post a task to the IO thread to run the actual deletion on the
DB thread. This will ensure it is run after the db has been closed. 

However if you prefer I will move this step to the base class.

The original code in the first revision of this CL went even further and deleted
the file in the actual database class. This is the safest solution as deletion
is perfectly synchronized with the rest of the db handling code but as you can
see from the size of the commit it required much bigger intervention on the
code. This also answers your question in the previous comment why am I asking
for the structure of the appcache and the need of the other commit that deleted
the file cache separately.

http://codereview.chromium.org/6077005/diff/10001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:70: AutoLock lock(lock_);
On 2011/01/05 00:59:14, michaeln wrote:
> Instead of a lock, it might be better to post a task to set the flag on the IO
> thread. As currently coded, you have a race condition around this flag. What
if
> the pref is changed after the 'service' was constructed but before
> InitializeOnIOThread has been run.
> 
> This is a fairly common pattern to avoid locks.
> 
> void ChromeAppCacheService::SetClearLocalStateOnExit(bool clear_local_state) {
>   if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
>     PostTask(SetClearLocalStateOnExit, clear_local_state);
>     return;
>   }
>   clear_local_state_on_exit_ = clear_local_state;
> }

Done.

[jochen (gone - plz use gerrit), 2011-01-05 10:55:44]

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
nit. it's 2011

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.h:68: };
any reason this class is not disallow copy and assign?

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:9: #include
"base/stl_util-inl.h"
why do you include this header?

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:10: #include
"base/time.h"
and this?

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:12: #include
"chrome/browser/appcache/chrome_appcache_service.h"
alphabetical ordering

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:15: #include
"googleurl/src/gurl.h"
isn't used either

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:18: class
ChromeAppCacheServiceTest : public testing::Test {
can you move everything into an anonymous namespace?

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:47:
temp_dir_.path().Append(chrome::kAppCacheDirname)));
can you put this into a variable instead of recreating this path several times?

[pastarmovj, 2011-01-05 12:15:13]

Fixed the style issues and advices.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.h:1: // Copyright (c) 2010 The
Chromium Authors. All rights reserved.
On 2011/01/05 10:55:45, jochen wrote:
> nit. it's 2011

Done.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.h:68: };
On 2011/01/05 10:55:45, jochen wrote:
> any reason this class is not disallow copy and assign?

Done.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:9: #include
"base/stl_util-inl.h"
On 2011/01/05 10:55:45, jochen wrote:
> why do you include this header?

Relict from the test I used as template for this file.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:10: #include
"base/time.h"
On 2011/01/05 10:55:45, jochen wrote:
> and this?

This too.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:12: #include
"chrome/browser/appcache/chrome_appcache_service.h"
On 2011/01/05 10:55:45, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:15: #include
"googleurl/src/gurl.h"
On 2011/01/05 10:55:45, jochen wrote:
> isn't used either

Done.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:18: class
ChromeAppCacheServiceTest : public testing::Test {
On 2011/01/05 10:55:45, jochen wrote:
> can you move everything into an anonymous namespace?

Done.

http://codereview.chromium.org/6077005/diff/16001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:47:
temp_dir_.path().Append(chrome::kAppCacheDirname)));
On 2011/01/05 10:55:45, jochen wrote:
> can you put this into a variable instead of recreating this path several
times?

Done.

[jochen (gone - plz use gerrit), 2011-01-05 12:29:51]

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:64: if
(clear_local_state_on_exit_ && !cache_path_.empty())
use { ... }.

also, please add an comment why this is neccessary

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:25: protected:
why protected and not private?

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:68: // Create some
fake cache index.
duplicate line

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
2011 plz

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1221: if (appcache_service_)
use { ... }

[pastarmovj, 2011-01-05 13:47:37]

Fixed nits.

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:64: if
(clear_local_state_on_exit_ && !cache_path_.empty())
On 2011/01/05 12:29:51, jochen wrote:
> use { ... }.
> 
> also, please add an comment why this is neccessary

Done.

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:25: protected:
On 2011/01/05 12:29:51, jochen wrote:
> why protected and not private?

At least message_loop_ and temp_dir_ are used in tests so they need to be
protected the rest I moved to private section.

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:68: // Create some
fake cache index.
On 2011/01/05 12:29:51, jochen wrote:
> duplicate line

Done.

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service_unittest.cc:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
On 2011/01/05 12:29:51, jochen wrote:
> 2011 plz

Done.

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6077005/diff/22001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1221: if (appcache_service_)
On 2011/01/05 12:29:51, jochen wrote:
> use { ... }

Done.

[pastarmovj, 2011-01-05 14:10:36]

Fixed the last file with date 2010 in the copyright comment. I thought it is
based on last real edit not commit. :)

[michaeln, 2011-01-05 21:03:05]

http://codereview.chromium.org/6077005/diff/34001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/34001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:58:
TEST_F(ChromeAppCacheServiceTest, RemoveOnDestruction) {
The ChromeAppCacheService code looks great, I like keeping this at the /chrome
level too. But I wonder if the test can be made better such that it actually
tests the full shutdown sequence including the closing of open database and
disk_cache files.

Question: Have you observed the deletion working properly when the files are
actually open upon dtor invocation, particularly on windows where opened files
can't be deleted?

The test right now demonstrates that DeleteDir is called on the directory, but
does not verify that this actually works when the appcache service has it's
files open (since it opens them lazily). Setting up that that precondition would
be a little tricky...

* you could call AppCacheDatabase::LazyOpen(true) to open the db file
* poke 0 into AppCacheStorageImpl::last_cache_id_ (to convince the storage impl
that its initialized)
* invoke AppCacheStorageImpl::disk_cache() to get that initted. But that is
actually async under the covers, it happens on the CACHE_THREAD so you may need
another BrowserThread for that.
* call RunAllPending() so the disk_cache() init is really done
* check for the existence of the Index file and Cache subdirectory
* finally... release the service instance to trigger destruction

[pastarmovj, 2011-01-07 13:28:21]

Fixed the tests to create and use real store instead of creating a fake one.

http://codereview.chromium.org/6077005/diff/34001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/34001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:58:
TEST_F(ChromeAppCacheServiceTest, RemoveOnDestruction) {
On 2011/01/05 21:03:05, michaeln wrote:
> The ChromeAppCacheService code looks great, I like keeping this at the /chrome
> level too. But I wonder if the test can be made better such that it actually
> tests the full shutdown sequence including the closing of open database and
> disk_cache files.
> 
> Question: Have you observed the deletion working properly when the files are
> actually open upon dtor invocation, particularly on windows where opened files
> can't be deleted?
> 
> The test right now demonstrates that DeleteDir is called on the directory, but
> does not verify that this actually works when the appcache service has it's
> files open (since it opens them lazily). Setting up that that precondition
would
> be a little tricky...
> 
> * you could call AppCacheDatabase::LazyOpen(true) to open the db file
> * poke 0 into AppCacheStorageImpl::last_cache_id_ (to convince the storage
impl
> that its initialized)
> * invoke AppCacheStorageImpl::disk_cache() to get that initted. But that is
> actually async under the covers, it happens on the CACHE_THREAD so you may
need
> another BrowserThread for that.
> * call RunAllPending() so the disk_cache() init is really done
> * check for the existence of the Index file and Cache subdirectory
> * finally... release the service instance to trigger destruction

Done.

[pastarmovj, 2011-01-07 13:48:31]

Just uploaded one more revision but the only change there is s/2010/2011/ in
appcache_store_impl.h. None functional changes done/intended.

[jochen (gone - plz use gerrit), 2011-01-07 14:58:00]

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:27: &file_util::Delete,
cache_path, true));
hum, you include a reference to a temporary object (cache_path) here, i'm not
sure this is gonna work

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:56: Initialize(cache_path_ ,
no space before ,

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:18: #include
"chrome/browser/appcache/chrome_appcache_service.h"
do you still need this?

[pastarmovj, 2011-01-07 15:15:35]

Fixed the nits and answered the question about file_util::Delete.

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:27: &file_util::Delete,
cache_path, true));
On 2011/01/07 14:58:01, jochen wrote:
> hum, you include a reference to a temporary object (cache_path) here, i'm not
> sure this is gonna work

Actually if I follow the logic of NewRunnableFunction correctly because of the
explicit template specification the second parameter is copied in a new variable
(inside the parameter tuple). 

I didn't paid so close attention when I got this example from existing code, but
now I see that it uses a small trick by explicitly stating the first piece of
the param tuple to be an object and not a reference which forces a copy but
doesn't brake the function call.

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:56: Initialize(cache_path_ ,
On 2011/01/07 14:58:01, jochen wrote:
> no space before ,

Done.

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:18: #include
"chrome/browser/appcache/chrome_appcache_service.h"
On 2011/01/07 14:58:01, jochen wrote:
> do you still need this?

Done.

[michaeln, 2011-01-07 19:53:04]

LGTM, thanx for working on the test

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/6077005/diff/44001/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service.cc:27: &file_util::Delete,
cache_path, true));
On 2011/01/07 15:15:35, pastarmovj wrote:
> On 2011/01/07 14:58:01, jochen wrote:
> > hum, you include a reference to a temporary object (cache_path) here, i'm
not
> > sure this is gonna work
> 
> Actually if I follow the logic of NewRunnableFunction correctly because of the
> explicit template specification the second parameter is copied in a new
variable
> (inside the parameter tuple). 
> 
> I didn't paid so close attention when I got this example from existing code,
but
> now I see that it uses a small trick by explicitly stating the first piece of
> the param tuple to be an object and not a reference which forces a copy but
> doesn't brake the function call.

Yes, const T& is fine here. I think you could also make the arg to
DeleteLocalStateOnIOThread a const ref.

http://codereview.chromium.org/6077005/diff/49002/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/49002/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:56: ASSERT_LE(0,
storage->NewCacheId());
should this be ASSERT_EQ(1, storage->NewCacheId()), here and below?

[michaeln, 2011-01-07 19:56:52]

http://codereview.chromium.org/6077005/diff/49002/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.h (right):

http://codereview.chromium.org/6077005/diff/49002/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.h:55: protected:
oh wait... why have these new protected data members. Could you just make the
friend declarations and otherwise not change the interface. It's ok to have the
test poke into the private section.

[pastarmovj, 2011-01-10 11:02:01]

Fixed comments. All non expected file changes are results of rebasing the code
on top of trunk. In essence I only changed chrome_appcache_service_unittest.cc
and appcache_storage_impl.h since the last review.

http://codereview.chromium.org/6077005/diff/49002/chrome/browser/appcache/chr...
File chrome/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/6077005/diff/49002/chrome/browser/appcache/chr...
chrome/browser/appcache/chrome_appcache_service_unittest.cc:56: ASSERT_LE(0,
storage->NewCacheId());
On 2011/01/07 19:53:05, michaeln wrote:
> should this be ASSERT_EQ(1, storage->NewCacheId()), here and below?

I though keeping it >= 0 is more generic and still correct but you are right it
should be 1 here.

http://codereview.chromium.org/6077005/diff/49002/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.h (right):

http://codereview.chromium.org/6077005/diff/49002/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.h:55: protected:
On 2011/01/07 19:56:52, michaeln wrote:
> oh wait... why have these new protected data members. Could you just make the
> friend declarations and otherwise not change the interface. It's ok to have
the
> test poke into the private section.

Sorry forgot to move this back to private I was experimenting with some stuff
before that.

[michaeln, 2011-01-10 20:34:03]

Thnx... LGTM!