Converting BookmarkModel and HistoryService to ProfileKeyedServices. This just performs the initial…

chrome/browser/profiles/profile_impl.cc

 // Copyright (c) 2012 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/profiles/profile_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/environment.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/version.h"
 #include "chrome/browser/autocomplete/autocomplete_classifier.h"
 #include "chrome/browser/background/background_contents_service_factory.h"
 #include "chrome/browser/background/background_mode_manager.h"
-#include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_plugin_service_filter.h"
 #include "chrome/browser/content_settings/cookie_settings.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/extensions/extension_event_router.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
 #include "chrome/browser/extensions/extension_pref_value_map.h"
 #include "chrome/browser/extensions/extension_pref_value_map_factory.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/user_script_master.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/geolocation/chrome_geolocation_permission_context.h"
 #include "chrome/browser/history/history.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/shortcuts_backend.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/instant/instant_controller.h"
 #include "chrome/browser/metrics/metrics_service.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/browser/net/net_pref_observer.h"
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/net/proxy_service_factory.h"
 #include "chrome/browser/net/ssl_config_service_manager.h"
 #include "chrome/browser/net/url_fixer_upper.h"
@@ skipping 164 matching lines @@
 }
 
 ProfileImpl::ProfileImpl(const FilePath& path,
                          Delegate* delegate,
                          CreateMode create_mode)
     : path_(path),
       ALLOW_THIS_IN_INITIALIZER_LIST(visited_link_event_listener_(
           new VisitedLinkEventListener(this))),
       ALLOW_THIS_IN_INITIALIZER_LIST(io_data_(this)),
       host_content_settings_map_(NULL),
