[Payment Request] Picker view + showcase integration + egtests

[Payment Request] Picker view + showcase integration + egtests

http://imgur.com/a/S1tcB

BUG=602666
Review-Url: https://codereview.chromium.org/2778343002
Cr-Commit-Position: refs/heads/master@{#461989}
Committed: https://chromium.googlesource.com/chromium/src/+/14a3ede2a87ed2e81396338aaa61e747e4103100

[Moe, 2017-03-28 21:02:06]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[Moe, 2017-03-28 21:02:18]

Description was changed from

==========
[Payment Request] Picker view + showcase integration + egtests

BUG=602666
==========

to

==========
[Payment Request] Picker view + showcase integration + egtests

http://imgur.com/a/S1tcB

BUG=602666
==========

[commit-bot: I haz the power, 2017-03-28 21:03:30]

Dry run: 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-03-28 21:15:36]

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

[commit-bot: I haz the power, 2017-03-28 21:15:36]

Dry run: CQ has no permission to schedule in bucket
master.tryserver.chromium.linux

[Moe, 2017-03-29 05:39:01]

Patchset #1 (id:1) has been deleted

[Moe, 2017-03-29 05:39:30]

mahmadi@chromium.org changed reviewers:
+ lpromero@chromium.org

[Moe, 2017-03-29 05:39:31]

Hi Louis, Please take a look at this CL.

