Log all PWAs and bookmark apps.

Instead of only logging PWAs that are on a white list, use
from_bookmark() to determine whether an extension is a PWA.
from_bookmark() returns true for both PWAs and bookmark apps,
so this is used to log bookmark apps as well.

Bug: 899123
Change-Id: Ia6cc4f11fff13ff249786f229d21c6d15d1d8e15
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1599811Reviewed-by: Robert Kaplow (slow) <rkaplow@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Charles . <charleszhao@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Paul Dyson <pdyson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664123}

chrome/browser/ui/app_list/app_launch_event_logger.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/metrics/chrome_metrics_service_client.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "components/arc/arc_prefs.h"
 #include "components/prefs/pref_service.h"
 #include "components/ukm/app_source_url_recorder.h"
@@ -123,7 +124,6 @@ AppLaunchEventLogger::AppLaunchEventLogger()
   task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
       {base::TaskPriority::BEST_EFFORT,
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
-  PopulatePwaIdUrlMap();
 }
 
 AppLaunchEventLogger::~AppLaunchEventLogger() {}
@@ -192,20 +192,9 @@ std::string AppLaunchEventLogger::RemoveScheme(const std::string& id) {
   return app_id;
 }
 
-void AppLaunchEventLogger::PopulatePwaIdUrlMap() {
-  pwa_id_url_map_ = base::flat_map<std::string, std::string>{
-      // Google+
-      {"dcdbodpaldbchkfinnjphocleggfceip", "https://plus.google.com/discover"},
-      // Photos
-      {"ncmjhecbjeaamljdfahankockkkdmedg", "https://photos.google.com/"}};
-}
-
-const std::string& AppLaunchEventLogger::GetPwaUrl(const std::string& id) {
-  auto search = pwa_id_url_map_.find(id);
-  if (search != pwa_id_url_map_.end()) {
-    return search->second;
-  }
-  return base::EmptyString();
+const GURL& AppLaunchEventLogger::GetLaunchWebURL(
+    const extensions::Extension* extension) {
+  return extensions::AppLaunchInfo::GetLaunchWebURL(extension);
 }
 
 void AppLaunchEventLogger::OkApp(AppLaunchEvent_AppType app_type,
@@ -247,7 +236,7 @@ void AppLaunchEventLogger::EnforceLoggingPolicy() {
     app.second.set_is_policy_compliant(false);
   }
 
-  // Store all Chrome and PWA apps.
+  // Store all Chrome, PWA and bookmark apps.
   // registry_ can be nullptr in tests.
   if (registry_) {
     std::unique_ptr<extensions::ExtensionSet> extensions =
@@ -257,13 +246,11 @@ void AppLaunchEventLogger::EnforceLoggingPolicy() {
       if (extension->from_webstore()) {
         OkApp(AppLaunchEvent_AppType_CHROME, extension->id(),
               base::EmptyString(), base::EmptyString());
-      } else {
-        // Only allow PWA apps on the whitelist.
-        auto search = pwa_id_url_map_.find(extension->id());
-        if (search != pwa_id_url_map_.end()) {
-          OkApp(AppLaunchEvent_AppType_PWA, extension->id(),
-                base::EmptyString(), search->second);
-        }
+        // PWA apps have from_bookmark() true. This will also categorize
+        // bookmark apps as AppLaunchEvent_AppType_PWA.
+      } else if (extension->from_bookmark()) {
+        OkApp(AppLaunchEvent_AppType_PWA, extension->id(), base::EmptyString(),
+              GetLaunchWebURL(extension.get()).spec());
       }
     }
   }
@@ -351,7 +338,7 @@ ukm::SourceId AppLaunchEventLogger::GetSourceId(
     return ukm::AppSourceUrlRecorder::GetSourceIdForArc(arc_package_name);
   } else {
     // Either app is Crostini; or Chrome but not in app store; or Arc but not
-    // syncable; or PWA but not in whitelist.
+    // syncable.
     return ukm::kInvalidSourceId;
   }
 }

chrome/browser/ui/app_list/app_launch_event_logger.h

@@ -36,11 +36,11 @@ namespace app_list {
 
 // This class logs metrics associated with clicking on apps in ChromeOS.
 // Logging is restricted to Arc apps with sync enabled, Chrome apps from the
-// app store and PWAs from a whitelist. This class uses UKM for logging,
+// app store, PWAs and bookmark apps. This class uses UKM for logging,
 // however, the metrics are not keyed by navigational urls. Instead, for Chrome
 // apps the keys are based upon the app id, for Arc apps the keys are based upon
-// a hash of the package name and for PWAs the keys are the urls associated with
-// the PWA.
+// a hash of the package name, and for PWAs and bookmark apps the keys are the
+// urls associated with the PWA/bookmark.
 // At the time of app launch this class logs metrics about the app clicked on
 // and another five apps that were not clicked on, chosen at random.
 class AppLaunchEventLogger {
@@ -65,23 +65,21 @@ class AppLaunchEventLogger {
   static const char kPackageName[];
   static const char kShouldSync[];
 
+ protected:
+  // Get the url used to launch a PWA or bookmark app.
+  virtual const GURL& GetLaunchWebURL(const extensions::Extension* extension);
+
  private:
   // Removes any leading "chrome-extension://" or "arc://". Also remove any
   // trailing "/".
   std::string RemoveScheme(const std::string& id);
-  // Creates the mapping from PWA app id to PWA url. This mapping also acts as a
-  // whitelist for which PWA apps can be logged here.
-  void PopulatePwaIdUrlMap();
-  // Gets the PWA url from its app id. Returns base::EmptyString() if no match
-  // found.
-  const std::string& GetPwaUrl(const std::string& id);
+
   // Marks app as ok for policy compliance. If the app is not in
   // |app_features_map_| then add it.
   void OkApp(AppLaunchEvent_AppType app_type,
              const std::string& app_id,
              const std::string& arc_package_name,
              const std::string& pwa_url);
-
   // Enforces logging policy, ensuring that the |app_features_map_| only
   // contains apps that are allowed to be logged. All apps are rechecked in case
   // they have been uninstalled since the previous check.
@@ -89,7 +87,7 @@ class AppLaunchEventLogger {
   // Updates the app data following a click.
   void ProcessClick(const AppLaunchEvent& event, const base::Time& now);
   // Returns a source id. |arc_package_name| is only required for Arc apps,
-  // |pwa_url| is only required for PWA apps.
+  // |pwa_url| is only required for PWAs and bookmark apps.
   ukm::SourceId GetSourceId(AppLaunchEvent_AppType app_type,
                             const std::string& app_id,
                             const std::string& arc_package_name,
@@ -110,10 +108,6 @@ class AppLaunchEventLogger {
   // Logs the app click using UKM.
   void Log(AppLaunchEvent app_launch_event);
 
-  // Map from PWA app id to PWA url. This also acts as a whitelist of PWA apps
-  // the can be logged.
-  base::flat_map<std::string, std::string> pwa_id_url_map_;
-
   // The arc apps installed on the device.
   const base::DictionaryValue* arc_apps_;
   // The arc packages installed on the device.

chrome/browser/ui/app_list/app_launch_event_logger.proto

@@ -22,6 +22,7 @@ message AppLaunchEvent {
     OTHER = 0;
     CHROME = 1;
     PLAY = 2;
+    // PWA AppType includes bookmark apps in addition to PWAs.
     PWA = 3;
   }
   // The type of App.

chrome/browser/ui/app_list/app_launch_event_logger_unittest.cc

@@ -26,6 +26,7 @@ namespace app_list {
 const char kGmailChromeApp[] = "pjkljhegncpnkpknbcohdijeoejaedia";
 const char kMapsArcApp[] = "gmhipfhgnoelkiiofcnimehjnpaejiel";
 const char kPhotosPWAApp[] = "ncmjhecbjeaamljdfahankockkkdmedg";
+const GURL kPhotosPWAUrl = GURL("http://photos.google.com/");
 
 const char kMapsPackageName[] = "com.google.android.apps.maps";
 
@@ -37,6 +38,13 @@ bool TestIsWebstoreExtension(base::StringPiece id) {
 
 }  // namespace
 
+class AppLaunchEventLoggerForTest : public AppLaunchEventLogger {
+ protected:
+  const GURL& GetLaunchWebURL(const extensions::Extension* extension) override {
+    return kPhotosPWAUrl;
+  }
+};
+
 class AppLaunchEventLoggerTest : public testing::Test {
  protected:
   void SetUp() override {
@@ -51,12 +59,14 @@ class AppLaunchEventLoggerTest : public testing::Test {
 TEST_F(AppLaunchEventLoggerTest, CheckUkmCodePWA) {
   extensions::ExtensionRegistry registry(nullptr);
   scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionBuilder("test").SetID(kPhotosPWAApp).Build();
-  registry.AddEnabled(extension);
+      extensions::ExtensionBuilder("test")
+          .SetID(kPhotosPWAApp)
+          .AddFlags(extensions::Extension::FROM_BOOKMARK)
+          .Build();
 
-  GURL url("https://photos.google.com/");
+  registry.AddEnabled(extension);
 
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(&registry, nullptr, nullptr);
   app_launch_event_logger_.OnGridClicked(kPhotosPWAApp);
 
@@ -65,7 +75,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodePWA) {
   const auto entries = test_ukm_recorder_.GetEntriesByName("AppListAppLaunch");
   ASSERT_EQ(1ul, entries.size());
   const auto* entry = entries.back();
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AllClicksLast24Hours", 1);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AllClicksLastHour", 1);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AppType", 3);
@@ -76,7 +86,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodePWA) {
       test_ukm_recorder_.GetEntriesByName("AppListAppClickData");
   ASSERT_EQ(1ul, click_entries.size());
   const auto* photos_entry = click_entries.back();
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(photos_entry, url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(photos_entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntryMetric(photos_entry, "AppLaunchId",
                                        entry->source_id);
   test_ukm_recorder_.ExpectEntryMetric(photos_entry, "AppType", 3);
@@ -97,7 +107,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeChrome) {
   test_ukm_recorder_.SetIsWebstoreExtensionCallback(
       base::BindRepeating(&TestIsWebstoreExtension));
 
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(&registry, nullptr, nullptr);
   app_launch_event_logger_.OnGridClicked(kGmailChromeApp);
 
@@ -126,7 +136,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeArc) {
 
   GURL url("app://play/gbpfhehadcpcndihhameeacbdmbjbhgi");
 
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(nullptr, arc_apps.get(),
                                                 packages.get());
   app_launch_event_logger_.OnGridClicked(kMapsArcApp);
@@ -145,11 +155,12 @@ TEST_F(AppLaunchEventLoggerTest, CheckMultipleClicks) {
   // Click on PWA photos, then Chrome Maps, then PWA Photos again.
   extensions::ExtensionRegistry registry(nullptr);
   scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionBuilder("test").SetID(kPhotosPWAApp).Build();
+      extensions::ExtensionBuilder("test")
+          .SetID(kPhotosPWAApp)
+          .AddFlags(extensions::Extension::FROM_BOOKMARK)
+          .Build();
   registry.AddEnabled(extension);
 
-  GURL photos_url("https://photos.google.com/");
-
   base::Value package(base::Value::Type::DICTIONARY);
   package.SetKey(AppLaunchEventLogger::kShouldSync, base::Value(true));
 
@@ -164,7 +175,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckMultipleClicks) {
 
   GURL maps_url("app://play/gbpfhehadcpcndihhameeacbdmbjbhgi");
 
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(&registry, arc_apps.get(),
                                                 packages.get());
   app_launch_event_logger_.OnGridClicked(kPhotosPWAApp);
@@ -178,7 +189,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckMultipleClicks) {
   const auto entries = test_ukm_recorder_.GetEntriesByName("AppListAppLaunch");
   ASSERT_EQ(4ul, entries.size());
   const auto* entry = entries.back();
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, photos_url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AllClicksLast24Hours", 4);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AllClicksLastHour", 4);
   test_ukm_recorder_.ExpectEntryMetric(entry, "AppType", 3);
@@ -192,12 +203,12 @@ TEST_F(AppLaunchEventLoggerTest, CheckMultipleClicks) {
   const auto* maps_entry = click_entries.at(6);
   const auto* photos_entry = click_entries.at(7);
   if (test_ukm_recorder_.GetSourceForSourceId(maps_entry->source_id)->url() ==
-      photos_url) {
+      kPhotosPWAUrl) {
     const auto* tmp_entry = photos_entry;
     photos_entry = maps_entry;
     maps_entry = tmp_entry;
   }
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(photos_entry, photos_url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(photos_entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntrySourceHasUrl(maps_entry, maps_url);
   test_ukm_recorder_.ExpectEntryMetric(photos_entry, "AppLaunchId",
                                        entry->source_id);
@@ -220,12 +231,13 @@ TEST_F(AppLaunchEventLoggerTest, CheckMultipleClicks) {
 TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeSuggestionChip) {
   extensions::ExtensionRegistry registry(nullptr);
   scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionBuilder("test").SetID(kPhotosPWAApp).Build();
+      extensions::ExtensionBuilder("test")
+          .SetID(kPhotosPWAApp)
+          .AddFlags(extensions::Extension::FROM_BOOKMARK)
+          .Build();
   registry.AddEnabled(extension);
 
-  GURL url("https://photos.google.com/");
-
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(&registry, nullptr, nullptr);
   app_launch_event_logger_.OnSuggestionChipOrSearchBoxClicked(kPhotosPWAApp, 3,
                                                               2);
@@ -235,7 +247,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeSuggestionChip) {
   const auto entries = test_ukm_recorder_.GetEntriesByName("AppListAppLaunch");
   ASSERT_EQ(1ul, entries.size());
   const auto* entry = entries.back();
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntryMetric(entry, "PositionIndex", 3);
   test_ukm_recorder_.ExpectEntryMetric(entry, "LaunchedFrom", 2);
 }
@@ -243,12 +255,13 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeSuggestionChip) {
 TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeSearchBox) {
   extensions::ExtensionRegistry registry(nullptr);
   scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionBuilder("test").SetID(kPhotosPWAApp).Build();
+      extensions::ExtensionBuilder("test")
+          .SetID(kPhotosPWAApp)
+          .AddFlags(extensions::Extension::FROM_BOOKMARK)
+          .Build();
   registry.AddEnabled(extension);
 
-  GURL url("https://photos.google.com/");
-
-  AppLaunchEventLogger app_launch_event_logger_;
+  AppLaunchEventLoggerForTest app_launch_event_logger_;
   app_launch_event_logger_.SetAppDataForTesting(&registry, nullptr, nullptr);
   app_launch_event_logger_.OnSuggestionChipOrSearchBoxClicked(kPhotosPWAApp, 3,
                                                               4);
@@ -258,7 +271,7 @@ TEST_F(AppLaunchEventLoggerTest, CheckUkmCodeSearchBox) {
   const auto entries = test_ukm_recorder_.GetEntriesByName("AppListAppLaunch");
   ASSERT_EQ(1ul, entries.size());
   const auto* entry = entries.back();
-  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, url);
+  test_ukm_recorder_.ExpectEntrySourceHasUrl(entry, kPhotosPWAUrl);
   test_ukm_recorder_.ExpectEntryMetric(entry, "PositionIndex", 3);
   test_ukm_recorder_.ExpectEntryMetric(entry, "LaunchedFrom", 4);
 }

components/ukm/app_source_url_recorder.h

@@ -34,7 +34,7 @@ class AppSourceUrlRecorder {
   // Get a UKM SourceId for an Arc app.
   static SourceId GetSourceIdForArc(const std::string& package_name);
 
-  // Get a UKM SourceId for a PWA.
+  // Get a UKM SourceId for a PWA or bookmark app.
   static SourceId GetSourceIdForPWA(const GURL& url);
 
   // For internal use only.

tools/metrics/ukm/ukm.xml

@@ -333,7 +333,7 @@ be describing additional metrics about the same event.
   </metric>
   <metric name="AppType">
     <summary>
-      The type of app. 1: CHROME, 2: PLAY, 3: PWA.
+      The type of app. 1: CHROME, 2: PLAY, 3: PWA/Bookmark app.
     </summary>
   </metric>
   <metric name="ClicksEachHour00">
@@ -561,7 +561,8 @@ be describing additional metrics about the same event.
     from the suggestion chip, or from the grid of apps. The UKM metrics are not
     keyed by navigational urls. Instead, for Chrome apps the keys are based upon
     the app id, for Play apps the keys are based upon a hash of the package name
-    and for PWAs the keys are the urls associated with the PWA.
+    and for PWAs and bookmark apps the keys are the urls associated with the
+    PWA/bookmark.
   </summary>
   <metric name="AllClicksLast24Hours">
     <summary>
@@ -579,7 +580,7 @@ be describing additional metrics about the same event.
   </metric>
   <metric name="AppType">
     <summary>
-      The type of app. 1: CHROME, 2: PLAY, 3: PWA.
+      The type of app. 1: CHROME, 2: PLAY, 3: PWA/Bookmark app.
     </summary>
   </metric>
   <metric name="ClickMethod">