Commit 11942ab7 authored by Javier Ernesto Flores Robles's avatar Javier Ernesto Flores Robles Committed by Commit Bot

[iOS][MF] Password tweaks

Improves metrics by preventing sending an empty metric on start.
Adds a nice loading cell while the passwords are fetched.
Removes a race condition when "All passwords" was open while loading.

Bug: 878388, 911086
Change-Id: If87dc51ee360af32de5bb5539d88bb5926dbfb63
Reviewed-on: https://chromium-review.googlesource.com/c/1356552
Commit-Queue: Javier Ernesto Flores Robles <javierrobles@chromium.org>
Reviewed-by: default avatarOlivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613123}
parent a6ae6b35
...@@ -119,6 +119,7 @@ source_set("manual_fill_ui") { ...@@ -119,6 +119,7 @@ source_set("manual_fill_ui") {
"//ios/chrome/browser/ui/table_view:styler", "//ios/chrome/browser/ui/table_view:styler",
"//ios/chrome/browser/ui/table_view:table_view", "//ios/chrome/browser/ui/table_view:table_view",
"//ios/chrome/common/ui_util:ui_util", "//ios/chrome/common/ui_util:ui_util",
"//ios/third_party/material_components_ios",
"//net:net", "//net:net",
"//third_party/material_design_icons:ic_credit_card", "//third_party/material_design_icons:ic_credit_card",
"//third_party/material_design_icons:ic_place", "//third_party/material_design_icons:ic_place",
......
...@@ -65,12 +65,12 @@ constexpr float PopoverMaxHeight = 360; ...@@ -65,12 +65,12 @@ constexpr float PopoverMaxHeight = 360;
self.tableView.separatorInset = UIEdgeInsetsMake(0, 0, 0, 0); self.tableView.separatorInset = UIEdgeInsetsMake(0, 0, 0, 0);
self.tableView.allowsSelection = NO; self.tableView.allowsSelection = NO;
self.definesPresentationContext = YES; self.definesPresentationContext = YES;
[self startLoadingIndicatorWithLoadingMessage:@""];
} }
- (void)presentDataItems:(NSArray<TableViewItem*>*)items { - (void)presentDataItems:(NSArray<TableViewItem*>*)items {
if (!self.tableViewModel) { [self createModelIfNeeded];
[self loadModel];
}
BOOL sectionExist = [self.tableViewModel BOOL sectionExist = [self.tableViewModel
hasSectionForSectionIdentifier:ItemsSectionIdentifier]; hasSectionForSectionIdentifier:ItemsSectionIdentifier];
// If there are no passed items, remove section if exist. // If there are no passed items, remove section if exist.
...@@ -84,9 +84,7 @@ constexpr float PopoverMaxHeight = 360; ...@@ -84,9 +84,7 @@ constexpr float PopoverMaxHeight = 360;
} }
- (void)presentActionItems:(NSArray<TableViewItem*>*)actions { - (void)presentActionItems:(NSArray<TableViewItem*>*)actions {
if (!self.tableViewModel) { [self createModelIfNeeded];
[self loadModel];
}
BOOL sectionExist = [self.tableViewModel BOOL sectionExist = [self.tableViewModel
hasSectionForSectionIdentifier:ActionsSectionIdentifier]; hasSectionForSectionIdentifier:ActionsSectionIdentifier];
// If there are no passed items, remove section if exist. // If there are no passed items, remove section if exist.
...@@ -100,6 +98,13 @@ constexpr float PopoverMaxHeight = 360; ...@@ -100,6 +98,13 @@ constexpr float PopoverMaxHeight = 360;
#pragma mark - Private #pragma mark - Private
- (void)createModelIfNeeded {
if (!self.tableViewModel) {
[self loadModel];
[self stopLoadingIndicatorWithCompletion:nil];
}
}
- (void)handleKeyboardDidHide:(NSNotification*)notification { - (void)handleKeyboardDidHide:(NSNotification*)notification {
if (self.contentInsetsAlwaysEqualToSafeArea && !IsIPadIdiom()) { if (self.contentInsetsAlwaysEqualToSafeArea && !IsIPadIdiom()) {
// Resets the table view content inssets to be equal to the safe area // Resets the table view content inssets to be equal to the safe area
......
...@@ -187,6 +187,9 @@ animationControllerForDismissedController:(UIViewController*)dismissed { ...@@ -187,6 +187,9 @@ animationControllerForDismissedController:(UIViewController*)dismissed {
[self.allPasswordsViewController.presentingViewController [self.allPasswordsViewController.presentingViewController
dismissViewControllerAnimated:YES dismissViewControllerAnimated:YES
completion:^{ completion:^{
weakSelf.passwordMediator.disableFilter = NO;
weakSelf.passwordMediator.consumer =
weakSelf.passwordViewController;
if (weakSelf.presentingButton) { if (weakSelf.presentingButton) {
[weakSelf [weakSelf
presentFromButton:weakSelf.presentingButton]; presentFromButton:weakSelf.presentingButton];
......
...@@ -54,30 +54,29 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -54,30 +54,29 @@ BOOL AreCredentialsAtIndexesConnected(
@interface ManualFillPasswordMediator ()<ManualFillContentDelegate, @interface ManualFillPasswordMediator ()<ManualFillContentDelegate,
PasswordFetcherDelegate> PasswordFetcherDelegate>
// The |WebStateList| containing the active web state. Used to filter the list // The |WebStateList| containing the active web state. Used to filter the list
// of credentials based on the active web state. // of credentials based on the active web state.
@property(nonatomic, assign) WebStateList* webStateList; @property(nonatomic, assign) WebStateList* webStateList;
// The password fetcher to query the user profile. // The password fetcher to query the user profile.
@property(nonatomic, strong) PasswordFetcher* passwordFetcher; @property(nonatomic, strong) PasswordFetcher* passwordFetcher;
// A cache of the credentials fetched from the store, not synced. Useful to // A cache of the credentials fetched from the store, not synced. Useful to
// reuse the mediator. // reuse the mediator.
@property(nonatomic, strong) NSArray<ManualFillCredential*>* credentials; @property(nonatomic, strong) NSArray<ManualFillCredential*>* credentials;
// If the filter is disabled, the "Show All Passwords" button is not included // If the filter is disabled, the "Show All Passwords" button is not included
// in the model. // in the model.
@property(nonatomic, assign, readonly) BOOL isAllPasswordButtonEnabled; @property(nonatomic, assign, readonly) BOOL isAllPasswordButtonEnabled;
// YES if the password fetcher has completed at least one fetch.
@property(nonatomic, assign) BOOL passwordFetcherDidFetch;
@end @end
@implementation ManualFillPasswordMediator @implementation ManualFillPasswordMediator
@synthesize consumer = _consumer;
@synthesize contentDelegate = _contentDelegate;
@synthesize credentials = _credentials;
@synthesize disableFilter = _disableFilter;
@synthesize navigationDelegate = _navigationDelegate;
@synthesize passwordFetcher = _passwordFetcher;
@synthesize webStateList = _webStateList;
- (instancetype)initWithWebStateList:(WebStateList*)webStateList - (instancetype)initWithWebStateList:(WebStateList*)webStateList
passwordStore: passwordStore:
(scoped_refptr<password_manager::PasswordStore>) (scoped_refptr<password_manager::PasswordStore>)
...@@ -93,15 +92,6 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -93,15 +92,6 @@ BOOL AreCredentialsAtIndexesConnected(
return self; return self;
} }
- (void)setConsumer:(id<ManualFillPasswordConsumer>)consumer {
if (consumer == _consumer) {
return;
}
_consumer = consumer;
[self postCredentialsToConsumer];
[self postActionsToConsumer];
}
#pragma mark - PasswordFetcherDelegate #pragma mark - PasswordFetcherDelegate
- (void)passwordFetcher:(PasswordFetcher*)passwordFetcher - (void)passwordFetcher:(PasswordFetcher*)passwordFetcher
...@@ -116,7 +106,8 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -116,7 +106,8 @@ BOOL AreCredentialsAtIndexesConnected(
[credentials addObject:credential]; [credentials addObject:credential];
} }
self.credentials = credentials; self.credentials = credentials;
[self postCredentialsToConsumer]; self.passwordFetcherDidFetch = YES;
[self postDataToConsumer];
} }
#pragma mark - UISearchResultsUpdating #pragma mark - UISearchResultsUpdating
...@@ -141,6 +132,17 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -141,6 +132,17 @@ BOOL AreCredentialsAtIndexesConnected(
#pragma mark - Private #pragma mark - Private
- (void)postDataToConsumer {
// To avoid duplicating the metric tracking how many passwords are sent to the
// consumer, only post credentials if at least the first fetch is done. Or
// else there will be spam metrics with 0 passwords everytime the screen is
// open.
if (self.passwordFetcherDidFetch) {
[self postCredentialsToConsumer];
[self postActionsToConsumer];
}
}
// Posts the credentials to the consumer. If filtered is |YES| it only post the // Posts the credentials to the consumer. If filtered is |YES| it only post the
// ones associated with the active web state. // ones associated with the active web state.
- (void)postCredentialsToConsumer { - (void)postCredentialsToConsumer {
...@@ -193,15 +195,6 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -193,15 +195,6 @@ BOOL AreCredentialsAtIndexesConnected(
delegate:self]; delegate:self];
[items addObject:item]; [items addObject:item];
} }
if (credentials.count == 0) {
auto noItemsItem = [[ManualFillActionItem alloc]
initWithTitle:l10n_util::GetNSString(
IDS_IOS_MANUAL_FALLBACK_NO_PASSWORDS_FOR_SITE)
action:nil];
noItemsItem.enabled = NO;
noItemsItem.showSeparator = YES;
[items addObject:noItemsItem];
}
return items; return items;
} }
...@@ -253,6 +246,16 @@ BOOL AreCredentialsAtIndexesConnected( ...@@ -253,6 +246,16 @@ BOOL AreCredentialsAtIndexesConnected(
return !self.disableFilter; return !self.disableFilter;
} }
#pragma mark - Setters
- (void)setConsumer:(id<ManualFillPasswordConsumer>)consumer {
if (consumer == _consumer) {
return;
}
_consumer = consumer;
[self postDataToConsumer];
}
#pragma mark - ManualFillContentDelegate #pragma mark - ManualFillContentDelegate
- (BOOL)canUserInjectInPasswordField:(BOOL)passwordField - (BOOL)canUserInjectInPasswordField:(BOOL)passwordField
......
...@@ -72,7 +72,20 @@ NSString* const PasswordTableViewAccessibilityIdentifier = ...@@ -72,7 +72,20 @@ NSString* const PasswordTableViewAccessibilityIdentifier =
UMA_HISTOGRAM_COUNTS_100("ManualFallback.PresentedOptions.Passwords", UMA_HISTOGRAM_COUNTS_100("ManualFallback.PresentedOptions.Passwords",
credentials.count); credentials.count);
} }
[self presentDataItems:(NSArray<TableViewItem*>*)credentials]; // If no items were posted and there is no search bar, present the empty item
// and return.
if (!credentials.count && !self.searchController) {
ManualFillActionItem* emptyCredentialItem = [[ManualFillActionItem alloc]
initWithTitle:l10n_util::GetNSString(
IDS_IOS_MANUAL_FALLBACK_NO_PASSWORDS_FOR_SITE)
action:nil];
emptyCredentialItem.enabled = NO;
emptyCredentialItem.showSeparator = YES;
[self presentDataItems:@[ emptyCredentialItem ]];
return;
}
[self presentDataItems:credentials];
} }
- (void)presentActions:(NSArray<ManualFillActionItem*>*)actions { - (void)presentActions:(NSArray<ManualFillActionItem*>*)actions {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment