[ios] BVC owns NTP coordinator.

Moves ownership of the NTP coordinator from the NTP tab helper to the BVC.

Bug: 826369
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I470d344778f844145bbd4a615f3753b3980f1614
Reviewed-on: https://chromium-review.googlesource.com/c/1287321Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601602}

ios/chrome/browser/ntp/BUILD.gn

@@ -13,11 +13,7 @@ source_set("ntp") {
     "//base:base",
     "//components/strings:components_strings_grit",
     "//ios/chrome/browser",
-    "//ios/chrome/browser/browser_state",
     "//ios/chrome/browser/ui:feature_flags",
-    "//ios/chrome/browser/ui/ntp",
-    "//ios/chrome/browser/ui/ntp:coordinator",
-    "//ios/chrome/browser/web_state_list",
     "//ios/web/public",
     "//ui/base:base",
   ]
@@ -37,9 +33,7 @@ source_set("unit_tests") {
     "//ios/chrome/browser/ntp",
     "//ios/chrome/browser/ntp_snippets:ntp_snippets",
     "//ios/chrome/browser/search_engines:search_engines",
-    "//ios/chrome/browser/ui",
     "//ios/chrome/browser/ui:feature_flags",
-    "//ios/chrome/browser/ui/ntp",
     "//ios/chrome/browser/web_state_list",
     "//ios/chrome/browser/web_state_list:test_support",
     "//ios/chrome/test:test_support",

ios/chrome/browser/ntp/new_tab_page_tab_helper.h

@@ -8,20 +8,10 @@
 #import <UIKit/UIKit.h>
 
 #include "base/macros.h"
-#import "ios/chrome/browser/ui/ntp/new_tab_page_owning.h"
 #include "ios/web/public/web_state/web_state_observer.h"
 #import "ios/web/public/web_state/web_state_user_data.h"
 
-class WebStateList;
 @protocol NewTabPageTabHelperDelegate;
-@protocol NewTabPageControllerDelegate;
-@protocol ApplicationCommands;
-@protocol BrowserCommands;
-@protocol OmniboxFocuser;
-@protocol FakeboxFocuser;
-@protocol SnackbarCommands;
-@protocol UrlLoader;
-@class NewTabPageCoordinator;
 
 // NewTabPageTabHelper which manages a single NTP per tab.
 class NewTabPageTabHelper : public web::WebStateObserver,
@@ -29,52 +19,21 @@ class NewTabPageTabHelper : public web::WebStateObserver,
  public:
   ~NewTabPageTabHelper() override;
 
-  static void CreateForWebState(
-      web::WebState* web_state,
-      WebStateList* web_state_list,
-      id<NewTabPageTabHelperDelegate> delegate,
-      id<UrlLoader> loader,
-      id<NewTabPageControllerDelegate> toolbar_delegate,
-      id<ApplicationCommands,
-         BrowserCommands,
-         OmniboxFocuser,
-         FakeboxFocuser,
-         SnackbarCommands,
-         UrlLoader> dispatcher);
+  static void CreateForWebState(web::WebState* web_state,
+                                id<NewTabPageTabHelperDelegate> delegate);
 
   // Returns true when the current web_state is an NTP and the underlying
   // controllers have been created.
   bool IsActive() const;
 
-  // Returns the current NTP controller.
-  id<NewTabPageOwning> GetController() const;
-
   // Disables this tab helper.  This is useful when navigating away from an NTP,
   // so the tab helper can be disabled immediately, and before any potential
   // WebStateObserver callback.
   void Deactivate();
 
-  // Returns the UIViewController for the current NTP.
-  // TODO(crbug.com/826369): Currently there's a 1:1 relationship between the
-  // webState and the NTP, so we can't delegate this coordinator to the BVC.
-  // Consider sharing one NTP per BVC, and removing this ownership.
-  UIViewController* GetViewController() const;
-
-  // Instructs the current NTP to dismiss any context menus.
-  void DismissModals() const;
-
  private:
   NewTabPageTabHelper(web::WebState* web_state,
-                      WebStateList* web_state_list,
-                      id<NewTabPageTabHelperDelegate> delegate,
-                      id<UrlLoader> loader,
-                      id<NewTabPageControllerDelegate> toolbar_delegate,
-                      id<ApplicationCommands,
-                         BrowserCommands,
-                         OmniboxFocuser,
-                         FakeboxFocuser,
-                         SnackbarCommands,
-                         UrlLoader> dispatcher);
+                      id<NewTabPageTabHelperDelegate> delegate);
 
   // web::WebStateObserver overrides:
   void WebStateDestroyed(web::WebState* web_state) override;
