AppCaches which belong to hosted apps are not protected from deletion

The problem:

When the user has the "Clear cookies and other site and plug-in data when I close my browser" setting enabled, all appcache data is deleted when the user closes the browser. The intention is to protect an appcache from deletion, if the user has installed a hosted app which refers to the origin of the appcache.

BUG=79604
TEST=ChromeAppCacheService.KeepOnDestruction, ChromeAppCacheService.RemoveOnDestruction


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

[michaeln, 2011-07-02 00:29:09]

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

http://codereview.chromium.org/7210006/diff/14017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:596: event->Wait();
We should not block the UI thread for this as profiles can be deleted at times
other than shutdown.

I think a decent place to wait for completion would be in the vicinity of the
call to WaitForPluginDataRemoverToFinish() in ~BrowserProcessImpl(). The trick
is figuring out what to wait on exactly. 

We might be able to generalize this a bit. The code below is not correct since
it creates complex objects as statics (the WaitableEvent), but i'm just trying
to get across the general idea here. If this existed i imagine
WaitForPluginDataRemoverToFinish() could be implemented in terms of this class
too. For ChromeAppCacheService to utilize a class like this it would have to be
in the /content layer.

class PreventBrowserShutdown {
 public:
  // Created by classes like the ChromeAppCacheService prior to starting
  // its 'clearonexit' work, and then deleted by the ChromeAppCacheService when
  // that work is done.
  PreventBrowserShutdown();
  ~PreventBrowserShutdown();

  // Called by ~BrowserProcessImpl to wait for all instances to be
  // deleted prior to tearing down the background threads.
  static void Wait(int timeout_milliseconds);

 private:
  static base::AtomicRefCount instance_count_;
  static WaitableEvent event_;
};

PreventBrowserShutdown::PreventBrowserShutdown() {
  AtomicRefCountInc(&instance_count_);
  event_.Reset();
}

PreventBrowserShutdown::~PreventBrowserShutdown() {
  if (!AtomicRefCountDec(&instance_count_))
    event_.Signal();
}

