[ios] Improve toolbar in tab_grid

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 9ee9700b5b41eb6a12677446277f2dbadc36421c..701010d696079a9b930e6e935e892f4ad497792a 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
@@ -19,17 +19,12 @@
 #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"
+#import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_toolbar.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
 #endif
 
-namespace {
-// Height of toolbar in tab grid.
-const CGFloat kToolbarHeight = 64.0f;
-}
-
 @interface TabGridViewController ()<SettingsActions,
                                     TabGridActions,
                                     UICollectionViewDataSource,
@@ -37,6 +32,7 @@ const CGFloat kToolbarHeight = 64.0f;
                                     SessionCellDelegate>
 @property(nonatomic, weak) UICollectionView* grid;
 @property(nonatomic, weak) UIView* noTabsOverlay;
+@property(nonatomic, strong) TabGridToolbar* toolbar;

[marq (ping after 24h), 2017/03/02 06:04:54]

Prefer to be consistent with weak vs strong for subviews. I prefer the view
hierarchy to be the only strong owner of views that are always present; that's
the approach used for |grid| and |noTabsOverlay|.

[edchin, 2017/03/02 07:12:44]

Will do.

[edchin, 2017/03/16 07:19:59]

Done.

 @property(nonatomic, strong) MDCFloatingButton* floatingNewTabButton;
 @end
 
@@ -48,20 +44,10 @@ const CGFloat kToolbarHeight = 64.0f;
 @synthesize tabCommandHandler = _tabCommandHandler;
 @synthesize grid = _grid;
 @synthesize noTabsOverlay = _noTabsOverlay;
+@synthesize toolbar = _toolbar;
 @synthesize floatingNewTabButton = _floatingNewTabButton;
 
 - (void)viewDidLoad {
-  UIView* toolbar = [UIStackView cr_tabGridToolbarStackView];
-  [self.view addSubview:toolbar];
-
-  toolbar.translatesAutoresizingMaskIntoConstraints = NO;
-  [NSLayoutConstraint activateConstraints:@[
-    [toolbar.heightAnchor constraintEqualToConstant:kToolbarHeight],
-    [toolbar.widthAnchor constraintEqualToAnchor:self.view.widthAnchor],
-    [toolbar.topAnchor constraintEqualToAnchor:self.view.topAnchor],
-    [toolbar.centerXAnchor constraintEqualToAnchor:self.view.centerXAnchor]
-  ]];
-
   TabGridCollectionViewLayout* layout =
       [[TabGridCollectionViewLayout alloc] init];
   UICollectionView* grid = [[UICollectionView alloc] initWithFrame:CGRectZero
@@ -77,11 +63,25 @@ const CGFloat kToolbarHeight = 64.0f;
       forCellWithReuseIdentifier:[TabGridTabCell identifier]];
 
   [NSLayoutConstraint activateConstraints:@[
-    [self.grid.topAnchor constraintEqualToAnchor:toolbar.bottomAnchor],
+    [self.grid.topAnchor constraintEqualToAnchor:self.view.topAnchor],
     [self.grid.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor],
     [self.grid.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
     [self.grid.trailingAnchor constraintEqualToAnchor:self.view.trailingAnchor],
   ]];
+
+  self.toolbar = [[TabGridToolbar alloc] init];
+  [self.view addSubview:self.toolbar];
+  self.toolbar.translatesAutoresizingMaskIntoConstraints = NO;
+  [NSLayoutConstraint activateConstraints:@[
+    [self.toolbar.topAnchor constraintEqualToAnchor:self.view.topAnchor],
+    [self.toolbar.heightAnchor
+        constraintEqualToAnchor:self.topLayoutGuide.heightAnchor
+                       constant:self.toolbar.intrinsicContentSize.height],

[marq (ping after 24h), 2017/03/02 06:04:54]

I find this a bit confusing. Isn't the point of having an intrinsic height that
you don't have to set a height constraint? 

[edchin, 2017/03/02 07:12:44]

If we didn't care about the status bar, we could anchor to the topLayoutGuide
of the superview and be done with it. There would be no height constraint. 
However, we want to expand the toolbar's background into the space 
behind the status bar. This means we need a height constraint that adds
the heights of the status bar and the toolbar's intrinsic height.

+    [self.toolbar.leadingAnchor
+        constraintEqualToAnchor:self.view.leadingAnchor],
+    [self.toolbar.trailingAnchor
+        constraintEqualToAnchor:self.view.trailingAnchor]
+  ]];
 }
 
 - (void)viewWillAppear:(BOOL)animated {
@@ -92,6 +92,12 @@ const CGFloat kToolbarHeight = 64.0f;
   [self.view addSubview:self.floatingNewTabButton];
 }
 
+- (void)viewDidLayoutSubviews {
+  [super viewDidLayoutSubviews];
+  self.grid.contentInset =

[marq (ping after 24h), 2017/03/02 06:04:54]

Does setting this trigger another layout pass? (that is, after this assignment
is made, does self.grid need layout again?)

[edchin, 2017/03/02 07:12:44]

Good question. I can investigate. Can you clarify any concern you may have, 
so I can think about it?

[edchin, 2017/03/16 07:19:59]

After looking into this more, I don't think setting the contentInset causes
another layout pass. 
I've seen similar overrides of this method in MDC source code in which frames
have been modified
and scrollview content sizes have been changed. Also, the description of this
method's purpose
indicates it is meant for this purpose of modifying after being laid out but
before being visible.

+      UIEdgeInsetsMake(CGRectGetMaxY(self.toolbar.frame), 0, 0, 0);
+}
+
 - (UIStatusBarStyle)preferredStatusBarStyle {
   return UIStatusBarStyleLightContent;
 }