[tab_grid] Hack-in border for active tab.

[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5WmpVS0tHcl9tN0E

BUG=686770
Review-Url: https://codereview.chromium.org/2709563002
Cr-Commit-Position: refs/heads/master@{#452796}
Committed: https://chromium.googlesource.com/chromium/src/+/a10c1ff3e00da36f1532264aa1866433aedaaae5

[edchin, 2017-02-22 23:51:17]

edchin@chromium.org changed reviewers:
+ jif@chromium.org, lpromero@chromium.org, marq@chromium.org, sczs@chromium.org

[edchin, 2017-02-22 23:53:40]

Description was changed from

==========
[tab_grid] Hack-in border for active tab.

BUG=686770
==========

to

==========
[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5RHlfNXJ3d1lDMEU

BUG=686770
==========

[marq (ping after 24h), 2017-02-23 11:04:30]

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm (right):

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm:78: //
PLACEHOLDER: This will be refined to UI mocks.
I don't want to put placeholder code into ios/chrome.

I'd suggest subclassing this class in clean/ instead.

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_controller.mm
(right):

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_controller.mm:273: -
(BOOL)collectionView:(UICollectionView*)collectionView
Definitely not worth adding this code just to keep the selection border code in
the same class.

[edchin, 2017-02-23 17:39:55]

Description was changed from

==========
[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5RHlfNXJ3d1lDMEU

BUG=686770
==========

to

==========
[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5WmpVS0tHcl9tN0E

BUG=686770
==========

[edchin, 2017-02-23 17:41:54]

The screenshot is updated. Implemented a cleaner and more elegant solution.

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm (right):

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm:78: //
PLACEHOLDER: This will be refined to UI mocks.
On 2017/02/23 11:04:30, marq wrote:
> I don't want to put placeholder code into ios/chrome.
> 
> I'd suggest subclassing this class in clean/ instead.

Good suggestion. Done.

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_controller.mm
(right):

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_controller.mm:273: -
(BOOL)collectionView:(UICollectionView*)collectionView
On 2017/02/23 11:04:30, marq wrote:
> Definitely not worth adding this code just to keep the selection border code
in
> the same class.

Agreed! Your suggestion is very elegant.

[jif, 2017-02-23 17:43:21]

lgtm

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm (right):

https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm:78: //
PLACEHOLDER: This will be refined to UI mocks.
On 2017/02/23 11:04:30, marq wrote:
> I don't want to put placeholder code into ios/chrome.
> 
> I'd suggest subclassing this class in clean/ instead.

Or duplicate it in ios/clean?

[edchin, 2017-02-23 22:15:14]

On 2017/02/23 17:43:21, jif wrote:
> lgtm
> 
>
https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
> File ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm (right):
> 
>
https://codereview.chromium.org/2709563002/diff/20001/ios/chrome/browser/ui/t...
> ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.mm:78: //
> PLACEHOLDER: This will be refined to UI mocks.
> On 2017/02/23 11:04:30, marq wrote:
> > I don't want to put placeholder code into ios/chrome.
> > 
> > I'd suggest subclassing this class in clean/ instead.
> 
> Or duplicate it in ios/clean?

I subclassed instead of duplicating for now. I will fork as late as possible.

[marq (ping after 24h), 2017-02-24 09:28:21]

lgtm

[edchin, 2017-02-24 10:30:24]

The CQ bit was checked by edchin@chromium.org

[commit-bot: I haz the power, 2017-02-24 10:30:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 10:37:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 10:37:56]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[edchin, 2017-02-24 11:26:05]

The CQ bit was checked by edchin@chromium.org

[edchin, 2017-02-24 11:26:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2709563002/#ps60001
(title: "Update showcase")

[commit-bot: I haz the power, 2017-02-24 11:26:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 11:35:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 11:35:13]

Failed to apply patch for
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:
While running git apply --index -p1;
  error: patch failed:
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:9
  error: ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm: patch
does not apply

Patch:       ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
Index: ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
diff --git a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
index
dd85b856122b0482f828615ced9c46712e65c261..22264b376e7dd9236f86750faced269f65064f8d
100644
--- a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
+++ b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
@@ -9,7 +9,6 @@
 #import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h"
 
 #include "base/mac/foundation_util.h"
-#import "ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.h"
 #import
"ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_collection_view_layout.h"
 #import "ios/clean/chrome/browser/ui/actions/settings_actions.h"
 #import "ios/clean/chrome/browser/ui/actions/tab_grid_actions.h"
@@ -18,6 +17,7 @@
 #import "ios/clean/chrome/browser/ui/commands/tab_grid_commands.h"
 #import
"ios/clean/chrome/browser/ui/tab_grid/mdc_floating_button+cr_tab_grid.h"
 #import
"ios/clean/chrome/browser/ui/tab_grid/tab_grid_collection_view_layout.h"
+#import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_tab_cell.h"
 #import "ios/clean/chrome/browser/ui/tab_grid/ui_stack_view+cr_tab_grid.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -70,8 +70,8 @@ const CGFloat kToolbarHeight = 64.0f;
   self.grid = grid;
   self.grid.dataSource = self;
   self.grid.delegate = self;
-  [self.grid registerClass:[TabSwitcherLocalSessionCell class]
-      forCellWithReuseIdentifier:[TabSwitcherLocalSessionCell identifier]];
+  [self.grid registerClass:[TabGridTabCell class]
+      forCellWithReuseIdentifier:[TabGridTabCell identifier]];
 
   [NSLayoutConstraint activateConstraints:@[
     [self.grid.topAnchor constraintEqualToAnchor:toolbar.bottomAnchor],
@@ -108,19 +108,27 @@ const CGFloat kToolbarHeight = 64.0f;
 
 - (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView
                  cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
-  TabSwitcherLocalSessionCell* cell =
-      base::mac::ObjCCastStrict<TabSwitcherLocalSessionCell>([collectionView
-          dequeueReusableCellWithReuseIdentifier:
-                              [TabSwitcherLocalSessionCell identifier]
+  TabGridTabCell* cell =
+      base::mac::ObjCCastStrict<TabGridTabCell>([collectionView
+          dequeueReusableCellWithReuseIdentifier:[TabGridTabCell identifier]
                                     forIndexPath:indexPath]);
   cell.delegate = self;
   [cell setSessionType:TabSwitcherSessionType::REGULAR_SESSION];
   [cell setAppearanceForTabTitle:[self.dataSource titleAtIndex:indexPath.item]
                          favicon:nil
                         cellSize:CGSizeZero];
+  [cell setSelected:(indexPath.item == [self.dataSource indexOfActiveTab])];
   return cell;
 }
 
+#pragma mark - UICollectionViewDelegate methods
+
+- (BOOL)collectionView:(UICollectionView*)collectionView
+    shouldSelectItemAtIndexPath:(NSIndexPath*)indexPath {
+  // Prevent user selection of items.
+  return NO;
+}
+
 #pragma mark - ZoomTransitionDelegate methods
 
 - (CGRect)rectForZoomWithKey:(NSObject*)key inView:(UIView*)view {
@@ -149,6 +157,13 @@ const CGFloat kToolbarHeight = 64.0f;
   NSInteger index = [self.grid numberOfItemsInSection:0];
   NSIndexPath* indexPath = [NSIndexPath indexPathForItem:index inSection:0];
   auto updateBlock = ^{
+    // Unselect current selected item.
+    NSInteger selectedIndex = [self.dataSource indexOfActiveTab];
+    NSIndexPath* selectedIndexPath =
+        [NSIndexPath indexPathForItem:selectedIndex inSection:0];
+    [self.grid reloadItemsAtIndexPaths:@[ selectedIndexPath ]];
+
+    // Create and show new tab.
     [self.tabCommandHandler createNewTabAtIndexPath:indexPath];
     [self.tabCommandHandler showTabAtIndexPath:indexPath];
     [self.grid insertItemsAtIndexPaths:@[ indexPath ]];
@@ -164,7 +179,14 @@ const CGFloat kToolbarHeight = 64.0f;
 }
 
 - (void)cellPressed:(UICollectionViewCell*)cell {
-  [self.tabCommandHandler showTabAtIndexPath:[self.grid
indexPathForCell:cell]];
+  NSInteger selectedIndex = [self.dataSource indexOfActiveTab];
+  NSIndexPath* selectedIndexPath =
+      [NSIndexPath indexPathForItem:selectedIndex inSection:0];
+
+  NSIndexPath* newSelectedIndexPath = [self.grid indexPathForCell:cell];
+  [self.tabCommandHandler showTabAtIndexPath:newSelectedIndexPath];
+  [self.grid
+      reloadItemsAtIndexPaths:@[ selectedIndexPath, newSelectedIndexPath ]];
 }
 
 - (void)deleteButtonPressedForCell:(UICollectionViewCell*)cell {

[edchin, 2017-02-24 11:41:45]

The CQ bit was checked by edchin@chromium.org

[commit-bot: I haz the power, 2017-02-24 11:41:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 11:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 11:45:52]

Failed to apply patch for
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:
While running git apply --index -p1;
  error: patch failed:
ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm:9
  error: ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm: patch
does not apply

Patch:       ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
Index: ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
diff --git a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
index
dd85b856122b0482f828615ced9c46712e65c261..22264b376e7dd9236f86750faced269f65064f8d
100644
--- a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
+++ b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.mm
@@ -9,7 +9,6 @@
 #import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h"
 
 #include "base/mac/foundation_util.h"
-#import "ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_cell.h"
 #import
"ios/chrome/browser/ui/tab_switcher/tab_switcher_panel_collection_view_layout.h"
 #import "ios/clean/chrome/browser/ui/actions/settings_actions.h"
 #import "ios/clean/chrome/browser/ui/actions/tab_grid_actions.h"
@@ -18,6 +17,7 @@
 #import "ios/clean/chrome/browser/ui/commands/tab_grid_commands.h"
 #import
"ios/clean/chrome/browser/ui/tab_grid/mdc_floating_button+cr_tab_grid.h"
 #import
"ios/clean/chrome/browser/ui/tab_grid/tab_grid_collection_view_layout.h"
+#import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_tab_cell.h"
 #import "ios/clean/chrome/browser/ui/tab_grid/ui_stack_view+cr_tab_grid.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -70,8 +70,8 @@ const CGFloat kToolbarHeight = 64.0f;
   self.grid = grid;
   self.grid.dataSource = self;
   self.grid.delegate = self;
-  [self.grid registerClass:[TabSwitcherLocalSessionCell class]
-      forCellWithReuseIdentifier:[TabSwitcherLocalSessionCell identifier]];
+  [self.grid registerClass:[TabGridTabCell class]
+      forCellWithReuseIdentifier:[TabGridTabCell identifier]];
 
   [NSLayoutConstraint activateConstraints:@[
     [self.grid.topAnchor constraintEqualToAnchor:toolbar.bottomAnchor],
@@ -108,19 +108,27 @@ const CGFloat kToolbarHeight = 64.0f;
 
 - (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView
                  cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
-  TabSwitcherLocalSessionCell* cell =
-      base::mac::ObjCCastStrict<TabSwitcherLocalSessionCell>([collectionView
-          dequeueReusableCellWithReuseIdentifier:
-                              [TabSwitcherLocalSessionCell identifier]
+  TabGridTabCell* cell =
+      base::mac::ObjCCastStrict<TabGridTabCell>([collectionView
+          dequeueReusableCellWithReuseIdentifier:[TabGridTabCell identifier]
                                     forIndexPath:indexPath]);
   cell.delegate = self;
   [cell setSessionType:TabSwitcherSessionType::REGULAR_SESSION];
   [cell setAppearanceForTabTitle:[self.dataSource titleAtIndex:indexPath.item]
                          favicon:nil
                         cellSize:CGSizeZero];
+  [cell setSelected:(indexPath.item == [self.dataSource indexOfActiveTab])];
   return cell;
 }
 
+#pragma mark - UICollectionViewDelegate methods
+
+- (BOOL)collectionView:(UICollectionView*)collectionView
+    shouldSelectItemAtIndexPath:(NSIndexPath*)indexPath {
+  // Prevent user selection of items.
+  return NO;
+}
+
 #pragma mark - ZoomTransitionDelegate methods
 
 - (CGRect)rectForZoomWithKey:(NSObject*)key inView:(UIView*)view {
@@ -149,6 +157,13 @@ const CGFloat kToolbarHeight = 64.0f;
   NSInteger index = [self.grid numberOfItemsInSection:0];
   NSIndexPath* indexPath = [NSIndexPath indexPathForItem:index inSection:0];
   auto updateBlock = ^{
+    // Unselect current selected item.
+    NSInteger selectedIndex = [self.dataSource indexOfActiveTab];
+    NSIndexPath* selectedIndexPath =
+        [NSIndexPath indexPathForItem:selectedIndex inSection:0];
+    [self.grid reloadItemsAtIndexPaths:@[ selectedIndexPath ]];
+
+    // Create and show new tab.
     [self.tabCommandHandler createNewTabAtIndexPath:indexPath];
     [self.tabCommandHandler showTabAtIndexPath:indexPath];
     [self.grid insertItemsAtIndexPaths:@[ indexPath ]];
@@ -164,7 +179,14 @@ const CGFloat kToolbarHeight = 64.0f;
 }
 
 - (void)cellPressed:(UICollectionViewCell*)cell {
-  [self.tabCommandHandler showTabAtIndexPath:[self.grid
indexPathForCell:cell]];
+  NSInteger selectedIndex = [self.dataSource indexOfActiveTab];
+  NSIndexPath* selectedIndexPath =
+      [NSIndexPath indexPathForItem:selectedIndex inSection:0];
+
+  NSIndexPath* newSelectedIndexPath = [self.grid indexPathForCell:cell];
+  [self.tabCommandHandler showTabAtIndexPath:newSelectedIndexPath];
+  [self.grid
+      reloadItemsAtIndexPaths:@[ selectedIndexPath, newSelectedIndexPath ]];
 }
 
 - (void)deleteButtonPressedForCell:(UICollectionViewCell*)cell {

[edchin, 2017-02-24 11:54:26]

The CQ bit was checked by edchin@chromium.org

[edchin, 2017-02-24 11:54:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2709563002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-24 11:54:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 12:07:52]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487937266908330,
"parent_rev": "f365c932c836f3e663eca835186d92f3e6879440", "commit_rev":
"a10c1ff3e00da36f1532264aa1866433aedaaae5"}

[commit-bot: I haz the power, 2017-02-24 12:08:25]

Description was changed from

==========
[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5WmpVS0tHcl9tN0E

BUG=686770
==========

to

==========
[tab_grid] Hack-in border for active tab.

Screenshot:
https://drive.google.com/open?id=0BwS6YyQeisH5WmpVS0tHcl9tN0E

BUG=686770

Review-Url: https://codereview.chromium.org/2709563002
Cr-Commit-Position: refs/heads/master@{#452796}
Committed:
https://chromium.googlesource.com/chromium/src/+/a10c1ff3e00da36f1532264aa186...
==========

[commit-bot: I haz the power, 2017-02-24 12:08:26]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a10c1ff3e00da36f1532264aa186...

[lody, 2017-02-24 13:42:41]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2714763003/ by lod@chromium.org.

The reason for reverting is: Test "testLaunchAndTappingCell" in
ios_showcase_egtests is broken by this CL.
https://build.chromium.org/p/chromium.fyi/builders/EarlGreyiOS/builds/24682.