Misc. cleanup for components/webdata/.

The biggest change here is removing the idea of load success callbacks, which
are never used.

Bug: none
Change-Id: I31c1790e2c7e3bb1c3275327893ddee32533c808
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1962610
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Cait Phillips <caitkp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727016}

components/webdata/common/web_data_service_base.cc

@@ -4,21 +4,10 @@
 
 #include "components/webdata/common/web_data_service_base.h"
 
-#include "base/bind.h"
 #include "base/single_thread_task_runner.h"
-#include "base/stl_util.h"
 #include "base/threading/thread.h"
 #include "components/webdata/common/web_database_service.h"
 
-////////////////////////////////////////////////////////////////////////////////
-//
-// WebDataServiceBase implementation.
-//
-////////////////////////////////////////////////////////////////////////////////
-
-using base::Bind;
-using base::Time;
-
 WebDataServiceBase::WebDataServiceBase(
     scoped_refptr<WebDatabaseService> wdbs,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner)
@@ -34,34 +23,17 @@ void WebDataServiceBase::Init(ProfileErrorCallback callback) {
 }
 
 void WebDataServiceBase::ShutdownDatabase() {
-  if (!wdbs_)
-    return;
-  wdbs_->ShutdownDatabase();
+  if (wdbs_)
+    wdbs_->ShutdownDatabase();
 }
 
 void WebDataServiceBase::CancelRequest(Handle h) {
-  if (!wdbs_)
-    return;
-  wdbs_->CancelRequest(h);
-}
-
-bool WebDataServiceBase::IsDatabaseLoaded() {
-  if (!wdbs_)
-    return false;
-  return wdbs_->db_loaded();
-}
-
-void WebDataServiceBase::RegisterDBLoadedCallback(DBLoadedCallback callback) {
-  if (!wdbs_)
-    return;
-  wdbs_->RegisterDBLoadedCallback(std::move(callback));
+  if (wdbs_)
+    wdbs_->CancelRequest(h);
 }
 
 WebDatabase* WebDataServiceBase::GetDatabase() {
-  if (!wdbs_)
-    return nullptr;
-  return wdbs_->GetDatabaseOnDB();
+  return wdbs_ ? wdbs_->GetDatabaseOnDB() : nullptr;
 }
 
-WebDataServiceBase::~WebDataServiceBase() {
-}
+WebDataServiceBase::~WebDataServiceBase() = default;

components/webdata/common/web_data_service_base.h

@@ -36,8 +36,6 @@ class WEBDATA_EXPORT WebDataServiceBase
   using ProfileErrorCallback =
       base::OnceCallback<void(sql::InitStatus, const std::string&)>;
 
-  using DBLoadedCallback = base::OnceClosure;
-
   // |callback| will only be invoked on error, and only if
   // |callback.is_null()| evaluates to false.
   //
@@ -65,20 +63,8 @@ class WEBDATA_EXPORT WebDataServiceBase
   // Unloads the database and shuts down service.
   void ShutdownDatabase();
 
-  // Register a callback to be notified that the database has loaded. Multiple
-  // callbacks may be registered, and each will be called at most once
-  // (following a successful database load), then cleared.
-  // Note: if the database load is already complete, then the callback will NOT
-  // be stored or called.
-  virtual void RegisterDBLoadedCallback(DBLoadedCallback callback);
-
-  // Returns true if the database load has completetd successfully, and
-  // ShutdownOnUISequence() has not yet been called.
-  virtual bool IsDatabaseLoaded();
-
   // Returns a pointer to the DB (used by SyncableServices). May return NULL if
-  // the database is not loaded or otherwise unavailable. Must be called on DB
-  // sequence.
+  // the database is unavailable. Must be called on DB sequence.
   virtual WebDatabase* GetDatabase();
 
  protected:

components/webdata/common/web_database_backend.cc

@@ -14,20 +14,13 @@
 #include "components/webdata/common/web_database_table.h"
 #include "sql/error_delegate_util.h"
 
