Adding mediator for Sign-in promo

ios/chrome/browser/ui/authentication/signin_promo_view.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/ui/authentication/signin_promo_view.h"
+
+#include "base/logging.h"
+#import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h"
+#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#include "ios/chrome/grit/ios_strings.h"
+#import "ios/third_party/material_components_ios/src/components/Buttons/src/MaterialButtons.h"
+#import "ios/third_party/material_components_ios/src/components/Typography/src/MaterialTypography.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace {
+// Horizontal padding for label and buttons.
+const CGFloat kHorizontalPadding = 40;
+// Vertical padding for the image and the label.
+const CGFloat kVerticalPadding = 12;
+// Vertical padding for buttons.
+const CGFloat kButtonVerticalPadding = 6;
+// Image size for warm start.
+const CGFloat kProfileImageFixedSize = 48;
+// Image size for cold start.
+const CGFloat kChromeImageFixedSize = 24;

[msarda, 2017/03/22 12:18:39]

I expect the profile image and the chrome image should have the same size. I
think there is a problem in the mocks.

If this is the case, then we can use a single size for images and have it be the
same constraint in both modes.

[lpromero, 2017/03/24 10:36:20]

That would be simpler for us. Try to push back to UX for either a larger chrome
logo, or an asset with padding? Otherwise, we can handle it in code by creating
an image with padding. Or with constraints, but it's less readable and
encapsulated.

[jlebel, 2017/03/24 20:59:08]

I don't understand my code already handles the 2 sizes. I'm not sure why should
we push back for juste a size problem.

