WebSQL: Simplify API for DatabaseTracker::DeleteDataModifiedSince.

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

content/web_test/browser/web_test_control_host.cc

@@ -1613,8 +1613,7 @@ void WebTestControlHost::ClearAllDatabases() {
   auto run_on_database_sequence =
       [](scoped_refptr<storage::DatabaseTracker> db_tracker) {
         DCHECK(db_tracker->task_runner()->RunsTasksInCurrentSequence());
-        db_tracker->DeleteDataModifiedSince(base::Time(),
-                                            net::CompletionOnceCallback());
+        db_tracker->DeleteDataModifiedSince(base::Time(), base::DoNothing());
       };
 
   BrowserContext* browser_context =

storage/browser/database/database_tracker.cc

@@ -691,18 +691,23 @@ void DatabaseTracker::DeleteDatabase(const std::string& origin_identifier,
   std::move(callback).Run(net::OK);
 }
 
-int DatabaseTracker::DeleteDataModifiedSince(
+void DatabaseTracker::DeleteDataModifiedSince(
     const base::Time& cutoff,
     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;
+  }
 
   std::vector<std::string> origins_identifiers;
-  if (!databases_table_->GetAllOriginIdentifiers(&origins_identifiers))
-    return net::ERR_FAILED;
+  if (!databases_table_->GetAllOriginIdentifiers(&origins_identifiers)) {
+    std::move(callback).Run(net::ERR_FAILED);
+    return;
+  }
+
+  DatabaseSet to_be_deleted;
   int rv = net::OK;
   for (const auto& origin : origins_identifiers) {
     if (special_storage_policy_.get() &&
@@ -713,8 +718,9 @@ int DatabaseTracker::DeleteDataModifiedSince(
 
     std::vector<DatabaseDetails> details;
     if (!databases_table_->GetAllDatabaseDetailsForOriginIdentifier(origin,
-                                                                    &details))
+                                                                    &details)) {
       rv = net::ERR_FAILED;
+    }
     for (const DatabaseDetails& db : details) {
       base::FilePath db_file = GetFullDBFilePath(origin, db.database_name);
       base::File::Info file_info;
@@ -723,21 +729,26 @@ int DatabaseTracker::DeleteDataModifiedSince(
         continue;
 
       // Check if the database is opened by any renderer.
-      if (database_connections_.IsDatabaseOpened(origin, db.database_name))
+      if (database_connections_.IsDatabaseOpened(origin, db.database_name)) {
         to_be_deleted[origin].insert(db.database_name);
-      else
+      } else {
         DeleteClosedDatabase(origin, db.database_name);
+      }
     }
   }
 
-  if (rv != net::OK)
-    return rv;
+  if (rv != net::OK) {
+    DCHECK_EQ(rv, net::ERR_FAILED);
+    std::move(callback).Run(rv);
+    return;
+  }
 
   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);
 }
 
 void DatabaseTracker::DeleteDataForOrigin(

storage/browser/database/database_tracker.h

@@ -142,14 +142,17 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) DatabaseTracker
                       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|.
-  // 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. Protected origins, according the the SpecialStoragePolicy,
-  // are not deleted by this method.
-  int DeleteDataModifiedSince(const base::Time& cutoff,
-                              net::CompletionOnceCallback callback);
+  // Deletes databases touched since `cutoff`.
+  //
+  // Does not delete databases belonging to origins designated as protected by
+  // the SpecialStoragePolicy passed to the DatabaseTracker constructor.
+  //
+  // `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.
+  void DeleteDataModifiedSince(const base::Time& cutoff,
+                               net::CompletionOnceCallback callback);
 
   // Deletes all databases that belong to the given origin.
   //

storage/browser/database/database_tracker_unittest.cc

@@ -284,16 +284,15 @@ class DatabaseTracker_TestHelper_Test {
           base::Time yesterday = base::Time::Now();
           yesterday -= base::TimeDelta::FromDays(1);
 
-          net::TestCompletionCallback callback2;
-          int result =
-              tracker->DeleteDataModifiedSince(yesterday, callback2.callback());
-          EXPECT_EQ(net::ERR_IO_PENDING, result);
-          ASSERT_FALSE(callback2.have_result());
+          net::TestCompletionCallback delete_data_modified_since_callback;
+          tracker->DeleteDataModifiedSince(
+              yesterday, delete_data_modified_since_callback.callback());
+          EXPECT_FALSE(delete_data_modified_since_callback.have_result());
           EXPECT_TRUE(observer.DidReceiveNewNotification());
           tracker->DatabaseClosed(kOrigin1, kDB1);
           tracker->DatabaseClosed(kOrigin2, kDB2);
-          result = callback2.GetResult(result);
-          EXPECT_EQ(net::OK, result);
+          EXPECT_EQ(net::OK,
+                    delete_data_modified_since_callback.WaitForResult());
           EXPECT_FALSE(base::PathExists(tracker->GetOriginDirectory(kOrigin1)));
           EXPECT_TRUE(
               base::PathExists(tracker->GetFullDBFilePath(kOrigin2, kDB2)));