-using base::Bind;
-using base::FilePath;
-
 WebDatabaseBackend::WebDatabaseBackend(
-    const FilePath& path,
-    Delegate* delegate,
+    const base::FilePath& path,
+    std::unique_ptr<Delegate> delegate,
     const scoped_refptr<base::SingleThreadTaskRunner>& db_thread)
     : base::RefCountedDeleteOnSequence<WebDatabaseBackend>(db_thread),
       db_path_(path),
-      request_manager_(new WebDataRequestManager()),
-      init_status_(sql::INIT_FAILURE),
-      init_complete_(false),
-      catastrophic_error_occurred_(false),
-      delegate_(delegate) {}
+      delegate_(std::move(delegate)) {}
 
 void WebDatabaseBackend::AddTable(std::unique_ptr<WebDatabaseTable> table) {
   DCHECK(!db_);
@@ -36,15 +29,14 @@ void WebDatabaseBackend::AddTable(std::unique_ptr<WebDatabaseTable> table) {
 
 void WebDatabaseBackend::InitDatabase() {
   LoadDatabaseIfNecessary();
-  if (delegate_) {
+  if (delegate_)
     delegate_->DBLoaded(init_status_, diagnostics_);
-  }
 }
 
 void WebDatabaseBackend::ShutdownDatabase() {
   if (db_ && init_status_ == sql::INIT_OK)
     db_->CommitTransaction();
-  db_.reset(nullptr);
+  db_.reset();
   init_complete_ = true;  // Ensures the init sequence is not re-run.
   init_status_ = sql::INIT_FAILURE;
 }
@@ -81,10 +73,8 @@ void WebDatabaseBackend::DBReadTaskWrapper(
 std::unique_ptr<WDTypedResult> WebDatabaseBackend::ExecuteReadTask(
     WebDatabaseService::ReadTask task) {
   LoadDatabaseIfNecessary();
-  if (db_ && init_status_ == sql::INIT_OK) {
-    return std::move(task).Run(db_.get());
-  }
-  return nullptr;
+  return (db_ && init_status_ == sql::INIT_OK) ? std::move(task).Run(db_.get())
+                                               : nullptr;
 }
 
 WebDatabaseBackend::~WebDatabaseBackend() {
@@ -96,7 +86,7 @@ void WebDatabaseBackend::LoadDatabaseIfNecessary() {
     return;
 
   init_complete_ = true;
-  db_.reset(new WebDatabase());
+  db_ = std::make_unique<WebDatabase>();
 
   for (const auto& table : tables_)
     db_->AddTable(table.get());
@@ -109,17 +99,13 @@ void WebDatabaseBackend::LoadDatabaseIfNecessary() {
   init_status_ = db_->Init(db_path_);
 
   if (init_status_ != sql::INIT_OK) {
-    LOG(ERROR) << "Cannot initialize the web database: " << init_status_;
     db_.reset();
     return;
   }
 
   // A catastrophic error might have happened and recovered.
-  if (catastrophic_error_occurred_) {
+  if (catastrophic_error_occurred_)
     init_status_ = sql::INIT_OK_WITH_DATA_LOSS;
-    LOG(WARNING)
-        << "Webdata recovered from a catastrophic error. Data loss possible.";
-  }
   db_->BeginTransaction();
 }
 

components/webdata/common/web_database_backend.h

@@ -15,6 +15,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/ref_counted_delete_on_sequence.h"
 #include "base/single_thread_task_runner.h"
+#include "components/webdata/common/web_data_request_manager.h"
 #include "components/webdata/common/web_database_service.h"
 #include "components/webdata/common/webdata_export.h"
 
@@ -44,7 +45,7 @@ class WEBDATA_EXPORT WebDatabaseBackend
 
   WebDatabaseBackend(
       const base::FilePath& path,
-      Delegate* delegate,
+      std::unique_ptr<Delegate> delegate,
       const scoped_refptr<base::SingleThreadTaskRunner>& db_thread);
 
   // Must call only before InitDatabaseWithCallback.
@@ -111,19 +112,20 @@ class WEBDATA_EXPORT WebDatabaseBackend
   std::unique_ptr<WebDatabase> db_;
 
   // Keeps track of all pending requests made to the db.
-  scoped_refptr<WebDataRequestManager> request_manager_;
+  scoped_refptr<WebDataRequestManager> request_manager_ =
+      base::MakeRefCounted<WebDataRequestManager>();
 
   // State of database initialization. Used to prevent the executing of tasks
   // before the db is ready.
-  sql::InitStatus init_status_;
+  sql::InitStatus init_status_ = sql::INIT_FAILURE;
 
   // True if an attempt has been made to load the database (even if the attempt
   // fails), used to avoid continually trying to reinit if the db init fails.
-  bool init_complete_;
+  bool init_complete_ = false;
 
   // True if a catastrophic database error occurs and further error callbacks
   // from the database should be ignored.
-  bool catastrophic_error_occurred_;
+  bool catastrophic_error_occurred_ = false;
 
   // If a catastrophic database error has occurred, this contains any available
   // diagnostic information.

components/webdata/common/web_database_service.cc

@@ -15,9 +15,6 @@
 #include "components/webdata/common/web_data_service_consumer.h"
 #include "components/webdata/common/web_database_backend.h"
 
-using base::Bind;
-using base::FilePath;
-
 // Receives messages from the backend on the DB sequence, posts them to
 // WebDatabaseService on the UI sequence.
 class WebDatabaseService::BackendDelegate
@@ -44,19 +41,18 @@ WebDatabaseService::WebDatabaseService(
     scoped_refptr<base::SingleThreadTaskRunner> db_task_runner)
     : base::RefCountedDeleteOnSequence<WebDatabaseService>(ui_task_runner),
       path_(path),
-      db_loaded_(false),
       db_task_runner_(db_task_runner) {
   DCHECK(ui_task_runner->RunsTasksInCurrentSequence());
   DCHECK(db_task_runner_);
 }
 
-WebDatabaseService::~WebDatabaseService() {
-}
+WebDatabaseService::~WebDatabaseService() = default;
 
 void WebDatabaseService::AddTable(std::unique_ptr<WebDatabaseTable> table) {
   if (!web_db_backend_) {
-    web_db_backend_ = new WebDatabaseBackend(
-        path_, new BackendDelegate(weak_ptr_factory_.GetWeakPtr()),
+    web_db_backend_ = base::MakeRefCounted<WebDatabaseBackend>(
+        path_,
+        std::make_unique<BackendDelegate>(weak_ptr_factory_.GetWeakPtr()),
         db_task_runner_);
   }
   web_db_backend_->AddTable(std::move(table));
@@ -69,8 +65,6 @@ void WebDatabaseService::LoadDatabase() {
 }
 
 void WebDatabaseService::ShutdownDatabase() {
-  db_loaded_ = false;
-  loaded_callbacks_.clear();
   error_callbacks_.clear();
   weak_ptr_factory_.InvalidateWeakPtrs();
   if (!web_db_backend_)
@@ -117,13 +111,8 @@ WebDataServiceBase::Handle WebDatabaseService::ScheduleDBTaskWithResult(
 }
 
 void WebDatabaseService::CancelRequest(WebDataServiceBase::Handle h) {
-  if (!web_db_backend_)
-    return;
-  web_db_backend_->request_manager()->CancelRequest(h);
-}
-
-void WebDatabaseService::RegisterDBLoadedCallback(DBLoadedCallback callback) {
-  loaded_callbacks_.push_back(std::move(callback));
+  if (web_db_backend_)
+    web_db_backend_->request_manager()->CancelRequest(h);
 }
 
 void WebDatabaseService::RegisterDBErrorCallback(DBLoadErrorCallback callback) {
@@ -134,29 +123,19 @@ void WebDatabaseService::OnDatabaseLoadDone(sql::InitStatus status,
                                             const std::string& diagnostics) {
   // The INIT_OK_WITH_DATA_LOSS status is an initialization success but with
   // suspected data loss, so we also run the error callbacks.
-  if (status != sql::INIT_OK) {
-    // Notify that the database load failed.
-    while (!error_callbacks_.empty()) {
-      // The profile error callback is a message box that runs in a nested run
-      // loop. While it's being displayed, other OnDatabaseLoadDone() will run
-      // (posted from WebDatabaseBackend::Delegate::DBLoaded()). We need to make
-      // sure that after the callback running the message box returns, it checks
-      // |error_callbacks_| before it accesses it.
-      DBLoadErrorCallback error_callback = std::move(error_callbacks_.back());
-      error_callbacks_.pop_back();
-      if (error_callback)
-        std::move(error_callback).Run(status, diagnostics);
-    }
-  }
-
-  if (status == sql::INIT_OK || status == sql::INIT_OK_WITH_DATA_LOSS) {
-    db_loaded_ = true;
+  if (status == sql::INIT_OK)
+    return;
 
-    while (!loaded_callbacks_.empty()) {
-      DBLoadedCallback loaded_callback = std::move(loaded_callbacks_.back());
-      loaded_callbacks_.pop_back();
-      if (loaded_callback)
-        std::move(loaded_callback).Run();
-    }
+  // Notify that the database load failed.
+  while (!error_callbacks_.empty()) {
+    // The profile error callback is a message box that runs in a nested run
+    // loop. While it's being displayed, other OnDatabaseLoadDone() will run
+    // (posted from WebDatabaseBackend::Delegate::DBLoaded()). We need to make
+    // sure that after the callback running the message box returns, it checks
+    // |error_callbacks_| before it accesses it.
+    DBLoadErrorCallback error_callback = std::move(error_callbacks_.back());
+    error_callbacks_.pop_back();
+    if (!error_callback.is_null())
+      std::move(error_callback).Run(status, diagnostics);
   }
 }

components/webdata/common/web_database_service.h

@@ -50,7 +50,6 @@ class WEBDATA_EXPORT WebDatabaseService
   using WriteTask = base::OnceCallback<WebDatabase::State(WebDatabase*)>;
 
   // Types for managing DB loading callbacks.
-  using DBLoadedCallback = base::OnceClosure;
   using DBLoadErrorCallback =
       base::OnceCallback<void(sql::InitStatus, const std::string&)>;
 
@@ -93,13 +92,6 @@ class WEBDATA_EXPORT WebDatabaseService
   // somewhere else.
   virtual void CancelRequest(WebDataServiceBase::Handle h);
 
-  // Register a callback to be notified that the database has loaded. Multiple
-  // callbacks may be registered, and each will be called at most once
-  // (following a successful database load), then cleared.
-  // Note: if the database load is already complete, then the callback will NOT
-  // be stored or called.
-  void RegisterDBLoadedCallback(DBLoadedCallback callback);
-
   // Register a callback to be notified that the database has failed to load.
   // Multiple callbacks may be registered, and each will be called at most once
   // (following a database load failure), then cleared.
@@ -107,15 +99,12 @@ class WEBDATA_EXPORT WebDatabaseService
   // be stored or called.
   void RegisterDBErrorCallback(DBLoadErrorCallback callback);
 
-  bool db_loaded() const { return db_loaded_; }
-
  private:
   class BackendDelegate;
   friend class BackendDelegate;
   friend class base::RefCountedDeleteOnSequence<WebDatabaseService>;
   friend class base::DeleteHelper<WebDatabaseService>;
 
-  using LoadedCallbacks = std::vector<DBLoadedCallback>;
   using ErrorCallbacks = std::vector<DBLoadErrorCallback>;
 
   virtual ~WebDatabaseService();
@@ -129,15 +118,9 @@ class WEBDATA_EXPORT WebDatabaseService
   // PostTask on DB sequence may outlive us.
   scoped_refptr<WebDatabaseBackend> web_db_backend_;
 
-  // Callbacks to be called once the DB has loaded.
-  LoadedCallbacks loaded_callbacks_;
-
   // Callbacks to be called if the DB has failed to load.
   ErrorCallbacks error_callbacks_;
 
-  // True if the WebDatabase has loaded.
-  bool db_loaded_;
-
   scoped_refptr<base::SingleThreadTaskRunner> db_task_runner_;
 
   // All vended weak pointers are invalidated in ShutdownDatabase().