[ios] Improves InfobarBanner queueing.

Currently we were using a NSDictionary to store all Coordinators,
using the InfobarType as Key. The Problem is that there can be more
than one Infobar with type InfobarTypeConfirm, this meant that we weren't
keeping track of the Infobars correctly. Or this meant that the wrong Infobar
would be removed on childCoordinatorStopped.

This CL creates an Array to store all Infobars and a Dictionary only for
Infobars with badges, in this case the type matters for the Badge Commands.


Bug: 961343
Change-Id: If47b2da8c3158ab3c7e093f5b1673a40464ce3ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1824073
Commit-Queue: Sergio Collazos <sczs@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Chris Lu <thegreenfrog@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699954}

ios/chrome/browser/ui/infobars/coordinators/infobar_confirm_coordinator.mm

@@ -71,7 +71,7 @@
     // from memory.
     self.delegate->RemoveInfoBar();
     _confirmInfobarDelegate = nil;
-    [self.infobarContainer childCoordinatorStopped:self.infobarType];
+    [self.infobarContainer childCoordinatorStopped:self];
   }
 }
 

ios/chrome/browser/ui/infobars/coordinators/infobar_coordinator.mm

@@ -93,6 +93,7 @@ const CGFloat kiPadBannerOverlapWithOmnibox = 10.0;
   DCHECK(self.browserState);
   DCHECK(self.baseViewController);
   DCHECK(self.bannerViewController);
+  DCHECK(self.started);
 
   // If |self.baseViewController| is not part of the ViewHierarchy the banner
   // shouldn't be presented.
@@ -140,6 +141,7 @@ const CGFloat kiPadBannerOverlapWithOmnibox = 10.0;
 }
 
 - (void)presentInfobarModal {
+  DCHECK(self.started);
   ProceduralBlock modalPresentation = ^{
     DCHECK(self.infobarBannerState !=
            InfobarBannerPresentationState::Presented);
@@ -245,7 +247,7 @@ const CGFloat kiPadBannerOverlapWithOmnibox = 10.0;
   self.bannerTransitionDriver = nil;
   animatedFullscreenDisabler_ = nullptr;
   [self infobarWasDismissed];
-  [self.infobarContainer childCoordinatorBannerWasDismissed:self.infobarType];
+  [self.infobarContainer childCoordinatorBannerWasDismissed:self];
 }
 
 #pragma mark InfobarBannerPositioner

ios/chrome/browser/ui/infobars/coordinators/infobar_password_coordinator.mm

@@ -99,7 +99,7 @@
     // from memory.
     self.delegate->RemoveInfoBar();
     _passwordInfoBarDelegate = nil;
-    [self.infobarContainer childCoordinatorStopped:self.infobarType];
+    [self.infobarContainer childCoordinatorStopped:self];
   }
 }
 

ios/chrome/browser/ui/infobars/infobar_container.h

@@ -7,19 +7,20 @@
 
 #import <Foundation/Foundation.h>
 
-enum class InfobarType;
+@class InfobarCoordinator;
 
 // Protocol for the InfobarCoordinators to communicate with the InfobarContainer
 // Coordinator.
 @protocol InfobarContainer
 
-// Informs the InfobarContainer Coordinator that its child coordinator of type
-// |infobarType| has dismissed its banner.
-- (void)childCoordinatorBannerWasDismissed:(InfobarType)infobarType;
+// Informs the InfobarContainerCoordinator that |infobarCoordinator| has
+// dismissed its banner.
+- (void)childCoordinatorBannerWasDismissed:
+    (InfobarCoordinator*)infobarCoordinator;
 
-// Informs the InfobarContainer Coordinator that its child coordinator of type
-// |infobarType| has stopped.
-- (void)childCoordinatorStopped:(InfobarType)infobarType;
+// Informs the InfobarContainerCoordinator that |infobarCoordinator| has
+// stopped.
+- (void)childCoordinatorStopped:(InfobarCoordinator*)infobarCoordinator;
 
 @end
 