void PreventBrowserShutdown::Wait(int timeout_millis) {
  // We can assume that no new instances will be created after
  // the time that this method is invoked.
  if (AtomicRefCountIsZero(&instance_count_))
    return;
  event_.TimedWait(timeout_millis);
}

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:98:
++appcaches_to_be_deleted_count_;
maybe origins_to_be_deleted_count_

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:99:
DeleteAppCachesForOrigin(origin->first, &appcache_deleted_callback_);
maybe origin_deleted_callback_

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:140: {
style: bracket belongs at the end of the previous line

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:144: {
style: ditto

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:147: void
ChromeAppCacheService::ClearAppcacheHelper::OnClearedAppCache()
This class seems heavier weight than it needs to be for what it does... consider
just CACS->AddRef() and CACS->Release() at the new/delete callsites of this
object instead of having a new class/object.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:58: // unit tests
i'd nuke this comment, i think it's stating the obvious

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:61: class ClearAppcacheHelper
{
This class seems heavier weight than it needs to be for what it does... consider
just CACS->AddRef() and CACS->Release() at the new/delete callsites of this
object instead of having a new class/object.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:84: // Appcache clearing
helpers and callbacks
AppCache with an upper 'C' is more in keeping with the existing names thruout
the project.

[michaeln, 2011-07-02 00:32:46]

cc'ing jochen to see what he thinks about the PreventBrowserShutdown thing too

[jochen (gone - plz use gerrit), 2011-07-06 09:41:45]

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

http://codereview.chromium.org/7210006/diff/14017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:596: event->Wait();
I agree that we shouldn't wait on the UI thread. OTOH, why wait for the deletion
to complete? 

At this point, we can be sure that the IO thread will be drained before the
browser shuts down, so we can just fire the task to the IO thread and forget
about it

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:76: clear_appcache_helper_ =
new ClearAppcacheHelper(this, callback);
you could just this->Ref(); and Deref() later, no need to have an extra class

[marja(google), 2011-07-06 11:44:39]

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

http://codereview.chromium.org/7210006/diff/14017/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:596: event->Wait();
On 2011/07/06 09:41:45, jochen wrote:
> I agree that we shouldn't wait on the UI thread. OTOH, why wait for the
deletion
> to complete? 
> 
> At this point, we can be sure that the IO thread will be drained before the
> browser shuts down, so we can just fire the task to the IO thread and forget
> about it

The appcache deletion procedure posts tasks across the IO and DB threads (so, a
task is posted to the IO thread, when that is executed, it posts a task to the
DB thread, when that is executed, it posts again to the IO thread, etc.) So it
could happen that there are no tasks in the IO thread, so the IO thread can shut
down, and when the task in the DB thread is executed, it's too late to post
anything to the IO thread any more. Or am I missing something?

[marja(google), 2011-07-06 14:34:09]

Marking the small changes "done". The bigger change (where & how to wait) is
undone.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:76: clear_appcache_helper_ =
new ClearAppcacheHelper(this, callback);
On 2011/07/06 09:41:45, jochen wrote:
> you could just this->Ref(); and Deref() later, no need to have an extra class

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:98:
++appcaches_to_be_deleted_count_;
On 2011/07/02 00:29:09, michaeln wrote:
> maybe origins_to_be_deleted_count_

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:99:
DeleteAppCachesForOrigin(origin->first, &appcache_deleted_callback_);
On 2011/07/02 00:29:09, michaeln wrote:
> maybe origin_deleted_callback_

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:140: {
On 2011/07/02 00:29:09, michaeln wrote:
> style: bracket belongs at the end of the previous line

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:144: {
On 2011/07/02 00:29:09, michaeln wrote:
> style: ditto

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:147: void
ChromeAppCacheService::ClearAppcacheHelper::OnClearedAppCache()
On 2011/07/02 00:29:09, michaeln wrote:
> This class seems heavier weight than it needs to be for what it does...
consider
> just CACS->AddRef() and CACS->Release() at the new/delete callsites of this
> object instead of having a new class/object.

Done.

[jochen (gone - plz use gerrit), 2011-07-07 14:38:05]

On 2011/07/06 11:44:39, marja wrote:
>
http://codereview.chromium.org/7210006/diff/14017/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
http://codereview.chromium.org/7210006/diff/14017/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_impl.cc:596: event->Wait();
> On 2011/07/06 09:41:45, jochen wrote:
> > I agree that we shouldn't wait on the UI thread. OTOH, why wait for the
> deletion
> > to complete? 
> > 
> > At this point, we can be sure that the IO thread will be drained before the
> > browser shuts down, so we can just fire the task to the IO thread and forget
> > about it
> 
> The appcache deletion procedure posts tasks across the IO and DB threads (so,
a
> task is posted to the IO thread, when that is executed, it posts a task to the
> DB thread, when that is executed, it posts again to the IO thread, etc.) So it
> could happen that there are no tasks in the IO thread, so the IO thread can
shut
> down, and when the task in the DB thread is executed, it's too late to post
> anything to the IO thread any more. Or am I missing something?

I think that's only required if you delete databases during runtime (i.e. the
browser data remover use case). For the profile shutdown use case, you can just
close all databases, then post a task to the db thread, and delete the ones you
don't want any longer.

That means you'll need a second code path, but for not blocking the UI thread,
that's totally worth it

[michaeln, 2011-07-07 18:58:41]

> I think that's only required if you delete databases during runtime (i.e. the
> browser data remover use case). For the profile shutdown use case, you can
just
> close all databases, then post a task to the db thread, and delete the ones
you
> don't want any longer.
> 
> That means you'll need a second code path, but for not blocking the UI thread,
> that's totally worth it

That could work too although it's unfortunate to have the second code path.
Given that the shutdown code path is already waiting for data removal to finish
(via WaitForPluginDataRemoverToFinish), it's not clear that adding the second
code path is preferable to generalizing that wait (ala PreventBrowserShutdown
mentioned earlier). Do you intend to remove/revamp
WaitForPluginDataRemoverToFinish stuff?

If we were to have a second code path in the appcache system for use
specifically at profile/browser shutdown, a reasonable way to do may be to add a
clear_on_exit_ flag to AppCacheStorageImpl, set that flag during profile
shutdown, test that flag in ~AppCacheStorageImpl, and if set post the task to
delete unprotected origins prior to posting the task delete the AppCacheDatabase
instance.

[michaeln, 2011-07-07 19:06:51]

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:20: const
FilePath::CharType kTestingAppCacheDirname[] =
is this needed anymore?

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:58: {
style nit: bracket goes on the previous line

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:64: {
style nit: bracket goes on the previous line

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:66:
collection->infos_by_origin[kProtectedOrigin];
Did you mean to make an assignment here to info_vector, or maybe you can remove
the info_vector local variable since it doesn't seem to be needed?

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:74: {
style nit: bracket goes on the previous line

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:79: FROM_HERE,
style nit: indent the params by 2 more spaces

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:117: {
style nit: bracket goes on the previous line

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:202: FilePath(""),
i think the default ctor would work here (and elsewhere)... FilePath()

[jochen (gone - plz use gerrit), 2011-07-07 20:01:50]

On 2011/07/07 18:58:41, michaeln wrote:
> > I think that's only required if you delete databases during runtime (i.e.
the
> > browser data remover use case). For the profile shutdown use case, you can
> just
> > close all databases, then post a task to the db thread, and delete the ones
> you
> > don't want any longer.
> > 
> > That means you'll need a second code path, but for not blocking the UI
thread,
> > that's totally worth it
> 
> That could work too although it's unfortunate to have the second code path.
> Given that the shutdown code path is already waiting for data removal to
finish
> (via WaitForPluginDataRemoverToFinish), it's not clear that adding the second
> code path is preferable to generalizing that wait (ala PreventBrowserShutdown
> mentioned earlier). Do you intend to remove/revamp
> WaitForPluginDataRemoverToFinish stuff?

That one is going to be removed soon.

> 
> If we were to have a second code path in the appcache system for use
> specifically at profile/browser shutdown, a reasonable way to do may be to add
a
> clear_on_exit_ flag to AppCacheStorageImpl, set that flag during profile
> shutdown, test that flag in ~AppCacheStorageImpl, and if set post the task to
> delete unprotected origins prior to posting the task delete the
AppCacheDatabase
> instance.
sounds good

[marja(google), 2011-07-12 14:25:26]

Adding the synchronous code path turned out to be a bit hairier (*) than
estimated, so we decided with Jochen to try out the "block the ui thread
sufficiently late in the shutdown phase" alternative.

(*) AppCacheDiskCache doens't want to be accessed in the DB thread, and uses
async disk cache API's.

It seems that Profile's are currently not deleted until shutdown, so the changes
I made should work in that case. If somebody modifies the code so that Profiles
are deleted earlier, then this part also needs changes. Should I prepare for
that or is this ok? (Dunno really how to prepare for it in a non-ugly way.)

[michaeln, 2011-07-12 18:28:35]

I haven't taken a look at the new CL yet.

> Adding the synchronous code path turned out to be a bit hairier (*) than
> estimated, so we decided with Jochen to try out the "block the ui thread
> sufficiently late in the shutdown phase" alternative.
> 
> (*) AppCacheDiskCache doens't want to be accessed in the DB thread, and uses
> async disk cache API's.

I question whether we actually need to wait for deletion from the DiskCache. By
removing related Group/Cache/Entry records from the DB (and adding the response
ids to the 'DeletableResponseIds' table), the responses will no longer be
retrievable and will be scheduled for removal from the DiskCache shortly after
the browser's next launch.

> It seems that Profile's are currently not deleted until shutdown, so the
changes
> I made should work in that case. If somebody modifies the code so that
Profiles
> are deleted earlier, then this part also needs changes. Should I prepare for
> that or is this ok? (Dunno really how to prepare for it in a non-ugly way.)

That's probably not a safe assumption to make given the multi profile stuff on
the horizon.

[michaeln, 2011-07-12 18:45:38]

If it really is a requirement to wait for the DiskCache to be scrubbed (i'd for
for it not to be), then this CL doesn't meet those requirements yet. Before we
start trying to "solve that problem", i need to ask if that is a problem that
needs to be solved.

http://codereview.chromium.org/7210006/diff/40005/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/7210006/diff/40005/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:117: void
ChromeAppCacheService::OnClearedAppCache() {
Fyi, at the time this callback is invoked, the responses may have not (likely
have not) actually been removed from the DiskCache. The responses are lazily
scavenged after the Group/Cache/Entry records have been deleted, this completion
callback occurs when those records have been deleted by prior to the scavenging
having taken place.

[marja(google), 2011-07-13 07:17:04]

On 2011/07/12 18:28:35, michaeln wrote:
> I question whether we actually need to wait for deletion from the DiskCache.
By
> removing related Group/Cache/Entry records from the DB (and adding the
response
> ids to the 'DeletableResponseIds' table), the responses will no longer be
> retrievable and will be scheduled for removal from the DiskCache shortly after
> the browser's next launch.

Ah, probably we don't need to wait. I was just blindly following the async code
path and trying to make everything sync. I missed the fact that it's enough to
mark the response id's as deletable in the db. Thanks for pointing it out, it
simplifies a lot :) I'll try the sync code path approach again.

[marja(google), 2011-07-13 11:06:59]

Some small changes are now done, some have been reverted.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.cc (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:76: clear_appcache_helper_ =
new ClearAppcacheHelper(this, callback);
On 2011/07/06 09:41:45, jochen wrote:
> you could just this->Ref(); and Deref() later, no need to have an extra class

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:98:
++appcaches_to_be_deleted_count_;
On 2011/07/02 00:29:09, michaeln wrote:
> maybe origins_to_be_deleted_count_

I had changed this, but changed it back because of the DeleteAppCachesForOrigin
-> DeleteAppCacheGroup change. Now it's appcaches_to_be_deleted_count_ like it
was before, but now we're counting appcaches, not origins.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.cc:99:
DeleteAppCachesForOrigin(origin->first, &appcache_deleted_callback_);
On 2011/07/02 00:29:09, michaeln wrote:
> maybe origin_deleted_callback_

Same here.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:58: // unit tests
On 2011/07/02 00:29:09, michaeln wrote:
> i'd nuke this comment, i think it's stating the obvious

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:61: class ClearAppcacheHelper
{
On 2011/07/02 00:29:09, michaeln wrote:
> This class seems heavier weight than it needs to be for what it does...
consider
> just CACS->AddRef() and CACS->Release() at the new/delete callsites of this
> object instead of having a new class/object.

Done.

http://codereview.chromium.org/7210006/diff/14017/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:84: // Appcache clearing
helpers and callbacks
On 2011/07/02 00:29:09, michaeln wrote:
> AppCache with an upper 'C' is more in keeping with the existing names thruout
> the project.

Done.

[marja(google), 2011-07-13 11:13:16]

Style fixes done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:20: const
FilePath::CharType kTestingAppCacheDirname[] =
On 2011/07/07 19:06:51, michaeln wrote:
> is this needed anymore?

Done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:64: {
On 2011/07/07 19:06:51, michaeln wrote:
> style nit: bracket goes on the previous line

Done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:66:
collection->infos_by_origin[kProtectedOrigin];
On 2011/07/07 19:06:51, michaeln wrote:
> Did you mean to make an assignment here to info_vector, or maybe you can
remove
> the info_vector local variable since it doesn't seem to be needed?

Done.

I rewrote this func; now there shouldn't be any unneeded variables.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:74: {
On 2011/07/07 19:06:51, michaeln wrote:
> style nit: bracket goes on the previous line

Done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:79: FROM_HERE,
On 2011/07/07 19:06:51, michaeln wrote:
> style nit: indent the params by 2 more spaces

Done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:117: {
On 2011/07/07 19:06:51, michaeln wrote:
> style nit: bracket goes on the previous line

Done.

http://codereview.chromium.org/7210006/diff/29001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:202: FilePath(""),
On 2011/07/07 19:06:51, michaeln wrote:
> i think the default ctor would work here (and elsewhere)... FilePath()

Done.

[marja(google), 2011-07-13 12:42:22]

The sync code path is added now. I also hope I've now done all the suggested
modifications, at least that was my intention :)

[michaeln, 2011-07-14 00:42:34]

This CL is a bit different than what i was expecting after this earlier exchange
in the review log...

>> If we were to have a second code path in the appcache system for use
specifically
>> at profile/browser shutdown, a reasonable way to do may be to add a
>> clear_on_exit_ flag, set that flag during profile
>> shutdown, test that flag in ~AppCacheStorageImpl, and if set post the task to
>> delete unprotected origins prior to posting the task delete the 
>> AppCacheDatabase instance.
>
> sounds good

We shouldn't add the sync code paths in the current patch set since its
performing blocking file io on the IO thread and is using non-thread-safe
classes in unsafe ways. What I had in mind with that earlier comment about
taking care of this in ~AppCacheStorageImpl is more spelled out below...

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:526:
appcache_service_->ClearAppCache(&appcache_cleared_callback_);
The code in this CL isn't looking at the delete_begin_ time anymore.

We have plans to replace the BDR's handling of (filesystem + appcache + indexdb
+ sqldb) with code that utilizes methods of the QuotaManager to indirectly poke
at those systems. Given those plans, I think we can just leave this file as is
for now.
http://code.google.com/p/chromium/issues/detail?id=86308

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

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:594:
appcache_service_->SyncClearAppCacheOnIOThread();
Just post a task to set the service's clear_on_exit_ flag. In most other places
the clear_local_state_on_exit_ data member is used to test for that pref
setting.

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
Given the approach of deleting in the storage dtor, i don't  need this method.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.h:81: virtual void
SyncGetAllAppCacheInfo(AppCacheInfoCollection* collection);
Please remove the Sync prefixed methods from this class.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:81: virtual void SyncGetAllInfo(Delegate*
delegate) = 0;
Please remove all of these Sync prefixed methods from this class and its
derivatives.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1082:
STLDeleteElements(&pending_simple_tasks_);
I this this CL can be simplified a lot by posting a task to read what needs to
be read and delete things accordingly... assuming the service has a
clear_on_exit flag...

if (!cache_directory_.empty() && service_->clear_on_exit()) {
  AppCacheThread::PostTask(AppCacheThread::db(), FROM_HERE,
      NewRunnableFunction(ClearOnExit, 
          database_, service_->special_storage_policy()));
}


static void ClearOnExit(AppCacheDatabase* db, scoped_refptr<T> policy) {
  std::set<GURL> origins
  db->FindOriginsWithGroups(&origins);
  if (origins.empty())
    return;  // nothing to delete

  sql::Connection* connection = database_->db_connection();
  if (!connection) {
    NOTREACHED() << "Missing database connection.";
    return;
  }

  foreach origin in origins {
    if (policy && policy->IsProtected(origin))
      continue;

    std::vector<AppCacheDatabase::GroupRecord> groups;
    FindGroupsForOrigin(origin, &groups);
    foreach group in groups {
      sql::Transaction transaction(connection);
      if (!transaction.Begin()) {
        NOTREACHED() << "Failed to start transaction";
        return;
      }

      bool success = false;

      // Logic lifted from MakeGroupObsoleteTask::Run, could be shared.
      AppCacheDatabase::CacheRecord cache_record;
      if (db->FindCacheForGroup(group.group_id, &cache_record)) {
        std::vector<int64> response_ids
        db->FindResponseIdsForCacheAsVector(cache_record.cache_id,
                                            &response_ids);
        success =
          db->DeleteGroup(group.group_id,) &&
          db->DeleteCache(cache_record.cache_id) &&
          db->DeleteEntriesForCache(cache_record.cache_id) &&
          db->DeleteFallbackNameSpacesForCache(cache_record.cache_id) &&
          db->DeleteOnlineWhiteListForCache(cache_record.cache_id) &&
          db->InsertDeletableResponseIds(response_ids);
      } else {
        NOTREACHED() << "A existing group without a cache is unexpected";
        success = db->DeleteGroup(group_id_);
      }

      success = success && transaction.Commit();
      DCHECK(success);
    }
  }
}

[marja(google), 2011-07-14 10:31:58]

Second round of sync code path, this time following Michael's idea more closely
(I had somehow gotten wrong impression on how the sync code path should be like,
sorry...).

I ended up adding the "clear local data on exit" setting to AppCacheService (not
to ChromeAppCacheService), since in ~AppCacheStorageImpl, ~AppCacheService is in
the call stack and ~ChromeAppCacheService is already done, and so the
appcache_service_ is no longer a ChromeAppCacheService, that part has been
destructed.

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:526:
appcache_service_->ClearAppCache(&appcache_cleared_callback_);
On 2011/07/14 00:42:34, michaeln wrote:
> The code in this CL isn't looking at the delete_begin_ time anymore.

Ah, indeed, that's a bug. Thanks for pointing it out.
 
> We have plans to replace the BDR's handling of (filesystem + appcache +
indexdb
> + sqldb) with code that utilizes methods of the QuotaManager to indirectly
poke
> at those systems. Given those plans, I think we can just leave this file as is
> for now.
> http://code.google.com/p/chromium/issues/detail?id=86308

Hmm, by "as is" do you mean "as it was before this changelist (so, no moving the
code from BDR to CACS)", or "as it is in the current version of the changelist
(with the bug)"?

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

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:594:
appcache_service_->SyncClearAppCacheOnIOThread();
On 2011/07/14 00:42:34, michaeln wrote:
> Just post a task to set the service's clear_on_exit_ flag. In most other
places
> the clear_local_state_on_exit_ data member is used to test for that pref
> setting.

Done.

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
On 2011/07/14 00:42:34, michaeln wrote:
> Given the approach of deleting in the storage dtor, i don't  need this method.

Done.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.h:81: virtual void
SyncGetAllAppCacheInfo(AppCacheInfoCollection* collection);
On 2011/07/14 00:42:34, michaeln wrote:
> Please remove the Sync prefixed methods from this class.

Done.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:81: virtual void SyncGetAllInfo(Delegate*
delegate) = 0;
On 2011/07/14 00:42:34, michaeln wrote:
> Please remove all of these Sync prefixed methods from this class and its
> derivatives.

Done.

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/37023/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1082:
STLDeleteElements(&pending_simple_tasks_);
On 2011/07/14 00:42:34, michaeln wrote:
> I this this CL can be simplified a lot by posting a task to read what needs to
> be read and delete things accordingly... assuming the service has a
> clear_on_exit flag...
> 
> if (!cache_directory_.empty() && service_->clear_on_exit()) {
>   AppCacheThread::PostTask(AppCacheThread::db(), FROM_HERE,
>       NewRunnableFunction(ClearOnExit, 
>           database_, service_->special_storage_policy()));
> }
> 
> 
> static void ClearOnExit(AppCacheDatabase* db, scoped_refptr<T> policy) {
>   std::set<GURL> origins
>   db->FindOriginsWithGroups(&origins);
>   if (origins.empty())
>     return;  // nothing to delete
> 
>   sql::Connection* connection = database_->db_connection();
>   if (!connection) {
>     NOTREACHED() << "Missing database connection.";
>     return;
>   }
> 
>   foreach origin in origins {
>     if (policy && policy->IsProtected(origin))
>       continue;
> 
>     std::vector<AppCacheDatabase::GroupRecord> groups;
>     FindGroupsForOrigin(origin, &groups);
>     foreach group in groups {
>       sql::Transaction transaction(connection);
>       if (!transaction.Begin()) {
>         NOTREACHED() << "Failed to start transaction";
>         return;
>       }
> 
>       bool success = false;
> 
>       // Logic lifted from MakeGroupObsoleteTask::Run, could be shared.
>       AppCacheDatabase::CacheRecord cache_record;
>       if (db->FindCacheForGroup(group.group_id, &cache_record)) {
>         std::vector<int64> response_ids
>         db->FindResponseIdsForCacheAsVector(cache_record.cache_id,
>                                             &response_ids);
>         success =
>           db->DeleteGroup(group.group_id,) &&
>           db->DeleteCache(cache_record.cache_id) &&
>           db->DeleteEntriesForCache(cache_record.cache_id) &&
>           db->DeleteFallbackNameSpacesForCache(cache_record.cache_id) &&
>           db->DeleteOnlineWhiteListForCache(cache_record.cache_id) &&
>           db->InsertDeletableResponseIds(response_ids);
>       } else {
>         NOTREACHED() << "A existing group without a cache is unexpected";
>         success = db->DeleteGroup(group_id_);
>       }
> 
>       success = success && transaction.Commit();
>       DCHECK(success);
>     }
>   }
> }

Done.

[michaeln, 2011-07-14 22:53:16]

Yup... getting there...

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
File chrome/browser/browsing_data_remover.cc (right):

http://codereview.chromium.org/7210006/diff/37023/chrome/browser/browsing_dat...
chrome/browser/browsing_data_remover.cc:526:
appcache_service_->ClearAppCache(&appcache_cleared_callback_);
>> The code in this CL isn't looking at the delete_begin_ time anymore.
> Ah, indeed, that's a bug. Thanks for pointing it out.
...
> Hmm, by "as is" do you mean "as it was before this changelist
> (so, no moving the code from BDR to CACS)", or "as it is in
> the current version of the changelist (with the bug)"?

I meant to suggest the former, "as it was  before this changelist". Definitely
not suggesting adding a new bug :)

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
On 2011/07/14 10:31:59, marja wrote:
> On 2011/07/14 00:42:34, michaeln wrote:
> > Given the approach of deleting in the storage dtor, i don't  need this
method.
> 
> Done.

But this method is still in the CL?

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
If we don't make the changes to the BDR in this CL, we don't need this method at
all since it's no longer used in the OnExit code path.

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:145:
TEST_F(ChromeAppCacheServiceTest, ClearAppCache) {
And if we don't add the ClearAppCache method, we wouldn't need this test either.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.h:155: // accesses special_storage_policy_.
right... that's pretty subtle... commenting the data member ordering helps, not
sure but it may be better to explicit invoke storage_.reset() in
~AppCacheService() so we're not dependent on the order of the data members

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (left):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:316: 
it be better to revert this file and removing it from you CL

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:32: namespace {
If this anon namespace is moved down a couple of lines and nested in the
appcache namespace, then we could get rid of all of the appcache:: qualifiers.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:71: // Logic lifted from
MakeGroupObsoleteTask::Run, could be shared.
Can you take a stab at sharing this logic with MakeGroupObsoleteTask::Run? A
helper function in the anon namespace could work...

bool DeleteGroupAndRelatedRecords(db, group, opt_deletable_response_ids);

...that's used by both MakeGroupObsoleteTask::Run and this function.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1149: if
(service()->clear_local_state_on_exit() && database_) {
nit: maybe test if (database_) once and {
  if (clear_on_exit())
    PostTask(ClearOnExit);
  DeleteSoon(database_);
}

[marja(google), 2011-07-15 11:03:41]

Thanks for clarifications,

I cancelled the code move (BrowsingDataRemover -> ChromeAppCacheService).
ChromeAppCacheService unit tests were obsoleted by that change, so I removed
them and added new ones for testing the "on exit" code path. The new tests
needed some testhelpers which already existed in BrowsingDataRemoverTest, so I
moved them into a separate cc file.

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/37023/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
Oops, misread which func this comment refers to. Now really done.

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service.h:52: base::WaitableEvent*
ClearAppCache(net::CompletionCallback* callback);
On 2011/07/14 22:53:16, michaeln wrote:
> If we don't make the changes to the BDR in this CL, we don't need this method
at
> all since it's no longer used in the OnExit code path.

Done.

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/57001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:145:
TEST_F(ChromeAppCacheServiceTest, ClearAppCache) {
On 2011/07/14 22:53:16, michaeln wrote:
> And if we don't add the ClearAppCache method, we wouldn't need this test
either.

I rewrote the ChromeAppCacheService tests, for testing the "on exit" code path.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_se...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_se...
webkit/appcache/appcache_service.h:155: // accesses special_storage_policy_.
On 2011/07/14 22:53:16, michaeln wrote:
> right... that's pretty subtle... commenting the data member ordering helps,
not
> sure but it may be better to explicit invoke storage_.reset() in
> ~AppCacheService() so we're not dependent on the order of the data members

Done.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage.h (left):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage.h:316: 
On 2011/07/14 22:53:16, michaeln wrote:
> it be better to revert this file and removing it from you CL

Done.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:32: namespace {
On 2011/07/14 22:53:16, michaeln wrote:
> If this anon namespace is moved down a couple of lines and nested in the
> appcache namespace, then we could get rid of all of the appcache:: qualifiers.

Done.

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:71: // Logic lifted from
MakeGroupObsoleteTask::Run, could be shared.
On 2011/07/14 22:53:16, michaeln wrote:
> Can you take a stab at sharing this logic with MakeGroupObsoleteTask::Run? A
> helper function in the anon namespace could work...
> 
> bool DeleteGroupAndRelatedRecords(db, group, opt_deletable_response_ids);
> 
> ...that's used by both MakeGroupObsoleteTask::Run and this function.

Done.

Btw, how about AppCacheGroup::AddNewlyDeletableResponseIds;
MakeGroupObsoleteTask does that but the ClearOnExit doesn't. Maybe it doesn't
matter in the current situation, since the Profiles are deleted only on exit.
But what if the Profiles are deleted sooner? Should the ClearOnExit also do
AddNewlyDeletableResponseIds?

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:1149: if
(service()->clear_local_state_on_exit() && database_) {
On 2011/07/14 22:53:16, michaeln wrote:
> nit: maybe test if (database_) once and {
>   if (clear_on_exit())
>     PostTask(ClearOnExit);
>   DeleteSoon(database_);
> }

Done.

[michaeln, 2011-07-15 19:15:46]

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/57001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:71: // Logic lifted from
MakeGroupObsoleteTask::Run, could be shared.
> Btw, how about AppCacheGroup::AddNewlyDeletableResponseIds;
> MakeGroupObsoleteTask does that but the ClearOnExit doesn't. Maybe it doesn't
> matter in the current situation, since the Profiles are deleted only on exit.
> But what if the Profiles are deleted sooner? Should the ClearOnExit also do
> AddNewlyDeletableResponseIds?

By the time our ClearOnExit function is run, there are no AppCacheGroup
instances in memory anymore. That stuff has to do with lazily scavenging
responses from the DiskCache, the AppCacheStorageImpl class is what conducts
that scavenging... but the service and the storage class are gone by ClearOnExit
time.

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h
File chrome/test/appcache_util.h (right):

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:17: class AppCacheInserter : public
appcache::AppCacheStorage::Delegate {
Now that this class is promoted to a separate .h file, I'm looking at these
interfaces more critically. Maybe the two classes defined in this .h file can be
merged into a single class with the two methods needed in the tests...

class AppCacheTestHelper {
  void AddGroupAndCache(service, manifest_url);
  void GetOriginsWithCaches(service, std::set<GURL>* origins);
}

... if you buy that, the filename should change accordingly.

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:22: virtual void OnGroupAndNewestCacheStored(
I'd suggest making this method private or protected. AddGroupAndCache() is the
public interface this class provides, the delegate callback is just an
implementation detail.

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:45: std::set<GURL>* ReadAppCaches();
This method name seems a little vague, and the return value of a ptr to a
private data member is a little dangerous since the caller has to know not to
access the ptr after this object is deleted and to know that subsequent calls to
ReadAppCaches will overwrite previous results. Maybe...

  void GetOriginsWithCaches(service, std::set<GURL>* origins);

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:28: const GURL
kNormalManifest("http://www.normal.com/cache.manifest");
Can you make these non-primitives data members of ChromeAppCacheServiceTest to
avoid the construction at module load time (or do something else to avoid
running GURL ctors at load time). In production code, we pretty much dissallow
non-primitive globals.

Some ideas...

- make these GURLs data members in contruct them in ChromeAppCacheServiceTest()
- put const char kProtectedManifest[] =
"http://www.protected.com/cache.manifest" as globals and construct
GURL(kProtectedManifest) as needed in the tests code

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:62:
scoped_refptr<ChromeAppCacheService>& appcache_service,
Google style thing... we discourage non-const references

It'd be fine to return a scoped_refptr<T> from this method.

http://codereview.chromium.org/7210006/diff/65001/webkit/appcache/appcache_st...
File webkit/appcache/appcache_storage_impl.cc (right):

http://codereview.chromium.org/7210006/diff/65001/webkit/appcache/appcache_st...
webkit/appcache/appcache_storage_impl.cc:49: bool
DeleteGroupAndRelatedRecords(AppCacheDatabase* database,
Thnx for refactoring!

[marja(google), 2011-07-18 09:55:40]

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h
File chrome/test/appcache_util.h (right):

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:17: class AppCacheInserter : public
appcache::AppCacheStorage::Delegate {
On 2011/07/15 19:15:46, michaeln wrote:
> Now that this class is promoted to a separate .h file, I'm looking at these
> interfaces more critically. Maybe the two classes defined in this .h file can
be
> merged into a single class with the two methods needed in the tests...
> 
> class AppCacheTestHelper {
>   void AddGroupAndCache(service, manifest_url);
>   void GetOriginsWithCaches(service, std::set<GURL>* origins);
> }
> 
> ... if you buy that, the filename should change accordingly.

Done.

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:22: virtual void OnGroupAndNewestCacheStored(
On 2011/07/15 19:15:46, michaeln wrote:
> I'd suggest making this method private or protected. AddGroupAndCache() is the
> public interface this class provides, the delegate callback is just an
> implementation detail.

Done.

http://codereview.chromium.org/7210006/diff/65001/chrome/test/appcache_util.h...
chrome/test/appcache_util.h:45: std::set<GURL>* ReadAppCaches();
On 2011/07/15 19:15:46, michaeln wrote:
> This method name seems a little vague, and the return value of a ptr to a
> private data member is a little dangerous since the caller has to know not to
> access the ptr after this object is deleted and to know that subsequent calls
to
> ReadAppCaches will overwrite previous results. Maybe...
> 
>   void GetOriginsWithCaches(service, std::set<GURL>* origins);

Done.

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:28: const GURL
kNormalManifest("http://www.normal.com/cache.manifest");
On 2011/07/15 19:15:46, michaeln wrote:
> Can you make these non-primitives data members of ChromeAppCacheServiceTest to
> avoid the construction at module load time (or do something else to avoid
> running GURL ctors at load time). In production code, we pretty much dissallow
> non-primitive globals.
> 
> Some ideas...
> 
> - make these GURLs data members in contruct them in
ChromeAppCacheServiceTest()
> - put const char kProtectedManifest[] =
> "http://www.protected.com/cache.manifest" as globals and construct
> GURL(kProtectedManifest) as needed in the tests code

Done for this file. But I left the GURLs in browsing_data_remover_unittest.cc
untouched (some of them were added by me in a previous cl, some of them were
added by others). I can create a separate cl for that.

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:62:
scoped_refptr<ChromeAppCacheService>& appcache_service,
On 2011/07/15 19:15:46, michaeln wrote:
> Google style thing... we discourage non-const references
> 
> It'd be fine to return a scoped_refptr<T> from this method.

The reason for not returning scoped_refptr was to be able to have ASSERTs in the
function. I made the appcache_service parameter a pointer instead of a non-const
reference, is that ok?

[marja(google), 2011-07-18 15:03:20]

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
File content/browser/appcache/chrome_appcache_service_unittest.cc (right):

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:62:
scoped_refptr<ChromeAppCacheService>& appcache_service,
On 2011/07/18 09:55:41, marja wrote:
> On 2011/07/15 19:15:46, michaeln wrote:
> > Google style thing... we discourage non-const references
> > 
> > It'd be fine to return a scoped_refptr<T> from this method.
> 
> The reason for not returning scoped_refptr was to be able to have ASSERTs in
the
> function. I made the appcache_service parameter a pointer instead of a
non-const
> reference, is that ok?

Hmm, it seems that ASSSERT_* in a non-test function didn't do what I expected it
to do; it marked the test as failure, but didn't terminate it. I modified the
return value to be scoped_refptr<ChromeAppCacheService>. I didn't think it's
worth to also "return" (via a pointer or such) a bool which would contain the
outcome of the removed ASSERTs, that the test would've then needed to check. Is
the current version ok?

http://codereview.chromium.org/7210006/diff/65001/content/browser/appcache/ch...
content/browser/appcache/chrome_appcache_service_unittest.cc:62:
scoped_refptr<ChromeAppCacheService>& appcache_service,
On 2011/07/15 19:15:46, michaeln wrote:
> Google style thing... we discourage non-const references
> 
> It'd be fine to return a scoped_refptr<T> from this method.

Done.

[michaeln, 2011-07-18 18:54:17]

LGTM and thnx for doing this!

[marja(google), 2011-07-20 08:52:21]

On 2011/07/18 18:54:17, michaeln wrote:
> LGTM and thnx for doing this!

Thanks for the review & comments. I'm committing this now (Jochen also said
"commit").

[commit-bot: I haz the power, 2011-07-20 08:52:51]

Presubmit check for 7210006-70021 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile.cc,chrome/browser/profiles/profile_impl.cc

Presubmit checks took 1.2s to calculate.

[marja(google), 2011-07-20 09:01:50]

Hi Miranda,

As an owner of chrome/browser/profiles/*, could you review the relevant parts of
this CL? (chrome/browser/profiles/profile.cc &
chrome/browser/profiles/profile_impl.cc).

(Sorry, I should've notified you earlier, but this OWNERS thing temporarily
slipped out of my noogler mind...)

[marja(google), 2011-07-20 09:10:11]

Hi Jay,

As an owner of chrome/test, could you review that the files added by that
directory by this CL (appcache_test_helper.h & .cc) are OK? Thanks in advance.

[Miranda Callahan, 2011-07-20 12:51:12]

On 2011/07/20 09:01:50, marja wrote:
> Hi Miranda,
> 
> As an owner of chrome/browser/profiles/*, could you review the relevant parts
of
> this CL? (chrome/browser/profiles/profile.cc &
> chrome/browser/profiles/profile_impl.cc).
> 
> (Sorry, I should've notified you earlier, but this OWNERS thing temporarily
> slipped out of my noogler mind...)

Profile stuff LGTM!

[michaeln, 2011-07-20 20:25:12]

On 2011/07/20 09:10:11, marja wrote:
> Hi Jay,
> 
> As an owner of chrome/test, could you review that the files added by that
> directory by this CL (appcache_test_helper.h & .cc) are OK? Thanks in advance.

Another option could be to move the appcache_test_helper.* files to
webkit/appcache. I don't see any chrome/ dependencies in there (after replacing
usages of type ChromeAppCacheService with it's base class
appcache::AppCacheService).

[marja(google), 2011-07-21 08:57:23]

On 2011/07/20 20:25:12, michaeln wrote:
> Another option could be to move the appcache_test_helper.* files to
> webkit/appcache. I don't see any chrome/ dependencies in there (after
replacing
> usages of type ChromeAppCacheService with it's base class
> appcache::AppCacheService).

Thanks, I did that. I replaced ChromeAppCacheService* with AppCacheService*, and
I also included the AppCacheTestHelper class into the namespace appcache. Is
this ok? Still LGTY?

[michaeln, 2011-07-21 18:55:57]

yup... lgtm still :)

[commit-bot: I haz the power, 2011-07-22 07:56:39]

Try job failure for 7210006-79001 (retry) on linux for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-07-22 10:21:09]

Change committed as 93597