Skip to content

Commit

Permalink
[iOS] Standardize handlers for settings view controllers.
Browse files Browse the repository at this point in the history
Handler setup for settings view controllers was inconsistent. Most have
access to a 'dispatcher' property that combines Application, Browser,
and BrowsingData commands. This property is defined in the
SettingsRootViewControlling property that most of these view controllers
conform to by virtue of being subclasses of SettingsRootTableView-Controller.

Others also have a separate property for Snackbar commands, and some
have an additional Application commands handler, even though that can
be done through the Dispatcher.

Command injection was also heterogenous; the dispatcher property was
typically set in the various convenience initializers of Settings
NavigationController via various methods of the delegate protocol, even
though all of these initializers also pass a Browser which provides
a command dispatcher that can be used for all of these handlers.

SettingsNavigationControllerDelegate defined three methods for fetching
command handlers. One got the three-command 'handler for settings', one
got just an Application command handler, and one got the Snackbar
handler. Some implementations of this protocol had one or more of these
methods as no-ops with NOTREACHED(); this is not something that should
ever be done in a protocol (where parts of the API can just be makred
@optional if they are actually optional).

This CL standardizes things by:

- Defining separate handler properties for all command protocols in
SettingsRootViewControlling.

- Getting rid of the mis-named 'dispatcher' property.

- Setting handlers on view controllers based on the Browser that
SettingsNavigationController is initialized with, and removing the
handler fetching methods from that class's delegate protocol.

- Ensuring all handlers are set for all view controllers. Some utility
functions/methods are added to make this less boilerplate-y, although
coordinators do use the same chunk of setup code to set all of the
handlers on their view controllers.

Tests are updated to configure command dispatch correctly.

After this CL lands, follow-up CLs will prune some of these command
protocols from SettingsRootViewControlling, to remove unneeded
dependencies.

Bug: 1045047
Change-Id: I4bdcda997763170595e2416df39575cc7b9e33cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4895672
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1210137}
  • Loading branch information
marcq authored and Chromium LUCI CQ committed Oct 16, 2023
1 parent 8624b86 commit 8d267e6
Show file tree
Hide file tree
Showing 34 changed files with 313 additions and 254 deletions.
21 changes: 1 addition & 20 deletions ios/chrome/browser/shared/coordinator/scene/scene_controller.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1658,8 +1658,7 @@ - (void)presentReportAnIssueViewController:(UIViewController*)baseViewController
self.settingsNavigationController =
[SettingsNavigationController userFeedbackControllerForBrowser:browser
delegate:self
userFeedbackData:data
handler:handler];
userFeedbackData:data];
[baseViewController presentViewController:self.settingsNavigationController
animated:YES
completion:nil];
Expand Down Expand Up @@ -2395,24 +2394,6 @@ - (void)settingsWasDismissed {
self.settingsNavigationController = nil;
}

- (id<ApplicationCommands, BrowserCommands>)handlerForSettings {
// Assume that settings always wants the dispatcher from the main BVC.
return static_cast<id<ApplicationCommands, BrowserCommands>>(
self.mainInterface.browser->GetCommandDispatcher());
}

- (id<ApplicationCommands>)handlerForApplicationCommands {
// Assume that settings always wants the dispatcher from the main BVC.
return HandlerForProtocol(self.mainInterface.browser->GetCommandDispatcher(),
ApplicationCommands);
}

- (id<SnackbarCommands>)handlerForSnackbarCommands {
// Assume that settings always wants the dispatcher from the main BVC.
return HandlerForProtocol(self.mainInterface.browser->GetCommandDispatcher(),
SnackbarCommands);
}

#pragma mark - TabGridCoordinatorDelegate

- (void)tabGrid:(TabGridCoordinator*)tabGrid
Expand Down
1 change: 1 addition & 0 deletions ios/chrome/browser/ui/authentication/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ source_set("unit_tests") {
"//ios/chrome/browser/ui/authentication/unified_consent:unified_consent_ui",
"//ios/chrome/browser/unified_consent/model",
"//ios/chrome/test:test_support",
"//ios/testing:protocol_fake",
"//ios/web/public/test",
"//testing/gtest",
"//third_party/ocmock",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -599,22 +599,6 @@ - (void)settingsWasDismissed {
_navigationController = nil;
}

- (id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>)
handlerForSettings {
NOTREACHED();
return nil;
}

- (id<ApplicationCommands>)handlerForApplicationCommands {
NOTREACHED();
return nil;
}

- (id<SnackbarCommands>)handlerForSnackbarCommands {
NOTREACHED();
return nil;
}

#pragma mark - Private

- (void)updateUserPolicyNotificationStatusIfNeeded:(PrefService*)prefService {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,23 @@

#import "ios/chrome/browser/ui/authentication/authentication_flow_performer.h"

#import <objc/runtime.h>

#import "ios/chrome/browser/shared/model/application_context/application_context.h"
#import "ios/chrome/browser/shared/model/browser/test/test_browser.h"
#import "ios/chrome/browser/shared/model/browser_state/test_chrome_browser_state.h"
#import "ios/chrome/browser/shared/public/commands/application_commands.h"
#import "ios/chrome/browser/shared/public/commands/browser_commands.h"
#import "ios/chrome/browser/shared/public/commands/browsing_data_commands.h"
#import "ios/chrome/browser/shared/public/commands/command_dispatcher.h"
#import "ios/chrome/browser/shared/public/commands/snackbar_commands.h"
#import "ios/chrome/browser/signin/authentication_service.h"
#import "ios/chrome/browser/signin/authentication_service_factory.h"
#import "ios/chrome/browser/signin/fake_authentication_service_delegate.h"
#import "ios/chrome/browser/signin/fake_system_identity.h"
#import "ios/chrome/browser/signin/fake_system_identity_manager.h"
#import "ios/chrome/test/ios_chrome_scoped_testing_local_state.h"
#import "ios/testing/protocol_fake.h"
#import "ios/web/public/test/web_task_environment.h"
#import "testing/platform_test.h"
#import "third_party/ocmock/gtest_support.h"
Expand All @@ -35,6 +43,20 @@ void SetUp() override {
std::make_unique<FakeAuthenticationServiceDelegate>());
browser_ = std::make_unique<TestBrowser>(browser_state_.get());
fake_identity_ = [FakeSystemIdentity fakeIdentity1];

NSArray<Protocol*>* command_protocols = @[
@protocol(ApplicationCommands), @protocol(BrowserCommands),
@protocol(BrowsingDataCommands), @protocol(ApplicationSettingsCommands),
@protocol(SnackbarCommands)
];
fake_command_endpoint_ =
[[ProtocolFake alloc] initWithProtocols:command_protocols];
for (Protocol* protocol in command_protocols) {
[browser_->GetCommandDispatcher()
startDispatchingToTarget:fake_command_endpoint_
forProtocol:protocol];
}

FakeSystemIdentityManager* fake_system_identity_manager =
FakeSystemIdentityManager::FromSystemIdentityManager(
GetApplicationContext()->GetSystemIdentityManager());
Expand All @@ -59,6 +81,7 @@ void TearDown() override {
id<AuthenticationFlowPerformerDelegate>
authentication_flow_performer_delegate_mock_ = nil;
FakeSystemIdentity* fake_identity_ = nil;
ProtocolFake* fake_command_endpoint_ = nil;
};

// Tests interrupt call with `SigninCoordinatorInterrupt::UIShutdownNoDismiss`.
Expand Down
1 change: 1 addition & 0 deletions ios/chrome/browser/ui/settings/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ source_set("unit_tests") {
"//ios/chrome/common",
"//ios/chrome/test:test_support",
"//ios/chrome/test/app:test_support",
"//ios/testing:protocol_fake",
"//ios/web/public/test",
"//net",
"//net:test_support",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,6 @@
@interface AboutChromeTableViewController
: SettingsRootTableViewController <SettingsControllerProtocol>

// ApplicationCommands handler.
@property(nonatomic, weak) id<ApplicationCommands> applicationCommandsHandler;

// SnackbarCommands handler.
@property(nonatomic, weak) id<SnackbarCommands> snackbarCommandsHandler;

- (instancetype)init NS_DESIGNATED_INITIALIZER;

- (instancetype)initWithStyle:(UITableViewStyle)style NS_UNAVAILABLE;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,14 +168,14 @@ - (void)didTapVersionFooter:(VersionFooter*)footer {
MDCSnackbarMessage* message =
[MDCSnackbarMessage messageWithText:messageText];
message.category = @"version copied";
[self.snackbarCommandsHandler showSnackbarMessage:message bottomOffset:0];
[self.snackbarHandler showSnackbarMessage:message bottomOffset:0];
}

#pragma mark - Private methods

- (void)openURL:(GURL)URL {
OpenNewTabCommand* command = [OpenNewTabCommand commandWithURLFromChrome:URL];
[self.applicationCommandsHandler closeSettingsUIAndOpenURL:command];
[self.applicationHandler closeSettingsUIAndOpenURL:command];
}

- (std::string)versionString {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ - (void)editButtonPressed {
GURL paymentsURL = autofill::payments::GetManageInstrumentsUrl();
OpenNewTabCommand* command =
[OpenNewTabCommand commandWithURLFromChrome:paymentsURL];
[self.dispatcher closeSettingsUIAndOpenURL:command];
[self.applicationHandler closeSettingsUIAndOpenURL:command];

// Don't call [super editButtonPressed] because edit mode is not actually
// entered in this case.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ - (void)tableView:(UITableView*)tableView
[[AutofillCreditCardEditTableViewController alloc]
initWithCreditCard:*creditCards[indexPath.item]
personalDataManager:_personalDataManager];
controller.dispatcher = self.dispatcher;
[self configureHandlersForRootViewController:controller];
[self.navigationController pushViewController:controller animated:YES];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ - (void)tableView:(UITableView*)tableView
BlockPopupsTableViewController* controller =
[[BlockPopupsTableViewController alloc]
initWithBrowserState:_browser->GetBrowserState()];
controller.dispatcher = self.dispatcher;
[self configureHandlersForRootViewController:controller];
[self.navigationController pushViewController:controller animated:YES];
break;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -977,11 +977,7 @@ - (void)openPassphraseDialog {
// Verify that the accounts table is displayed from a navigation controller.
DCHECK(self.navigationController);

// TODO(crbug.com/1045047): Use HandlerForProtocol after commands protocol
// clean up.
controllerToPush.dispatcher = static_cast<
id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>>(
_browser->GetCommandDispatcher());
[self configureHandlersForRootViewController:controllerToPush];
[self.navigationController pushViewController:controllerToPush animated:YES];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
#import "ios/chrome/browser/shared/model/browser_state/chrome_browser_state.h"
#import "ios/chrome/browser/shared/model/url/chrome_url_constants.h"
#import "ios/chrome/browser/shared/public/commands/application_commands.h"
#import "ios/chrome/browser/shared/public/commands/browser_commands.h"
#import "ios/chrome/browser/shared/public/commands/browsing_data_commands.h"
#import "ios/chrome/browser/shared/public/commands/command_dispatcher.h"
#import "ios/chrome/browser/shared/public/commands/open_new_tab_command.h"
#import "ios/chrome/browser/shared/public/commands/show_signin_command.h"
#import "ios/chrome/browser/shared/public/commands/snackbar_commands.h"
#import "ios/chrome/browser/shared/public/features/features.h"
#import "ios/chrome/browser/shared/ui/table_view/table_view_utils.h"
#import "ios/chrome/browser/signin/authentication_service.h"
Expand Down Expand Up @@ -98,9 +100,19 @@ - (void)start {
self.mediator.commandHandler = self;
viewController.modelDelegate = self.mediator;
viewController.serviceDelegate = self.mediator;
viewController.dispatcher = static_cast<
id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>>(
self.browser->GetCommandDispatcher());

CommandDispatcher* dispatcher = self.browser->GetCommandDispatcher();
viewController.applicationHandler =
HandlerForProtocol(dispatcher, ApplicationCommands);
viewController.browserHandler =
HandlerForProtocol(dispatcher, BrowserCommands);
viewController.browsingDataHandler =
HandlerForProtocol(dispatcher, BrowsingDataCommands);
viewController.settingsHandler =
HandlerForProtocol(dispatcher, ApplicationSettingsCommands);
viewController.snackbarHandler =
HandlerForProtocol(dispatcher, SnackbarCommands);

DCHECK(self.baseNavigationController);
[self.baseNavigationController pushViewController:self.viewController
animated:YES];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@
#import "ios/chrome/browser/shared/model/browser_state/chrome_browser_state.h"
#import "ios/chrome/browser/shared/model/url/chrome_url_constants.h"
#import "ios/chrome/browser/shared/public/commands/application_commands.h"
#import "ios/chrome/browser/shared/public/commands/browser_commands.h"
#import "ios/chrome/browser/shared/public/commands/browsing_data_commands.h"
#import "ios/chrome/browser/shared/public/commands/command_dispatcher.h"
#import "ios/chrome/browser/shared/public/commands/open_new_tab_command.h"
#import "ios/chrome/browser/shared/public/commands/show_signin_command.h"
#import "ios/chrome/browser/shared/public/commands/snackbar_commands.h"
#import "ios/chrome/browser/shared/ui/symbols/chrome_icon.h"
#import "ios/chrome/browser/shared/ui/table_view/table_view_utils.h"
#import "ios/chrome/browser/signin/authentication_service.h"
Expand Down Expand Up @@ -151,25 +153,35 @@ - (void)start {
UITableViewStyle style = _accountState == SyncSettingsAccountState::kSignedIn
? UITableViewStyleInsetGrouped
: ChromeTableViewStyle();
self.viewController =
ManageSyncSettingsTableViewController* viewController =
[[ManageSyncSettingsTableViewController alloc] initWithStyle:style];
self.viewController = viewController;

NSString* title = self.mediator.overrideViewControllerTitle;
if (!title) {
title = self.delegate.manageSyncSettingsCoordinatorTitle;
}
self.viewController.title = title;
self.viewController.isAccountStateSignedIn =
viewController.title = title;
viewController.isAccountStateSignedIn =
_accountState == SyncSettingsAccountState::kSignedIn;
self.viewController.serviceDelegate = self.mediator;
self.viewController.presentationDelegate = self;
self.viewController.modelDelegate = self.mediator;
self.viewController.dispatcher = static_cast<
id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>>(
self.browser->GetCommandDispatcher());

self.mediator.consumer = self.viewController;
[self.baseNavigationController pushViewController:self.viewController
viewController.serviceDelegate = self.mediator;
viewController.presentationDelegate = self;
viewController.modelDelegate = self.mediator;

CommandDispatcher* dispatcher = self.browser->GetCommandDispatcher();
viewController.applicationHandler =
HandlerForProtocol(dispatcher, ApplicationCommands);
viewController.browserHandler =
HandlerForProtocol(dispatcher, BrowserCommands);
viewController.browsingDataHandler =
HandlerForProtocol(dispatcher, BrowsingDataCommands);
viewController.settingsHandler =
HandlerForProtocol(dispatcher, ApplicationSettingsCommands);
viewController.snackbarHandler =
HandlerForProtocol(dispatcher, SnackbarCommands);

self.mediator.consumer = viewController;
[self.baseNavigationController pushViewController:viewController
animated:YES];
_syncObserver = std::make_unique<SyncObserverBridge>(self, self.syncService);
}
Expand Down Expand Up @@ -394,11 +406,8 @@ - (void)openPassphraseDialog {
controllerToPush = [[SyncEncryptionTableViewController alloc]
initWithBrowser:self.browser];
}
// TODO(crbug.com/1045047): Use HandlerForProtocol after commands protocol
// clean up.
controllerToPush.dispatcher = static_cast<
id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>>(
self.browser->GetCommandDispatcher());

[self.viewController configureHandlersForRootViewController:controllerToPush];
[self.baseNavigationController pushViewController:controllerToPush
animated:YES];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ source_set("unit_tests") {
"//ios/chrome/common/ui/reauthentication",
"//ios/chrome/test:test_support",
"//ios/chrome/test/app:test_support",
"//ios/testing:protocol_fake",
"//ios/web/public/test",
"//testing/gmock",
"//testing/gtest",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,11 +442,12 @@ - (void)showMovedToAccountSnackbarWithPasswordCount:(int)count
pattern, "COUNT", count, "EMAIL", base::UTF8ToUTF16(email));

TriggerHapticFeedbackForNotification(UINotificationFeedbackTypeSuccess);
[self.handlerForSnackbarCommands
showSnackbarWithMessage:base::SysUTF16ToNSString(result)
buttonText:nil
messageAction:nil
completionAction:nil];
id<SnackbarCommands> handler = HandlerForProtocol(
self.browser->GetCommandDispatcher(), SnackbarCommands);
[handler showSnackbarWithMessage:base::SysUTF16ToNSString(result)
buttonText:nil
messageAction:nil
completionAction:nil];
}

#pragma mark - PasswordExportHandler
Expand Down Expand Up @@ -559,22 +560,6 @@ - (void)settingsWasDismissed {
[self.delegate passwordSettingsCoordinatorDidRemove:self];
}

- (id<ApplicationCommands, BrowserCommands, BrowsingDataCommands>)
handlerForSettings {
NOTREACHED();
return nil;
}

- (id<ApplicationCommands>)handlerForApplicationCommands {
NOTREACHED();
return nil;
}

- (id<SnackbarCommands>)handlerForSnackbarCommands {
return HandlerForProtocol(self.browser->GetCommandDispatcher(),
SnackbarCommands);
}

#pragma mark - ReauthenticationCoordinatorDelegate

- (void)successfulReauthenticationWithCoordinator:
Expand Down

0 comments on commit 8d267e6

Please sign in to comment.