WebSQL: Simplify API for DatabaseTracker::DeleteDatabase.

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: Iedf252bae4f6dc82c5570ccc5f1d70f8b2e11c2e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2635679
Auto-Submit: Victor Costan <pwnall@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844862}

storage/browser/database/database_tracker.cc

@@ -9,6 +9,7 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -176,8 +177,7 @@ void DatabaseTracker::HandleSqliteError(
   // Note: the client-side filters out all but these two errors as
   // a small optimization, see WebDatabaseObserverImpl::HandleSqliteError.
   if (error == SQLITE_CORRUPT || error == SQLITE_NOTADB) {
-    DeleteDatabase(origin_identifier, database_name,
-                   net::CompletionOnceCallback());
+    DeleteDatabase(origin_identifier, database_name, base::DoNothing());
   }
 }
 
@@ -677,25 +677,27 @@ void DatabaseTracker::ScheduleDatabasesForDeletion(
   }
 }
 
-int DatabaseTracker::DeleteDatabase(const std::string& origin_identifier,
-                                    const base::string16& database_name,
-                                    net::CompletionOnceCallback callback) {
+void DatabaseTracker::DeleteDatabase(const std::string& origin_identifier,
+                                     const base::string16& database_name,
+                                     net::CompletionOnceCallback callback) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
-  if (!LazyInit())
-    return net::ERR_FAILED;
+  DCHECK(!callback.is_null());
+  if (!LazyInit()) {
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
 
   if (database_connections_.IsDatabaseOpened(origin_identifier,
                                              database_name)) {
-    if (!callback.is_null()) {
-      DatabaseSet set;
-      set[origin_identifier].insert(database_name);
-      deletion_callbacks_.emplace_back(std::move(callback), set);
-    }
+    DatabaseSet set;
+    set[origin_identifier].insert(database_name);
+    deletion_callbacks_.emplace_back(std::move(callback), std::move(set));
     ScheduleDatabaseForDeletion(origin_identifier, database_name);
-    return net::ERR_IO_PENDING;
+    return;
   }
+
   DeleteClosedDatabase(origin_identifier, database_name);
-  return net::OK;
+  std::move(callback).Run(net::OK);
 }
 
 int DatabaseTracker::DeleteDataModifiedSince(

storage/browser/database/database_tracker.h

@@ -138,12 +138,14 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) DatabaseTracker
   bool IsDatabaseScheduledForDeletion(const std::string& origin_identifier,
                                       const base::string16& database_name);
 
-  // Deletes a single database. Returns net::OK on success, net::FAILED on
-  // failure, or net::ERR_IO_PENDING and |callback| is invoked upon completion,
-  // if non-null.
-  int DeleteDatabase(const std::string& origin_identifier,
-                     const base::string16& database_name,
-                     net::CompletionOnceCallback callback);
+  // Deletes a single database.
+  //
+  // `callback` must be non-null, and is invoked upon completion with a
+  // net::Error, which will most likely be net::OK or net::FAILED. `callback`
+  // may be called before this method returns.
+  void DeleteDatabase(const std::string& origin_identifier,
+                      const base::string16& database_name,
+                      net::CompletionOnceCallback callback);
 
   // Delete any databases that have been touched since the cutoff date that's
   // supplied, omitting any that match IDs within |protected_origins|.

storage/browser/database/database_tracker_unittest.cc

@@ -248,17 +248,15 @@ class DatabaseTracker_TestHelper_Test {
           // Delete db1. Should also delete origin1.
           TestObserver observer;
           tracker->AddObserver(&observer);
-          net::TestCompletionCallback callback1;
-          int result =
-              tracker->DeleteDatabase(kOrigin1, kDB1, callback1.callback());
-          EXPECT_EQ(net::ERR_IO_PENDING, result);
-          ASSERT_FALSE(callback1.have_result());
+          net::TestCompletionCallback delete_database_callback;
+          tracker->DeleteDatabase(kOrigin1, kDB1,
+                                  delete_database_callback.callback());
+          EXPECT_FALSE(delete_database_callback.have_result());
           EXPECT_TRUE(observer.DidReceiveNewNotification());
           EXPECT_EQ(kOrigin1, observer.GetNotificationOriginIdentifier());
           EXPECT_EQ(kDB1, observer.GetNotificationDatabaseName());
           tracker->DatabaseClosed(kOrigin1, kDB1);
-          result = callback1.GetResult(result);
-          EXPECT_EQ(net::OK, result);
+          EXPECT_EQ(net::OK, delete_database_callback.WaitForResult());
           EXPECT_FALSE(base::PathExists(tracker->GetOriginDirectory(kOrigin1)));
 
           // Recreate db1.
@@ -287,7 +285,7 @@ class DatabaseTracker_TestHelper_Test {
           yesterday -= base::TimeDelta::FromDays(1);
 
           net::TestCompletionCallback callback2;
-          result =
+          int result =
               tracker->DeleteDataModifiedSince(yesterday, callback2.callback());
           EXPECT_EQ(net::ERR_IO_PENDING, result);
           ASSERT_FALSE(callback2.have_result());
@@ -514,9 +512,11 @@ class DatabaseTracker_TestHelper_Test {
 
           tracker->DatabaseClosed(kOriginId, kName);
           EXPECT_TRUE(test_quota_proxy->WasAccessNotified(kOrigin));
-          EXPECT_EQ(net::OK,
-                    tracker->DeleteDatabase(kOriginId, kName,
-                                            net::CompletionOnceCallback()));
+          net::TestCompletionCallback delete_database_callback;
+          tracker->DeleteDatabase(kOriginId, kName,
+                                  delete_database_callback.callback());
+          EXPECT_TRUE(delete_database_callback.have_result());
+          EXPECT_EQ(net::OK, delete_database_callback.WaitForResult());
           EXPECT_TRUE(test_quota_proxy->WasModificationNotified(kOrigin, -100));
           test_quota_proxy->reset();
 
@@ -542,9 +542,10 @@ class DatabaseTracker_TestHelper_Test {
           EXPECT_TRUE(test_quota_proxy->WasModificationNotified(kOrigin, 100));
           test_quota_proxy->reset();
 
-          EXPECT_EQ(net::ERR_IO_PENDING,
-                    tracker->DeleteDatabase(kOriginId, kName,
-                                            net::CompletionOnceCallback()));
+          net::TestCompletionCallback delete_database_callback2;
+          tracker->DeleteDatabase(kOriginId, kName,
+                                  delete_database_callback2.callback());
+          EXPECT_FALSE(delete_database_callback2.have_result());
           EXPECT_FALSE(
               test_quota_proxy->WasModificationNotified(kOrigin, -100));
           EXPECT_TRUE(base::PathExists(tracker->GetOriginDirectory(kOriginId)));
@@ -554,6 +555,8 @@ class DatabaseTracker_TestHelper_Test {
           EXPECT_TRUE(test_quota_proxy->WasModificationNotified(kOrigin, -100));
           EXPECT_FALSE(
               base::PathExists(tracker->GetOriginDirectory(kOriginId)));
+          EXPECT_TRUE(delete_database_callback2.have_result());
+          EXPECT_EQ(net::OK, delete_database_callback2.WaitForResult());
           test_quota_proxy->reset();
 
           // Create a database and up the file size without telling
@@ -818,9 +821,11 @@ class DatabaseTracker_TestHelper_Test {
           tracker->DatabaseClosed(kOriginId, kEmptyName);
 
           // Deleting it should return to the initial state.
-          EXPECT_EQ(net::OK,
-                    tracker->DeleteDatabase(kOriginId, kEmptyName,
-                                            net::CompletionOnceCallback()));
+          net::TestCompletionCallback delete_database_callback;
+          tracker->DeleteDatabase(kOriginId, kEmptyName,
+                                  delete_database_callback.callback());
+          EXPECT_TRUE(delete_database_callback.have_result());
+          EXPECT_EQ(net::OK, delete_database_callback.WaitForResult());
           infos.clear();
           EXPECT_TRUE(tracker->GetAllOriginsInfo(&infos));
           EXPECT_TRUE(infos.empty());