@@ -84,18 +43,15 @@ class NewTabPageTabHelper : public web::WebStateObserver,
   // Enable or disable the tab helper.
   void SetActive(bool active);
 
-  // The Objective-C NTP coordinator instance.
-  // TODO(crbug.com/826369): Currently there's a 1:1 relationship between the
-  // webState and the NTP, so we can't delegate this coordinator to the BVC.
-  // Consider sharing one NTP per BVC, and moving this to a delegate.
-  __strong NewTabPageCoordinator* ntp_coordinator_ = nil;
-
   // Used to present and dismiss the NTP.
   __weak id<NewTabPageTabHelperDelegate> delegate_ = nil;
 
   // The WebState with which this object is associated.
   web::WebState* web_state_ = nullptr;
 
+  // |YES| if the current tab helper is active.
+  BOOL active_;
+
   DISALLOW_COPY_AND_ASSIGN(NewTabPageTabHelper);
 };
 

ios/chrome/browser/ntp/new_tab_page_tab_helper.mm

@@ -10,12 +10,9 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "components/strings/grit/components_strings.h"
-#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
 #include "ios/chrome/browser/ntp/new_tab_page_tab_helper_delegate.h"
-#import "ios/chrome/browser/ui/ntp/new_tab_page_coordinator.h"
 #include "ios/chrome/browser/ui/ui_feature_flags.h"
-#import "ios/chrome/browser/web_state_list/web_state_list.h"
 #import "ios/web/public/navigation_item.h"
 #import "ios/web/public/navigation_manager.h"
 #import "ios/web/public/web_state/navigation_context.h"
@@ -29,22 +26,12 @@
 // static
 void NewTabPageTabHelper::CreateForWebState(
     web::WebState* web_state,
-    WebStateList* web_state_list,
-    id<NewTabPageTabHelperDelegate> delegate,
-    id<UrlLoader> url_loader,
-    id<NewTabPageControllerDelegate> toolbar_delegate,
-    id<ApplicationCommands,
-       BrowserCommands,
-       OmniboxFocuser,
-       FakeboxFocuser,
-       SnackbarCommands,
-       UrlLoader> dispatcher) {
+    id<NewTabPageTabHelperDelegate> delegate) {
   DCHECK(web_state);
   if (!FromWebState(web_state)) {
-    web_state->SetUserData(UserDataKey(),
-                           base::WrapUnique(new NewTabPageTabHelper(
-                               web_state, web_state_list, delegate, url_loader,
-                               toolbar_delegate, dispatcher)));
+    web_state->SetUserData(
+        UserDataKey(),
+        base::WrapUnique(new NewTabPageTabHelper(web_state, delegate)));
   }
 }
 
