WebApp: Prepare integration with USS (sync)

In previous episodes:
https://chromium-review.googlesource.com/c/chromium/src/+/1295529

We create new ModelType: WEB_APPS.
It is hidden behind kDesktopPWAsUSS base feature (disabled by default).

WEB_APPS model type is included in the existing Apps toggle in sync settings
(We reuse UserSelectableType::kApps).

This CL follows The Sync Integration Checklist described here:
https://chromium.googlesource.com/chromium/src/+/HEAD/docs/sync/model_api.md

TBR=skyostil@chromium.org

Bug: 902214
Change-Id: I473df00a65348663d4dba78261d023bc57039aac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1685033
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675889}

base/trace_event/memory_infra_background_whitelist.cc

@@ -355,6 +355,7 @@ const char* const kAllocatorDumpNameWhitelist[] = {
     "sync/0x?/model_type/USER_CONSENT",
     "sync/0x?/model_type/USER_EVENT",
     "sync/0x?/model_type/WALLET_METADATA",
+    "sync/0x?/model_type/WEB_APP",
     "sync/0x?/model_type/WIFI_CONFIGURATION",
     "sync/0x?/model_type/WIFI_CREDENTIAL",
     "tab_restore/service_helper_0x?/entries",

chrome/browser/sync/chrome_sync_client.cc

@@ -9,6 +9,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/path_service.h"
 #include "base/syslog_logging.h"
 #include "base/task/post_task.h"
@@ -38,9 +39,13 @@
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/themes/theme_syncable_service.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/browser/web_applications/web_app_sync_bridge.h"
+#include "chrome/browser/web_applications/web_app_sync_manager.h"
 #include "chrome/browser/web_data_service_factory.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/common/channel_info.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "components/autofill/core/browser/webdata/autocomplete_sync_bridge.h"
@@ -338,6 +343,18 @@ ChromeSyncClient::CreateDataTypeControllers(syncer::SyncService* sync_service) {
             profile_, syncer::APP_SETTINGS),
         dump_stack, profile_));
   }
+
+  // Web Apps sync is disabled by default.
+  if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions) &&
+      base::FeatureList::IsEnabled(features::kDesktopPWAsUSS) &&
+      web_app::WebAppProvider::Get(profile_)) {
+    if (!disabled_types.Has(syncer::WEB_APPS)) {
+      controllers.push_back(std::make_unique<syncer::ModelTypeController>(
+          syncer::WEB_APPS,
+          std::make_unique<syncer::ForwardingModelTypeControllerDelegate>(
+              GetControllerDelegateForModelType(syncer::WEB_APPS).get())));
+    }
+  }
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 
 #if !defined(OS_ANDROID)
