[Payment Request] Generic edit form

ios/chrome/browser/payments/payment_request_edit_view_controller.h

+// 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.
+
+#ifndef IOS_CHROME_BROWSER_PAYMENTS_PAYMENT_REQUEST_EDIT_VIEW_CONTROLLER_H_
+#define IOS_CHROME_BROWSER_PAYMENTS_PAYMENT_REQUEST_EDIT_VIEW_CONTROLLER_H_
+
+#import <UIKit/UIKit.h>
+#include <vector>
+
+#import "ios/chrome/browser/payments/payment_request_edit_view_controller_actions.h"
+#import "ios/chrome/browser/ui/collection_view/collection_view_controller.h"
+#import "ios/chrome/browser/ui/settings/cells/autofill_edit_item.h"
+
+// Use these as the starting values for section identifier and item type enums
+// in subclasses. These values are chosen to prevent overlapping with the
+// section identifier and item type enums of this class.
+const NSInteger kSectionIdentifierEnumStart = kSectionIdentifierEnumZero + 20;

[lpromero, 2017/03/22 10:51:33]

Move declaration to internal header (see below) and definition to implementation
file.

Moreover, they are not yet used, so I'd advise to add them only in the follow-up
CL with subclasses.

[Moe, 2017/03/22 17:54:31]

Yep they're not used. sorry, aftermath of breaking a large CL...

As for why they're defined here, for some reason the compiler doesn't like it
when these are defined in the implementation. It complains that the "enumerator
value is not a constant expression". But i'll deal with it in the follow up
subclass CL.

 

+const NSInteger kItemTypeEnumStart = kItemTypeEnumZero + 100;
+
+// Field definition for an editor field. Used for building the UI and
+// validation.
+@interface EditorField : NSObject

[lpromero, 2017/03/22 10:51:33]

Can you move this into its own files?

[Moe, 2017/03/22 17:54:31]

Done.

+
+// Autofill type for the field. Corresponds to autofill::ServerFieldType
+@property(nonatomic, assign) NSInteger autofillType;
+// Label for the field.
+@property(nonatomic, copy) NSString* label;
+// Value of the field. May be nil.
+@property(nonatomic, copy) NSString* value;
+// Whether the field is required.
+@property(nonatomic, getter=isRequired) BOOL required;
+// The associated AutofillEditItem instance. May be nil.
+@property(nonatomic, assign) AutofillEditItem* item;
+// The section identifier for the associated AutofillEditItem.
+@property(nonatomic, assign) NSInteger sectionIdentifier;
+
+- (instancetype)initWithAutofillType:(NSInteger)autofillType
+                               label:(NSString*)label
+                               value:(NSString*)value
+                            required:(BOOL)required;
+
+@end
+
+@class PaymentRequestEditViewController;
+
+// Delegate protocol for PaymentRequestEditViewController.
+@protocol PaymentRequestEditViewControllerDelegate<NSObject>
+
+// Notifies the delegate that the user has finished editing the fields supplied
+// to the initializer. The value property of each field reflects the submitted
+// value.
+- (void)paymentRequestEditViewController:
+            (PaymentRequestEditViewController*)controller
+                  didFinishEditingFields:(NSArray<EditorField*>*)fields;
+
+// Notifies the delegate that the user has chosen to return to the previous
+// screen.
+- (void)paymentRequestEditViewControllerDidReturn:
+    (PaymentRequestEditViewController*)controller;
+
+@end
+
+// Validator protocol for PaymentRequestEditViewController.
+@protocol PaymentRequestEditViewControllerValidator<NSObject>
+
+// Returns the validation error string for |value|. |autofillType| corresponds
+// to autofill::ServerFieldType. |required| indicates whether this is a required
+// field. If there are no validation errors, an empty string is returned.
+- (NSString*)paymentRequestEditViewController:
+                 (PaymentRequestEditViewController*)controller
+                                validateValue:(NSString*)value
+                                 autofillType:(NSInteger)autofillType
+                                     required:(BOOL)required;
+
+@end
+
+// The collection view controller for a generic Payment Request edit screen. It
+// features sections for every EditorField supplied to the initializer. Each
+// section has a text field as well as an error message item which is visible
+// wehn the value of its respective text field is invalid.
+@interface PaymentRequestEditViewController
+    : CollectionViewController<UITextFieldDelegate,

[lpromero, 2017/03/22 10:51:33]

These are implementation details, they can move to the impl file (not even to
the internal header I propose below, I think.)

[Moe, 2017/03/22 17:54:32]

Done.
"In retrospect", the subclasses may need to override methods from
UITextFieldDelegate. So it'll have to move to the Internal category. Also, the
subclasses's validator delegate will being calling the base class's
implementation after doing it's own specific validation so that'll have to
remain public. But, we will cross that bridge when we come to it. Moving these
to the implementation for now :)

+                               PaymentRequestEditViewControllerActions,
+                               PaymentRequestEditViewControllerValidator>
+
+// The delegate to be notified when the user returns or finishes editing the
+// fields.
+@property(nonatomic, weak) id<PaymentRequestEditViewControllerDelegate>
+    editorDelegate;
+
+// The delegate to be called for validating the fields.

[lpromero, 2017/03/22 10:51:33]

Add a comment that by default, this object is the validator. Will the validator
ever change?

[Moe, 2017/03/22 17:54:31]

Done. Yes, the base class's default implementation only checks if the field is
required since it shouldn't know the logic of validating autofill types. For
credit cards or autofill profiles, it'll be the appropriate coordinator who'll
be doing the specific field validation. 

+@property(nonatomic, weak) id<PaymentRequestEditViewControllerValidator>
+    validatorDelegate;
+
+// Validates each field. If there is a validation error, displays an error
+// message item in the same section as the field and returns NO. Otherwise
+// removes the error message item in that section if one exists and sets the
+// value on the field. Returns YES if all the fields are validated successfully.
+- (BOOL)validateForm;
+
+// Called after the editor field items have been added to the the collection
+// view model. Subclasses override this method to add items after the editor
+// fields.
+- (void)loadFooterItems;
+
+// Returns the index path for the cell associated with the currently focused
+// text field.
+- (NSIndexPath*)indexPathForCurrentTextField;
+
+// Adds an error message item in the section |sectionIdentifier| if
+// |errorMessage| is non-empty. Otherwise removes such an item if one exists.
+- (void)addOrRemoveErrorMessage:(NSString*)errorMessage

[lpromero, 2017/03/22 10:51:33]

Many things in this header are for subclasses use only. Can you create 
ios/chrome/browser/payments/payment_request_edit_view_controller+internal.h and
move all these details there?

[Moe, 2017/03/22 17:54:32]

Done. Everything here is for subclasses use. The reason I made them public was
that I remembered from a previous review you suggesting there's no harm in doing
so. I personally like the idea of the Internal category.
https://codereview.chromium.org/2731293002/diff/1/ios/chrome/browser/ui/setti...

+        inSectionWithIdentifier:(NSInteger)sectionIdentifier;
+
+// Initializes this instance with a list of field definitions for the editor.
+- (instancetype)initWithEditorFields:(NSArray<EditorField*>*)fields
+    NS_DESIGNATED_INITIALIZER;
+
+- (instancetype)init NS_UNAVAILABLE;
+
+- (instancetype)initWithStyle:(CollectionViewControllerStyle)style
+    NS_UNAVAILABLE;
+
+@end
+
+#endif  // IOS_CHROME_BROWSER_PAYMENTS_PAYMENT_REQUEST_EDIT_VIEW_CONTROLLER_H_