Fix crash when finalizing holding space items.

The views for holding space items are inserted at a calculated index
during item finalization. This requires us to determine which section of
holding space UI these views belong to which is based on type.

The previous calculation which returned whether or not two types
belonged to the same section of holding space UI was flawed which could
result in mis-calculation of index during item finalization.

Bug: 1130625
Change-Id: Ib2211531c3281d525dd2bf2a5eac0ae985d94755
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2519057Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#824139}

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -921,47 +921,70 @@ TEST_F(HoldingSpaceTrayTest, PartialNearbyShareItemWithExistingDownloadItems) {
 
   // Add partially initialized nearby share item - verify it doesn't get shown
   // in the UI yet.
-  HoldingSpaceItem* item_1 = AddPartiallyInitializedItem(
-      HoldingSpaceItem::Type::kNearbyShare, base::FilePath("/tmp/fake_1"));
+  HoldingSpaceItem* nearby_share_item =
+      AddPartiallyInitializedItem(HoldingSpaceItem::Type::kNearbyShare,
+                                  base::FilePath("/tmp/nearby_share"));
   EXPECT_FALSE(test_api()->RecentFilesContainerShown());
   EXPECT_TRUE(test_api()->GetDownloadChips().empty());
 
+  // Add partially initialized screenshot item - verify it doesn't get shown in
+  // the UI yet.
+  HoldingSpaceItem* screenshot_item = AddPartiallyInitializedItem(
+      HoldingSpaceItem::Type::kScreenshot, base::FilePath("/tmp/screenshot"));
+  EXPECT_FALSE(test_api()->RecentFilesContainerShown());
+
   // Add two download items.
-  HoldingSpaceItem* item_2 =
-      AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake_2"));
-  HoldingSpaceItem* item_3 =
-      AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake_3"));
+  HoldingSpaceItem* download_item_1 = AddItem(
+      HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/download_1"));
+  HoldingSpaceItem* download_item_2 = AddItem(
+      HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/download_2"));
   EXPECT_TRUE(test_api()->RecentFilesContainerShown());
   EXPECT_TRUE(test_api()->GetPinnedFileChips().empty());
   EXPECT_TRUE(test_api()->GetScreenCaptureViews().empty());
   std::vector<views::View*> download_chips = test_api()->GetDownloadChips();
   ASSERT_EQ(2u, download_chips.size());
-  EXPECT_EQ(item_3->id(),
+  EXPECT_EQ(download_item_2->id(),
             HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
-  EXPECT_EQ(item_2->id(),
+  EXPECT_EQ(download_item_1->id(),
             HoldingSpaceItemView::Cast(download_chips[1])->item()->id());
 
   // Finalize the nearby share item and verify it is not shown.
-  model()->FinalizeOrRemoveItem(item_1->id(), GURL("filesystem:fake_1"));
+  model()->FinalizeOrRemoveItem(nearby_share_item->id(),
+                                GURL("filesystem:nearby_share"));
 
   EXPECT_TRUE(test_api()->GetPinnedFileChips().empty());
   EXPECT_TRUE(test_api()->GetScreenCaptureViews().empty());
   download_chips = test_api()->GetDownloadChips();
   ASSERT_EQ(2u, download_chips.size());
-  EXPECT_EQ(item_3->id(),
+  EXPECT_EQ(download_item_2->id(),
             HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
-  EXPECT_EQ(item_2->id(),
+  EXPECT_EQ(download_item_1->id(),
             HoldingSpaceItemView::Cast(download_chips[1])->item()->id());
 
-  // Remove one of the fully initialized items, and verify the partially
-  // initialized item is not shown.
-  model()->RemoveItem(item_2->id());
+  // Finalize the screenshot item and verify it is shown. Note that the
+  // finalized screenshot item should not affect appearance of the downloads
+  // section of holding space UI. It shows in the screen captures section.
+  model()->FinalizeOrRemoveItem(screenshot_item->id(),
+                                GURL("filesystem:screenshot"));
 
+  EXPECT_TRUE(test_api()->GetPinnedFileChips().empty());
+  EXPECT_EQ(1u, test_api()->GetScreenCaptureViews().size());
   download_chips = test_api()->GetDownloadChips();
   ASSERT_EQ(2u, download_chips.size());
-  EXPECT_EQ(item_3->id(),
+  EXPECT_EQ(download_item_2->id(),
             HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
-  EXPECT_EQ(item_1->id(),
+  EXPECT_EQ(download_item_1->id(),
+            HoldingSpaceItemView::Cast(download_chips[1])->item()->id());
+
+  // Remove one of the fully initialized items, and verify the nearby share item
+  // that was finalized late is shown.
+  model()->RemoveItem(download_item_1->id());
+
+  download_chips = test_api()->GetDownloadChips();
+  ASSERT_EQ(2u, download_chips.size());
+  EXPECT_EQ(download_item_2->id(),
+            HoldingSpaceItemView::Cast(download_chips[0])->item()->id());
+  EXPECT_EQ(nearby_share_item->id(),
             HoldingSpaceItemView::Cast(download_chips[1])->item()->id());
 
   test_api()->Close();

ash/system/holding_space/recent_files_container.cc

@@ -46,16 +46,17 @@ void SetupLabel(views::Label* label) {
   label->SetHorizontalAlignment(gfx::HorizontalAlignment::ALIGN_LEFT);
 }
 
+// Returns if an item of the specified `type` belongs in the downloads section.
 bool BelongsToDownloadsSection(HoldingSpaceItem::Type type) {
   return type == HoldingSpaceItem::Type::kDownload ||
          type == HoldingSpaceItem::Type::kNearbyShare;
 }
 
-bool ItemsBelongsToSameSection(HoldingSpaceItem::Type candidate_type,
-                               HoldingSpaceItem::Type item_type) {
-  return candidate_type == item_type ||
-         (BelongsToDownloadsSection(candidate_type) &&
-          BelongsToDownloadsSection(candidate_type));
+// Returns if items of the specified types belong to the same section.
+bool BelongToSameSection(HoldingSpaceItem::Type type,
+                         HoldingSpaceItem::Type other_type) {
+  return type == other_type || (BelongsToDownloadsSection(type) &&
+                                BelongsToDownloadsSection(other_type));
 }
 
 // DownloadsHeader--------------------------------------------------------------
@@ -176,7 +177,7 @@ void RecentFilesContainer::AddHoldingSpaceItemView(const HoldingSpaceItem* item,
       if (candidate->id() == item->id())
         break;
       if (candidate->IsFinalized() &&
-          ItemsBelongsToSameSection(candidate->type(), item->type())) {
+          BelongToSameSection(candidate->type(), item->type())) {
         ++index;
       }
     }