Factor ActivityServiceCommands out of BrowserCommands

As part of separating the command protocols to make BrowserCommands
better-defined and easier to use with HandlerForProtocol(), this CL
removes the ActivityServiceCommands protocol from BrowserCommands.

The primary user of this protocol is ToolbarButtonActionsHandler, which
is updated to have separate per-protocol command handlers.

Since there is no longer a code path where the BVC is created and loads
its view without having a browser and browser state set, the conditional
call to addUIFunctionalityForBrowserAndBrowserState is now unconditional,
and some of the code in that method is hoisted into -viewDidLoad so the
ordering is correct for setting up command handling.

Similarly, the toolbar button factory is moved to be initialized after
the location bar, since it (now) requires OmniboxCommands to be usable.

Bug: 1045047
Change-Id: Ie3de78e1bc1f83dfaf14cd438c5b369a21aef206
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3613320
Reviewed-by: David Jean <djean@chromium.org>
Commit-Queue: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1001075}

ios/chrome/browser/ui/browser_view/browser_view_controller.mm

@@ -1248,6 +1248,8 @@ - (BOOL)shouldRegisterKeyboardCommands {
 #pragma mark - UIViewController
 
 - (void)viewDidLoad {
+  DCHECK(self.browser);
+
   CGRect initialViewsRect = self.view.bounds;
   UIViewAutoresizing initialViewAutoresizing =
       UIViewAutoresizingFlexibleWidth | UIViewAutoresizingFlexibleHeight;
@@ -1275,13 +1277,24 @@ - (void)viewDidLoad {
 
   // Install fake status bar for iPad iOS7
   [self installFakeStatusBar];
+
+  // TODO(crbug.com/1272534): Move BubblePresenter to BrowserCoordinator.
+  self.bubblePresenter =
+      [[BubblePresenter alloc] initWithBrowserState:self.browserState
+                                           delegate:self
+                                 rootViewController:self];
+  self.bubblePresenter.toolbarHandler =
+      HandlerForProtocol(self.browser->GetCommandDispatcher(), ToolbarCommands);
+  [self.browser->GetCommandDispatcher()
+      startDispatchingToTarget:self.bubblePresenter
+                   forProtocol:@protocol(HelpCommands)];
+
   [self buildToolbarAndTabStrip];
   [self setUpViewLayout:YES];
   [self addConstraintsToToolbar];
 
-  // If the browser and browser state are valid, finish initialization.
-  if (self.browser && self.browserState)
-    [self addUIFunctionalityForBrowserAndBrowserState];
+  // Finish initialization.
+  [self addUIFunctionalityForBrowserAndBrowserState];
 
   // Add a tap gesture recognizer to save the last tap location for the source
   // location of the new tab animation.
@@ -1791,6 +1804,13 @@ - (void)buildToolbarAndTabStrip {
       _locationBarModelDelegate.get(), kMaxURLDisplayChars);
   self.helper = [_dependencyFactory newBrowserViewControllerHelper];
 
+  self.popupMenuCoordinator =
+      [[PopupMenuCoordinator alloc] initWithBaseViewController:self
+                                                       browser:self.browser];
+  self.popupMenuCoordinator.bubblePresenter = self.bubblePresenter;
+  self.popupMenuCoordinator.UIUpdater = _toolbarCoordinatorAdaptor;
+  [self.popupMenuCoordinator start];
+
   PrimaryToolbarCoordinator* topToolbarCoordinator =
       [[PrimaryToolbarCoordinator alloc] initWithBrowser:self.browser];
   self.primaryToolbarCoordinator = topToolbarCoordinator;
@@ -2029,26 +2049,9 @@ - (void)addUIFunctionalityForBrowserAndBrowserState {
       [NamedGuide guideWithName:kSecondaryToolbarGuide view:self.contentArea]
           .heightAnchor;
 
-  // TODO(crbug.com/1272534): Move BubblePresenter to BrowserCoordinator.
-  self.bubblePresenter =
-      [[BubblePresenter alloc] initWithBrowserState:self.browserState
-                                           delegate:self
-                                 rootViewController:self];
-  self.bubblePresenter.toolbarHandler =
-      HandlerForProtocol(self.browser->GetCommandDispatcher(), ToolbarCommands);
-  [self.browser->GetCommandDispatcher()
-      startDispatchingToTarget:self.bubblePresenter
-                   forProtocol:@protocol(HelpCommands)];
   self.helpHandler =
       HandlerForProtocol(self.browser->GetCommandDispatcher(), HelpCommands);
 
-  self.popupMenuCoordinator =
-      [[PopupMenuCoordinator alloc] initWithBaseViewController:self
-                                                       browser:self.browser];
-  self.popupMenuCoordinator.bubblePresenter = self.bubblePresenter;
-  self.popupMenuCoordinator.UIUpdater = _toolbarCoordinatorAdaptor;
-  [self.popupMenuCoordinator start];
-
   self.primaryToolbarCoordinator.longPressDelegate = self.popupMenuCoordinator;
   self.secondaryToolbarCoordinator.longPressDelegate =
       self.popupMenuCoordinator;

ios/chrome/browser/ui/browser_view/browser_view_controller_unittest.mm

@@ -27,6 +27,7 @@
 #import "ios/chrome/browser/ui/browser_view/browser_view_controller_dependency_factory.h"
 #import "ios/chrome/browser/ui/browser_view/browser_view_controller_helper.h"
 #import "ios/chrome/browser/ui/browser_view/key_commands_provider.h"
+#import "ios/chrome/browser/ui/commands/activity_service_commands.h"
 #import "ios/chrome/browser/ui/commands/application_commands.h"
 #import "ios/chrome/browser/ui/commands/command_dispatcher.h"
 #import "ios/chrome/browser/ui/commands/find_in_page_commands.h"
@@ -125,6 +126,11 @@ void SetUp() override {
 
     SceneStateBrowserAgent::CreateForBrowser(browser_.get(), scene_state_);
 
+    id mockActivityServiceCommandHandler =
+        OCMProtocolMock(@protocol(ActivityServiceCommands));
+    [browser_->GetCommandDispatcher()
+        startDispatchingToTarget:mockActivityServiceCommandHandler
+                     forProtocol:@protocol(ActivityServiceCommands)];
     id mockFindInPageCommandHandler =
         OCMProtocolMock(@protocol(FindInPageCommands));
     [browser_->GetCommandDispatcher()

ios/chrome/browser/ui/commands/browser_commands.h

@@ -8,7 +8,6 @@
 #import <Foundation/Foundation.h>
 #import <UIKit/UIKit.h>
 
-#import "ios/chrome/browser/ui/commands/activity_service_commands.h"
 #import "ios/chrome/browser/ui/commands/browser_coordinator_commands.h"
 #import "ios/chrome/browser/ui/commands/lens_commands.h"
 #import "ios/chrome/browser/ui/commands/new_tab_page_commands.h"
@@ -26,7 +25,6 @@ class GURL;
 // TODO(crbug.com/906662) : Extract BrowserCoordinatorCommands from
 // BrowserCommands.
 @protocol BrowserCommands <NSObject,
-                           ActivityServiceCommands,
                            BrowserCoordinatorCommands,
                            NewTabPageCommands,
                            PageInfoCommands,

ios/chrome/browser/ui/toolbar/adaptive_toolbar_coordinator.mm

@@ -9,7 +9,12 @@
 #import "ios/chrome/browser/main/browser.h"
 #import "ios/chrome/browser/overlays/public/overlay_presenter.h"
 #import "ios/chrome/browser/search_engines/template_url_service_factory.h"
+#import "ios/chrome/browser/ui/commands/activity_service_commands.h"
+#import "ios/chrome/browser/ui/commands/application_commands.h"
 #import "ios/chrome/browser/ui/commands/command_dispatcher.h"
+#import "ios/chrome/browser/ui/commands/find_in_page_commands.h"
+#import "ios/chrome/browser/ui/commands/omnibox_commands.h"
+#import "ios/chrome/browser/ui/commands/popup_menu_commands.h"
 #import "ios/chrome/browser/ui/menu/browser_action_factory.h"
 #import "ios/chrome/browser/ui/ntp/ntp_util.h"
 #import "ios/chrome/browser/ui/toolbar/adaptive_toolbar_coordinator+subclassing.h"
@@ -132,20 +137,30 @@ - (ToolbarButtonFactory*)buttonFactoryWithType:(ToolbarType)type {
   BOOL isIncognito = self.browser->GetBrowserState()->IsOffTheRecord();
   ToolbarStyle style = isIncognito ? INCOGNITO : NORMAL;
 
-  self.actionHandler = [[ToolbarButtonActionsHandler alloc] init];
-  // TODO(crbug.com/1045047): Use HandlerForProtocol after commands protocol
-  // clean up.
-  self.actionHandler.dispatcher =
-      static_cast<id<ApplicationCommands, BrowserCommands, FindInPageCommands,
-                     OmniboxCommands>>(self.browser->GetCommandDispatcher());
-  self.actionHandler.incognito =
-      self.browser->GetBrowserState()->IsOffTheRecord();
-  self.actionHandler.navigationAgent =
+  ToolbarButtonActionsHandler* actionHandler =
+      [[ToolbarButtonActionsHandler alloc] init];
+
+  CommandDispatcher* dispatcher = self.browser->GetCommandDispatcher();
+
+  actionHandler.applicationHandler =
+      HandlerForProtocol(dispatcher, ApplicationCommands);
+  actionHandler.activityHandler =
+      HandlerForProtocol(dispatcher, ActivityServiceCommands);
+  actionHandler.menuHandler = HandlerForProtocol(dispatcher, PopupMenuCommands);
+  actionHandler.findHandler =
+      HandlerForProtocol(dispatcher, FindInPageCommands);
+  actionHandler.omniboxHandler =
+      HandlerForProtocol(dispatcher, OmniboxCommands);
+
+  actionHandler.incognito = isIncognito;
+  actionHandler.navigationAgent =
       WebNavigationBrowserAgent::FromBrowser(self.browser);
 
+  self.actionHandler = actionHandler;
+
   ToolbarButtonFactory* buttonFactory =
       [[ToolbarButtonFactory alloc] initWithStyle:style];
-  buttonFactory.actionHandler = self.actionHandler;
+  buttonFactory.actionHandler = actionHandler;
   buttonFactory.visibilityConfiguration =
       [[ToolbarButtonVisibilityConfiguration alloc] initWithType:type];
 

ios/chrome/browser/ui/toolbar/buttons/toolbar_button_actions_handler.h

@@ -8,20 +8,22 @@
 #import <Foundation/Foundation.h>
 
 @protocol ApplicationCommands;
-@protocol BrowserCommands;
+@protocol ActivityServiceCommands;
+@protocol PopupMenuCommands;
 @protocol FindInPageCommands;
 @protocol OmniboxCommands;
+
 class WebNavigationBrowserAgent;
 
 // Handler for the actions associated with the different toolbar buttons.
 @interface ToolbarButtonActionsHandler : NSObject
 
-// Dispatcher for the actions.
-@property(nonatomic, weak) id<ApplicationCommands,
-                              BrowserCommands,
-                              FindInPageCommands,
-                              OmniboxCommands>
-    dispatcher;
+// Action Handlers
+@property(nonatomic, weak) id<ApplicationCommands> applicationHandler;
+@property(nonatomic, weak) id<ActivityServiceCommands> activityHandler;
+@property(nonatomic, weak) id<PopupMenuCommands> menuHandler;
+@property(nonatomic, weak) id<FindInPageCommands> findHandler;
+@property(nonatomic, weak) id<OmniboxCommands> omniboxHandler;
 
 @property(nonatomic, assign) WebNavigationBrowserAgent* navigationAgent;
 

ios/chrome/browser/ui/toolbar/buttons/toolbar_button_actions_handler.mm

@@ -8,6 +8,7 @@
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#import "ios/chrome/browser/ui/commands/activity_service_commands.h"
 #import "ios/chrome/browser/ui/commands/application_commands.h"
 #import "ios/chrome/browser/ui/commands/browser_commands.h"
 #import "ios/chrome/browser/ui/commands/find_in_page_commands.h"
@@ -31,19 +32,19 @@ - (void)forwardAction {
 }
 
 - (void)tabGridTouchDown {
-  [self.dispatcher prepareTabSwitcher];
+  [self.applicationHandler prepareTabSwitcher];
 }
 
 - (void)tabGridTouchUp {
-  [self.dispatcher displayTabSwitcherInGridLayout];
+  [self.applicationHandler displayTabSwitcherInGridLayout];
 }
 
 - (void)toolsMenuAction {
-  [self.dispatcher showToolsMenuPopup];
+  [self.menuHandler showToolsMenuPopup];
 }
 
 - (void)shareAction {
-  [self.dispatcher sharePage];
+  [self.activityHandler sharePage];
 }
 
 - (void)reloadAction {
@@ -55,18 +56,18 @@ - (void)stopAction {
 }
 
 - (void)searchAction:(id)sender {
-  [self.dispatcher closeFindInPage];
+  [self.findHandler closeFindInPage];
   UIView* senderView = base::mac::ObjCCastStrict<UIView>(sender);
   CGPoint center = [senderView.superview convertPoint:senderView.center
                                                toView:nil];
   OpenNewTabCommand* command =
       [OpenNewTabCommand commandWithIncognito:self.incognito
                                   originPoint:center];
-  [self.dispatcher openURLInNewTab:command];
+  [self.applicationHandler openURLInNewTab:command];
 }
 
 - (void)cancelOmniboxFocusAction {
-  [self.dispatcher cancelOmniboxEdit];
+  [self.omniboxHandler cancelOmniboxEdit];
 }
 
 @end

ios/chrome/browser/ui/toolbar/primary_toolbar_coordinator.mm

@@ -73,7 +73,6 @@ - (void)start {
   self.viewController = [[PrimaryToolbarViewController alloc] init];
   self.viewController.shouldHideOmniboxOnNTP =
       !self.browser->GetBrowserState()->IsOffTheRecord();
-  self.viewController.buttonFactory = [self buttonFactoryWithType:PRIMARY];
   // TODO(crbug.com/1045047): Use HandlerForProtocol after commands protocol
   // clean up.
   self.viewController.dispatcher =
@@ -85,6 +84,11 @@ - (void)start {
   self.orchestrator.toolbarAnimatee = self.viewController;
 
   [self setUpLocationBar];
+
+  // Button factory requires that the omnibox commands are set up, which is
+  // done by the location bar.
+  self.viewController.buttonFactory = [self buttonFactoryWithType:PRIMARY];
+
   self.viewController.locationBarViewController =
       self.locationBarCoordinator.locationBarViewController;
   self.orchestrator.locationBarAnimatee =