+// Button height.
+const CGFloat kButtonHeight = 36;
+}
+
+@implementation SigninPromoView {
+  SigninPromoViewMode _mode;

[lpromero, 2017/03/24 10:36:20]

These 5 ivars are not needed, as they are generated by the @synthesize below.

[jlebel, 2017/03/24 20:59:09]

Done.

+  UIImageView* _imageView;
+  UILabel* _textLabel;
+  MDCFlatButton* _primaryButton;
+  MDCFlatButton* _secondaryButton;
+
+  NSArray<NSLayoutConstraint*>* _coldStartConstraints;

[msarda, 2017/03/22 12:18:39]

s/_coldStartConstraints/_secondaryButtonVisibleConstraints

[lpromero, 2017/03/24 10:36:20]

Actually, I like the abstraction the names give.

[jlebel, 2017/03/24 20:59:09]

Acknowledged.

+  NSArray<NSLayoutConstraint*>* _warmStartConstraints;

[msarda, 2017/03/22 12:18:39]

s/_coldStartConstraints/_secondaryButtonHiddenConstraints

[jlebel, 2017/03/24 20:59:09]

Acknowledged.

+}
+
+@synthesize mode = _mode;
+@synthesize imageView = _imageView;
+@synthesize textLabel = _textLabel;
+@synthesize primaryButton = _primaryButton;
+@synthesize secondaryButton = _secondaryButton;
+
+- (instancetype)initWithCoder:(NSCoder*)aDecoder {

[lpromero, 2017/03/24 10:36:20]

No need for this (see similar comment above or below, not sure).

[jlebel, 2017/03/24 20:59:09]

Done.

+  NOTREACHED();
+  return nil;
+}
+
+- (instancetype)initWithFrame:(CGRect)frame {
+  self = [super initWithFrame:frame];
+  if (self) {
+    self.translatesAutoresizingMaskIntoConstraints = NO;
+    self.isAccessibilityElement = YES;
+    [self addSubviews];
+    [self setDefaultViewStyling];
+    [self setViewConstraints];

[msarda, 2017/03/22 12:18:39]

Consider calling [self setMode:SigninPromoViewColdStartMode] (one line instead
of 2).

[lpromero, 2017/03/24 10:36:20]

Ha! Mihai, you advised against calling methods in init in another file :)
Personally, I just put all the code within init, no -addSubviews,
-setDefaultViewStyling, etc. But looks good to me. Either keep the previous
calls and add -setMode:, or move everything inline in init.

[jlebel, 2017/03/24 20:59:09]

I can't call -[SigninPromoView setMode:] in the init. It would return without
doing anything.

+    _mode = SigninPromoViewColdStartMode;
+    [self activateColdMode];
+  }
+  return self;
+}
+
+- (void)addSubviews {
+  self.clipsToBounds = YES;
+
+  _imageView = [[UIImageView alloc] init];
+  _imageView.translatesAutoresizingMaskIntoConstraints = NO;
+  [self addSubview:_imageView];
+
+  _textLabel = [[UILabel alloc] init];
+  _textLabel.translatesAutoresizingMaskIntoConstraints = NO;
+  [self addSubview:_textLabel];
+
+  _primaryButton = [[MDCFlatButton alloc] init];
+  _primaryButton.translatesAutoresizingMaskIntoConstraints = NO;
+  _primaryButton.accessibilityIdentifier = @"signin_promo_primary_button";
+  [self addSubview:_primaryButton];
+
+  _secondaryButton = [[MDCFlatButton alloc] init];
+  _secondaryButton.translatesAutoresizingMaskIntoConstraints = NO;
+  _secondaryButton.accessibilityIdentifier = @"signin_promo_secondary_button";
+  [self addSubview:_secondaryButton];
+}
+
+- (void)setDefaultViewStyling {
+  _imageView.contentMode = UIViewContentModeCenter;
+  _imageView.layer.masksToBounds = YES;
+  _imageView.contentMode = UIViewContentModeScaleAspectFit;
+
+  _textLabel.font = [MDCTypography buttonFont];
+  _textLabel.textColor = [[MDCPalette greyPalette] tint900];
+  _textLabel.numberOfLines = 0;
+  _textLabel.textAlignment = NSTextAlignmentCenter;
+
+  [_primaryButton setBackgroundColor:[[MDCPalette cr_bluePalette] tint500]
+                            forState:UIControlStateNormal];
+  _primaryButton.customTitleColor = [UIColor whiteColor];
+  _primaryButton.inkColor = [UIColor colorWithWhite:1 alpha:0.2];
+
+  _secondaryButton.customTitleColor = [[MDCPalette cr_bluePalette] tint500];
+  _secondaryButton.uppercaseTitle = NO;
+}
+
+- (void)setViewConstraints {
+  NSDictionary* metrics = @{
+    @"kButtonHeight" : @(kButtonHeight),
+    @"kButtonVerticalPadding" : @(kButtonVerticalPadding),
+    @"kButtonVerticalPaddingx2" : @(kButtonVerticalPadding * 2),
+    @"kChromeImageFixedSize" : @(kChromeImageFixedSize),
+    @"kHorizontalPadding" : @(kHorizontalPadding),
+    @"kProfileImageFixedSize" : @(kProfileImageFixedSize),
+    @"kVerticalPadding" : @(kVerticalPadding),
+    @"kVerticalPaddingx2" : @(kVerticalPadding * 2),
+    @"kVerticalPaddingkButtonVerticalPadding" :
+        @(kVerticalPadding + kButtonVerticalPadding),
+  };
+  NSDictionary* views = @{
+    @"imageView" : _imageView,
+    @"primaryButton" : _primaryButton,
+    @"secondaryButton" : _secondaryButton,
+    @"textLabel" : _textLabel,
+  };
+
+  // Constraints in shared between modes.

[msarda, 2017/03/22 12:18:39]

Constraints shared between modes.

[jlebel, 2017/03/24 20:59:09]

Done.

+  NSString* formatString =
+      @"V:|-kVerticalPaddingx2-[imageView]-kVerticalPadding-[textLabel]-"
+      @"kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]";

[msarda, 2017/03/22 12:18:39]

Personal pref: Consider removing the @ on this second line.

[lpromero, 2017/03/24 10:36:20]

Agreed, I thought it was a second string.

[jlebel, 2017/03/24 20:59:09]

Done.

+  [NSLayoutConstraint
+      activateConstraints:
+          [NSLayoutConstraint
+              constraintsWithVisualFormat:formatString
+                                  options:NSLayoutFormatAlignAllCenterX
+                                  metrics:metrics
+                                    views:views]];
+
+  formatString = @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|";

[msarda, 2017/03/22 12:18:39]

You can put all these visual constraints in a single dictionary and apply them
once. This makes the code more compact and it easier to read - for example take
a look at
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/authentication/sig...

[jlebel, 2017/03/24 20:59:08]

Done.

+  [NSLayoutConstraint

[msarda, 2017/03/22 12:18:39]

I think this code would be so much simpler to read if we did the following:
* Add an anonymous namespace function that takes an array of NSStrings (the
layout format), the metrics and the views. We could name this
LayoutConstraintsFromVisualFormat(
* returns an NSArray of NSLayoutConstraint

Then here we coud be:
_coldStartConstraints = LayoutConstraintsFromVisualFormat(
   @{
     @"V:|-kVerticalPaddingx2-[imageView]-kVerticalPadding-[textLabel]-"
         
"kVerticalPaddingkButtonVerticalPadding-[primaryButton(kButtonHeight)]",
      @"H:|-kHorizontalPadding-[primaryButton]-kHorizontalPadding-|",
      @"V:[primaryButton]-kVerticalPaddingkButtonVerticalPadding-|",
      @"H:[imageView(kChromeImageFixedSize)]",
      @"V:[imageView(kChromeImageFixedSize)]"
   },
   metrics, views);

[jlebel, 2017/03/24 20:59:09]

I will do that in another patch.

[msarda, 2017/03/27 09:34:16]

I have a strong preference for this as I think it makes the code more readable.
If you want to do this in a future CL, please add a TODO for it.

+      activateConstraints:[NSLayoutConstraint
+                              constraintsWithVisualFormat:formatString
+                                                  options:0
+                                                  metrics:metrics
+                                                    views:views]];
+
+  // Constraints for cold start mode.
+  NSMutableArray* constraints = [NSMutableArray array];
+  formatString = @"V:[primaryButton]-kVerticalPaddingkButtonVerticalPadding-|";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  formatString = @"H:[imageView(kChromeImageFixedSize)]";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  formatString = @"V:[imageView(kChromeImageFixedSize)]";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  _coldStartConstraints = [constraints copy];
+
+  // Constraints for warm start mode.
+  constraints = [NSMutableArray array];
+  formatString = @"V:[primaryButton]-kButtonVerticalPaddingx2-[secondaryButton("
+                 @"kButtonHeight)]-kVerticalPaddingkButtonVerticalPadding-|";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  formatString =
+      @"H:|-kHorizontalPadding-[secondaryButton]-kHorizontalPadding-|";
+  [constraints addObjectsFromArray:
+                   [NSLayoutConstraint
+                       constraintsWithVisualFormat:formatString
+                                           options:NSLayoutFormatAlignAllCenterX

[lpromero, 2017/03/24 10:36:20]

Isn't tis option already done with line 138?

[jlebel, 2017/03/24 20:59:09]

I didn't know it applied to the secondary button even it was not used in the
visual constraint from line 138.

[lpromero, 2017/03/27 13:14:46]

Oh good catch. Line 138 might not set this then.

+                                           metrics:metrics
+                                             views:views]];
+  formatString = @"H:[imageView(kProfileImageFixedSize)]";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  formatString = @"V:[imageView(kProfileImageFixedSize)]";
+  [constraints addObjectsFromArray:[NSLayoutConstraint
+                                       constraintsWithVisualFormat:formatString
+                                                           options:0
+                                                           metrics:metrics
+                                                             views:views]];
+  _warmStartConstraints = [constraints copy];
+  _mode = SigninPromoViewWarmStartMode;

[msarda, 2017/03/22 12:18:39]

I think this line needs to be removed.

[jlebel, 2017/03/24 20:59:09]

Done.

+}
+
+- (void)setMode:(SigninPromoViewMode)mode {
+  if (mode == _mode) {
+    return;
+  }
+  _mode = mode;
+  switch (_mode) {
+    case SigninPromoViewColdStartMode:
+      [self activateColdMode];
+      return;
+    case SigninPromoViewWarmStartMode:
+      [self activateWarmMode];
+      return;
+  }
+  NOTREACHED();
+}
+
+- (void)activateColdMode {

[msarda, 2017/03/22 12:18:39]

DCHECK(_mode, COLD);

[jlebel, 2017/03/24 20:59:09]

Done.

+  // TODO(jlebel) Needs to set the chrome/chromium icon in |imageView|.

[msarda, 2017/03/22 12:18:39]

Please remove this TODO as it is the job of the configuration to set the right
image all the time.

[jlebel, 2017/03/24 20:59:09]

I'm going to follow Louis suggestion and keep it that way.

+  [NSLayoutConstraint deactivateConstraints:_warmStartConstraints];
+  [NSLayoutConstraint activateConstraints:_coldStartConstraints];
+  _secondaryButton.hidden = YES;
+}
+
+- (void)activateWarmMode {

[msarda, 2017/03/22 12:18:39]

DCHECK(_mode, WARM);

[jlebel, 2017/03/24 20:59:09]

Done.

+  [NSLayoutConstraint deactivateConstraints:_coldStartConstraints];
+  [NSLayoutConstraint activateConstraints:_warmStartConstraints];
+  _secondaryButton.hidden = NO;
+}
+
+- (void)setProfileImage:(UIImage*)image {

[lpromero, 2017/03/24 10:36:20]

Can you add a comment in the header that this is a no-op if the mode is COLD?
Because the following will just drop the image:
view.profileImage = <...>
view.mode = WARM

It needs to be:
view.mode = WARM
view.profileImage = <...>

[jlebel, 2017/03/24 20:59:09]

Should I rephrase:
"Should be called only with the "Warm State" mode. The image is not changed when
"Cold State" mode."?

+  if (SigninPromoViewColdStartMode == _mode) {
+    return;
+  }
+  _imageView.image = CircularImageFromImage(image, kProfileImageFixedSize);
+}
+
+- (void)accessibilityPrimaryAction:(id)unused {
+  [_primaryButton sendActionsForControlEvents:UIControlEventTouchUpInside];
+}
+
+- (void)accessibilitySecondaryAction:(id)unused {
+  [_secondaryButton sendActionsForControlEvents:UIControlEventTouchUpInside];
+}
+
+- (CGFloat)horizontalPadding {

[lpromero, 2017/03/24 10:36:20]

Doesn't seem used.

[jlebel, 2017/03/24 20:59:09]

It is used. It is explained in the header why this property exists:
"Horizontal padding used for |textLabel|, |primaryButton| and |secondaryButton|.
Used to compute the preferred max layout of |textLabel|."

[lpromero, 2017/03/27 13:14:46]

Sounds good.

+  return kHorizontalPadding;
+}
+
+#pragma mark - NSObject(Accessibility)
+
+- (NSArray<UIAccessibilityCustomAction*>*)accessibilityCustomActions {
+  NSString* primaryActionName =
+      [_primaryButton titleForState:UIControlStateNormal];
+  UIAccessibilityCustomAction* primaryCustomAction =
+      [[UIAccessibilityCustomAction alloc]
+          initWithName:primaryActionName
+                target:self
+              selector:@selector(accessibilityPrimaryAction:)];
+  if (_mode == SigninPromoViewColdStartMode) {
+    return @[ primaryCustomAction ];
+  }
+  NSString* secondaryActionName =
+      [_secondaryButton titleForState:UIControlStateNormal];
+  UIAccessibilityCustomAction* secondaryCustomAction =
+      [[UIAccessibilityCustomAction alloc]
+          initWithName:secondaryActionName
+                target:self
+              selector:@selector(accessibilitySecondaryAction:)];
+  return @[ primaryCustomAction, secondaryCustomAction ];
+}
+
+- (NSString*)accessibilityLabel {
+  return _textLabel.text;
+}
+
+@end