WebApp: Remove redundant app_id from proto values.

1) Update WebAppSpecifics proto to be in sync with WebAppProto,
launch_container field and enum added.

2) WebAppProto::app_id was a redundant value in the dictionary.

app_id is a hash of launch url and it can be inferred from launch_url
field. app_id is the client tag and the storage key.

WebAppSyncBridge::GetClientTag and WebAppSyncBridge::GetStorageKey
will be implemented to return GenerateAppIdFromURL(launch_url).

Bug: 891172
Change-Id: If3abbb96d2bffb368ce37b3d47d0691c3bf11c16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1792425Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695858}

chrome/browser/web_applications/proto/web_app.proto

@@ -16,12 +16,6 @@ message WebAppIconInfoProto {
   required int32 size_in_px = 2;
 }
 
-// TODO(loyso): Move this enum to components/sync/protocol/sync_enums.proto.
-enum LaunchContainerProto {
-  TAB = 1;
-  WINDOW = 2;
-}
-
 // A set to track simultaneous installs and uninstalls from multiple install
 // sources.
 message SourcesProto {
@@ -32,24 +26,29 @@ message SourcesProto {
   required bool default = 5;
 }
 
-// WebApp class data.
-// This should be a superset for WebAppSpecifics in
+// WebApp class data. 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;
-  required string launch_url = 2;
-  required string name = 3;
-  required LaunchContainerProto launch_container = 4;
-  optional uint32 theme_color = 5;
+  // Keep in sync with WebAppSpecifics::LaunchContainer.
+  enum LaunchContainer {
+    TAB = 1;
+    WINDOW = 2;
+  }
+
+  // app_id is a hash of launch_url. app_id is the client tag for sync system.
+  // app_id is the storage key in ModelTypeStore.
+  required string launch_url = 1;
+  required string name = 2;
+  required LaunchContainer launch_container = 3;
+  optional uint32 theme_color = 4;
 
   // Local data members, not to be synced:
-  optional string description = 6;
-  optional string scope = 7;
+  optional string description = 5;
+  optional string scope = 6;
 
-  required SourcesProto sources = 8;
-  required bool is_locally_installed = 9;
+  required SourcesProto sources = 7;
+  required bool is_locally_installed = 8;
 
   // A list of icon infos.
-  repeated WebAppIconInfoProto icons = 10;
+  repeated WebAppIconInfoProto icons = 9;
 }

chrome/browser/web_applications/web_app_database.cc

