[tab_container] Consolidate tabStrip into tab_container.

ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm

Index: ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm
diff --git a/ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm b/ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm
index c45b32ac744f960e91e57314ec65a643482d66a8..de798e21a808da397987ba6ad848c35d6a715e53 100644
--- a/ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm
+++ b/ios/clean/chrome/browser/ui/tab/tab_container_view_controller.mm
@@ -8,7 +8,6 @@
 
 #import "ios/clean/chrome/browser/ui/tab/tab_container_view_controller.h"
 
-#import "base/mac/foundation_util.h"
 #import "ios/clean/chrome/browser/ui/ui_types.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -16,113 +15,168 @@
 #endif
 
 namespace {
-CGFloat kToolbarHeight = 44.0;
+CGFloat kToolbarHeight = 44.0f;
+CGFloat kTabStripHeight = 200.0f;
 }
 
 @interface TabContainerViewController ()
 
-// Whichever view controller is at the top of the screen. This view controller
-// controls the status bar.
-@property(nonatomic, weak) UIViewController* topmostViewController;
+// Container views for child view controllers. The child view controller's
+// view is added as a subview that fills it's container view via autoresizing.
+@property(nonatomic, strong) UIView* tabStripView;
+@property(nonatomic, strong) UIView* toolbarView;
+@property(nonatomic, strong) UIView* contentView;
 
-@property(nonatomic, strong) Constraints* contentConstraintsWithToolbar;
-@property(nonatomic, strong) Constraints* contentConstraintsWithoutToolbar;
-@property(nonatomic, strong) Constraints* toolbarConstraints;
+// Constraints for the container views. This may be overrided in

[marq (ping after 24h), 2017/02/16 12:55:56]

overrided -> overridden. Yay, English!

+// -updateViewConstraints in a subclass.
+@property(nonatomic, strong) Constraints* constraints;
+
+// Height constraints for tabStripView and toolbarView.
+@property(nonatomic, strong) NSLayoutConstraint* tabStripHeightConstraint;
+@property(nonatomic, strong) NSLayoutConstraint* toolbarHeightConstraint;
 
 // Cache for forwarding methods to child view controllers.
 @property(nonatomic, assign) SEL actionToForward;
 @property(nonatomic, weak) UIResponder* forwardingTarget;
 
-// Contained view controller utility methods.
-- (void)removeChildViewController:(UIViewController*)viewController;
-
-// Called after a new content view controller is set, but before
-// |-didMoveToParentViewController:| is called on that view controller.
-- (void)didAddContentViewController;
-
-// Called after a new toolbar view controller is set, but before
-// |-didMoveToParentViewController:| is called on that view controller.
-- (void)didAddToolbarViewController;
-
-// Methods to populate the constraint properties.
-- (void)updateContentConstraintsWithToolbar;
-- (void)updateContentConstraintsWithoutToolbar;
-- (void)updateToolbarConstraints;
-
 @end
 
 @implementation TabContainerViewController
 
 @synthesize contentViewController = _contentViewController;
 @synthesize toolbarViewController = _toolbarViewController;
-@synthesize topmostViewController = _topmostViewController;
-@synthesize contentConstraintsWithToolbar = _contentConstraintsWithToolbar;
-@synthesize contentConstraintsWithoutToolbar =
-    _contentConstraintsWithoutToolbar;
-@synthesize toolbarConstraints = _toolbarConstraints;
+@synthesize tabStripViewController = _tabStripViewController;
+@synthesize tabStripView = _tabStripView;
+@synthesize toolbarView = _toolbarView;
+@synthesize contentView = _contentView;
+@synthesize constraints = _constraints;
+@synthesize tabStripHeightConstraint = _tabStripHeightConstraint;
+@synthesize toolbarHeightConstraint = _toolbarHeightConstraint;
 @synthesize actionToForward = _actionToForward;
 @synthesize forwardingTarget = _forwardingTarget;
 
