WebSQL: Simplify API for DatabaseTracker::DeleteDataForOrigin.

The method currently follows an unfortunate maybe-async pattern
established in //net whereby operations may either report an outcome
synchronously by returning a net::Error other than net::ERR_IO_PENDING,
or report the outcome asynchronously by invoking a callback.

This CL changes the method to unconditionally run the callback with the
operation outcome. This is intended to simplify the mental model and
control flow around the method.

Change-Id: I4337cb5e93820596510b02c0b6bde75a6f3111f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632853Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844996}

components/browsing_data/content/database_helper.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/callback_helpers.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
@@ -69,9 +70,8 @@ void DatabaseHelper::DeleteDatabase(const url::Origin& origin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   tracker_->task_runner()->PostTask(
       FROM_HERE,
-      base::BindOnce(
-          base::IgnoreResult(&storage::DatabaseTracker::DeleteDataForOrigin),
-          tracker_, origin, net::CompletionOnceCallback()));
+      base::BindOnce(&storage::DatabaseTracker::DeleteDataForOrigin, tracker_,
+                     origin, base::DoNothing::Once<int>()));
 }
 
 CannedDatabaseHelper::CannedDatabaseHelper(

storage/browser/database/database_quota_client.cc

@@ -72,15 +72,10 @@ std::vector<url::Origin> GetOriginsForHostOnDBThread(
   return host_origins;
 }
 
-void DidDeleteOriginData(base::SequencedTaskRunner* original_task_runner,
-                         QuotaClient::DeleteOriginDataCallback callback,
-                         int result) {
-  if (result == net::ERR_IO_PENDING) {
-    // The callback will be invoked via
-    // DatabaseTracker::ScheduleDatabasesForDeletion.
-    return;
-  }
-
+void DidDeleteOriginData(
+    scoped_refptr<base::SequencedTaskRunner> original_task_runner,
+    QuotaClient::DeleteOriginDataCallback callback,
+    int result) {
   blink::mojom::QuotaStatusCode status;
   if (result == net::OK)
     status = blink::mojom::QuotaStatusCode::kOk;
@@ -161,20 +156,12 @@ void DatabaseQuotaClient::DeleteOriginData(const url::Origin& origin,
   DCHECK(!callback.is_null());
   DCHECK_EQ(type, StorageType::kTemporary);
 
-  // DidDeleteOriginData() translates the net::Error response to a
-  // blink::mojom::QuotaStatusCode if necessary, and no-ops as appropriate if
-  // DatabaseTracker::ScheduleDatabasesForDeletion will also invoke the
-  // callback.
-  auto delete_callback = base::BindRepeating(
-      &DidDeleteOriginData,
-      base::RetainedRef(base::SequencedTaskRunnerHandle::Get()),
-      base::AdaptCallbackForRepeating(std::move(callback)));
-
-  base::PostTaskAndReplyWithResult(
-      db_tracker_->task_runner(), FROM_HERE,
+  db_tracker_->task_runner()->PostTask(
+      FROM_HERE,
       base::BindOnce(&DatabaseTracker::DeleteDataForOrigin, db_tracker_, origin,
-                     delete_callback),
-      net::CompletionOnceCallback(delete_callback));
+                     base::BindOnce(&DidDeleteOriginData,
+                                    base::SequencedTaskRunnerHandle::Get(),
+                                    std::move(callback))));
 }
 
 void DatabaseQuotaClient::PerformStorageCleanup(

storage/browser/database/database_quota_client_unittest.cc

@@ -69,17 +69,17 @@ class MockDatabaseTracker : public DatabaseTracker {
     return true;
   }
 
-  int DeleteDataForOrigin(const url::Origin& origin,
-                          net::CompletionOnceCallback callback) override {
+  void DeleteDataForOrigin(const url::Origin& origin,
+                           net::CompletionOnceCallback callback) override {
     ++delete_called_count_;
     if (async_delete()) {
       base::SequencedTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
           base::BindOnce(&MockDatabaseTracker::AsyncDeleteDataForOrigin, this,
                          std::move(callback)));
-      return net::ERR_IO_PENDING;
+      return;
     }
-    return net::OK;
+    std::move(callback).Run(net::OK);
   }
 
   void AsyncDeleteDataForOrigin(net::CompletionOnceCallback callback) {

storage/browser/database/database_tracker.cc

@@ -740,33 +740,41 @@ int DatabaseTracker::DeleteDataModifiedSince(
   return net::OK;
 }
 
-int DatabaseTracker::DeleteDataForOrigin(const url::Origin& origin,
-                                         net::CompletionOnceCallback callback) {
+void DatabaseTracker::DeleteDataForOrigin(
+    const url::Origin& origin,
+    net::CompletionOnceCallback callback) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
-  if (!LazyInit())
-    return net::ERR_FAILED;
-
-  DatabaseSet to_be_deleted;
+  DCHECK(!callback.is_null());
+  if (!LazyInit()) {
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
 
   const std::string identifier = GetIdentifierFromOrigin(origin);
 
   std::vector<DatabaseDetails> details;
   if (!databases_table_->GetAllDatabaseDetailsForOriginIdentifier(identifier,
-                                                                  &details))
-    return net::ERR_FAILED;
-  for (const auto& db : details) {
+                                                                  &details)) {
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
+
+  DatabaseSet to_be_deleted;
+  for (const DatabaseDetails& db : details) {
     // Check if the database is opened by any renderer.
-    if (database_connections_.IsDatabaseOpened(identifier, db.database_name))
+    if (database_connections_.IsDatabaseOpened(identifier, db.database_name)) {
       to_be_deleted[identifier].insert(db.database_name);
-    else
+    } else {
       DeleteClosedDatabase(identifier, db.database_name);
+    }
   }
 
   if (!to_be_deleted.empty()) {
     ScheduleDatabasesForDeletion(to_be_deleted, std::move(callback));
-    return net::ERR_IO_PENDING;
+    return;
   }
-  return net::OK;
+
+  std::move(callback).Run(net::OK);
 }
 
 const base::File* DatabaseTracker::GetIncognitoFile(

storage/browser/database/database_tracker.h

@@ -154,12 +154,14 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) DatabaseTracker
   int DeleteDataModifiedSince(const base::Time& cutoff,
                               net::CompletionOnceCallback callback);
 
-  // Delete all databases that belong to the given origin. Returns net::OK on
-  // success, net::FAILED if not all databases could be deleted, and
-  // net::ERR_IO_PENDING and |callback| is invoked upon completion, if non-null.
-  // virtual for unit testing only
-  virtual int DeleteDataForOrigin(const url::Origin& origin,
-                                  net::CompletionOnceCallback callback);
+  // Deletes all databases that belong to the given origin.
+  //
+  // `callback` must must be non-null, and is invoked upon completion with a
+  // net::Error. The status will be net::OK on success, or net::FAILED if not
+  // all databases could be deleted. `callback` may be called before this method
+  // returns.
+  virtual void DeleteDataForOrigin(const url::Origin& origin,
+                                   net::CompletionOnceCallback callback);
 
   bool IsIncognitoProfile() const { return is_incognito_; }