-      history_service_created_(false),
       favicon_service_created_(false),
       clear_local_state_on_exit_(false),
       start_time_(Time::Now()),
       delegate_(delegate),
       predictor_(NULL),
       session_restore_enabled_(false) {
   DCHECK(!path.empty()) << "Using an empty path will attempt to write " <<
                             "profile files to the root directory!";
 
 #if defined(ENABLE_SESSION_SERVICE)
@@ skipping 274 matching lines @@
   // The HistoryService maintains threads for background processing. Its
   // possible each thread still has tasks on it that have increased the ref
   // count of the service. In such a situation, when we decrement the refcount,
   // it won't be 0, and the threads/databases aren't properly shut down. By
   // explicitly calling Cleanup/Shutdown we ensure the databases are properly
   // closed.
 
   if (top_sites_.get())
     top_sites_->Shutdown();
 
-  if (bookmark_bar_model_.get()) {
-    // It's possible that bookmarks haven't loaded and history is waiting for
-    // bookmarks to complete loading. In such a situation history can't shutdown
-    // (meaning if we invoked history_service_->Cleanup now, we would
-    // deadlock). To break the deadlock we tell BookmarkModel it's about to be
-    // deleted so that it can release the signal history is waiting on, allowing
-    // history to shutdown (history_service_->Cleanup to complete). In such a
-    // scenario history sees an incorrect view of bookmarks, but it's better
-    // than a deadlock.
-    bookmark_bar_model_->Cleanup();
-  }
-
-  if (history_service_.get())
-    history_service_->Cleanup();
-
-  // HistoryService may call into the BookmarkModel, as such we need to

[sky, 2012/05/21 14:23:06]

How is this handled now?

[rpetterson, 2012/05/21 18:01:49]

HistoryService is marked as being dependent on the BookmarkModel. This means
that when shutdown occurs, we'll delete HistoryService before the BookmarkModel
based on the dependency graph.

In order to make sure the cleanup is performed in the correct order, when we
call Shutdown on the HistoryService, it will also call the BookmarkModel's
Shutdown function (see HistoryService::ShutdownOnUIThread
(http://codereview.chromium.org/10399087/patch/5003/7012). The BookmarkModel's
Shutdown method will also get called again when it is deleted, but releasing
that signal again shouldn't matter if history has already been deleted.

By putting the signal code in BookmarkModel::Shutdown, we ensure it gets called
whether there is a history service or not. If this is unnecessary, I can move it
back to being in a Cleanup method.

[sky, 2012/05/21 20:37:17]

What you have is fine. I just wanted to make sure I understood how it's working.

-  // delete HistoryService before the BookmarkModel. The destructor for
-  // HistoryService will join with HistoryService's backend thread so that
-  // by the time the destructor has finished we're sure it will no longer call
-  // into the BookmarkModel.
-  history_service_ = NULL;
-  bookmark_bar_model_.reset();
-
   // FaviconService depends on HistoryServce so make sure we delete
   // HistoryService first.
   favicon_service_.reset();
 
   if (pref_proxy_config_tracker_.get())
     pref_proxy_config_tracker_->DetachFromPrefService();
 
   if (protocol_handler_registry_)
     protocol_handler_registry_->Finalize();
 
@@ skipping 232 matching lines @@
 
 GAIAInfoUpdateService* ProfileImpl::GetGAIAInfoUpdateService() {
   if (!gaia_info_update_service_.get() &&
       GAIAInfoUpdateService::ShouldUseGAIAProfileInfo(this)) {
     gaia_info_update_service_.reset(new GAIAInfoUpdateService(this));
   }
   return gaia_info_update_service_.get();
 }
 
 HistoryService* ProfileImpl::GetHistoryService(ServiceAccessType sat) {
-  // If saving history is disabled, only allow explicit access.
-  if (GetPrefs()->GetBoolean(prefs::kSavingBrowserHistoryDisabled) &&
-      sat != EXPLICIT_ACCESS)
-    return NULL;
-
-  if (!history_service_created_) {
-    history_service_created_ = true;
-    scoped_refptr<HistoryService> history(new HistoryService(this));
-    if (!history->Init(GetPath(), GetBookmarkModel()))
-      return NULL;
-    history_service_.swap(history);
-  }
-  return history_service_.get();
+  return HistoryServiceFactory::GetForProfile(this, sat).get();
 }
 
 HistoryService* ProfileImpl::GetHistoryServiceWithoutCreating() {
-  return history_service_.get();
+  return HistoryServiceFactory::GetForProfileIfExists(this).get();
 }
 
 AutocompleteClassifier* ProfileImpl::GetAutocompleteClassifier() {
   if (!autocomplete_classifier_.get())
     autocomplete_classifier_.reset(new AutocompleteClassifier(this));
   return autocomplete_classifier_.get();
 }
 
 history::ShortcutsBackend* ProfileImpl::GetShortcutsBackend() {
   // This is called on one thread only - UI, so no magic is needed to protect
@@ skipping 14 matching lines @@
   // it to be set by asking for the prefs.
   GetPrefs();
   return last_session_exited_cleanly_;
 }
 
 quota::SpecialStoragePolicy* ProfileImpl::GetSpecialStoragePolicy() {
   return GetExtensionSpecialStoragePolicy();
 }
 
 BookmarkModel* ProfileImpl::GetBookmarkModel() {
-  if (!bookmark_bar_model_.get()) {
-    bookmark_bar_model_.reset(new BookmarkModel(this));
-    bookmark_bar_model_->Load();
-  }
-  return bookmark_bar_model_.get();
+  return BookmarkModelFactory::GetForProfile(this);
 }
 
 ProtocolHandlerRegistry* ProfileImpl::GetProtocolHandlerRegistry() {
   return protocol_handler_registry_.get();
 }
 
 bool ProfileImpl::IsSameProfile(Profile* profile) {
   if (profile == static_cast<Profile*>(this))
     return true;
   Profile* otr_profile = off_the_record_profile_.get();
@@ skipping 289 matching lines @@
   if (!path.empty())
     *cache_path = path;
   *max_size = is_media_context ? prefs_->GetInteger(prefs::kMediaCacheSize) :
                                  prefs_->GetInteger(prefs::kDiskCacheSize);
 }
 
 base::Callback<ChromeURLDataManagerBackend*(void)>
     ProfileImpl::GetChromeURLDataManagerBackendGetter() const {
   return io_data_.GetChromeURLDataManagerBackendGetter();
 }