@@ -52,52 +39,20 @@ NewTabPageTabHelper::~NewTabPageTabHelper() = default;
 
 NewTabPageTabHelper::NewTabPageTabHelper(
     web::WebState* web_state,
-    WebStateList* web_state_list,
-    id<NewTabPageTabHelperDelegate> delegate,
-    id<UrlLoader> url_loader,
-    id<NewTabPageControllerDelegate> toolbar_delegate,
-    id<ApplicationCommands,
-       BrowserCommands,
-       OmniboxFocuser,
-       FakeboxFocuser,
-       SnackbarCommands,
-       UrlLoader> dispatcher)
+    id<NewTabPageTabHelperDelegate> delegate)
     : delegate_(delegate), web_state_(web_state) {
   DCHECK(delegate);
   DCHECK(base::FeatureList::IsEnabled(kBrowserContainerContainsNTP));
 
-  ios::ChromeBrowserState* browser_state =
-      ios::ChromeBrowserState::FromBrowserState(web_state->GetBrowserState());
-
-  ntp_coordinator_ =
-      [[NewTabPageCoordinator alloc] initWithBrowserState:browser_state];
-  ntp_coordinator_.webStateList = web_state_list;
-  ntp_coordinator_.dispatcher = dispatcher;
-  ntp_coordinator_.URLLoader = url_loader;
-  ntp_coordinator_.toolbarDelegate = toolbar_delegate;
-
   web_state->AddObserver(this);
 
-  if (web_state->GetVisibleURL().GetOrigin() == kChromeUINewTabURL) {
-    [ntp_coordinator_ start];
-  }
+  active_ = web_state->GetVisibleURL().GetOrigin() == kChromeUINewTabURL;
+  if (active_)
+    [delegate_ newTabPageHelperDidChangeVisibility:this forWebState:web_state_];
 }
 
 bool NewTabPageTabHelper::IsActive() const {
-  return ntp_coordinator_.started;
-}
-
-UIViewController* NewTabPageTabHelper::GetViewController() const {
-  DCHECK(IsActive());
-  return ntp_coordinator_.viewController;
-}
-
-id<NewTabPageOwning> NewTabPageTabHelper::GetController() const {
-  return ntp_coordinator_;
-}
-
-void NewTabPageTabHelper::DismissModals() const {
-  return [ntp_coordinator_ dismissModals];
+  return active_;
 }
 
 void NewTabPageTabHelper::Deactivate() {
@@ -108,6 +63,7 @@ void NewTabPageTabHelper::Deactivate() {
 
 void NewTabPageTabHelper::WebStateDestroyed(web::WebState* web_state) {
   web_state->RemoveObserver(this);
+  SetActive(false);
 }
 
 void NewTabPageTabHelper::DidStartNavigation(
@@ -123,31 +79,18 @@ void NewTabPageTabHelper::DidStartNavigation(
 #pragma mark - Private
 
 void NewTabPageTabHelper::SetActive(bool active) {
-  // Save the NTP scroll offset before we navigate away.
-  web::NavigationManager* manager = web_state_->GetNavigationManager();
-  if (web_state_->GetLastCommittedURL().GetOrigin() == kChromeUINewTabURL) {
-    web::NavigationItem* item = manager->GetLastCommittedItem();
-    web::PageDisplayState displayState;
-    CGPoint scrollOffset = ntp_coordinator_.scrollOffset;
-    displayState.scroll_state().set_offset_x(scrollOffset.x);
-    displayState.scroll_state().set_offset_y(scrollOffset.y);
-    item->SetPageDisplayState(displayState);
-  }
-
-  bool was_active = IsActive();
+  bool was_active = active_;
+  active_ = active;
 
   if (active) {
     web::NavigationManager* manager = web_state_->GetNavigationManager();
     web::NavigationItem* item = manager->GetPendingItem();
     if (item)
       item->SetTitle(l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE));
-    [ntp_coordinator_ start];
-  } else {
-    [ntp_coordinator_ stop];
   }
 
   // Tell |delegate_| to show or hide the NTP, if necessary.
-  if (IsActive() != was_active) {
-    [delegate_ newTabPageHelperDidChangeVisibility:this];
+  if (active_ != was_active) {
+    [delegate_ newTabPageHelperDidChangeVisibility:this forWebState:web_state_];
   }
 }

ios/chrome/browser/ntp/new_tab_page_tab_helper_delegate.h

@@ -10,7 +10,8 @@ class NewTabPageTabHelper;
 // Protocol defining a delegate for the NTP tab helper.
 @protocol NewTabPageTabHelperDelegate
 // Tells the delegate the NTP needs to be shown or hidden.
-- (void)newTabPageHelperDidChangeVisibility:(NewTabPageTabHelper*)NTPHelper;
+- (void)newTabPageHelperDidChangeVisibility:(NewTabPageTabHelper*)NTPHelper
+                                forWebState:(web::WebState*)webState;
 @end
 
 #endif  // IOS_CHROME_BROWSER_NTP_NEW_TAB_PAGE_TAB_HELPER_DELEGATE_H_

ios/chrome/browser/ntp/new_tab_page_tab_helper_unittest.mm

@@ -16,9 +16,7 @@
 #include "ios/chrome/browser/ntp/new_tab_page_tab_helper_delegate.h"
 #include "ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
 #include "ios/chrome/browser/search_engines/template_url_service_factory.h"
-#import "ios/chrome/browser/ui/ntp/new_tab_page_controller_delegate.h"
 #include "ios/chrome/browser/ui/ui_feature_flags.h"
-#import "ios/chrome/browser/ui/url_loader.h"
 #import "ios/chrome/browser/web_state_list/fake_web_state_list_delegate.h"
 #include "ios/chrome/browser/web_state_list/web_state_list.h"
 #include "ios/chrome/test/ios_chrome_scoped_testing_chrome_browser_state_manager.h"
@@ -38,14 +36,6 @@ namespace {
 const char kTestURL[] = "http://foo.bar";
 }  // namespace
 
-@protocol NewTabPageTabDispatcher<ApplicationCommands,
-                                  BrowserCommands,
-                                  OmniboxFocuser,
-                                  FakeboxFocuser,
-                                  SnackbarCommands,
-                                  UrlLoader>
-@end
-
 // Test fixture for testing NewTabPageTabHelper class.
 class NewTabPageTabHelperTest : public PlatformTest {
  protected:
@@ -74,11 +64,6 @@ class NewTabPageTabHelperTest : public PlatformTest {
     test_web_state_.SetBrowserState(chrome_browser_state_.get());
 
     delegate_ = OCMProtocolMock(@protocol(NewTabPageTabHelperDelegate));
-    loader_ = OCMProtocolMock(@protocol(UrlLoader));
-    toolbar_delegate_ =
-        OCMProtocolMock(@protocol(NewTabPageControllerDelegate));
-    dispatcher_ = OCMProtocolMock(@protocol(NewTabPageTabDispatcher));
-    web_state_list_ = std::make_unique<WebStateList>(&web_state_list_delegate_);
   }
 
   NewTabPageTabHelper* tab_helper() {
@@ -86,14 +71,9 @@ class NewTabPageTabHelperTest : public PlatformTest {
   }
 
   void CreateTabHelper() {
-    NewTabPageTabHelper::CreateForWebState(
-        &test_web_state_, web_state_list_.get(), delegate_, loader_,
-        toolbar_delegate_, dispatcher_);
+    NewTabPageTabHelper::CreateForWebState(&test_web_state_, delegate_);
   }
 
-  id dispatcher_;
-  id toolbar_delegate_;
-  id loader_;
   id delegate_;
   web::TestWebThreadBundle thread_bundle_;
   IOSChromeScopedTestingChromeBrowserStateManager scoped_browser_state_manager_;

ios/chrome/browser/tabs/BUILD.gn

@@ -99,7 +99,6 @@ source_set("tabs_internal") {
     "//ios/chrome/browser/language",
     "//ios/chrome/browser/metrics",
     "//ios/chrome/browser/metrics:metrics_internal",
-    "//ios/chrome/browser/ntp",
     "//ios/chrome/browser/passwords",
     "//ios/chrome/browser/prerender",
     "//ios/chrome/browser/reading_list",

ios/chrome/browser/tabs/tab.mm

@@ -49,7 +49,6 @@
 #include "ios/chrome/browser/history/top_sites_factory.h"
 #include "ios/chrome/browser/infobars/infobar_manager_impl.h"
 #import "ios/chrome/browser/metrics/tab_usage_recorder.h"
-#import "ios/chrome/browser/ntp/new_tab_page_tab_helper.h"
 #include "ios/chrome/browser/pref_names.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
 #include "ios/chrome/browser/search_engines/template_url_service_factory.h"
@@ -306,12 +305,6 @@ NSString* const kTabUrlKey = @"url";
   if (self.webState->IsEvicted() && [_parentTabModel tabUsageRecorder])
     [_parentTabModel tabUsageRecorder]->RecordPageLoadStart(self.webState);
 
-  // Any early returns (such as below) must happen after the above call to
-  // RecordPageLoadStart, which is used for metric logging.
-  NewTabPageTabHelper* NTP = NewTabPageTabHelper::FromWebState(self.webState);
-  if (NTP && NTP->IsActive())
-    return [NTP->GetController() view];
-
   // Do not trigger the load if the tab has crashed. SadTabTabHelper is
   // responsible for handing reload logic for crashed tabs.
   if (!self.webState->IsCrashed()) {

ios/chrome/browser/ui/BUILD.gn

@@ -376,6 +376,7 @@ source_set("ui_internal") {
     "//ios/chrome/browser/ui/main_content:main_content_ui",
     "//ios/chrome/browser/ui/main_content:main_content_ui_broadcasting_util",
     "//ios/chrome/browser/ui/ntp",
+    "//ios/chrome/browser/ui/ntp:coordinator",
     "//ios/chrome/browser/ui/ntp:ntp_controller",
     "//ios/chrome/browser/ui/ntp:util",
     "//ios/chrome/browser/ui/omnibox:omnibox_internal",

ios/chrome/browser/ui/ntp/new_tab_page_coordinator.mm

@@ -81,6 +81,7 @@
 #pragma mark - Properties
 
 - (UIViewController*)viewController {
+  [self start];
   if (self.browserState->IsOffTheRecord()) {
     return self.incognitoViewController;
   } else {