WebApp: Redesign RegisterApp and UnregisterApp methods.

Support creation and deletion of apps as a part of single update.

RegisterApp, UnregisterApp and UnregisterAll methods now can be expressed
within one WebAppRegistryUpdate (one transaction).

Report kWriteDataFailed in InstallFinalizer if commit fails.

WebAppRegistryUpdate CreateApp/UpdateApp/DeleteApp is now the only
writable database API (as in Database CRUD: Create/Update/Delete).

Bug: 860583
Change-Id: Ia2e23a4d4a0d47e62b815abd55803616a204e813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1824043Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700088}

chrome/browser/web_applications/test/test_web_app_registry_controller.cc

@@ -7,6 +7,7 @@
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/web_applications/test/test_web_app_database_factory.h"
+#include "chrome/browser/web_applications/web_app_registry_update.h"
 #include "chrome/browser/web_applications/web_app_sync_bridge.h"
 
 namespace web_app {
@@ -33,6 +34,23 @@ void TestWebAppRegistryController::Init() {
   run_loop.Run();
 }
 
+void TestWebAppRegistryController::RegisterApp(
+    std::unique_ptr<WebApp> web_app) {
+  ScopedRegistryUpdate update(sync_bridge_.get());
+  update->CreateApp(std::move(web_app));
+}
+
+void TestWebAppRegistryController::UnregisterApp(const AppId& app_id) {
+  ScopedRegistryUpdate update(sync_bridge_.get());
+  update->DeleteApp(app_id);
+}
+
+void TestWebAppRegistryController::UnregisterAll() {
+  ScopedRegistryUpdate update(sync_bridge_.get());
+  for (const AppId& app_id : registrar().GetAppIds())
+    update->DeleteApp(app_id);
+}
+
 void TestWebAppRegistryController::DestroySubsystems() {
   mutable_registrar_.reset();
   sync_bridge_.reset();

chrome/browser/web_applications/test/test_web_app_registry_controller.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "components/sync/model/mock_model_type_change_processor.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -17,6 +18,7 @@ namespace web_app {
 
 class TestWebAppDatabaseFactory;
 class WebAppSyncBridge;
+class WebApp;
 
 class TestWebAppRegistryController {
  public:
@@ -29,6 +31,10 @@ class TestWebAppRegistryController {
   // metadata.
   void Init();
 
+  void RegisterApp(std::unique_ptr<WebApp> web_app);
+  void UnregisterApp(const AppId& app_id);
+  void UnregisterAll();
+
   void DestroySubsystems();
 
   TestWebAppDatabaseFactory& database_factory() { return *database_factory_; }

chrome/browser/web_applications/web_app_database.cc

@@ -10,6 +10,7 @@
 #include "chrome/browser/web_applications/proto/web_app.pb.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_database_factory.h"
+#include "chrome/browser/web_applications/web_app_registry_update.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/model/model_error.h"
 #include "components/sync/protocol/web_app_specifics.pb.h"
@@ -38,32 +39,45 @@ void WebAppDatabase::OpenDatabase(RegistryOpenedCallback callback) {
                           weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void WebAppDatabase::WriteWebApps(AppsToWrite apps,
-                                  CompletionCallback callback) {
+void WebAppDatabase::BeginTransaction() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(opened_);
 
-  BeginTransaction();
-
-  for (const WebApp* web_app : apps) {
-    auto proto = CreateWebAppProto(*web_app);
-    write_batch_->WriteData(web_app->app_id(), proto->SerializeAsString());
-  }
-
-  CommitTransaction(std::move(callback));
+  DCHECK(!write_batch_);
+  write_batch_ = store_->CreateWriteBatch();
 }
 
-void WebAppDatabase::DeleteWebApps(std::vector<AppId> app_ids,
-                                   CompletionCallback callback) {
+void WebAppDatabase::CommitTransaction(const RegistryUpdateData& update_data,
+                                       CompletionCallback callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(opened_);
 
-  BeginTransaction();
+  DCHECK(write_batch_);
+  DCHECK(!update_data.IsEmpty());
 
-  for (auto& app_id : app_ids)
+  for (const std::unique_ptr<WebApp>& web_app : update_data.apps_to_create) {
+    auto proto = CreateWebAppProto(*web_app);
+    write_batch_->WriteData(web_app->app_id(), proto->SerializeAsString());
+  }
+
+  for (const AppId& app_id : update_data.apps_to_delete)
     write_batch_->DeleteData(app_id);
 
-  CommitTransaction(std::move(callback));
+  for (const WebApp* web_app : update_data.apps_to_update) {
+    auto proto = CreateWebAppProto(*web_app);
+    write_batch_->WriteData(web_app->app_id(), proto->SerializeAsString());
+  }
+
+  store_->CommitWriteBatch(
+      std::move(write_batch_),
+      base::BindOnce(&WebAppDatabase::OnDataWritten,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  write_batch_.reset();
+}
+
+void WebAppDatabase::CancelTransaction() {
+  DCHECK(write_batch_);
+  write_batch_.reset();
 }
 
 // static
@@ -279,21 +293,4 @@ std::unique_ptr<WebApp> WebAppDatabase::ParseWebApp(const AppId& app_id,
   return web_app;
 }
 
-void WebAppDatabase::BeginTransaction() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(!write_batch_);
-  write_batch_ = store_->CreateWriteBatch();
-}
-
-void WebAppDatabase::CommitTransaction(CompletionCallback callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(write_batch_);
-
-  store_->CommitWriteBatch(
-      std::move(write_batch_),
-      base::BindOnce(&WebAppDatabase::OnDataWritten,
-                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
-  write_batch_.reset();
-}
-
 }  // namespace web_app

chrome/browser/web_applications/web_app_database.h

@@ -8,7 +8,6 @@
 #include <memory>
 
 #include "base/callback_forward.h"
-#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
@@ -26,6 +25,7 @@ namespace web_app {
 class AbstractWebAppDatabaseFactory;
 class WebApp;
 class WebAppProto;
+struct RegistryUpdateData;
 
 // Exclusively used from the UI thread.
 class WebAppDatabase {
@@ -34,13 +34,15 @@ class WebAppDatabase {
   ~WebAppDatabase();
 
   using RegistryOpenedCallback = base::OnceCallback<void(Registry registry)>;
-  using CompletionCallback = base::OnceCallback<void(bool success)>;
-  using AppsToWrite = base::flat_set<const WebApp*>;
-
   // Open existing or create new DB. Read all data and return it via callback.
   void OpenDatabase(RegistryOpenedCallback callback);
-  void WriteWebApps(AppsToWrite apps, CompletionCallback callback);
-  void DeleteWebApps(std::vector<AppId> app_ids, CompletionCallback callback);
+
+  using CompletionCallback = base::OnceCallback<void(bool success)>;
+  // There can be only 1 transaction at a time.
+  void BeginTransaction();
+  void CommitTransaction(const RegistryUpdateData& update_data,
+                         CompletionCallback callback);
+  void CancelTransaction();
 
   // Exposed for testing.
   static std::unique_ptr<WebAppProto> CreateWebAppProto(const WebApp& web_app);
@@ -63,9 +65,6 @@ class WebAppDatabase {
   void OnDataWritten(CompletionCallback callback,
                      const base::Optional<syncer::ModelError>& error);
 
-  void BeginTransaction();
-  void CommitTransaction(CompletionCallback callback);
-
   std::unique_ptr<syncer::ModelTypeStore> store_;
   std::unique_ptr<syncer::ModelTypeStore::WriteBatch> write_batch_;
   AbstractWebAppDatabaseFactory* database_factory_;

chrome/browser/web_applications/web_app_database_unittest.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/web_applications/test/web_app_test.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
+#include "chrome/browser/web_applications/web_app_registry_update.h"
 #include "chrome/browser/web_applications/web_app_sync_bridge.h"
 #include "components/sync/model/model_type_store.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -132,10 +133,6 @@ class WebAppDatabaseTest : public WebAppTest {
     return controller().database_factory();
   }
 
-  WebAppDatabase& database() {
-    return controller().sync_bridge().database_for_testing();
-  }
-
   WebAppRegistrar& registrar() { return controller().registrar(); }
 
   WebAppRegistrarMutable& mutable_registrar() {
@@ -157,19 +154,19 @@ TEST_F(WebAppDatabaseTest, WriteAndReadRegistry) {
 
   auto app = CreateWebApp(base_url, 0);
   auto app_id = app->app_id();
-  sync_bridge().RegisterApp(std::move(app));
+  controller().RegisterApp(std::move(app));
   EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
 
   for (int i = 1; i <= num_apps; ++i) {
     auto extra_app = CreateWebApp(base_url, i);
-    sync_bridge().RegisterApp(std::move(extra_app));
+    controller().RegisterApp(std::move(extra_app));
   }
   EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
 
-  sync_bridge().UnregisterApp(app_id);
+  controller().UnregisterApp(app_id);
   EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
 
-  sync_bridge().UnregisterAll();
+  controller().UnregisterAll();
   EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
 }
 
@@ -180,37 +177,51 @@ TEST_F(WebAppDatabaseTest, WriteAndDeleteAppsWithCallbacks) {
   const int num_apps = 10;
   const std::string base_url = "https://example.com/path";
 
-  Registry registry;
-  WebAppDatabase::AppsToWrite apps_to_write;
-  std::vector<AppId> apps_to_delete;
+  RegistryUpdateData::AppsToCreate apps_to_create;
+  RegistryUpdateData::AppsToDelete apps_to_delete;
+  Registry expected_registry;
 
   for (int i = 0; i < num_apps; ++i) {
-    auto app = CreateWebApp(base_url, i);
-    apps_to_write.insert(app.get());
+    std::unique_ptr<WebApp> app = CreateWebApp(base_url, i);
     apps_to_delete.push_back(app->app_id());
-    registry.emplace(app->app_id(), std::move(app));
+    apps_to_create.push_back(std::move(app));
+
+    std::unique_ptr<WebApp> expected_app = CreateWebApp(base_url, i);
+    expected_registry.emplace(expected_app->app_id(), std::move(expected_app));
   }
 
   {
     base::RunLoop run_loop;
-    database().WriteWebApps(std::move(apps_to_write),
-                            base::BindLambdaForTesting([&](bool success) {
-                              EXPECT_TRUE(success);
-                              run_loop.Quit();
-                            }));
+
+    std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+    for (std::unique_ptr<WebApp>& web_app : apps_to_create)
+      update->CreateApp(std::move(web_app));
+
+    sync_bridge().CommitUpdate(std::move(update),
+                               base::BindLambdaForTesting([&](bool success) {
+                                 EXPECT_TRUE(success);
+                                 run_loop.Quit();
+                               }));
     run_loop.Run();
 
     Registry registry_written = database_factory().ReadRegistry();
-    EXPECT_TRUE(IsRegistryEqual(registry_written, registry));
+    EXPECT_TRUE(IsRegistryEqual(registry_written, expected_registry));
   }
 
   {
     base::RunLoop run_loop;
-    database().DeleteWebApps(std::move(apps_to_delete),
-                             base::BindLambdaForTesting([&](bool success) {
-                               EXPECT_TRUE(success);
-                               run_loop.Quit();
-                             }));
+
+    std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+    for (const AppId& app_id : apps_to_delete)
+      update->DeleteApp(app_id);
+
+    sync_bridge().CommitUpdate(std::move(update),
+                               base::BindLambdaForTesting([&](bool success) {
+                                 EXPECT_TRUE(success);
+                                 run_loop.Quit();
+                               }));
     run_loop.Run();
 
     Registry registry_deleted = database_factory().ReadRegistry();
@@ -252,7 +263,7 @@ TEST_F(WebAppDatabaseTest, WebAppWithoutOptionalFields) {
   EXPECT_TRUE(app->scope().is_empty());
   EXPECT_FALSE(app->theme_color().has_value());
   EXPECT_TRUE(app->icons().empty());
-  sync_bridge().RegisterApp(std::move(app));
+  controller().RegisterApp(std::move(app));
 
   Registry registry = database_factory().ReadRegistry();
   EXPECT_EQ(1UL, registry.size());
@@ -298,7 +309,7 @@ TEST_F(WebAppDatabaseTest, WebAppWithManyIcons) {
   }
   app->SetIcons(std::move(icons));
 
-  sync_bridge().RegisterApp(std::move(app));
+  controller().RegisterApp(std::move(app));
 
   Registry registry = database_factory().ReadRegistry();
   EXPECT_EQ(1UL, registry.size());

chrome/browser/web_applications/web_app_icon_manager_unittest.cc

@@ -116,7 +116,7 @@ TEST_F(WebAppIconManagerTest, WriteAndReadIcon) {
 
   web_app->SetIcons(ListIcons(web_app->launch_url(), sizes_px));
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  controller().RegisterApp(std::move(web_app));
 
   {
     base::RunLoop run_loop;
@@ -145,7 +145,7 @@ TEST_F(WebAppIconManagerTest, ReadIconFailed) {
   icons.push_back({icon_url, icon_size_px});
   web_app->SetIcons(std::move(icons));
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  controller().RegisterApp(std::move(web_app));
 
   // Request non-existing icon size.
   EXPECT_FALSE(
@@ -175,7 +175,7 @@ TEST_F(WebAppIconManagerTest, FindExact) {
 
   web_app->SetIcons(ListIcons(web_app->launch_url(), sizes_px));
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  controller().RegisterApp(std::move(web_app));
 
   {
     const bool icon_requested = icon_manager().ReadIcon(
@@ -210,7 +210,7 @@ TEST_F(WebAppIconManagerTest, FindSmallest) {
 
   web_app->SetIcons(ListIcons(web_app->launch_url(), sizes_px));
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  controller().RegisterApp(std::move(web_app));
 
   {
     const bool icon_requested = icon_manager().ReadSmallestIcon(

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -131,7 +131,7 @@ void WebAppInstallFinalizer::FinalizeInstall(
 
   icon_manager_->WriteData(
       std::move(app_id), std::make_unique<WebApplicationInfo>(web_app_info),
-      base::BindOnce(&WebAppInstallFinalizer::OnDataWritten,
+      base::BindOnce(&WebAppInstallFinalizer::OnIconsDataWritten,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback),
                      std::move(web_app)));
 }
@@ -150,23 +150,40 @@ void WebAppInstallFinalizer::UninstallWebApp(const AppId& app_id,
   NOTIMPLEMENTED();
 }
 
-void WebAppInstallFinalizer::OnDataWritten(InstallFinalizedCallback callback,
-                                           std::unique_ptr<WebApp> web_app,
-                                           bool success) {
+void WebAppInstallFinalizer::OnIconsDataWritten(
+    InstallFinalizedCallback callback,
+    std::unique_ptr<WebApp> web_app,
+    bool success) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!success) {
     std::move(callback).Run(AppId(), InstallResultCode::kWriteDataFailed);
     return;
   }
 
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge_->BeginUpdate();
+
   AppId app_id = web_app->app_id();
+  update->CreateApp(std::move(web_app));
 
-  sync_bridge_->RegisterApp(std::move(web_app));
-  // TODO(loyso): NotifyWebAppInstalled should be a part of RegisterApp.
-  registrar().NotifyWebAppInstalled(app_id);
+  sync_bridge_->CommitUpdate(
+      std::move(update),
+      base::BindOnce(&WebAppInstallFinalizer::OnDatabaseCommitCompleted,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
+                     std::move(app_id)));
+}
+
+void WebAppInstallFinalizer::OnDatabaseCommitCompleted(
+    InstallFinalizedCallback callback,
+    const AppId& app_id,
+    bool success) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!success) {
+    std::move(callback).Run(AppId(), InstallResultCode::kWriteDataFailed);
+    return;
+  }
 
-  std::move(callback).Run(std::move(app_id),
-                          InstallResultCode::kSuccessNewInstall);
+  registrar().NotifyWebAppInstalled(app_id);
+  std::move(callback).Run(app_id, InstallResultCode::kSuccessNewInstall);
 }
 
 bool WebAppInstallFinalizer::CanCreateOsShortcuts() const {

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -44,9 +44,12 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
   bool CanUserUninstallFromSync(const AppId& app_id) const override;
 
  private:
-  void OnDataWritten(InstallFinalizedCallback callback,
-                     std::unique_ptr<WebApp> web_app,
-                     bool success);
+  void OnIconsDataWritten(InstallFinalizedCallback callback,
+                          std::unique_ptr<WebApp> web_app,
+                          bool success);
+  void OnDatabaseCommitCompleted(InstallFinalizedCallback callback,
+                                 const AppId& app_id,
+                                 bool success);
 
   WebAppSyncBridge* sync_bridge_;
   WebAppIconManager* const icon_manager_;

chrome/browser/web_applications/web_app_registrar_unittest.cc

@@ -73,14 +73,25 @@ class WebAppRegistrarTest : public WebAppTest {
   std::set<AppId> RegisterAppsForTesting(Registry registry) {
     std::set<AppId> ids;
 
+    ScopedRegistryUpdate update(&sync_bridge());
     for (auto& kv : registry) {
       ids.insert(kv.first);
-      sync_bridge().RegisterApp(std::move(kv.second));
+      update->CreateApp(std::move(kv.second));
     }
 
     return ids;
   }
 
+  void RegisterApp(std::unique_ptr<WebApp> web_app) {
+    controller().RegisterApp(std::move(web_app));
+  }
+
+  void UnregisterApp(const AppId& app_id) {
+    controller().UnregisterApp(app_id);
+  }
+
+  void UnregisterAll() { controller().UnregisterAll(); }
+
   AppId InitRegistrarWithApp(std::unique_ptr<WebApp> app) {
     DCHECK(registrar().is_empty());
 
@@ -175,7 +186,7 @@ TEST_F(WebAppRegistrarTest, CreateRegisterUnregister) {
   EXPECT_EQ(nullptr, registrar().GetAppById(app_id2));
   EXPECT_TRUE(registrar().is_empty());
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  RegisterApp(std::move(web_app));
   EXPECT_TRUE(registrar().IsInstalled(app_id));
   const WebApp* app = registrar().GetAppById(app_id);
 
@@ -189,13 +200,13 @@ TEST_F(WebAppRegistrarTest, CreateRegisterUnregister) {
   EXPECT_EQ(nullptr, registrar().GetAppById(app_id2));
   EXPECT_FALSE(registrar().is_empty());
 
-  sync_bridge().RegisterApp(std::move(web_app2));
+  RegisterApp(std::move(web_app2));
   EXPECT_TRUE(registrar().IsInstalled(app_id2));
   const WebApp* app2 = registrar().GetAppById(app_id2);
   EXPECT_EQ(app_id2, app2->app_id());
   EXPECT_FALSE(registrar().is_empty());
 
-  sync_bridge().UnregisterApp(app_id);
+  UnregisterApp(app_id);
   EXPECT_FALSE(registrar().IsInstalled(app_id));
   EXPECT_EQ(nullptr, registrar().GetAppById(app_id));
   EXPECT_FALSE(registrar().is_empty());
@@ -205,7 +216,7 @@ TEST_F(WebAppRegistrarTest, CreateRegisterUnregister) {
   EXPECT_TRUE(registrar().IsInstalled(app_id2));
   EXPECT_EQ(app_id2, app2->app_id());
 
-  sync_bridge().UnregisterApp(app_id2);
+  UnregisterApp(app_id2);
   EXPECT_FALSE(registrar().IsInstalled(app_id2));
   EXPECT_EQ(nullptr, registrar().GetAppById(app_id2));
   EXPECT_TRUE(registrar().is_empty());
@@ -215,10 +226,10 @@ TEST_F(WebAppRegistrarTest, DestroyRegistrarOwningRegisteredApps) {
   controller().Init();
 
   auto web_app = CreateWebApp("https://example.com/path");
-  sync_bridge().RegisterApp(std::move(web_app));
+  RegisterApp(std::move(web_app));
 
   auto web_app2 = CreateWebApp("https://example.com/path2");
-  sync_bridge().RegisterApp(std::move(web_app2));
+  RegisterApp(std::move(web_app2));
 
   controller().DestroySubsystems();
 }
@@ -260,7 +271,7 @@ TEST_F(WebAppRegistrarTest, DoForEachAndUnregisterAllApps) {
   EXPECT_TRUE(ids.empty());
 
   EXPECT_FALSE(registrar().is_empty());
-  sync_bridge().UnregisterAll();
+  UnregisterAll();
   EXPECT_TRUE(registrar().is_empty());
 }
 
@@ -271,18 +282,18 @@ TEST_F(WebAppRegistrarTest, WebAppSyncBridge) {
   auto web_app = CreateWebApp("https://example.com/path");
   const AppId app_id = web_app->app_id();
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  RegisterApp(std::move(web_app));
 
   EXPECT_EQ(101UL, database_factory().ReadAllAppIds().size());
   EXPECT_EQ(101UL, controller().mutable_registrar().registry().size());
 
   // Remove 1 app after Init.
-  sync_bridge().UnregisterApp(app_id);
+  UnregisterApp(app_id);
   EXPECT_EQ(100UL, controller().mutable_registrar().registry().size());
   EXPECT_EQ(100UL, database_factory().ReadAllAppIds().size());
 
   // Remove 100 apps after Init.
-  sync_bridge().UnregisterAll();
+  UnregisterAll();
   EXPECT_TRUE(database_factory().ReadAllAppIds().empty());
   EXPECT_TRUE(registrar().is_empty());
 }
@@ -311,7 +322,7 @@ TEST_F(WebAppRegistrarTest, GetAppDataFields) {
   web_app->SetLaunchContainer(launch_container);
   web_app->SetIsLocallyInstalled(/*is_locally_installed*/ false);
 
-  sync_bridge().RegisterApp(std::move(web_app));
+  RegisterApp(std::move(web_app));
 
   EXPECT_EQ(name, registrar().GetAppShortName(app_id));
   EXPECT_EQ(description, registrar().GetAppDescription(app_id));
@@ -358,7 +369,7 @@ TEST_F(WebAppRegistrarTest, CanFindAppsInScope) {
 
   auto app1 = CreateWebApp(app1_scope.spec());
   app1->SetScope(app1_scope);
-  sync_bridge().RegisterApp(std::move(app1));
+  RegisterApp(std::move(app1));
 
   in_scope = registrar().FindAppsInScope(origin_scope);
   EXPECT_THAT(in_scope, testing::UnorderedElementsAre(app1_id));
@@ -368,7 +379,7 @@ TEST_F(WebAppRegistrarTest, CanFindAppsInScope) {
 
   auto app2 = CreateWebApp(app2_scope.spec());
   app2->SetScope(app2_scope);
-  sync_bridge().RegisterApp(std::move(app2));
+  RegisterApp(std::move(app2));
 
   in_scope = registrar().FindAppsInScope(origin_scope);
   EXPECT_THAT(in_scope, testing::UnorderedElementsAre(app1_id, app2_id));
@@ -381,7 +392,7 @@ TEST_F(WebAppRegistrarTest, CanFindAppsInScope) {
 
   auto app3 = CreateWebApp(app3_scope.spec());
   app3->SetScope(app3_scope);
-  sync_bridge().RegisterApp(std::move(app3));
+  RegisterApp(std::move(app3));
 
   in_scope = registrar().FindAppsInScope(origin_scope);
   EXPECT_THAT(in_scope, testing::UnorderedElementsAre(app1_id, app2_id));
@@ -405,7 +416,7 @@ TEST_F(WebAppRegistrarTest, CanFindAppWithUrlInScope) {
 
   auto app1 = CreateWebApp(app1_scope.spec());
   app1->SetScope(app1_scope);
-  sync_bridge().RegisterApp(std::move(app1));
+  RegisterApp(std::move(app1));
 
   base::Optional<AppId> app2_match =
       registrar().FindAppWithUrlInScope(app2_scope);
@@ -418,11 +429,11 @@ TEST_F(WebAppRegistrarTest, CanFindAppWithUrlInScope) {
 
   auto app2 = CreateWebApp(app2_scope.spec());
   app2->SetScope(app2_scope);
-  sync_bridge().RegisterApp(std::move(app2));
+  RegisterApp(std::move(app2));
 
   auto app3 = CreateWebApp(app3_scope.spec());
   app3->SetScope(app3_scope);
-  sync_bridge().RegisterApp(std::move(app3));
+  RegisterApp(std::move(app3));
 
   base::Optional<AppId> origin_match =
       registrar().FindAppWithUrlInScope(origin_scope);
@@ -459,7 +470,7 @@ TEST_F(WebAppRegistrarTest, CanFindShortcutWithUrlInScope) {
 
   // Implicit scope "https://example.com/app/"
   auto app1 = CreateWebApp(app1_launch.spec());
-  sync_bridge().RegisterApp(std::move(app1));
+  RegisterApp(std::move(app1));
 
   base::Optional<AppId> app2_match =
       registrar().FindAppWithUrlInScope(app2_page);
@@ -470,10 +481,10 @@ TEST_F(WebAppRegistrarTest, CanFindShortcutWithUrlInScope) {
   EXPECT_FALSE(app3_match);
 
   auto app2 = CreateWebApp(app2_launch.spec());
-  sync_bridge().RegisterApp(std::move(app2));
+  RegisterApp(std::move(app2));
 
   auto app3 = CreateWebApp(app3_launch.spec());
-  sync_bridge().RegisterApp(std::move(app3));
+  RegisterApp(std::move(app3));
 
   base::Optional<AppId> app1_match =
       registrar().FindAppWithUrlInScope(app1_page);
@@ -501,14 +512,14 @@ TEST_F(WebAppRegistrarTest, FindPwaOverShortcut) {
   const GURL app3_launch("https://example.com/app/specific/launch3");
 
   auto app1 = CreateWebApp(app1_launch.spec());
-  sync_bridge().RegisterApp(std::move(app1));
+  RegisterApp(std::move(app1));
 
   auto app2 = CreateWebApp(app2_scope.spec());
   app2->SetScope(app2_scope);
-  sync_bridge().RegisterApp(std::move(app2));
+  RegisterApp(std::move(app2));
 
   auto app3 = CreateWebApp(app3_launch.spec());
-  sync_bridge().RegisterApp(std::move(app3));
+  RegisterApp(std::move(app3));
 
   base::Optional<AppId> app2_match =
       registrar().FindAppWithUrlInScope(app2_page);

chrome/browser/web_applications/web_app_registry_update.cc

@@ -5,29 +5,59 @@
 #include "chrome/browser/web_applications/web_app_registry_update.h"
 
 #include "base/bind_helpers.h"
+#include "base/stl_util.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "chrome/browser/web_applications/web_app_sync_bridge.h"
 
 namespace web_app {
 
+RegistryUpdateData::RegistryUpdateData() = default;
+
+RegistryUpdateData::~RegistryUpdateData() = default;
+
+bool RegistryUpdateData::IsEmpty() const {
+  return apps_to_create.empty() && apps_to_delete.empty() &&
+         apps_to_update.empty();
+}
+
 WebAppRegistryUpdate::WebAppRegistryUpdate(
     WebAppRegistrarMutable* mutable_registrar)
-    : mutable_registrar_(mutable_registrar) {}
+    : mutable_registrar_(mutable_registrar) {
+  DCHECK(mutable_registrar_);
+  update_data_ = std::make_unique<RegistryUpdateData>();
+}
 
 WebAppRegistryUpdate::~WebAppRegistryUpdate() = default;
 
+void WebAppRegistryUpdate::CreateApp(std::unique_ptr<WebApp> web_app) {
+  DCHECK(update_data_);
+  DCHECK(!web_app->app_id().empty());
+  DCHECK(!mutable_registrar_->GetAppById(web_app->app_id()));
+  DCHECK(!base::Contains(update_data_->apps_to_create, web_app));
+
+  update_data_->apps_to_create.push_back(std::move(web_app));
+}
+
+void WebAppRegistryUpdate::DeleteApp(const AppId& app_id) {
+  DCHECK(update_data_);
+  DCHECK(!app_id.empty());
+  DCHECK(mutable_registrar_->GetAppById(app_id));
+  DCHECK(!base::Contains(update_data_->apps_to_delete, app_id));
+
+  update_data_->apps_to_delete.push_back(app_id);
+}
+
 WebApp* WebAppRegistryUpdate::UpdateApp(const AppId& app_id) {
+  DCHECK(update_data_);
   WebApp* app = mutable_registrar_->GetAppByIdMutable(app_id);
   if (app)
-    apps_to_update_.insert(app);
+    update_data_->apps_to_update.insert(app);
 
   return app;
 }
 
-WebAppRegistryUpdate::AppsToUpdate WebAppRegistryUpdate::TakeAppsToUpdate() {
-  AppsToUpdate apps_to_update = std::move(apps_to_update_);
-  apps_to_update_.clear();
-  return apps_to_update;
+std::unique_ptr<RegistryUpdateData> WebAppRegistryUpdate::TakeUpdateData() {
+  return std::move(update_data_);
 }
 
 ScopedRegistryUpdate::ScopedRegistryUpdate(WebAppSyncBridge* sync_bridge)

chrome/browser/web_applications/web_app_registry_update.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_REGISTRY_UPDATE_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/containers/flat_set.h"
@@ -18,25 +19,45 @@ class WebApp;
 class WebAppRegistrarMutable;
 class WebAppSyncBridge;
 
+// A raw registry update data.
+struct RegistryUpdateData {
+  RegistryUpdateData();
+  ~RegistryUpdateData();
+
+  using AppsToCreate = std::vector<std::unique_ptr<WebApp>>;
+  AppsToCreate apps_to_create;
+
+  using AppsToDelete = std::vector<AppId>;
+  AppsToDelete apps_to_delete;
+
+  using AppsToUpdate = base::flat_set<const WebApp*>;
+  AppsToUpdate apps_to_update;
+
+  bool IsEmpty() const;
+
+  DISALLOW_COPY_AND_ASSIGN(RegistryUpdateData);
+};
+
 // An explicit writable "view" for the registry. Any write operations must be
 // batched as a part of WebAppRegistryUpdate object. Effectively
 // WebAppRegistryUpdate is a part of WebAppSyncBridge class.
-//
-// TODO(loyso): Support creation and deletion of apps as a part of single
-// update (As in Database CRUD: Create/Update/Delete).
 class WebAppRegistryUpdate {
  public:
   explicit WebAppRegistryUpdate(WebAppRegistrarMutable* mutable_registrar);
   ~WebAppRegistryUpdate();
 
-  // Acquire a mutable app object to set new field values.
+  // Register a new app.
+  void CreateApp(std::unique_ptr<WebApp> web_app);
+  // Delete registered app.
+  void DeleteApp(const AppId& app_id);
+  // Acquire a mutable existing app to set new field values.
   WebApp* UpdateApp(const AppId& app_id);
 
-  using AppsToUpdate = base::flat_set<const WebApp*>;
-  AppsToUpdate TakeAppsToUpdate();
+  const RegistryUpdateData& update_data() const { return *update_data_; }
+  std::unique_ptr<RegistryUpdateData> TakeUpdateData();
 
  private:
-  AppsToUpdate apps_to_update_;
+  std::unique_ptr<RegistryUpdateData> update_data_;
   WebAppRegistrarMutable* mutable_registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(WebAppRegistryUpdate);

chrome/browser/web_applications/web_app_sync_bridge.cc

@@ -8,9 +8,9 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/logging.h"
-#include "base/memory/ptr_util.h"
 #include "base/optional.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/browser/web_applications/web_app_database.h"
 #include "chrome/browser/web_applications/web_app_database_factory.h"
 #include "chrome/browser/web_applications/web_app_registry_update.h"
 #include "chrome/common/channel_info.h"
@@ -49,48 +49,13 @@ WebAppSyncBridge::WebAppSyncBridge(
 
 WebAppSyncBridge::~WebAppSyncBridge() = default;
 
-void WebAppSyncBridge::RegisterApp(std::unique_ptr<WebApp> web_app) {
-  registrar_->CountMutation();
-
-  const auto app_id = web_app->app_id();
-  DCHECK(!app_id.empty());
-  DCHECK(!registrar_->GetAppById(app_id));
-
-  // TODO(loyso): Expose CompletionCallback as RegisterApp argument.
-  database_->WriteWebApps({web_app.get()}, base::DoNothing());
-
-  registrar_->registry().emplace(app_id, std::move(web_app));
-}
-
-std::unique_ptr<WebApp> WebAppSyncBridge::UnregisterApp(const AppId& app_id) {
-  registrar_->CountMutation();
-
-  DCHECK(!app_id.empty());
-
-  // TODO(loyso): Expose CompletionCallback as UnregisterApp argument.
-  database_->DeleteWebApps({app_id}, base::DoNothing());
-
-  auto it = registrar_->registry().find(app_id);
-  DCHECK(it != registrar_->registry().end());
-
-  auto web_app = std::move(it->second);
-  registrar_->registry().erase(it);
-  return web_app;
-}
-
-void WebAppSyncBridge::UnregisterAll() {
-  registrar_->CountMutation();
-
-  // TODO(loyso): Expose CompletionCallback as UnregisterAll argument.
-  database_->DeleteWebApps(registrar_->GetAppIds(), base::DoNothing());
-  registrar_->registry().clear();
-}
-
 std::unique_ptr<WebAppRegistryUpdate> WebAppSyncBridge::BeginUpdate() {
   DCHECK(!is_in_update_);
   is_in_update_ = true;
 
-  return base::WrapUnique(new WebAppRegistryUpdate(registrar_));
+  database_->BeginTransaction();
+
+  return std::make_unique<WebAppRegistryUpdate>(registrar_);
 }
 
 void WebAppSyncBridge::CommitUpdate(
@@ -99,28 +64,24 @@ void WebAppSyncBridge::CommitUpdate(
   DCHECK(is_in_update_);
   is_in_update_ = false;
 
-  if (update == nullptr) {
+  if (update == nullptr || update->update_data().IsEmpty()) {
+    database_->CancelTransaction();
     std::move(callback).Run(/*success*/ true);
     return;
   }
 
-  WebAppRegistryUpdate::AppsToUpdate apps_to_update =
-      update->TakeAppsToUpdate();
+  CheckRegistryUpdateData(update->update_data());
 
-  if (apps_to_update.empty()) {
-    std::move(callback).Run(/*success*/ true);
-    return;
-  }
+  registrar_->CountMutation();
 
-#if DCHECK_IS_ON()
-  for (auto* app : apps_to_update)
-    DCHECK(registrar_->GetAppById(app->app_id()));
-#endif
+  std::unique_ptr<RegistryUpdateData> update_data = update->TakeUpdateData();
 
-  database_->WriteWebApps(
-      std::move(apps_to_update),
+  database_->CommitTransaction(
+      *update_data,
       base::BindOnce(&WebAppSyncBridge::OnDataWritten,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+
+  UpdateRegistrar(std::move(update_data));
 }
 
 void WebAppSyncBridge::Init(base::OnceClosure callback) {
@@ -141,6 +102,35 @@ WebAppSyncBridge* WebAppSyncBridge::AsWebAppSyncBridge() {
   return this;
 }
 
+void WebAppSyncBridge::CheckRegistryUpdateData(
+    const RegistryUpdateData& update_data) const {
+#if DCHECK_IS_ON()
+  for (const std::unique_ptr<WebApp>& web_app : update_data.apps_to_create)
+    DCHECK(!registrar_->GetAppById(web_app->app_id()));
+
+  for (const AppId& app_id : update_data.apps_to_delete)
+    DCHECK(registrar_->GetAppById(app_id));
+
+  for (const WebApp* web_app : update_data.apps_to_update)
+    DCHECK(registrar_->GetAppById(web_app->app_id()));
+#endif
+}
+
+void WebAppSyncBridge::UpdateRegistrar(
+    std::unique_ptr<RegistryUpdateData> update_data) {
+  for (std::unique_ptr<WebApp>& web_app : update_data->apps_to_create) {
+    AppId app_id = web_app->app_id();
+    DCHECK(!registrar_->GetAppById(app_id));
+    registrar_->registry().emplace(std::move(app_id), std::move(web_app));
+  }
+
+  for (const AppId& app_id : update_data->apps_to_delete) {
+    auto it = registrar_->registry().find(app_id);
+    DCHECK(it != registrar_->registry().end());
+    registrar_->registry().erase(it);
+  }
+}
+
 void WebAppSyncBridge::OnDatabaseOpened(base::OnceClosure callback,
                                         Registry registry) {
   registrar_->InitRegistry(std::move(registry));

chrome/browser/web_applications/web_app_sync_bridge.h

@@ -13,7 +13,6 @@
 #include "base/optional.h"
 #include "chrome/browser/web_applications/components/app_registry_controller.h"
 #include "chrome/browser/web_applications/web_app.h"
-#include "chrome/browser/web_applications/web_app_database.h"
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "components/sync/model/model_type_sync_bridge.h"
 
@@ -26,7 +25,9 @@ class ModelTypeChangeProcessor;
 namespace web_app {
 
 class AbstractWebAppDatabaseFactory;
+class WebAppDatabase;
 class WebAppRegistryUpdate;
+struct RegistryUpdateData;
 
 // The sync bridge exclusively owns ModelTypeChangeProcessor and WebAppDatabase
 // (the storage).
@@ -44,12 +45,6 @@ class WebAppSyncBridge : public AppRegistryController,
       std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor);
   ~WebAppSyncBridge() override;
 
-  // TODO(loyso): Erase UnregisterAll. Move Register/Unregister app methods to
-  // BeginUpdate/CommitUpdate API.
-  void RegisterApp(std::unique_ptr<WebApp> web_app);
-  std::unique_ptr<WebApp> UnregisterApp(const AppId& app_id);
-  void UnregisterAll();
-
   using CommitCallback = base::OnceCallback<void(bool success)>;
   // This is the writable API for the registry. Any updates will be written to
   // LevelDb and sync service. There can be only 1 update at a time.
@@ -63,9 +58,11 @@ class WebAppSyncBridge : public AppRegistryController,
                              LaunchContainer launch_container) override;
   WebAppSyncBridge* AsWebAppSyncBridge() override;
 
-  WebAppDatabase& database_for_testing() { return *database_; }
-
  private:
+  void CheckRegistryUpdateData(const RegistryUpdateData& update_data) const;
+  // Update the in-memory model.
+  void UpdateRegistrar(std::unique_ptr<RegistryUpdateData> update_data);
+
   void OnDatabaseOpened(base::OnceClosure callback, Registry registry);
   void OnDataWritten(CommitCallback callback, bool success);