dpwa: Implement GetApps() to skip apps in sync install.

This CL introduces no behavior changes.
Now WebAppRegistrar::GetAppIds() uses foreach GetApps().

In follow up CLs:
- rename AllApps() to GetAppsIncludingStubs() (urgent)
- migrate more cases to use GetApps() instead of AllApps() (not urgent)

Implementation details:

AllApps() and GetApps() use protected method FilterApps(filter).
In general, FilterApps() allows to iterate over various subsets from
AllApps().

Internally, Iter::FilterAndSkipApps() filters out unwanted entries.

Bug: 891172
Change-Id: I9a136f9b4f77e77eac28e13d9281453015145925
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1295131Reviewed-by: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818710}

chrome/browser/web_applications/web_app_registrar.cc

@@ -181,12 +181,8 @@ WebAppRegistrar::GetAppDownloadedShortcutsMenuIconsSizes(
 std::vector<AppId> WebAppRegistrar::GetAppIds() const {
   std::vector<AppId> app_ids;
 
-  for (const WebApp& app : AllApps()) {
-    // Apps in sync install are being installed and should be hidden for
-    // most subsystems. OnWebAppInstalled() notification will be send out later.
-    if (!app.is_in_sync_install())
-      app_ids.push_back(app.app_id());
-  }
+  for (const WebApp& app : GetApps())
+    app_ids.push_back(app.app_id());
 
   return app_ids;
 }
@@ -219,8 +215,9 @@ void WebAppRegistrar::OnProfileMarkedForPermanentDeletion(
   registry_profile_being_deleted_ = true;
 }
 
-WebAppRegistrar::AppSet::AppSet(const WebAppRegistrar* registrar)
-    : registrar_(registrar)
+WebAppRegistrar::AppSet::AppSet(const WebAppRegistrar* registrar, Filter filter)
+    : registrar_(registrar),
+      filter_(filter)
 #if DCHECK_IS_ON()
       ,
       mutations_count_(registrar->mutations_count_)
@@ -235,29 +232,43 @@ WebAppRegistrar::AppSet::~AppSet() {
 }
 
 WebAppRegistrar::AppSet::iterator WebAppRegistrar::AppSet::begin() {
-  return iterator(registrar_->registry_.begin());
+  return iterator(registrar_->registry_.begin(), registrar_->registry_.end(),
+                  filter_);
 }
 
 WebAppRegistrar::AppSet::iterator WebAppRegistrar::AppSet::end() {
-  return iterator(registrar_->registry_.end());
+  return iterator(registrar_->registry_.end(), registrar_->registry_.end(),
+                  filter_);
 }
 
 WebAppRegistrar::AppSet::const_iterator WebAppRegistrar::AppSet::begin() const {
-  return const_iterator(registrar_->registry_.begin());
+  return const_iterator(registrar_->registry_.begin(),
+                        registrar_->registry_.end(), filter_);
 }
 
 WebAppRegistrar::AppSet::const_iterator WebAppRegistrar::AppSet::end() const {
-  return const_iterator(registrar_->registry_.end());
+  return const_iterator(registrar_->registry_.end(),
+                        registrar_->registry_.end(), filter_);
 }
 
 const WebAppRegistrar::AppSet WebAppRegistrar::AllApps() const {
-  return AppSet(this);
+  return AppSet(this, nullptr);
+}
+
+const WebAppRegistrar::AppSet WebAppRegistrar::GetApps() const {
+  return AppSet(this, [](const WebApp& web_app) {
+    return !web_app.is_in_sync_install();
+  });
 }
 
 void WebAppRegistrar::SetRegistry(Registry&& registry) {
   registry_ = std::move(registry);
 }
 
+const WebAppRegistrar::AppSet WebAppRegistrar::FilterApps(Filter filter) const {
+  return AppSet(this, filter);
+}
+
 void WebAppRegistrar::CountMutation() {
 #if DCHECK_IS_ON()
   ++mutations_count_;
@@ -278,8 +289,19 @@ WebApp* WebAppRegistrarMutable::GetAppByIdMutable(const AppId& app_id) {
   return const_cast<WebApp*>(GetAppById(app_id));
 }
 
+WebAppRegistrar::AppSet WebAppRegistrarMutable::FilterAppsMutable(
+    Filter filter) {
+  return AppSet(this, filter);
+}
+
 WebAppRegistrar::AppSet WebAppRegistrarMutable::AllAppsMutable() {
-  return AppSet(this);
+  return AppSet(this, nullptr);
+}
+
+WebAppRegistrar::AppSet WebAppRegistrarMutable::GetAppsMutable() {
+  return AppSet(this, [](const WebApp& web_app) {
+    return !web_app.is_in_sync_install();
+  });
 }
 
 bool IsRegistryEqual(const Registry& registry, const Registry& registry2) {

chrome/browser/web_applications/web_app_registrar.h

@@ -77,6 +77,9 @@ class WebAppRegistrar : public AppRegistrar, public ProfileManagerObserver {
   void OnProfileMarkedForPermanentDeletion(
       Profile* profile_to_be_deleted) override;
 
+  // A filter must return false to skip the |web_app|.
+  using Filter = bool (*)(const WebApp& web_app);
+
   // Only range-based |for| loop supported. Don't use AppSet directly.
   // Doesn't support registration and unregistration of WebApp while iterating.
   class AppSet {
@@ -87,24 +90,43 @@ class WebAppRegistrar : public AppRegistrar, public ProfileManagerObserver {
      public:
       using InternalIter = Registry::const_iterator;
 
-      explicit Iter(InternalIter&& internal_iter)
-          : internal_iter_(std::move(internal_iter)) {}
+      Iter(InternalIter&& internal_iter,
+           InternalIter&& internal_end,
+           Filter filter)
+          : internal_iter_(std::move(internal_iter)),
+            internal_end_(std::move(internal_end)),
+            filter_(filter) {
+        FilterAndSkipApps();
+      }
       Iter(Iter&&) = default;
       Iter(const Iter&) = delete;
       Iter& operator=(const Iter&) = delete;
       ~Iter() = default;
 
-      void operator++() { ++internal_iter_; }
+      void operator++() {
+        ++internal_iter_;
+        FilterAndSkipApps();
+      }
       WebAppType& operator*() const { return *internal_iter_->second.get(); }
       bool operator!=(const Iter& iter) const {
         return internal_iter_ != iter.internal_iter_;
       }
 
      private:
+      void FilterAndSkipApps() {
+        if (!filter_)
+          return;
+
+        while (internal_iter_ != internal_end_ && !filter_(**this))
+          ++internal_iter_;
+      }
+
       InternalIter internal_iter_;
+      InternalIter internal_end_;
+      Filter filter_;
     };
 
-    explicit AppSet(const WebAppRegistrar* registrar);
+    AppSet(const WebAppRegistrar* registrar, Filter filter);
     AppSet(AppSet&&) = default;
     AppSet(const AppSet&) = delete;
     AppSet& operator=(const AppSet&) = delete;
@@ -120,17 +142,26 @@ class WebAppRegistrar : public AppRegistrar, public ProfileManagerObserver {
 
    private:
     const WebAppRegistrar* const registrar_;
+    const Filter filter_;
 #if DCHECK_IS_ON()
     const size_t mutations_count_;
 #endif
   };
 
+  // Returns all apps in the registry (a superset) including stubs.
+  // TODO(loyso): Rename this method to GetAppsIncludingStubs().
   const AppSet AllApps() const;
+  // Returns all apps excluding stubs for apps in sync install. Apps in sync
+  // install are being installed and should be hidden for most subsystems. This
+  // is a subset of AllApps().
+  const AppSet GetApps() const;
 
  protected:
   Registry& registry() { return registry_; }
   void SetRegistry(Registry&& registry);
 
+  const AppSet FilterApps(Filter filter) const;
+
   void CountMutation();
 
  private:
@@ -151,7 +182,12 @@ class WebAppRegistrarMutable : public WebAppRegistrar {
   void InitRegistry(Registry&& registry);
 
   WebApp* GetAppByIdMutable(const AppId& app_id);
+
+  AppSet FilterAppsMutable(Filter filter);
+
+  // TODO(loyso): Rename this method to GetAppsIncludingStubsMutable().
   AppSet AllAppsMutable();
+  AppSet GetAppsMutable();
 
   using WebAppRegistrar::CountMutation;
   using WebAppRegistrar::registry;

chrome/browser/web_applications/web_app_registrar_unittest.cc

@@ -10,6 +10,8 @@
 #include <utility>
 
 #include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/notreached.h"
 #include "base/optional.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
@@ -73,6 +75,9 @@ class WebAppRegistrarTest : public WebAppTest {
     return controller().database_factory();
   }
   WebAppRegistrar& registrar() { return controller().registrar(); }
+  WebAppRegistrarMutable& mutable_registrar() {
+    return controller().mutable_registrar();
+  }
   WebAppSyncBridge& sync_bridge() { return controller().sync_bridge(); }
 
   std::set<AppId> RegisterAppsForTesting(Registry registry) {
@@ -273,7 +278,7 @@ TEST_F(WebAppRegistrarTest, InitRegistrarAndDoForEachApp) {
 TEST_F(WebAppRegistrarTest, AllAppsMutable) {
   std::set<AppId> ids = InitRegistrarWithApps("https://example.com/path", 10);
 
-  for (WebApp& web_app : controller().mutable_registrar().AllAppsMutable()) {
+  for (WebApp& web_app : mutable_registrar().AllAppsMutable()) {
     web_app.SetDisplayMode(DisplayMode::kStandalone);
     const size_t num_removed = ids.erase(web_app.app_id());
     EXPECT_EQ(1U, num_removed);
@@ -300,6 +305,73 @@ TEST_F(WebAppRegistrarTest, DoForEachAndUnregisterAllApps) {
   EXPECT_TRUE(registrar().is_empty());
 }
 
+TEST_F(WebAppRegistrarTest, FilterApps) {
+  controller().Init();
+
+  Registry registry = CreateRegistryForTesting("https://example.com/path", 100);
+  auto ids = RegisterAppsForTesting(std::move(registry));
+
+  for (const WebApp& web_app : mutable_registrar().FilterAppsMutable(
+           [](const WebApp& web_app) { return false; })) {
+    NOTREACHED();
+    ALLOW_UNUSED_LOCAL(web_app);
+  }
+
+  for (const WebApp& web_app : mutable_registrar().FilterAppsMutable(
+           [](const WebApp& web_app) { return true; })) {
+    const size_t num_removed = ids.erase(web_app.app_id());
+    EXPECT_EQ(1U, num_removed);
+  }
+  EXPECT_TRUE(ids.empty());
+}
+
+TEST_F(WebAppRegistrarTest, GetApps) {
+  std::set<AppId> ids = InitRegistrarWithApps("https://example.com/path", 10);
+
+  int not_in_sync_install_count = 0;
+  for (const WebApp& web_app : registrar().GetApps()) {
+    ++not_in_sync_install_count;
+    EXPECT_TRUE(base::Contains(ids, web_app.app_id()));
+  }
+  EXPECT_EQ(10, not_in_sync_install_count);
+
+  auto web_app_in_sync1 = CreateWebApp("https://example.org/sync1");
+  web_app_in_sync1->SetIsInSyncInstall(true);
+  const AppId web_app_id_in_sync1 = web_app_in_sync1->app_id();
+  RegisterApp(std::move(web_app_in_sync1));
+
+  auto web_app_in_sync2 = CreateWebApp("https://example.org/sync2");
+  web_app_in_sync2->SetIsInSyncInstall(true);
+  const AppId web_app_id_in_sync2 = web_app_in_sync2->app_id();
+  RegisterApp(std::move(web_app_in_sync2));
+
+  int all_apps_count = 0;
+  for (const WebApp& web_app : registrar().AllApps()) {
+    ALLOW_UNUSED_LOCAL(web_app);
+    ++all_apps_count;
+  }
+  EXPECT_EQ(12, all_apps_count);
+
+  for (const WebApp& web_app : registrar().GetApps()) {
+    EXPECT_NE(web_app_id_in_sync1, web_app.app_id());
+    EXPECT_NE(web_app_id_in_sync2, web_app.app_id());
+
+    const size_t num_removed = ids.erase(web_app.app_id());
+    EXPECT_EQ(1U, num_removed);
+  }
+  EXPECT_TRUE(ids.empty());
+
+  UnregisterApp(web_app_id_in_sync1);
+  UnregisterApp(web_app_id_in_sync2);
+
+  not_in_sync_install_count = 0;
+  for (const WebApp& web_app : registrar().GetApps()) {
+    ALLOW_UNUSED_LOCAL(web_app);
+    ++not_in_sync_install_count;
+  }
+  EXPECT_EQ(10, not_in_sync_install_count);
+}
+
 TEST_F(WebAppRegistrarTest, WebAppSyncBridge) {
   std::set<AppId> ids = InitRegistrarWithApps("https://example.com/path", 100);