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

chrome/browser/appcache/chrome_appcache_service.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/appcache/chrome_appcache_service.h"
 
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "chrome/browser/browser_list.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/notification_service.h"
 #include "net/base/net_errors.h"
 #include "webkit/appcache/appcache_thread.h"
 
 static bool has_initialized_thread_ids;
 
 // ----------------------------------------------------------------------------
 
 ChromeAppCacheService::ChromeAppCacheService() {
 }
 
 void ChromeAppCacheService::InitializeOnIOThread(
     const FilePath& profile_path, bool is_incognito,
-    scoped_refptr<HostContentSettingsMap> content_settings_map) {
+    scoped_refptr<HostContentSettingsMap> content_settings_map,
+    bool clear_local_state_on_exit) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (!has_initialized_thread_ids) {
     has_initialized_thread_ids = true;
     appcache::AppCacheThread::Init(BrowserThread::DB, BrowserThread::IO);
   }
 
   host_contents_settings_map_ = content_settings_map;
   registrar_.Add(
       this, NotificationType::PURGE_MEMORY, NotificationService::AllSources());
+  SetClearLocalStateOnExit(clear_local_state_on_exit);
+  if (!is_incognito)
+    cache_path_ = profile_path.Append(chrome::kAppCacheDirname);
 
   // Init our base class.
-  Initialize(
-      is_incognito ? FilePath() : profile_path.Append(chrome::kAppCacheDirname),
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE));
+  Initialize(cache_path_ ,
+             BrowserThread::GetMessageLoopProxyForThread(BrowserThread::CACHE));
   set_appcache_policy(this);
 }
 
 ChromeAppCacheService::~ChromeAppCacheService() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (clear_local_state_on_exit_ && !cache_path_.empty())
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableFunction(file_util::Delete, cache_path_, true));

[michaeln, 2011/01/05 00:59:14]

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).

[pastarmovj, 2011/01/05 10:47:07]

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. 

 }
 
 void ChromeAppCacheService::SetOriginQuotaInMemory(
     const GURL& origin, int64 quota) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (storage())
     storage()->SetOriginQuotaInMemory(origin, quota);
 }
 
 void ChromeAppCacheService::ResetOriginQuotaInMemory(const GURL& origin) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (storage())
     storage()->ResetOriginQuotaInMemory(origin);
 }
 
-// static
-void ChromeAppCacheService::ClearLocalState(const FilePath& profile_path) {
-  file_util::Delete(profile_path.Append(chrome::kAppCacheDirname), true);
+void ChromeAppCacheService::SetClearLocalStateOnExit(bool clear_local_state) {
+  AutoLock lock(lock_);

[michaeln, 2011/01/05 00:59:14]

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:07]

Done.

+  clear_local_state_on_exit_ = clear_local_state;
 }
 
 bool ChromeAppCacheService::CanLoadAppCache(const GURL& manifest_url) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   ContentSetting setting = host_contents_settings_map_->GetContentSetting(
       manifest_url, CONTENT_SETTINGS_TYPE_COOKIES, "");
   DCHECK(setting != CONTENT_SETTING_DEFAULT);
   // We don't prompt for read access.
   return setting != CONTENT_SETTING_BLOCK;
 }
@@ skipping 33 matching lines @@
     const tracked_objects::Location& from_here,
     Task* task) {
   return BrowserThread::PostTask(ToBrowserThreadID(id), from_here, task);
 }
 
 bool AppCacheThread::CurrentlyOn(int id) {
   return BrowserThread::CurrentlyOn(ToBrowserThreadID(id));
 }
 
 }  // namespace appcache