[lpromero, 2017-03-29 11:38:16]

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_row.h (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_row.h:15: // The value for
the row. This is not displayed in the UI and is optional.
If it's not displayed in the UI, what is its purpose?

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_view_controller.mm
(right):

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:25: 
Remove empty line.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:29:
UITableViewDelegate,
This is already done by the base class.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:73:
self.tableView.rowHeight = MDCCellDefaultOneLineHeight;
I'd suggest just setting your own value here, not depending on MDC. The imported
header can be removed then.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:126:
cell.accessibilityIdentifier =
If you remove the need for the 2 different IDs, you can set just one, around
line 117, since it won't change.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:147:
NSIndexPath* newSelectedIndexPath = [self indexPathForRow:self.selectedRow];
Isn't this equal to indexPath?

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:186:
return [sectionTitles array];
Should you cache this array? (You'll need to invalidate when displayedRows
changes.)
I guess if the list scrolls smoothly already on old devices, we can avoid
optimizing.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:198:
NSPredicate* prediacate = [NSPredicate
*predicate

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:200:
return [self.displayedRows filteredArrayUsingPredicate:prediacate];
idem, should this be cached? It is called several times, and at least once per
cell being shown on screen.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:221:
[self.tableView reloadData];
Showcase handles that more nicely, as it animates the cells in and out:

- (void)filterContentForSearchText:(NSString*)searchText {
  NSArray<showcase::ModelRow*>* newFilteredRows = self.allRows;

  if (![self.searchController.searchBar.text isEqualToString:@""]) {
    // The search is case-insensitive and searches both displayed texts.
    NSIndexSet* matchingRows =
        [self.allRows indexesOfObjectsPassingTest:^BOOL(
                          showcase::ModelRow* row, NSUInteger idx, BOOL* stop) {
          return [row[showcase::kClassForDisplayKey]
                     localizedCaseInsensitiveContainsString:searchText] ||
                 [row[showcase::kUseCaseKey]
                     localizedCaseInsensitiveContainsString:searchText];
        }];
    newFilteredRows = [self.allRows objectsAtIndexes:matchingRows];
  }

  NSArray<NSIndexPath*>* indexPathsToInsert =
      [self indexPathsToInsertFromArray:self.displayedRows
                                toArray:newFilteredRows
                       indexPathSection:0];
  NSArray<NSIndexPath*>* indexPathsToDelete =
      [self indexPathsToDeleteFromArray:self.displayedRows
                                toArray:newFilteredRows
                       indexPathSection:0];

  [self.tableView beginUpdates];
  [self.tableView insertRowsAtIndexPaths:indexPathsToInsert
                        withRowAnimation:UITableViewRowAnimationFade];
  [self.tableView deleteRowsAtIndexPaths:indexPathsToDelete
                        withRowAnimation:UITableViewRowAnimationFade];
  self.displayedRows = newFilteredRows;
  [self.tableView endUpdates];
}

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_picker_coordinator.mm (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_coordinator.mm:42:
didSelectRow:(PickerRow*)row {
You can use the ProtocolAlerter to trigger an alert. That way you can add a test
that checks this is called.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_picker_egtest.mm (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:20: id<GREYMatcher>
matcherForSectionWithTitle(NSString* title) {
I think the convention is to UpperCamelCase these.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:20: id<GREYMatcher>
matcherForSectionWithTitle(NSString* title) {
I'd even omit the "matcher" in all these. It makes the call site shorter and not
more difficult to read.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:29: selected ?
kPaymentRequestPickerSelectedRowAccessibilityID
Instead of having a different ID for both selected and unselected, you could
simplify by checking the UIAccessibilityTraitSelected. That way I think you
could remove the logic of two accessibility identifiers.

[Moe, 2017-03-29 17:04:10]

Thanks Louis! PTAL.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_row.h (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_row.h:15: // The value for
the row. This is not displayed in the UI and is optional.
On 2017/03/29 11:38:15, lpromero wrote:
> If it's not displayed in the UI, what is its purpose?

Similar to HTML option tag, label is for presentation and value is what actually
being selected.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_view_controller.mm
(right):

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:25: 
On 2017/03/29 11:38:15, lpromero wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:29:
UITableViewDelegate,
On 2017/03/29 11:38:15, lpromero wrote:
> This is already done by the base class.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:73:
self.tableView.rowHeight = MDCCellDefaultOneLineHeight;
On 2017/03/29 11:38:15, lpromero wrote:
> I'd suggest just setting your own value here, not depending on MDC. The
imported
> header can be removed then.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:126:
cell.accessibilityIdentifier =
On 2017/03/29 11:38:15, lpromero wrote:
> If you remove the need for the 2 different IDs, you can set just one, around
> line 117, since it won't change.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:147:
NSIndexPath* newSelectedIndexPath = [self indexPathForRow:self.selectedRow];
On 2017/03/29 11:38:15, lpromero wrote:
> Isn't this equal to indexPath?

Oops. Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:186:
return [sectionTitles array];
On 2017/03/29 11:38:15, lpromero wrote:
> Should you cache this array? (You'll need to invalidate when displayedRows
> changes.)
> I guess if the list scrolls smoothly already on old devices, we can avoid
> optimizing.

I'm now keeping a map of section titles to the section rows. This method now
returns the sorted keys which is pretty lightweight.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:198:
NSPredicate* prediacate = [NSPredicate
On 2017/03/29 11:38:15, lpromero wrote:
> *predicate

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:200:
return [self.displayedRows filteredArrayUsingPredicate:prediacate];
On 2017/03/29 11:38:15, lpromero wrote:
> idem, should this be cached? It is called several times, and at least once per
> cell being shown on screen.

with the map of section titles to the section rows, this is now a very
lightweight method.

https://codereview.chromium.org/2778343002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_view_controller.mm:221:
[self.tableView reloadData];
On 2017/03/29 11:38:15, lpromero wrote:
> Showcase handles that more nicely, as it animates the cells in and out:
> 
> - (void)filterContentForSearchText:(NSString*)searchText {
>   NSArray<showcase::ModelRow*>* newFilteredRows = self.allRows;
> 
>   if (![self.searchController.searchBar.text isEqualToString:@""]) {
>     // The search is case-insensitive and searches both displayed texts.
>     NSIndexSet* matchingRows =
>         [self.allRows indexesOfObjectsPassingTest:^BOOL(
>                           showcase::ModelRow* row, NSUInteger idx, BOOL* stop)
{
>           return [row[showcase::kClassForDisplayKey]
>                      localizedCaseInsensitiveContainsString:searchText] ||
>                  [row[showcase::kUseCaseKey]
>                      localizedCaseInsensitiveContainsString:searchText];
>         }];
>     newFilteredRows = [self.allRows objectsAtIndexes:matchingRows];
>   }
> 
>   NSArray<NSIndexPath*>* indexPathsToInsert =
>       [self indexPathsToInsertFromArray:self.displayedRows
>                                 toArray:newFilteredRows
>                        indexPathSection:0];
>   NSArray<NSIndexPath*>* indexPathsToDelete =
>       [self indexPathsToDeleteFromArray:self.displayedRows
>                                 toArray:newFilteredRows
>                        indexPathSection:0];
> 
>   [self.tableView beginUpdates];
>   [self.tableView insertRowsAtIndexPaths:indexPathsToInsert
>                         withRowAnimation:UITableViewRowAnimationFade];
>   [self.tableView deleteRowsAtIndexPaths:indexPathsToDelete
>                         withRowAnimation:UITableViewRowAnimationFade];
>   self.displayedRows = newFilteredRows;
>   [self.tableView endUpdates];
> }

Since we have sections here and they would have to get removed or added
accordingly, I imagine the logic to animate the cells in and out would be a lot
more involved. This screen will be used for picking a country when editing a
credit card. It simply isn't important enough to justify the effort.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_picker_coordinator.mm (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_coordinator.mm:42:
didSelectRow:(PickerRow*)row {
On 2017/03/29 11:38:15, lpromero wrote:
> You can use the ProtocolAlerter to trigger an alert. That way you can add a
test
> that checks this is called.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
File ios/showcase/payments/sc_payments_picker_egtest.mm (right):

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:20: id<GREYMatcher>
matcherForSectionWithTitle(NSString* title) {
On 2017/03/29 11:38:15, lpromero wrote:
> I think the convention is to UpperCamelCase these.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:20: id<GREYMatcher>
matcherForSectionWithTitle(NSString* title) {
On 2017/03/29 11:38:15, lpromero wrote:
> I'd even omit the "matcher" in all these. It makes the call site shorter and
not
> more difficult to read.

Done.

https://codereview.chromium.org/2778343002/diff/20001/ios/showcase/payments/s...
ios/showcase/payments/sc_payments_picker_egtest.mm:29: selected ?
kPaymentRequestPickerSelectedRowAccessibilityID
On 2017/03/29 11:38:15, lpromero wrote:
> Instead of having a different ID for both selected and unselected, you could
> simplify by checking the UIAccessibilityTraitSelected. That way I think you
> could remove the logic of two accessibility identifiers.

TIL! Done.

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
File ios/showcase/common/protocol_alerter.mm (right):

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
ios/showcase/common/protocol_alerter.mm:173: __weak id object;
without this the alerter deallocates the object arguments.

[lpromero, 2017-04-03 16:41:42]

lgtm

https://codereview.chromium.org/2778343002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_row.h (right):

https://codereview.chromium.org/2778343002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_row.h:18: -
(instancetype)initWithLabel:(NSString*)label value:(NSString*)value;
Nit: add a new line.

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
File ios/showcase/common/protocol_alerter.mm (right):

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
ios/showcase/common/protocol_alerter.mm:173: __weak id object;
On 2017/03/29 17:04:09, moe wrote:
> without this the alerter deallocates the object arguments.

Indeed: http://stackoverflow.com/a/16932813
But please use __unsafe_unretained instead.

[Moe, 2017-04-05 05:09:49]

Thank you.

https://codereview.chromium.org/2778343002/diff/40001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/payment_request_picker_row.h (right):

https://codereview.chromium.org/2778343002/diff/40001/ios/chrome/browser/paym...
ios/chrome/browser/payments/payment_request_picker_row.h:18: -
(instancetype)initWithLabel:(NSString*)label value:(NSString*)value;
On 2017/04/03 16:41:41, lpromero wrote:
> Nit: add a new line.

Done.

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
File ios/showcase/common/protocol_alerter.mm (right):

https://codereview.chromium.org/2778343002/diff/40001/ios/showcase/common/pro...
ios/showcase/common/protocol_alerter.mm:173: __weak id object;
On 2017/04/03 16:41:41, lpromero wrote:
> On 2017/03/29 17:04:09, moe wrote:
> > without this the alerter deallocates the object arguments.
> 
> Indeed: http://stackoverflow.com/a/16932813
> But please use __unsafe_unretained instead.

Done.

[Moe, 2017-04-05 05:09:53]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-04-05 05:09:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2778343002/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-04-05 05:10:15]

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-04-05 06:11:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491368993304510,
"parent_rev": "b4ad68a17ae6822adc2c4ed33b43712686198161", "commit_rev":
"14a3ede2a87ed2e81396338aaa61e747e4103100"}

[commit-bot: I haz the power, 2017-04-05 06:13:33]

Description was changed from

==========
[Payment Request] Picker view + showcase integration + egtests

http://imgur.com/a/S1tcB

BUG=602666
==========

to

==========
[Payment Request] Picker view + showcase integration + egtests

http://imgur.com/a/S1tcB

BUG=602666

Review-Url: https://codereview.chromium.org/2778343002
Cr-Commit-Position: refs/heads/master@{#461989}
Committed:
https://chromium.googlesource.com/chromium/src/+/14a3ede2a87ed2e81396338aaa61...
==========

[commit-bot: I haz the power, 2017-04-05 06:13:35]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/14a3ede2a87ed2e81396338aaa61...