@@ -44,9 +44,9 @@ void WebAppDatabase::WriteWebApps(AppsToWrite apps,
 
   BeginTransaction();
 
-  for (auto* web_app : apps) {
+  for (const WebApp* web_app : apps) {
     auto proto = CreateWebAppProto(*web_app);
-    write_batch_->WriteData(proto->app_id(), proto->SerializeAsString());
+    write_batch_->WriteData(web_app->app_id(), proto->SerializeAsString());
   }
 
   CommitTransaction(std::move(callback));
@@ -71,19 +71,21 @@ std::unique_ptr<WebAppProto> WebAppDatabase::CreateWebAppProto(
   auto proto = std::make_unique<WebAppProto>();
 
   // Required fields:
+  const GURL launch_url = web_app.launch_url();
+  DCHECK(!launch_url.is_empty() && launch_url.is_valid());
+
   DCHECK(!web_app.app_id().empty());
-  proto->set_app_id(web_app.app_id());
+  DCHECK_EQ(web_app.app_id(), GenerateAppIdFromURL(launch_url));
 
-  DCHECK(!web_app.launch_url().is_empty() && web_app.launch_url().is_valid());
-  proto->set_launch_url(web_app.launch_url().spec());
+  proto->set_launch_url(launch_url.spec());
 
   proto->set_name(web_app.name());
 
   DCHECK_NE(LaunchContainer::kDefault, web_app.launch_container());
   proto->set_launch_container(web_app.launch_container() ==
                                       LaunchContainer::kWindow
-                                  ? LaunchContainerProto::WINDOW
-                                  : LaunchContainerProto::TAB);
+                                  ? WebAppProto::WINDOW
+                                  : WebAppProto::TAB);
 
   DCHECK(web_app.sources_.any());
   proto->mutable_sources()->set_system(web_app.sources_[Source::kSystem]);
@@ -113,17 +115,20 @@ std::unique_ptr<WebAppProto> WebAppDatabase::CreateWebAppProto(
 
 // static
 std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(const WebAppProto& proto) {
-  auto web_app = std::make_unique<WebApp>(proto.app_id());
-
-  // Required fields:
+  // AppId is a hash of launch_url. Read launch_url first:
   GURL launch_url(proto.launch_url());
   if (launch_url.is_empty() || !launch_url.is_valid()) {
     DLOG(ERROR) << "WebApp proto launch_url parse error: "
                 << launch_url.possibly_invalid_spec();
     return nullptr;
   }
+
+  const AppId app_id = GenerateAppIdFromURL(launch_url);
+
+  auto web_app = std::make_unique<WebApp>(app_id);
   web_app->SetLaunchUrl(launch_url);
 
+  // Required fields:
   if (!proto.has_sources()) {
     DLOG(ERROR) << "WebApp proto parse error: no sources field";
     return nullptr;
@@ -151,8 +156,7 @@ std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(const WebAppProto& proto) {
     DLOG(ERROR) << "WebApp proto parse error: no launch_container field";
     return nullptr;
   }
-  web_app->SetLaunchContainer(proto.launch_container() ==
-                                      LaunchContainerProto::WINDOW
+  web_app->SetLaunchContainer(proto.launch_container() == WebAppProto::WINDOW
                                   ? LaunchContainer::kWindow
                                   : LaunchContainer::kTab);
 
@@ -267,12 +271,18 @@ std::unique_ptr<WebApp> WebAppDatabase::ParseWebApp(const AppId& app_id,
                                                     const std::string& value) {
   WebAppProto proto;
   const bool parsed = proto.ParseFromString(value);
-  if (!parsed || proto.app_id() != app_id) {
-    DLOG(ERROR) << "WebApps LevelDB parse error (unknown).";
+  if (!parsed) {
+    DLOG(ERROR) << "WebApps LevelDB parse error: can't parse proto.";
     return nullptr;
   }
 
-  return CreateWebApp(proto);
+  auto web_app = CreateWebApp(proto);
+  if (web_app->app_id() != app_id) {
+    DLOG(ERROR) << "WebApps LevelDB error: app_id doesn't match storage key";
+    return nullptr;
+  }
+
+  return web_app;
 }
 
 void WebAppDatabase::BeginTransaction() {

components/sync/protocol/proto_enum_conversions.cc

@@ -537,6 +537,17 @@ const char* ProtoEnumToString(
   return "";
 }
 
+const char* ProtoEnumToString(
+    sync_pb::WebAppSpecifics::LaunchContainer launch_container) {
+  ASSERT_ENUM_BOUNDS(sync_pb::WebAppSpecifics, LaunchContainer, TAB, WINDOW);
+  switch (launch_container) {
+    ENUM_CASE(sync_pb::WebAppSpecifics, TAB);
+    ENUM_CASE(sync_pb::WebAppSpecifics, WINDOW);
+  }
+  NOTREACHED();
+  return "";
+}
+
 const char* ProtoEnumToString(
     sync_pb::WifiConfigurationSpecificsData::SecurityType security_type) {
   ASSERT_ENUM_BOUNDS(sync_pb::WifiConfigurationSpecificsData, SecurityType,

components/sync/protocol/proto_enum_conversions.h

@@ -11,6 +11,7 @@
 #include "components/sync/protocol/reading_list_specifics.pb.h"
 #include "components/sync/protocol/session_specifics.pb.h"
 #include "components/sync/protocol/sync.pb.h"
+#include "components/sync/protocol/web_app_specifics.pb.h"
 
 // Keep this file in sync with the .proto files in this directory.
 //
@@ -109,6 +110,9 @@ const char* ProtoEnumToString(
 const char* ProtoEnumToString(
     sync_pb::WalletMetadataSpecifics::Type wallet_metadata_type);
 
+const char* ProtoEnumToString(
+    sync_pb::WebAppSpecifics::LaunchContainer launch_container);
+
 const char* ProtoEnumToString(
     sync_pb::WifiConfigurationSpecificsData::SecurityType security_type);
 

components/sync/protocol/proto_visitors.h

@@ -1123,9 +1123,9 @@ VISIT_PROTO_FIELDS(const sync_pb::WalletSyncFlags& proto) {
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::WebAppSpecifics& proto) {
-  VISIT(app_id);
   VISIT(launch_url);
   VISIT(name);
+  VISIT_ENUM(launch_container);
   VISIT(theme_color);
 }
 

components/sync/protocol/web_app_specifics.proto

@@ -14,9 +14,14 @@ package sync_pb;
 // WebApp data. This should be a subset of WebAppProto in
 // chrome/browser/web_applications/proto/web_app.proto
 message WebAppSpecifics {
-  // app_id is the client tag for sync system.
-  optional string app_id = 1;
-  optional string launch_url = 2;
-  optional string name = 3;
+  // Keep in sync with WebAppProto::LaunchContainer.
+  enum LaunchContainer {
+    TAB = 1;
+    WINDOW = 2;
+  }
+
+  optional string launch_url = 1;
+  optional string name = 2;
+  optional LaunchContainer launch_container = 3;
   optional uint32 theme_color = 4;
 }