ios/chrome/browser/ui/infobars/infobar_container_coordinator.mm

@@ -48,7 +48,7 @@
 // infobarCoordinators holds all InfobarCoordinators this ContainerCoordinator
 // can display.
 @property(nonatomic, strong)
-    NSMutableDictionary<NSNumber*, InfobarCoordinator*>* infobarCoordinators;
+    NSMutableArray<InfobarCoordinator*>* infobarCoordinators;
 // Array of Coordinators which banners haven't been presented yet. Once a
 // Coordinator banner is presented it should be removed from this Array. If
 // empty then it means there are no banners queued to be presented.
@@ -67,7 +67,7 @@
                               browserState:browserState];
   if (self) {
     _webStateList = webStateList;
-    _infobarCoordinators = [NSMutableDictionary dictionary];
+    _infobarCoordinators = [NSMutableArray array];
     _infobarCoordinatorsToPresent = [NSMutableArray array];
   }
   return self;
@@ -148,8 +148,7 @@
                           completion:(void (^)())completion {
   DCHECK(IsInfobarUIRebootEnabled());
 
-  for (InfobarCoordinator* infobarCoordinator in
-       [self.infobarCoordinators allValues]) {
+  for (InfobarCoordinator* infobarCoordinator in self.infobarCoordinators) {
     if (infobarCoordinator.infobarBannerState !=
         InfobarBannerPresentationState::NotPresented) {
       // Since only one Banner can be presented at any time, dismiss it.
@@ -173,8 +172,7 @@
 #pragma mark - ChromeCoordinator
 
 - (MutableCoordinatorArray*)childCoordinators {
-  return static_cast<MutableCoordinatorArray*>(
-      [self.infobarCoordinators allValues]);
+  return static_cast<MutableCoordinatorArray*>(self.infobarCoordinators);
 }
 
 #pragma mark - Accessors
@@ -196,8 +194,7 @@
 
 - (InfobarBannerPresentationState)infobarBannerState {
   DCHECK(IsInfobarUIRebootEnabled());
-  for (InfobarCoordinator* infobarCoordinator in
-       [self.infobarCoordinators allValues]) {
+  for (InfobarCoordinator* infobarCoordinator in self.infobarCoordinators) {
     if (infobarCoordinator.infobarBannerState !=
         InfobarBannerPresentationState::NotPresented) {
       // Since only one Banner can be presented at any time, early return.
@@ -216,11 +213,9 @@
   InfobarCoordinator* infobarCoordinator =
       static_cast<InfobarCoordinator*>(infoBarDelegate);
 
-  NSNumber* infobarKey =
-      [NSNumber numberWithInt:static_cast<int>(infoBarDelegate.infobarType)];
-  self.infobarCoordinators[infobarKey] = infobarCoordinator;
+  [self.infobarCoordinators addObject:infobarCoordinator];
 
-  // Present the InfobarBanner, and set the Coordinator and View hierarchies.
+  // Configure the Coordinator and try to present the Banner afterwards.
   [infobarCoordinator start];
   // Only set the infobarCoordinator's badgeDelegate if it supports a badge. Not
   // doing so might cause undefined behavior since no badge was added.
@@ -235,7 +230,7 @@
 }
 
 - (void)infobarManagerWillChange {
-  self.infobarCoordinators = [NSMutableDictionary dictionary];
+  self.infobarCoordinators = [NSMutableArray array];
   self.infobarCoordinatorsToPresent = [NSMutableArray array];
 }
 
@@ -251,25 +246,28 @@
 
 #pragma mark InfobarContainer
 
-- (void)childCoordinatorBannerWasDismissed:(InfobarType)infobarType {
+- (void)childCoordinatorBannerWasDismissed:
+    (InfobarCoordinator*)infobarCoordinator {
   InfobarCoordinator* coordinator =
       [self.infobarCoordinatorsToPresent firstObject];
   if (coordinator)
     [self presentBannerForInfobarCoordinator:coordinator];
 }
 
-- (void)childCoordinatorStopped:(InfobarType)infobarType {
-  DCHECK(IsInfobarUIRebootEnabled());
-  NSNumber* infobarKey = [NSNumber numberWithInt:static_cast<int>(infobarType)];
-  [self.infobarCoordinators removeObjectForKey:infobarKey];
+- (void)childCoordinatorStopped:(InfobarCoordinator*)infobarCoordinator {
+  [self.infobarCoordinators removeObject:infobarCoordinator];
+  // Also remove it from |infobarCoordinatorsToPresent| in case it was queued
+  // for a presentation.
+  [self.infobarCoordinatorsToPresent removeObject:infobarCoordinator];
 }
 
 #pragma mark InfobarCommands
 
 - (void)displayModalInfobar:(InfobarType)infobarType {
-  NSNumber* infobarKey = [NSNumber numberWithInt:static_cast<int>(infobarType)];
-  InfobarCoordinator* infobarCoordinator = self.infobarCoordinators[infobarKey];
+  InfobarCoordinator* infobarCoordinator =
+      [self infobarCoordinatorForInfobarTye:infobarType];
   DCHECK(infobarCoordinator);
+  DCHECK(infobarCoordinator.infobarType != InfobarType::kInfobarTypeConfirm);
   [infobarCoordinator presentInfobarModal];
 }
 
@@ -311,4 +309,17 @@
   }
 }
 
+// Returns the InfobarCoordinator for |infobarType|. If there's more than one
+// (e.g. kInfobarTypeConfirm) it will return the first one that was added. If no
+// InfobarCoordinator returns nil.
+- (InfobarCoordinator*)infobarCoordinatorForInfobarTye:
+    (InfobarType)infobarType {
+  for (InfobarCoordinator* coordinator in self.infobarCoordinators) {
+    if (coordinator.infobarType == infobarType) {
+      return coordinator;
+    }
+  }
+  return nil;
+}
+
 @end

ios/chrome/browser/ui/infobars/infobar_container_coordinator_unittest.mm

@@ -680,3 +680,37 @@ TEST_F(InfobarContainerCoordinatorTest,
   ASSERT_EQ(NSUInteger(2),
             infobar_container_coordinator_.childCoordinators.count);
 }
+
+// Tests that that a second Infobar (added right after the first one) is
+// not displayed if its destroyed before presentation.
+TEST_F(InfobarContainerCoordinatorTest, TestInfobarQueueStoppedNoDisplay) {
+  AddInfobar();
+  AddSecondInfobar();
+  ASSERT_EQ(NSUInteger(2),
+            infobar_container_coordinator_.childCoordinators.count);
+
+  EXPECT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(
+      base::test::ios::kWaitForUIElementTimeout, ^bool {
+        return coordinator_.infobarBannerState ==
+               InfobarBannerPresentationState::Presented;
+      }));
+  ASSERT_EQ(infobar_container_coordinator_.infobarBannerState,
+            InfobarBannerPresentationState::Presented);
+
+  [second_coordinator_ stop];
+  [infobar_container_coordinator_ dismissInfobarBannerAnimated:NO
+                                                    completion:nil];
+  ASSERT_TRUE(base::test::ios::WaitUntilConditionOrTimeout(
+      base::test::ios::kWaitForUIElementTimeout, ^bool {
+        return coordinator_.infobarBannerState ==
+               InfobarBannerPresentationState::NotPresented;
+      }));
+
+  ASSERT_EQ(infobar_container_coordinator_.infobarBannerState,
+            InfobarBannerPresentationState::NotPresented);
+  ASSERT_EQ(NSUInteger(1),
+            infobar_container_coordinator_.childCoordinators.count);
+}
+
+// TODO(crbug.com/961343): Add tests that use a BadgedInfobar, in order to do
+// this a new TestInfoBarDelegate needs to be created.