+#pragma mark - UIViewController
+
+- (void)viewDidLoad {
+  [super viewDidLoad];
+  self.tabStripView = [[UIView alloc] init];
+  self.toolbarView = [[UIView alloc] init];
+  self.contentView = [[UIView alloc] init];
+  [self.view addSubview:self.tabStripView];
+  [self.view addSubview:self.toolbarView];
+  [self.view addSubview:self.contentView];
+  self.tabStripView.translatesAutoresizingMaskIntoConstraints = NO;
+  self.toolbarView.translatesAutoresizingMaskIntoConstraints = NO;
+  self.contentView.translatesAutoresizingMaskIntoConstraints = NO;
+  self.view.backgroundColor = [UIColor blackColor];
+  self.tabStripView.backgroundColor = [UIColor blackColor];
+  self.toolbarView.backgroundColor = [UIColor blackColor];
+  self.contentView.backgroundColor = [UIColor blackColor];
+
+  [self addChildViewController:self.tabStripViewController
+                     toSubview:self.tabStripView];
+  [self addChildViewController:self.toolbarViewController
+                     toSubview:self.toolbarView];
+  [self addChildViewController:self.contentViewController
+                     toSubview:self.contentView];
+
+  self.tabStripHeightConstraint =
+      [self.tabStripView.heightAnchor constraintEqualToConstant:0.0f];
+  self.toolbarHeightConstraint =
+      [self.toolbarView.heightAnchor constraintEqualToConstant:0.0f];
+  if (self.toolbarViewController) {
+    self.toolbarHeightConstraint.constant = kToolbarHeight;
+  }
+}
+
+- (void)updateViewConstraints {
+  if (!self.constraints) {
+    self.constraints = @[
+      [self.tabStripView.topAnchor
+          constraintEqualToAnchor:self.topLayoutGuide.bottomAnchor],
+      [self.tabStripView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.tabStripView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      self.tabStripHeightConstraint,
+      [self.toolbarView.topAnchor
+          constraintEqualToAnchor:self.tabStripView.bottomAnchor],
+      [self.toolbarView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.toolbarView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      self.toolbarHeightConstraint,
+      [self.contentView.topAnchor
+          constraintEqualToAnchor:self.toolbarView.bottomAnchor],
+      [self.contentView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.contentView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      [self.contentView.bottomAnchor
+          constraintEqualToAnchor:self.bottomLayoutGuide.topAnchor],
+    ];
+    [NSLayoutConstraint activateConstraints:self.constraints];
+  }
+  [super updateViewConstraints];
+}
+
 #pragma mark - Public properties
 
 - (void)setContentViewController:(UIViewController*)contentViewController {
   if (self.contentViewController == contentViewController)
     return;
-
-  // Remove the current content view controller, if any.
-  [NSLayoutConstraint
-      deactivateConstraints:self.contentConstraintsWithoutToolbar];
-  [NSLayoutConstraint deactivateConstraints:self.contentConstraintsWithToolbar];
-  [self removeChildViewController:self.contentViewController];
-
-  // Add the new content view controller.
-  [self addChildViewController:contentViewController];
-  contentViewController.view.translatesAutoresizingMaskIntoConstraints = NO;
-  [self.view addSubview:contentViewController.view];
+  if ([self isViewLoaded]) {
+    [self removeChildViewController:self.contentViewController];
+    [self addChildViewController:contentViewController
+                       toSubview:self.contentView];
+  }
   _contentViewController = contentViewController;
-  [self didAddContentViewController];
-  [self.view setNeedsUpdateConstraints];
-  [self.contentViewController didMoveToParentViewController:self];
 }
 
 - (void)setToolbarViewController:(UIViewController*)toolbarViewController {
   if (self.toolbarViewController == toolbarViewController)
     return;
-
-  // Remove the current toolbar view controller, if any.
-  [NSLayoutConstraint deactivateConstraints:self.toolbarConstraints];
-  [NSLayoutConstraint deactivateConstraints:self.contentConstraintsWithToolbar];
-  [self removeChildViewController:self.toolbarViewController];
-
-  // Add the new toolbar view controller.
-  [self addChildViewController:toolbarViewController];
-  toolbarViewController.view.translatesAutoresizingMaskIntoConstraints = NO;
-  [self.view addSubview:toolbarViewController.view];
+  if ([self isViewLoaded]) {
+    [self removeChildViewController:self.toolbarViewController];
+    [self addChildViewController:toolbarViewController
+                       toSubview:self.toolbarView];
+  }
   _toolbarViewController = toolbarViewController;
-  [self didAddToolbarViewController];
-  [self.view setNeedsUpdateConstraints];
-  [self.toolbarViewController didMoveToParentViewController:self];
 }
 
-#pragma mark - UIViewController
-
-- (void)updateViewConstraints {
-  if (self.toolbarViewController) {
-    [NSLayoutConstraint activateConstraints:self.toolbarConstraints];
-    [NSLayoutConstraint activateConstraints:self.contentConstraintsWithToolbar];
-  } else {
-    [NSLayoutConstraint
-        activateConstraints:self.contentConstraintsWithoutToolbar];
+- (void)setTabStripViewController:(UIViewController*)tabStripViewController {
+  if (self.tabStripViewController == tabStripViewController)
+    return;
+  if ([self isViewLoaded]) {
+    [self removeChildViewController:self.tabStripViewController];
+    [self addChildViewController:tabStripViewController
+                       toSubview:self.tabStripView];
   }
-  [super updateViewConstraints];
+  _tabStripViewController = tabStripViewController;
 }
 
-- (UIViewController*)childViewControllerForStatusBarHidden {
-  return self.topmostViewController;
+#pragma mark - ChildViewController helper methods
+
+- (void)addChildViewController:(UIViewController*)viewController
+                     toSubview:(UIView*)subview {
+  if (!viewController || !subview) {
+    return;
+  }
+  [self addChildViewController:viewController];
+  viewController.view.translatesAutoresizingMaskIntoConstraints = YES;
+  viewController.view.autoresizingMask =
+      UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
+  viewController.view.frame = subview.bounds;
+  [subview addSubview:viewController.view];
+  [viewController didMoveToParentViewController:self];
 }
 
-- (UIViewController*)childViewControllerForStatusBarStyle {
-  return self.topmostViewController;
+- (void)removeChildViewController:(UIViewController*)viewController {
+  if (viewController.parentViewController != self)
+    return;
+  [viewController willMoveToParentViewController:nil];
+  [viewController.view removeFromSuperview];
+  [viewController removeFromParentViewController];
 }
 
 #pragma mark - MenuPresentationDelegate
@@ -199,132 +253,47 @@ CGFloat kToolbarHeight = 44.0;
   return nil;
 }
 
-#pragma mark - Private methods
+#pragma mark - TabStripActions
 
-- (void)removeChildViewController:(UIViewController*)viewController {
-  if (viewController.parentViewController != self)
-    return;
-  [viewController willMoveToParentViewController:nil];
-  [viewController.view removeFromSuperview];
-  [viewController removeFromParentViewController];
-}
-
-- (void)didAddContentViewController {
-  if (self.toolbarViewController) {
-    [self updateContentConstraintsWithToolbar];
-  } else {
-    self.topmostViewController = self.contentViewController;
-    [self updateContentConstraintsWithoutToolbar];
-  }
-}
-
-- (void)didAddToolbarViewController {
-  [self updateToolbarConstraints];
-  // If there's already a content view controller, update the constraints for
-  // that, too.
-  if (self.contentViewController) {
-    [self updateContentConstraintsWithToolbar];
-  }
-}
-
-- (void)updateContentConstraintsWithToolbar {
-  // Template method for subclasses to implement;
-}
-
-- (void)updateToolbarConstraints {
-  // Template method for subclasses to implement;
-}
-
-- (void)updateContentConstraintsWithoutToolbar {
-  UIView* contentView = self.contentViewController.view;
-  self.contentConstraintsWithoutToolbar = @[
-    [contentView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
-    [contentView.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor],
-    [contentView.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
-    [contentView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ];
-}
-
-@end
-
-@implementation TopToolbarTabViewController

[marq (ping after 24h), 2017/02/16 12:55:56]

I would like to keep TopToolbar and BottomToolbar as concrete subclasses of the
base class, rather than having Top be the default and Bottom a specialization.

[edchin, 2017/02/16 16:12:05]

I've implemented this in the updated patch.

-
-- (void)didAddContentViewController {
-  [super didAddContentViewController];
-  if (!self.toolbarViewController) {
-    self.topmostViewController = self.contentViewController;
-  }
-}
-
-- (void)didAddToolbarViewController {
-  [super didAddToolbarViewController];
-  self.topmostViewController = self.toolbarViewController;
-}
-
-- (void)updateContentConstraintsWithToolbar {
-  UIView* contentView = self.contentViewController.view;
-  self.contentConstraintsWithToolbar = @[
-    [contentView.topAnchor
-        constraintEqualToAnchor:self.toolbarViewController.view.bottomAnchor],
-    [contentView.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor],
-    [contentView.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
-    [contentView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ];
-}
-
-- (void)updateToolbarConstraints {
-  // HACK: This background is added so the status bar portion of the view is not
-  // transparent. This needs to be implemented properly for the top toolbar
-  // case.
-  self.view.backgroundColor = [UIColor lightGrayColor];
-  UIView* toolbarView = self.toolbarViewController.view;
-  self.toolbarConstraints = @[
-    [toolbarView.topAnchor constraintEqualToAnchor:self.view.topAnchor
-                                          constant:20.0],
-    [toolbarView.heightAnchor constraintEqualToConstant:kToolbarHeight],
-    [toolbarView.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
-    [toolbarView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ];
+- (void)toggleTabStrip:(id)sender {
+  self.tabStripHeightConstraint.constant =
+      self.tabStripHeightConstraint.constant > 0.0f ? 0.0f : kTabStripHeight;
 }
 
 @end
 
 @implementation BottomToolbarTabViewController
 
-- (void)didAddContentViewController {
-  [super didAddContentViewController];
-  self.topmostViewController = self.contentViewController;
-}
-
-// Note that this class doesn't override -didAddToolbarViewController; in the
-// case where there is a toolbar view controller set but not a content view
-// controller, functionally there is no topmost view controller, so no
-// additional action needs to be taken.
-
-- (void)updateContentConstraintsWithToolbar {
-  UIView* contentView = self.contentViewController.view;
-  self.contentConstraintsWithToolbar = @[
-    [contentView.topAnchor constraintEqualToAnchor:self.view.topAnchor],
-    [contentView.bottomAnchor
-        constraintEqualToAnchor:self.toolbarViewController.view.topAnchor],
-    [contentView.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
-    [contentView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ];
-}
-
-- (void)updateToolbarConstraints {
-  UIView* toolbarView = self.toolbarViewController.view;
-  self.toolbarConstraints = @[
-    [toolbarView.bottomAnchor constraintEqualToAnchor:self.view.bottomAnchor],
-    [toolbarView.heightAnchor constraintEqualToConstant:kToolbarHeight],
-    [toolbarView.leadingAnchor constraintEqualToAnchor:self.view.leadingAnchor],
-    [toolbarView.trailingAnchor
-        constraintEqualToAnchor:self.view.trailingAnchor],
-  ];
+// Override with different constraints.
+- (void)updateViewConstraints {
+  if (!self.constraints) {

[marq (ping after 24h), 2017/02/16 12:55:56]

Instead of overriding -updateViewConstraints, I'd prefer to have a template
helper method generate the constraints and have each subclass implement it. That
way common logic that belongs in -updateViewConstraints can stay shared.

[edchin, 2017/02/16 16:12:05]

The benefit of DRY is lost to lack of full comprehension. The SWE who implements
a subclass must identify what constraints are missing in the common constraints,
which is hard to reason about. It also curtails the SWE's autonomy in
subclassing. See updated patch for how it might look. For setting constraints, I
believe it is best practice to allow subclasses to reimplement the whole set of
constraints rather than a subset. 

[marq (ping after 24h), 2017/02/17 12:16:58]

Right now you have this (pseudocode):

Base Class:
- (void)updateConstraints
   if (no constraints)
     build constraints
     activate constraints
   endif
   [super updateConstraints]

With the same thing in the subclass. Because -updateViewWithConstraints must
call |super|, this means that the superclass and subclass have to carefully
coordinate to make sure they don't do the same thing twice.

What I'm suggesting is:

Base Class:
- (void)updateConstraints
   if (no constraints)
     [self buildConstraints]
     activate constraints
   endif
   [super updateConstraints]

- (void)buildConstraints
  // empty template method.

TopBar subclass
- (void)BuildConstraints
   build all constraints needed

BottomBar subclass
- (void)BuildConstraints
   build all constraints needed

So: Don't factor out the common constraints, because that makes the code hard to
maintain. But also don't have the child classes override -updateViewConstraints
while still doing specific work in the superclass implementation, because that's
also hard to maintain.

[edchin, 2017/02/17 19:48:09]

Updated: 
1) No more factoring out common constraints.
2) Subclasses must override a template base method called
-(Constraints*)subviewConstraints, which must return the full set of constraints
for all container subviews (i.e., tabstrip, toolbar, contents)
3) Moved the constraints activation from -updateViewConstraints to -viewDidLoad.
Since the constraints don't change at runtime, this is a simpler implementation
and less prone to error in the future.

+    self.constraints = @[
+      [self.tabStripView.topAnchor
+          constraintEqualToAnchor:self.topLayoutGuide.bottomAnchor],
+      [self.tabStripView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.tabStripView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      self.tabStripHeightConstraint,
+      [self.contentView.topAnchor
+          constraintEqualToAnchor:self.tabStripView.bottomAnchor],
+      [self.contentView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.contentView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      [self.toolbarView.topAnchor
+          constraintEqualToAnchor:self.contentView.bottomAnchor],
+      [self.toolbarView.leadingAnchor
+          constraintEqualToAnchor:self.view.leadingAnchor],
+      [self.toolbarView.trailingAnchor
+          constraintEqualToAnchor:self.view.trailingAnchor],
+      self.toolbarHeightConstraint,
+      [self.toolbarView.bottomAnchor
+          constraintEqualToAnchor:self.bottomLayoutGuide.topAnchor],
+    ];
+    [NSLayoutConstraint activateConstraints:self.constraints];
+  }
+  [super updateViewConstraints];
 }
 
 @end