Reland WebApp: Improve WebAppSyncBridge::UpdateSync to use both app states.

This is a reland of
https://chromium-review.googlesource.com/c/chromium/src/+/1914005 CL.

The problem was that two IsTrackingMetadata() calls were under DCHECK macro.
Those calls didn't happen in release builds with `dcheck_always_on=false`
GN arg.

The fix: promote DCHECK to CHECK to make it debug/release/dcheck_always
agnostic.

Original description:

Improve WebAppSyncBridge::UpdateSync to use both current_state (before commit)
and new_state (to be commit).

In the future, it will help to skip local updates which don't touch sync data.

Add WebAppSyncBridge::CommitUpdate unit tests (a.k.a. Local Changes).

These tests is a promised follow up for this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1830494

This code is disabled by default behind kDesktopPWAsWithoutExtensions and
kDesktopPWAsUSS base features.

TBR=treib@chromium.org

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

chrome/browser/web_applications/web_app_sync_bridge.cc

@@ -244,36 +244,39 @@ void WebAppSyncBridge::UpdateSync(
   if (!change_processor()->IsTrackingMetadata())
     return;
 
-  for (const std::unique_ptr<WebApp>& app : update_data.apps_to_create) {
-    if (app->IsSynced()) {
-      change_processor()->Put(app->app_id(), CreateSyncEntityData(*app),
+  for (const std::unique_ptr<WebApp>& new_app : update_data.apps_to_create) {
+    if (new_app->IsSynced()) {
+      change_processor()->Put(new_app->app_id(), CreateSyncEntityData(*new_app),
                               metadata_change_list);
     }
   }
 
-  // An app may obtain or may loose IsSynced flag without being deleted. We
-  // should conservatively include or exclude the app from the synced apps
-  // subset. TODO(loyso): Use previous state of the app (and CoW) to detect
-  // if IsSynced flag changed.
-  //
-  // TODO(loyso): Send an update to sync server only if any sync-specific
-  // data was changed. Implement some dirty flags in WebApp setter methods.
-  for (const std::unique_ptr<WebApp>& app : update_data.apps_to_update) {
-    if (app->IsSynced()) {
-      change_processor()->Put(app->app_id(), CreateSyncEntityData(*app),
+  for (const std::unique_ptr<WebApp>& new_state : update_data.apps_to_update) {
+    const AppId& app_id = new_state->app_id();
+    // Find the current state of the app to be overritten.
+    const WebApp* current_state = registrar_->GetAppById(app_id);
+    DCHECK(current_state);
+
+    // Include the app in the sync "view" if IsSynced flag becomes true. Update
+    // the app if IsSynced flag stays true. Exclude the app from the sync "view"
+    // if IsSynced flag becomes false.
+    //
+    // TODO(loyso): Send an update to sync server only if any sync-specific
+    // data was changed. Implement some dirty flags in WebApp setter methods.
+    if (new_state->IsSynced()) {
+      change_processor()->Put(app_id, CreateSyncEntityData(*new_state),
                               metadata_change_list);
-    } else {
-      change_processor()->Delete(app->app_id(), metadata_change_list);
+    } else if (current_state->IsSynced()) {
+      change_processor()->Delete(app_id, metadata_change_list);
     }
   }
 
-  // We should unconditionally delete from sync (in case IsSynced flag was
-  // removed during the update). TODO(loyso): Use previous state of the app (and
-  // CoW) to detect if IsSynced flag changed.
-  for (const AppId& app_id : update_data.apps_to_delete) {
-    const WebApp* app = registrar_->GetAppById(app_id);
-    DCHECK(app);
-    change_processor()->Delete(app_id, metadata_change_list);
+  for (const AppId& app_id_to_delete : update_data.apps_to_delete) {
+    const WebApp* current_state = registrar_->GetAppById(app_id_to_delete);
+    DCHECK(current_state);
+    // Exclude the app from the sync "view" if IsSynced flag was true.
+    if (current_state->IsSynced())
+      change_processor()->Delete(app_id_to_delete, metadata_change_list);
   }
 }
 
@@ -417,7 +420,7 @@ WebAppSyncBridge::CreateMetadataChangeList() {
 base::Optional<syncer::ModelError> WebAppSyncBridge::MergeSyncData(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
     syncer::EntityChangeList entity_data) {
-  DCHECK(change_processor()->IsTrackingMetadata());
+  CHECK(change_processor()->IsTrackingMetadata());
 
   auto update_local_data = std::make_unique<RegistryUpdateData>();
 
@@ -438,7 +441,7 @@ base::Optional<syncer::ModelError> WebAppSyncBridge::MergeSyncData(
 base::Optional<syncer::ModelError> WebAppSyncBridge::ApplySyncChanges(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
     syncer::EntityChangeList entity_changes) {
-  DCHECK(change_processor()->IsTrackingMetadata());
+  CHECK(change_processor()->IsTrackingMetadata());
 
   auto update_local_data = std::make_unique<RegistryUpdateData>();
 

chrome/browser/web_applications/web_app_sync_bridge_unittest.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/web_applications/test/test_web_app_registry_controller.h"
 #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_registry_update.h"
 #include "components/sync/model/data_batch.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/protocol/web_app_specifics.pb.h"
@@ -40,8 +41,9 @@ void RemoveWebAppFromAppsList(AppsList* apps_list, const AppId& app_id) {
   });
 }
 
-bool IsSyncDataEqual(const WebApp& expected_app,
-                     const syncer::EntityData& entity_data) {
+bool IsSyncDataEqualIfApplied(const WebApp& expected_app,
+                              std::unique_ptr<WebApp> app_to_apply_sync_data,
+                              const syncer::EntityData& entity_data) {
   if (!entity_data.specifics.has_web_app())
     return false;
 
@@ -49,10 +51,17 @@ bool IsSyncDataEqual(const WebApp& expected_app,
   if (expected_app.app_id() != GenerateAppIdFromURL(sync_launch_url))
     return false;
 
-  auto app_from_sync = std::make_unique<WebApp>(expected_app.app_id());
-  // ApplySyncDataToApp enforces kSync source on |app_from_sync|.
-  ApplySyncDataToApp(entity_data.specifics.web_app(), app_from_sync.get());
-  return expected_app == *app_from_sync;
+  // ApplySyncDataToApp enforces kSync source on |app_to_apply_sync_data|.
+  ApplySyncDataToApp(entity_data.specifics.web_app(),
+                     app_to_apply_sync_data.get());
+  return expected_app == *app_to_apply_sync_data;
+}
+
+bool IsSyncDataEqual(const WebApp& expected_app,
+                     const syncer::EntityData& entity_data) {
+  auto app_to_apply_sync_data = std::make_unique<WebApp>(expected_app.app_id());
+  return IsSyncDataEqualIfApplied(
+      expected_app, std::move(app_to_apply_sync_data), entity_data);
 }
 
 bool SyncDataBatchMatchesRegistry(
@@ -235,6 +244,17 @@ class WebAppSyncBridgeTest : public WebAppTest {
                     callback) { ADD_FAILURE(); }));
   }
 
+  void CommitUpdate(std::unique_ptr<WebAppRegistryUpdate> update) {
+    base::RunLoop run_loop;
+    sync_bridge().CommitUpdate(
+        std::move(update),
+        base::BindLambdaForTesting([&run_loop](bool success) {
+          ASSERT_TRUE(success);
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+  }
+
  protected:
   TestWebAppRegistryController& controller() {
     return *test_registry_controller_;
@@ -735,4 +755,268 @@ TEST_F(WebAppSyncBridgeTest,
   EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
 }
 
+TEST_F(WebAppSyncBridgeTest, CommitUpdate_CommitWhileNotTrackingMetadata) {
+  EXPECT_CALL(processor(), ModelReadyToSync(_)).Times(1);
+  InitSyncBridge();
+
+  AppsList sync_apps = CreateAppsList("https://example.com/", 10);
+  Registry expected_registry;
+  InsertAppsListIntoRegistry(&expected_registry, sync_apps);
+
+  EXPECT_CALL(processor(), Put(_, _, _)).Times(0);
+  EXPECT_CALL(processor(), Delete(_, _)).Times(0);
+  EXPECT_CALL(processor(), IsTrackingMetadata())
+      .WillOnce(testing::Return(false));
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (const std::unique_ptr<WebApp>& app : sync_apps)
+    update->CreateApp(std::make_unique<WebApp>(*app));
+
+  CommitUpdate(std::move(update));
+  testing::Mock::VerifyAndClear(&processor());
+
+  // Do MergeSyncData next.
+  base::RunLoop run_loop;
+  ON_CALL(processor(), Put(_, _, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         std::unique_ptr<syncer::EntityData> entity_data,
+                         syncer::MetadataChangeList* metadata) {
+        EXPECT_TRUE(RemoveEntityDataAppFromAppsList(storage_key, *entity_data,
+                                                    &sync_apps));
+        if (sync_apps.empty())
+          run_loop.Quit();
+      });
+
+  EXPECT_CALL(processor(), Delete(_, _)).Times(0);
+  EXPECT_CALL(processor(), IsTrackingMetadata())
+      .WillOnce(testing::Return(true));
+
+  sync_bridge().MergeSyncData(sync_bridge().CreateMetadataChangeList(),
+                              syncer::EntityChangeList{});
+  run_loop.Run();
+
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), expected_registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
+TEST_F(WebAppSyncBridgeTest, CommitUpdate_CreateSyncApp) {
+  InitSyncBridge();
+
+  AppsList sync_apps = CreateAppsList("https://example.com/", 10);
+  Registry expected_registry;
+  InsertAppsListIntoRegistry(&expected_registry, sync_apps);
+
+  ON_CALL(processor(), Put(_, _, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         std::unique_ptr<syncer::EntityData> entity_data,
+                         syncer::MetadataChangeList* metadata) {
+        ASSERT_TRUE(base::Contains(expected_registry, storage_key));
+        const std::unique_ptr<WebApp>& expected_app =
+            expected_registry.at(storage_key);
+        EXPECT_TRUE(IsSyncDataEqual(*expected_app, *entity_data));
+        RemoveWebAppFromAppsList(&sync_apps, storage_key);
+      });
+  EXPECT_CALL(processor(), Delete(_, _)).Times(0);
+  EXPECT_CALL(processor(), IsTrackingMetadata())
+      .WillOnce(testing::Return(true));
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (const std::unique_ptr<WebApp>& app : sync_apps)
+    update->CreateApp(std::make_unique<WebApp>(*app));
+
+  CommitUpdate(std::move(update));
+
+  EXPECT_TRUE(sync_apps.empty());
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), expected_registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
+TEST_F(WebAppSyncBridgeTest, CommitUpdate_UpdateSyncApp) {
+  AppsList sync_apps = CreateAppsList("https://example.com/", 10);
+  Registry registry;
+  InsertAppsListIntoRegistry(&registry, sync_apps);
+  database_factory().WriteRegistry(registry);
+  InitSyncBridge();
+
+  ON_CALL(processor(), Put(_, _, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         std::unique_ptr<syncer::EntityData> entity_data,
+                         syncer::MetadataChangeList* metadata) {
+        ASSERT_TRUE(base::Contains(registry, storage_key));
+        const std::unique_ptr<WebApp>& expected_app = registry.at(storage_key);
+        EXPECT_TRUE(IsSyncDataEqual(*expected_app, *entity_data));
+        RemoveWebAppFromAppsList(&sync_apps, storage_key);
+      });
+  EXPECT_CALL(processor(), Delete(_, _)).Times(0);
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (const std::unique_ptr<WebApp>& app : sync_apps) {
+    // Obtain a writeable handle.
+    WebApp* sync_app = update->UpdateApp(app->app_id());
+
+    WebApp::SyncData sync_data;
+    sync_data.name = "Updated Sync Name";
+    sync_data.theme_color = SK_ColorBLACK;
+    sync_app->SetSyncData(std::move(sync_data));
+    sync_app->SetUserDisplayMode(DisplayMode::kBrowser);
+
+    // Override the app in the expected registry.
+    registry[sync_app->app_id()] = std::make_unique<WebApp>(*sync_app);
+  }
+
+  CommitUpdate(std::move(update));
+
+  EXPECT_TRUE(sync_apps.empty());
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
+TEST_F(WebAppSyncBridgeTest, CommitUpdate_DeleteSyncApp) {
+  AppsList sync_apps = CreateAppsList("https://example.com/", 10);
+  Registry registry;
+  InsertAppsListIntoRegistry(&registry, sync_apps);
+  database_factory().WriteRegistry(registry);
+  InitSyncBridge();
+
+  EXPECT_CALL(processor(), Put(_, _, _)).Times(0);
+  ON_CALL(processor(), Delete(_, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         syncer::MetadataChangeList* metadata) {
+        EXPECT_TRUE(base::Contains(registry, storage_key));
+        RemoveWebAppFromAppsList(&sync_apps, storage_key);
+        // Delete the app in the expected registry.
+        registry.erase(storage_key);
+      });
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (const std::unique_ptr<WebApp>& app : sync_apps)
+    update->DeleteApp(app->app_id());
+
+  CommitUpdate(std::move(update));
+
+  EXPECT_TRUE(sync_apps.empty());
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
+TEST_F(WebAppSyncBridgeTest,
+       CommitUpdate_CreateSyncAppWithOverlappingPolicyApp) {
+  AppsList policy_apps;
+  for (int i = 0; i < 10; ++i) {
+    std::unique_ptr<WebApp> policy_app =
+        CreateWebApp("https://example.com/" + base::NumberToString(i));
+    // CreateWebApp does policy_app->SetName("Name");
+    policy_app->AddSource(Source::kPolicy);
+    policy_apps.push_back(std::move(policy_app));
+  }
+
+  Registry registry;
+  InsertAppsListIntoRegistry(&registry, policy_apps);
+  database_factory().WriteRegistry(registry);
+  InitSyncBridge();
+
+  ON_CALL(processor(), Put(_, _, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         std::unique_ptr<syncer::EntityData> entity_data,
+                         syncer::MetadataChangeList* metadata) {
+        ASSERT_TRUE(base::Contains(registry, storage_key));
+        const WebApp& expected_app = *registry.at(storage_key);
+
+        // kPolicy and Name is the difference for the sync "view". Add them to
+        // make operator== work.
+        std::unique_ptr<WebApp> entity_data_app =
+            std::make_unique<WebApp>(expected_app.app_id());
+        entity_data_app->AddSource(Source::kPolicy);
+        entity_data_app->SetName("Name");
+
+        EXPECT_TRUE(IsSyncDataEqualIfApplied(
+            expected_app, std::move(entity_data_app), *entity_data));
+
+        RemoveWebAppFromAppsList(&policy_apps, storage_key);
+      });
+  EXPECT_CALL(processor(), Delete(_, _)).Times(0);
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (int i = 0; i < 10; ++i) {
+    WebApp* app_to_update = update->UpdateApp(policy_apps[i]->app_id());
+
+    // Add kSync source to first 5 apps. Modify the rest 5 apps locally.
+    if (i < 5)
+      app_to_update->AddSource(Source::kSync);
+    else
+      app_to_update->SetDescription("Local policy app");
+
+    // Override the app in the expected registry.
+    registry[app_to_update->app_id()] =
+        std::make_unique<WebApp>(*app_to_update);
+  }
+
+  CommitUpdate(std::move(update));
+
+  EXPECT_EQ(5u, policy_apps.size());
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
+TEST_F(WebAppSyncBridgeTest,
+       CommitUpdate_DeleteSyncAppWithOverlappingPolicyApp) {
+  AppsList policy_and_sync_apps;
+  for (int i = 0; i < 10; ++i) {
+    std::unique_ptr<WebApp> policy_and_sync_app =
+        CreateWebApp("https://example.com/" + base::NumberToString(i));
+    // CreateWebApp does policy_app->SetName("Name");
+    policy_and_sync_app->AddSource(Source::kPolicy);
+    policy_and_sync_app->AddSource(Source::kSync);
+    policy_and_sync_apps.push_back(std::move(policy_and_sync_app));
+  }
+
+  Registry registry;
+  InsertAppsListIntoRegistry(&registry, policy_and_sync_apps);
+  database_factory().WriteRegistry(registry);
+  InitSyncBridge();
+
+  ON_CALL(processor(), Put(_, _, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         std::unique_ptr<syncer::EntityData> entity_data,
+                         syncer::MetadataChangeList* metadata) {
+        // Local changes to synced apps cause excessive |Put|.
+        // See TODO in WebAppSyncBridge::UpdateSync.
+        RemoveWebAppFromAppsList(&policy_and_sync_apps, storage_key);
+      });
+  ON_CALL(processor(), Delete(_, _))
+      .WillByDefault([&](const std::string& storage_key,
+                         syncer::MetadataChangeList* metadata) {
+        ASSERT_TRUE(base::Contains(registry, storage_key));
+        RemoveWebAppFromAppsList(&policy_and_sync_apps, storage_key);
+      });
+
+  std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+  for (int i = 0; i < 10; ++i) {
+    WebApp* app_to_update =
+        update->UpdateApp(policy_and_sync_apps[i]->app_id());
+
+    // Remove kSync source from first 5 apps. Modify the rest 5 apps locally.
+    if (i < 5)
+      app_to_update->RemoveSource(Source::kSync);
+    else
+      app_to_update->SetDescription("Local policy app");
+
+    // Override the app in the expected registry.
+    registry[app_to_update->app_id()] =
+        std::make_unique<WebApp>(*app_to_update);
+  }
+
+  CommitUpdate(std::move(update));
+
+  EXPECT_TRUE(policy_and_sync_apps.empty());
+  EXPECT_TRUE(IsRegistryEqual(registrar_registry(), registry));
+  EXPECT_TRUE(IsDatabaseRegistryEqualToRegistrar());
+}
+
 }  // namespace web_app