Pine: Change PineItemView arguments

Change PineItemView to accept an AppInfo object in its constructor,
rather than its individual fields.

Bug: b/328830102
Change-Id: I1dfed9b0cdf3bc7d761245c4dcb9d721e8683525
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5386536
Reviewed-by: Min Chen <minch@chromium.org>
Commit-Queue: Elijah Hewer <hewer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1277895}

ash/wm/window_restore/pine_item_view.cc

@@ -9,9 +9,12 @@
 #include "ash/shell.h"
 #include "ash/style/typography.h"
 #include "ash/wm/window_restore/pine_constants.h"
+#include "ash/wm/window_restore/window_restore_util.h"
 #include "base/barrier_callback.h"
 #include "base/i18n/number_formatting.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "components/services/app_service/public/cpp/app_registry_cache.h"
+#include "components/services/app_service/public/cpp/app_registry_cache_wrapper.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/views/background.h"
@@ -36,12 +39,10 @@ constexpr int kTabCountRounding = 6;
 
 }  // namespace
 
-PineItemView::PineItemView(const std::u16string& app_title,
-                           const std::vector<GURL>& favicons,
-                           const size_t tab_count,
+PineItemView::PineItemView(const PineContentsData::AppInfo& app_info,
                            bool inside_screenshot)
-    : tab_count_(tab_count), inside_screenshot_(inside_screenshot) {
-  SetBetweenChildSpacing(inside_screenshot
+    : tab_count_(app_info.tab_count), inside_screenshot_(inside_screenshot) {
+  SetBetweenChildSpacing(inside_screenshot_
                              ? pine::kScreenshotIconRowChildSpacing
                              : pine::kItemChildSpacing);
   SetCrossAxisAlignment(views::BoxLayout::CrossAxisAlignment::kCenter);
@@ -50,14 +51,14 @@ PineItemView::PineItemView(const std::u16string& app_title,
   auto* browser_image_view = AddChildView(
       views::Builder<views::ImageView>()
           .CopyAddressTo(&image_view_)
-          .SetImageSize(inside_screenshot
+          .SetImageSize(inside_screenshot_
                             ? pine::kScreenshotIconRowImageViewSize
                             : kItemIconPreferredSize)
-          .SetPreferredSize(inside_screenshot
+          .SetPreferredSize(inside_screenshot_
                                 ? pine::kScreenshotIconRowImageViewSize
                                 : pine::kItemIconBackgroundPreferredSize)
           .Build());
-  if (inside_screenshot) {
+  if (inside_screenshot_) {
     views::Separator* separator =
         AddChildView(std::make_unique<views::Separator>());
     separator->SetColorId(ui::kColorAshSystemUIMenuSeparator);
@@ -69,7 +70,7 @@ PineItemView::PineItemView(const std::u16string& app_title,
 
   // Add nested `BoxLayoutView`s, so we can have the title of the window on
   // top, and a row of favicons on the bottom.
-  if (inside_screenshot) {
+  if (inside_screenshot_) {
     AddChildView(
         views::Builder<views::BoxLayoutView>()
             .CopyAddressTo(&favicon_container_view_)
@@ -78,6 +79,25 @@ PineItemView::PineItemView(const std::u16string& app_title,
             .SetBetweenChildSpacing(pine::kScreenshotFaviconSpacing)
             .Build());
   } else {
+    // TODO(http://b/328830102): Handle case where the app is not ready or
+    // installed.
+    apps::AppRegistryCache* cache =
+        apps::AppRegistryCacheWrapper::Get().GetAppRegistryCache(
+            Shell::Get()->session_controller()->GetActiveAccountId());
+
+    // `title` will be the window title from the previous session stored in the
+    // full restore file. The title fetched from the app service would more
+    // accurate, but the app might not be installed yet. Browsers are always
+    // installed and `title` will be the active tab title fetched from session
+    // restore. `cache` might be null in a test environment.
+    // TODO(http://b/328830102): Title should be updated once app is installed.
+    std::string title = app_info.title;
+    if (cache && !IsBrowserAppId(app_info.app_id)) {
+      cache->ForOneApp(
+          app_info.app_id,
+          [&title](const apps::AppUpdate& update) { title = update.Name(); });
+    }
+
     AddChildView(
         views::Builder<views::BoxLayoutView>()
             .SetOrientation(views::BoxLayout::Orientation::kVertical)
@@ -90,7 +110,7 @@ PineItemView::PineItemView(const std::u16string& app_title,
                                                pine::kItemTitleFontSize,
                                                gfx::Font::Weight::BOLD))
                     .SetHorizontalAlignment(gfx::ALIGN_LEFT)
-                    .SetText(app_title),
+                    .SetText(base::UTF8ToUTF16(title)),
                 views::Builder<views::BoxLayoutView>()
                     .CopyAddressTo(&favicon_container_view_)
                     .SetOrientation(views::BoxLayout::Orientation::kHorizontal)
@@ -100,6 +120,7 @@ PineItemView::PineItemView(const std::u16string& app_title,
             .Build());
   }
 
+  const std::vector<GURL>& favicons = app_info.tab_urls;
   if (favicons.empty()) {
     return;
   }

ash/wm/window_restore/pine_item_view.h

@@ -6,6 +6,7 @@
 #define ASH_WM_WINDOW_RESTORE_PINE_ITEM_VIEW_H_
 
 #include "ash/ash_export.h"
+#include "ash/wm/window_restore/pine_contents_data.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "ui/base/metadata/metadata_impl_macros.h"
 #include "ui/views/layout/box_layout_view.h"
@@ -26,9 +27,7 @@ class ASH_EXPORT PineItemView : public views::BoxLayoutView {
   METADATA_HEADER(PineItemView, views::BoxLayoutView)
 
  public:
-  PineItemView(const std::u16string& app_title,
-               const std::vector<GURL>& favicons,
-               const size_t tab_count,
+  PineItemView(const PineContentsData::AppInfo& app_info,
                bool inside_screenshot);
   PineItemView(const PineItemView&) = delete;
   PineItemView& operator=(const PineItemView&) = delete;

ash/wm/window_restore/pine_items_container_view.cc

@@ -9,9 +9,6 @@
 #include "ash/wm/window_restore/pine_constants.h"
 #include "ash/wm/window_restore/pine_item_view.h"
 #include "ash/wm/window_restore/pine_items_overflow_view.h"
-#include "ash/wm/window_restore/window_restore_util.h"
-#include "components/services/app_service/public/cpp/app_registry_cache.h"
-#include "components/services/app_service/public/cpp/app_registry_cache_wrapper.h"
 #include "ui/chromeos/styles/cros_tokens_color_mappings.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/views/background.h"
@@ -39,13 +36,6 @@ PineItemsContainerView::PineItemsContainerView(
   SetMainAxisAlignment(views::BoxLayout::MainAxisAlignment::kStart);
   SetOrientation(views::BoxLayout::Orientation::kVertical);
 
-  // TODO(http://b/328830102): Handle case where the app is not ready or
-  // installed.
-  apps::AppRegistryCache* cache =
-      apps::AppRegistryCacheWrapper::Get().GetAppRegistryCache(
-          Shell::Get()->session_controller()->GetActiveAccountId());
-  auto* delegate = Shell::Get()->saved_desk_delegate();
-
   for (int i = 0; i < elements; ++i) {
     const PineContentsData::AppInfo& app_info = apps_infos[i];
     // If there are more than four elements, we will need to save the last
@@ -56,36 +46,21 @@ PineItemsContainerView::PineItemsContainerView(
       break;
     }
 
-    // `title` will be the window title from the previous session stored in the
-    // full restore file. The title fetched from the app service would more
-    // accurate, but the app might not be installed yet. Browsers are always
-    // installed and `title` will be the active tab title fetched from session
-    // restore. `cache` might be null in a test environment.
-    // TODO(http://b/328830102): Title should be updated once app is installed.
-    std::string title = app_info.title;
-    if (cache && !IsBrowserAppId(app_info.app_id)) {
-      cache->ForOneApp(
-          app_info.app_id,
-          [&title](const apps::AppUpdate& update) { title = update.Name(); });
-    }
-
-    // TODO(hewer|sammiequon): `PineItemView` should just take `app_info` and
-    // `cache` as a constructor argument.
-    PineItemView* item_view = AddChildView(std::make_unique<PineItemView>(
-        base::UTF8ToUTF16(title), app_info.tab_urls, app_info.tab_count,
-        /*inside_screenshot=*/false));
+    PineItemView* item_view = AddChildView(
+        std::make_unique<PineItemView>(app_info, /*inside_screenshot=*/false));
 
     // The callback may be called synchronously.
-    delegate->GetIconForAppId(app_info.app_id, pine::kAppImageSize,
-                              base::BindOnce(
-                                  [](base::WeakPtr<PineItemView> item_view_ptr,
-                                     const gfx::ImageSkia& icon) {
-                                    if (item_view_ptr) {
-                                      item_view_ptr->image_view()->SetImage(
-                                          ui::ImageModel::FromImageSkia(icon));
-                                    }
-                                  },
-                                  item_view->GetWeakPtr()));
+    Shell::Get()->saved_desk_delegate()->GetIconForAppId(
+        app_info.app_id, pine::kAppImageSize,
+        base::BindOnce(
+            [](base::WeakPtr<PineItemView> item_view_ptr,
+               const gfx::ImageSkia& icon) {
+              if (item_view_ptr) {
+                item_view_ptr->image_view()->SetImage(
+                    ui::ImageModel::FromImageSkia(icon));
+              }
+            },
+            item_view->GetWeakPtr()));
   }
 }
 

ash/wm/window_restore/pine_screenshot_icon_row_view.cc

@@ -68,9 +68,7 @@ PineScreenshotIconRowView::PineScreenshotIconRowView(
     const PineContentsData::AppInfo& app_info = apps_infos[0];
 
     PineItemView* item_view = AddChildView(
-        std::make_unique<PineItemView>(base::UTF8ToUTF16(app_info.title),
-                                       app_info.tab_urls, app_info.tab_count,
-                                       /*inside_screenshot=*/true));
+        std::make_unique<PineItemView>(app_info, /*inside_screenshot=*/true));
 
     // The callback may be called synchronously.
     Shell::Get()->saved_desk_delegate()->GetIconForAppId(

ash/wm/window_restore/pine_unittest.cc

@@ -503,10 +503,13 @@ TEST_F(PineTest, ClickRestoreToExit) {
 }
 
 TEST_F(PineTest, PineItemView) {
+  PineContentsData::AppInfo app_info(
+      "TEST_ID", "TEST_TITLE",
+      std::vector<GURL>{GURL(), GURL(), GURL(), GURL()}, 4u);
+
   // Test when the tab count is within regular limits.
-  auto item_view = std::make_unique<PineItemView>(
-      u"TEST", std::vector<GURL>{GURL(), GURL(), GURL(), GURL()}, 4u,
-      /*inside_screenshot=*/false);
+  auto item_view =
+      std::make_unique<PineItemView>(app_info, /*inside_screenshot=*/false);
   EXPECT_EQ(PineItemViewTestApi(item_view.get())
                 .favicon_container_view_for_testing()
                 ->children()
@@ -515,9 +518,9 @@ TEST_F(PineTest, PineItemView) {
   item_view.reset();
 
   // Test the when the tab count has overflow.
-  item_view = std::make_unique<PineItemView>(
-      u"TEST", std::vector<GURL>{GURL(), GURL(), GURL(), GURL(), GURL()}, 10u,
-      /*inside_screenshot=*/false);
+  app_info.tab_count = 10u;
+  item_view =
+      std::make_unique<PineItemView>(app_info, /*inside_screenshot=*/false);
   EXPECT_EQ(PineItemViewTestApi(item_view.get())
                 .favicon_container_view_for_testing()
                 ->children()