@@ -516,7 +533,19 @@ ChromeSyncClient::GetControllerDelegateForModelType(syncer::ModelType type) {
           ->GetSyncBridge()
           ->change_processor()
           ->GetControllerDelegate();
-
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+    case syncer::WEB_APPS: {
+      DCHECK(base::FeatureList::IsEnabled(
+          features::kDesktopPWAsWithoutExtensions));
+      DCHECK(base::FeatureList::IsEnabled(features::kDesktopPWAsUSS));
+      auto* provider = web_app::WebAppProvider::Get(profile_);
+      DCHECK(provider);
+      return provider->sync_manager()
+          .bridge()
+          .change_processor()
+          ->GetControllerDelegate();
+    }
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
     // We don't exercise this function for certain datatypes, because their
     // controllers get the delegate elsewhere.
     case syncer::AUTOFILL:

chrome/browser/sync/profile_sync_service_factory.cc

@@ -40,6 +40,7 @@
 #include "chrome/browser/sync/user_event_service_factory.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/undo/bookmark_undo_service_factory.h"
+#include "chrome/browser/web_applications/web_app_provider_factory.h"
 #include "chrome/browser/web_data_service_factory.h"
 #include "chrome/common/buildflags.h"
 #include "chrome/common/channel_info.h"
@@ -161,6 +162,7 @@ ProfileSyncServiceFactory::ProfileSyncServiceFactory()
   DependsOn(
       extensions::ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
   DependsOn(extensions::StorageFrontend::GetFactoryInstance());
+  DependsOn(web_app::WebAppProviderFactory::GetInstance());
 #endif  // BUILDFLAG(ENABLE_EXTENSIONS)
 #if defined(OS_CHROMEOS)
   DependsOn(chromeos::SyncedPrintersManagerFactory::GetInstance());

chrome/browser/sync/profile_sync_service_factory_unittest.cc

@@ -13,13 +13,15 @@
 #include "base/task/thread_pool/thread_pool.h"
 #include "build/build_config.h"
 #include "chrome/common/buildflags.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/browser_sync/browser_sync_switches.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/data_type_controller.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
+#include "extensions/buildflags/buildflags.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_CHROMEOS)
@@ -42,27 +44,46 @@ class ProfileSyncServiceFactoryTest : public testing::Test {
 
   // Returns the collection of default datatypes.
   std::vector<syncer::ModelType> DefaultDatatypes() {
-    static_assert(45 == syncer::ModelType::NUM_ENTRIES,
+    static_assert(46 == syncer::ModelType::NUM_ENTRIES,
                   "When adding a new type, you probably want to add it here as "
                   "well (assuming it is already enabled).");
 
     std::vector<syncer::ModelType> datatypes;
 
-    // Desktop types.
-#if !defined(OS_ANDROID)
+    // These preprocessor conditions and their order should be in sync with
+    // preprocessor conditions in ChromeSyncClient::CreateDataTypeControllers:
+
+    // ChromeSyncClient types.
+    datatypes.push_back(syncer::SECURITY_EVENTS);
+
+#if BUILDFLAG(ENABLE_SUPERVISED_USERS)
+    datatypes.push_back(syncer::SUPERVISED_USER_SETTINGS);
+    datatypes.push_back(syncer::SUPERVISED_USER_WHITELISTS);
+#endif  // BUILDFLAG(ENABLE_SUPERVISED_USERS)
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
     datatypes.push_back(syncer::APPS);
+    datatypes.push_back(syncer::EXTENSIONS);
+    datatypes.push_back(syncer::EXTENSION_SETTINGS);
+    datatypes.push_back(syncer::APP_SETTINGS);
+    if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions) &&
+        base::FeatureList::IsEnabled(features::kDesktopPWAsUSS)) {
+      datatypes.push_back(syncer::WEB_APPS);
+    }
+#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+
+#if !defined(OS_ANDROID)
+    datatypes.push_back(syncer::THEMES);
+    datatypes.push_back(syncer::SEARCH_ENGINES);
+#endif  // !defined(OS_ANDROID)
+
 #if BUILDFLAG(ENABLE_APP_LIST)
     datatypes.push_back(syncer::APP_LIST);
-#endif
-    datatypes.push_back(syncer::APP_SETTINGS);
-#if defined(OS_LINUX) || defined(OS_WIN) || defined(OS_CHROMEOS)
+#endif  // BUILDFLAG(ENABLE_APP_LIST)
+
+#if defined(OS_LINUX) || defined(OS_WIN)
     datatypes.push_back(syncer::DICTIONARY);
 #endif
-    datatypes.push_back(syncer::EXTENSIONS);
-    datatypes.push_back(syncer::EXTENSION_SETTINGS);
-    datatypes.push_back(syncer::SEARCH_ENGINES);
-    datatypes.push_back(syncer::THEMES);
-#endif  // !OS_ANDROID
 
 #if defined(OS_CHROMEOS)
     if (arc::IsArcAllowedForProfile(profile()))
@@ -88,9 +109,6 @@ class ProfileSyncServiceFactoryTest : public testing::Test {
     datatypes.push_back(syncer::PRIORITY_PREFERENCES);
     datatypes.push_back(syncer::SESSIONS);
     datatypes.push_back(syncer::PROXY_TABS);
-    datatypes.push_back(syncer::SECURITY_EVENTS);
-    datatypes.push_back(syncer::SUPERVISED_USER_SETTINGS);
-    datatypes.push_back(syncer::SUPERVISED_USER_WHITELISTS);
     datatypes.push_back(syncer::TYPED_URLS);
     datatypes.push_back(syncer::USER_EVENTS);
     datatypes.push_back(syncer::USER_CONSENTS);

chrome/browser/web_applications/BUILD.gn

@@ -34,6 +34,10 @@ source_set("web_applications") {
     "web_app_install_task.h",
     "web_app_registrar.cc",
     "web_app_registrar.h",
+    "web_app_sync_bridge.cc",
+    "web_app_sync_bridge.h",
+    "web_app_sync_manager.cc",
+    "web_app_sync_manager.h",
     "web_app_tab_helper.cc",
     "web_app_tab_helper.h",
   ]

chrome/browser/web_applications/proto/web_app.proto

@@ -8,7 +8,7 @@ option optimize_for = LITE_RUNTIME;
 
 package web_app;
 
-// Information about web app icon.
+// Local data: Information about web app icon.
 message WebAppIconInfoProto {
   // The URL of the app icon.
   required string url = 1;
@@ -17,18 +17,18 @@ message WebAppIconInfoProto {
 }
 
 // WebApp class data.
-// TODO(loyso): Consider moving this proto to components/sync/protocol/
-// crbug.com/902214.
+// This should be a superset for WebAppSpecifics in
+// components/sync/protocol/web_app_specifics.proto
 message WebAppProto {
   // app_id is the client tag for sync system.
   required string app_id = 1;
-  optional string name = 2;
-  optional string description = 3;
-  required string launch_url = 4;
-  optional string scope = 5;
-  optional uint32 theme_color = 6;
+  required string launch_url = 2;
+  required string name = 3;
+  required uint32 theme_color = 4;
 
   // Local data members, not to be synced:
+  optional string description = 5;
+  optional string scope = 6;
   // List of icon infos.
   repeated WebAppIconInfoProto icons = 7;
 }

chrome/browser/web_applications/web_app.cc

@@ -39,7 +39,6 @@ void WebApp::SetThemeColor(base::Optional<SkColor> theme_color) {
 }
 
 void WebApp::SetIcons(Icons icons) {
-  DCHECK(!icons.empty());
   icons_ = std::move(icons);
 }
 

chrome/browser/web_applications/web_app.h

@@ -28,6 +28,7 @@ class WebApp {
   const std::string& description() const { return description_; }
   const GURL& launch_url() const { return launch_url_; }
   const GURL& scope() const { return scope_; }
+  // TODO(loyso): Remove Optional. This is a required field now.
   const base::Optional<SkColor>& theme_color() const { return theme_color_; }
 
   struct IconInfo {

chrome/browser/web_applications/web_app_database.cc

@@ -63,14 +63,22 @@ std::unique_ptr<WebAppProto> WebAppDatabase::CreateWebAppProto(
     const WebApp& web_app) {
   auto proto = std::make_unique<WebAppProto>();
 
+  // Required fields:
+  DCHECK(!web_app.app_id().empty());
   proto->set_app_id(web_app.app_id());
+
+  DCHECK(!web_app.launch_url().is_empty() && web_app.launch_url().is_valid());
+  proto->set_launch_url(web_app.launch_url().spec());
+
   proto->set_name(web_app.name());
+
+  DCHECK(web_app.theme_color());
+  proto->set_theme_color(web_app.theme_color().value());
+
+  // Optional fields:
   proto->set_description(web_app.description());
-  proto->set_launch_url(web_app.launch_url().spec());
   if (!web_app.scope().is_empty())
     proto->set_scope(web_app.scope().spec());
-  if (web_app.theme_color())
-    proto->set_theme_color(web_app.theme_color().value());
 
   for (const WebApp::IconInfo& icon : web_app.icons()) {
     WebAppIconInfoProto* icon_proto = proto->add_icons();
@@ -85,43 +93,49 @@ std::unique_ptr<WebAppProto> WebAppDatabase::CreateWebAppProto(
 std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(const WebAppProto& proto) {
   auto web_app = std::make_unique<WebApp>(proto.app_id());
 
+  // Required fields:
   GURL launch_url(proto.launch_url());
   if (launch_url.is_empty() || !launch_url.is_valid()) {
-    LOG(ERROR) << "WebApp proto launch_url parse error: "
-               << launch_url.possibly_invalid_spec();
+    DLOG(ERROR) << "WebApp proto launch_url parse error: "
+                << launch_url.possibly_invalid_spec();
     return nullptr;
   }
-
   web_app->SetLaunchUrl(launch_url);
+
+  if (!proto.has_name()) {
+    DLOG(ERROR) << "WebApp proto parse error: no name field";
+    return nullptr;
+  }
   web_app->SetName(proto.name());
-  web_app->SetDescription(proto.description());
+
+  if (!proto.has_theme_color()) {
+    DLOG(ERROR) << "WebApp proto parse error: no theme_color field";
+    return nullptr;
+  }
+  web_app->SetThemeColor(proto.theme_color());
+
+  // Optional fields:
+  if (proto.has_description())
+    web_app->SetDescription(proto.description());
 
   if (proto.has_scope()) {
     GURL scope(proto.scope());
     if (scope.is_empty() || !scope.is_valid()) {
-      LOG(ERROR) << "WebApp proto scope parse error: "
-                 << scope.possibly_invalid_spec();
+      DLOG(ERROR) << "WebApp proto scope parse error: "
+                  << scope.possibly_invalid_spec();
       return nullptr;
     }
     web_app->SetScope(scope);
   }
 
-  if (proto.has_theme_color())
-    web_app->SetThemeColor(proto.theme_color());
-
-  if (proto.icons_size() == 0) {
-    LOG(ERROR) << "WebApp proto parse icons error: no icons";
-    return nullptr;
-  }
-
   WebApp::Icons icons;
   for (int i = 0; i < proto.icons_size(); ++i) {
     const WebAppIconInfoProto& icon_proto = proto.icons(i);
 
     GURL icon_url(icon_proto.url());
     if (icon_url.is_empty() || !icon_url.is_valid()) {
-      LOG(ERROR) << "WebApp IconInfo proto url parse error: "
-                 << icon_url.possibly_invalid_spec();
+      DLOG(ERROR) << "WebApp IconInfo proto url parse error: "
+                  << icon_url.possibly_invalid_spec();
       return nullptr;
     }
 
@@ -145,11 +159,8 @@ void WebAppDatabase::CreateStore(
     base::OnceClosure closure) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  // For now we use syncer:APPS prefix within local isolated LevelDB, no sync.
-  // TODO(loyso): Create separate ModelType::WEB_APPS before implementing sync.
-  // Otherwise it may interfere with existing APPS data.
   std::move(store_factory)
-      .Run(syncer::APPS,
+      .Run(syncer::WEB_APPS,
            base::BindOnce(&WebAppDatabase::OnStoreCreated,
                           weak_ptr_factory_.GetWeakPtr(), std::move(closure)));
 }
@@ -160,7 +171,7 @@ void WebAppDatabase::OnStoreCreated(
     std::unique_ptr<syncer::ModelTypeStore> store) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (error) {
-    LOG(ERROR) << "WebApps LevelDB opening error: " << error->ToString();
+    DLOG(ERROR) << "WebApps LevelDB opening error: " << error->ToString();
     return;
   }
 
@@ -174,7 +185,7 @@ void WebAppDatabase::OnAllDataRead(
     std::unique_ptr<syncer::ModelTypeStore::RecordList> data_records) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (error) {
-    LOG(ERROR) << "WebApps LevelDB read error: " << error->ToString();
+    DLOG(ERROR) << "WebApps LevelDB read error: " << error->ToString();
     return;
   }
 
@@ -194,7 +205,7 @@ void WebAppDatabase::OnDataWritten(
     const base::Optional<syncer::ModelError>& error) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (error)
-    LOG(ERROR) << "WebApps LevelDB write error: " << error->ToString();
+    DLOG(ERROR) << "WebApps LevelDB write error: " << error->ToString();
 }
 
 // static
@@ -203,7 +214,7 @@ std::unique_ptr<WebApp> WebAppDatabase::ParseWebApp(const AppId& app_id,
   WebAppProto proto;
   const bool parsed = proto.ParseFromString(value);
   if (!parsed || proto.app_id() != app_id) {
-    LOG(ERROR) << "WebApps LevelDB parse error (unknown).";
+    DLOG(ERROR) << "WebApps LevelDB parse error (unknown).";
     return nullptr;
   }