Remove the usage of CRWWebDelegate's openExternalURL method.

This is the last of CRWWebDelegate methods used by CRWWebController, removing it
will allow deleting the CRWWebDelegate entirely.
Also removed it from Tab and from PreloadController which were implementing them.
The logic of handling external apps is now entirely moved to AppLauncherTabHelper
which is a policy decider and is attached to the web state used by the CRWWebController.

Bug: 850760, 681867, 674991
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I2df6282ae561eef2e1da8fac605f42dbfe4cb97b
Reviewed-on: https://chromium-review.googlesource.com/1124942
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578447}

ios/chrome/browser/app_launcher/BUILD.gn

@@ -13,7 +13,10 @@ source_set("app_launcher") {
   ]
   deps = [
     "//base",
+    "//components/reading_list/core:core",
     "//ios/chrome/browser",
+    "//ios/chrome/browser/reading_list",
+    "//ios/chrome/browser/tabs",
     "//ios/chrome/browser/web:web_internal",
     "//ios/web/public",
     "//url",

ios/chrome/browser/app_launcher/app_launcher_tab_helper.h

@@ -54,6 +54,9 @@ class AppLauncherTabHelper
                        AppLauncherAbuseDetector* abuse_detector,
                        id<AppLauncherTabHelperDelegate> delegate);
 
+  // The WebState that this object is attached to.
+  web::WebState* web_state_ = nullptr;
+
   // Used to check for repeated launches and provide policy for launching apps.
   AppLauncherAbuseDetector* abuse_detector_ = nil;
 

ios/chrome/browser/app_launcher/app_launcher_tab_helper.mm

@@ -8,8 +8,14 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "components/reading_list/core/reading_list_model.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_tab_helper_delegate.h"
+#include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
+#import "ios/chrome/browser/tabs/legacy_tab_helper.h"
+#import "ios/chrome/browser/tabs/tab.h"
 #import "ios/chrome/browser/web/app_launcher_abuse_detector.h"
+#import "ios/web/public/navigation_item.h"
+#import "ios/web/public/navigation_manager.h"
 #import "ios/web/public/url_scheme_util.h"
 #import "ios/web/public/web_client.h"
 #import "net/base/mac/url_conversions.h"
@@ -21,6 +27,27 @@
 
 DEFINE_WEB_STATE_USER_DATA_KEY(AppLauncherTabHelper);
 
+namespace {
+
+bool IsValidAppUrl(const GURL& app_url) {
+  if (!app_url.is_valid())
+    return false;
+
+  if (!app_url.has_scheme())
+    return false;
+
+  // If the url is a direct FIDO U2F x-callback call, consider it as invalid, to
+  // prevent pages from spoofing requests with different origins.
+  if (app_url.SchemeIs("u2f-x-callback"))
+    return false;
+
+  // Block attempts to open this application's settings in the native system
+  // settings application.
+  if (app_url.SchemeIs("app-settings"))
+    return false;
+  return true;
+}
+
 // This enum used by the Applauncher to log to UMA, if App launching request was
 // allowed or blocked.
 // These values are persisted to logs. Entries should not be renumbered and
@@ -32,6 +59,8 @@ enum class ExternalURLRequestStatus {
   kCount,
 };
 
+}  // namespace
+
 void AppLauncherTabHelper::CreateForWebState(
     web::WebState* web_state,
     AppLauncherAbuseDetector* abuse_detector,
@@ -49,6 +78,7 @@ AppLauncherTabHelper::AppLauncherTabHelper(
     AppLauncherAbuseDetector* abuse_detector,
     id<AppLauncherTabHelperDelegate> delegate)
     : web::WebStatePolicyDecider(web_state),
+      web_state_(web_state),
       abuse_detector_(abuse_detector),
       delegate_(delegate),
       weak_factory_(this) {}
@@ -58,9 +88,6 @@ AppLauncherTabHelper::~AppLauncherTabHelper() = default;
 bool AppLauncherTabHelper::RequestToLaunchApp(const GURL& url,
                                               const GURL& source_page_url,
                                               bool link_tapped) {
-  if (!url.is_valid() || !url.has_scheme())
-    return false;
-
   // Don't open external application if chrome is not active.
   if ([[UIApplication sharedApplication] applicationState] !=
       UIApplicationStateActive) {
@@ -118,11 +145,11 @@ bool AppLauncherTabHelper::RequestToLaunchApp(const GURL& url,
 bool AppLauncherTabHelper::ShouldAllowRequest(
     NSURLRequest* request,
     const web::WebStatePolicyDecider::RequestInfo& request_info) {
-  GURL requestURL = net::GURLWithNSURL(request.URL);
-  if (web::UrlHasWebScheme(requestURL) ||
-      web::GetWebClient()->IsAppSpecificURL(requestURL) ||
-      requestURL.SchemeIs(url::kFileScheme) ||
-      requestURL.SchemeIs(url::kAboutScheme)) {
+  GURL request_url = net::GURLWithNSURL(request.URL);
+  if (web::UrlHasWebScheme(request_url) ||
+      web::GetWebClient()->IsAppSpecificURL(request_url) ||
+      request_url.SchemeIs(url::kFileScheme) ||
+      request_url.SchemeIs(url::kAboutScheme)) {
     // This URL can be handled by the WebState and doesn't require App launcher
     // handling.
     return true;
@@ -139,10 +166,44 @@ bool AppLauncherTabHelper::ShouldAllowRequest(
                          ? ExternalURLRequestStatus::kSubFrameRequestAllowed
                          : ExternalURLRequestStatus::kSubFrameRequestBlocked;
   }
-
   DCHECK_NE(request_status, ExternalURLRequestStatus::kCount);
   UMA_HISTOGRAM_ENUMERATION("WebController.ExternalURLRequestBlocking",
                             request_status, ExternalURLRequestStatus::kCount);
+  // Request is blocked.
+  if (request_status == ExternalURLRequestStatus::kSubFrameRequestBlocked)
+    return false;
 
-  return request_status != ExternalURLRequestStatus::kSubFrameRequestBlocked;
+  Tab* tab = LegacyTabHelper::GetTabForWebState(web_state_);
+
+  // If this is a Universal 2nd Factory (U2F) call, the origin needs to be
+  // checked to make sure it's secure and then update the |request_url| with
+  // the generated x-callback GURL based on x-callback-url specs.
+  if (request_url.SchemeIs("u2f")) {
+    GURL origin = web_state_->GetNavigationManager()
+                      ->GetLastCommittedItem()
+                      ->GetURL()
+                      .GetOrigin();
+    request_url = [tab XCallbackFromRequestURL:request_url originURL:origin];
+  }
+
+  const GURL& source_url = request_info.source_url;
+  bool is_link_transition = ui::PageTransitionTypeIncludingQualifiersIs(
+      request_info.transition_type, ui::PAGE_TRANSITION_LINK);
+  if (IsValidAppUrl(request_url) &&
+      RequestToLaunchApp(request_url, source_url, is_link_transition)) {
+    // Clears pending navigation history after successfully launching the
+    // external app.
+    web_state_->GetNavigationManager()->DiscardNonCommittedItems();
+
+    // When opening applications, the navigation is cancelled. Report the
+    // opening of the application to the ReadingListWebStateObserver to mark the
+    // entry as read if needed.
+    if (source_url.is_valid()) {
+      ReadingListModel* model =
+          ReadingListModelFactory::GetForBrowserState(tab.browserState);
+      if (model && model->loaded())
+        model->SetReadStatus(source_url, true);
+    }
+  }
+  return false;
 }

ios/chrome/browser/app_launcher/app_launcher_tab_helper_unittest.mm

@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_tab_helper_delegate.h"
 #import "ios/chrome/browser/web/app_launcher_abuse_detector.h"
+#import "ios/web/public/test/fakes/test_navigation_manager.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #import "testing/gtest_mac.h"
@@ -66,6 +67,19 @@
 }
 @end
 
+namespace {
+// A fake NavigationManager to be used by the WebState object for the
+// AppLauncher.
+class FakeNavigationManager : public web::TestNavigationManager {
+ public:
+  FakeNavigationManager() = default;
+
+  // web::NavigationManager implementation.
+  void DiscardNonCommittedItems() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(FakeNavigationManager);
+};
+
 // Test fixture for AppLauncherTabHelper class.
 class AppLauncherTabHelperTest : public PlatformTest {
  protected:
@@ -74,12 +88,15 @@ class AppLauncherTabHelperTest : public PlatformTest {
         delegate_([[FakeAppLauncherTabHelperDelegate alloc] init]) {
     AppLauncherTabHelper::CreateForWebState(&web_state_, abuse_detector_,
                                             delegate_);
+    // Allow is the default policy for this test.
+    abuse_detector_.policy = ExternalAppLaunchPolicyAllow;
+    web_state_.SetNavigationManager(std::make_unique<FakeNavigationManager>());
     tab_helper_ = AppLauncherTabHelper::FromWebState(&web_state_);
   }
 
-  bool VerifyRequestAllowed(NSString* url_string,
-                            bool target_frame_is_main,
-                            bool has_user_gesture) WARN_UNUSED_RESULT {
+  bool TestShouldAllowRequest(NSString* url_string,
+                              bool target_frame_is_main,
+                              bool has_user_gesture) WARN_UNUSED_RESULT {
     NSURL* url = [NSURL URLWithString:url_string];
     web::WebStatePolicyDecider::RequestInfo request_info(
         ui::PageTransition::PAGE_TRANSITION_LINK,
@@ -95,34 +112,22 @@ class AppLauncherTabHelperTest : public PlatformTest {
   AppLauncherTabHelper* tab_helper_;
 };
 
-// Tests that an empty URL does not show alert or launch app.
-TEST_F(AppLauncherTabHelperTest, EmptyUrl) {
-  tab_helper_->RequestToLaunchApp(GURL::EmptyGURL(), GURL::EmptyGURL(), false);
-  EXPECT_EQ(0U, delegate_.countOfAlertsShown);
-  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
-}
-
-// Tests that an invalid URL does not show alert or launch app.
-TEST_F(AppLauncherTabHelperTest, InvalidUrl) {
-  tab_helper_->RequestToLaunchApp(GURL("invalid"), GURL::EmptyGURL(), false);
-  EXPECT_EQ(0U, delegate_.countOfAlertsShown);
-  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
-}
-
-// Tests that a valid URL does launch app.
-TEST_F(AppLauncherTabHelperTest, ValidUrl) {
+// Tests that a valid URL launches app.
+TEST_F(AppLauncherTabHelperTest, AbuseDetectorPolicyAllowedForValidUrl) {
   abuse_detector_.policy = ExternalAppLaunchPolicyAllow;
-  tab_helper_->RequestToLaunchApp(GURL("valid://1234"), GURL::EmptyGURL(),
-                                  false);
+  EXPECT_FALSE(TestShouldAllowRequest(@"valid://1234",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
   EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
   EXPECT_EQ(GURL("valid://1234"), delegate_.lastLaunchedAppURL);
 }
 
 // Tests that a valid URL does not launch app when launch policy is to block.
-TEST_F(AppLauncherTabHelperTest, ValidUrlBlocked) {
+TEST_F(AppLauncherTabHelperTest, AbuseDetectorPolicyBlockedForValidUrl) {
   abuse_detector_.policy = ExternalAppLaunchPolicyBlock;
-  tab_helper_->RequestToLaunchApp(GURL("valid://1234"), GURL::EmptyGURL(),
-                                  false);
+  EXPECT_FALSE(TestShouldAllowRequest(@"valid://1234",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
   EXPECT_EQ(0U, delegate_.countOfAlertsShown);
   EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
 }
@@ -132,8 +137,10 @@ TEST_F(AppLauncherTabHelperTest, ValidUrlBlocked) {
 TEST_F(AppLauncherTabHelperTest, ValidUrlPromptUserAccepts) {
   abuse_detector_.policy = ExternalAppLaunchPolicyPrompt;
   delegate_.simulateUserAcceptingPrompt = YES;
-  tab_helper_->RequestToLaunchApp(GURL("valid://1234"), GURL::EmptyGURL(),
-                                  false);
+  EXPECT_FALSE(TestShouldAllowRequest(@"valid://1234",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+
   EXPECT_EQ(1U, delegate_.countOfAlertsShown);
   EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
   EXPECT_EQ(GURL("valid://1234"), delegate_.lastLaunchedAppURL);
@@ -144,37 +151,72 @@ TEST_F(AppLauncherTabHelperTest, ValidUrlPromptUserAccepts) {
 TEST_F(AppLauncherTabHelperTest, ValidUrlPromptUserRejects) {
   abuse_detector_.policy = ExternalAppLaunchPolicyPrompt;
   delegate_.simulateUserAcceptingPrompt = NO;
-  tab_helper_->RequestToLaunchApp(GURL("valid://1234"), GURL::EmptyGURL(),
-                                  false);
+  EXPECT_FALSE(TestShouldAllowRequest(@"valid://1234",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
   EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
 }
 
-// Tests that ShouldAllowRequest only allows requests for App Urls in main
-// frame, or iframe when there was a recent user interaction.
+// Tests that ShouldAllowRequest only launches apps for App Urls in main frame,
+// or iframe when there was a recent user interaction.
 TEST_F(AppLauncherTabHelperTest, ShouldAllowRequestWithAppUrl) {
   NSString* url_string = @"itms-apps://itunes.apple.com/us/app/appname/id123";
-  EXPECT_TRUE(VerifyRequestAllowed(url_string, /*target_frame_is_main=*/true,
-                                   /*has_user_gesture=*/false));
-  EXPECT_TRUE(VerifyRequestAllowed(url_string, /*target_frame_is_main=*/true,
-                                   /*has_user_gesture=*/true));
-  EXPECT_FALSE(VerifyRequestAllowed(url_string, /*target_frame_is_main=*/false,
-                                    /*has_user_gesture=*/false));
-  EXPECT_TRUE(VerifyRequestAllowed(url_string, /*target_frame_is_main=*/false,
-                                   /*has_user_gesture=*/true));
+  EXPECT_FALSE(TestShouldAllowRequest(url_string, /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+  EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
+
+  EXPECT_FALSE(TestShouldAllowRequest(url_string, /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/true));
+  EXPECT_EQ(2U, delegate_.countOfAppsLaunched);
+
+  EXPECT_FALSE(TestShouldAllowRequest(url_string,
+                                      /*target_frame_is_main=*/false,
+                                      /*has_user_gesture=*/false));
+  EXPECT_EQ(2U, delegate_.countOfAppsLaunched);
+
+  EXPECT_FALSE(TestShouldAllowRequest(url_string,
+                                      /*target_frame_is_main=*/false,
+                                      /*has_user_gesture=*/true));
+  EXPECT_EQ(3U, delegate_.countOfAppsLaunched);
 }
 
-// Tests that ShouldAllowRequest always allows requests for non App Urls.
+// Tests that ShouldAllowRequest always allows requests and does not launch
+// apps for non App Urls.
 TEST_F(AppLauncherTabHelperTest, ShouldAllowRequestWithNonAppUrl) {
-  EXPECT_TRUE(VerifyRequestAllowed(
+  EXPECT_TRUE(TestShouldAllowRequest(
       @"http://itunes.apple.com/us/app/appname/id123",
       /*target_frame_is_main=*/true, /*has_user_gesture=*/false));
-  EXPECT_TRUE(VerifyRequestAllowed(@"file://a/b/c",
-                                   /*target_frame_is_main=*/true,
-                                   /*has_user_gesture=*/true));
-  EXPECT_TRUE(VerifyRequestAllowed(@"about://test",
-                                   /*target_frame_is_main=*/false,
-                                   /*has_user_gesture=*/false));
-  EXPECT_TRUE(VerifyRequestAllowed(@"data://test",
-                                   /*target_frame_is_main=*/false,
-                                   /*has_user_gesture=*/true));
+  EXPECT_TRUE(TestShouldAllowRequest(@"file://a/b/c",
+                                     /*target_frame_is_main=*/true,
+                                     /*has_user_gesture=*/true));
+  EXPECT_TRUE(TestShouldAllowRequest(@"about://test",
+                                     /*target_frame_is_main=*/false,
+                                     /*has_user_gesture=*/false));
+  EXPECT_TRUE(TestShouldAllowRequest(@"data://test",
+                                     /*target_frame_is_main=*/false,
+                                     /*has_user_gesture=*/true));
+  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
+}
+
+// Tests that invalid Urls are completely blocked.
+TEST_F(AppLauncherTabHelperTest, InvalidUrls) {
+  EXPECT_FALSE(TestShouldAllowRequest(@"",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+  EXPECT_FALSE(TestShouldAllowRequest(@"invalid",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
+}
+
+// Tests that URLs with schemes that might be a security risk are blocked.
+TEST_F(AppLauncherTabHelperTest, InsecureUrls) {
+  EXPECT_FALSE(TestShouldAllowRequest(@"app-settings://",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+  EXPECT_FALSE(TestShouldAllowRequest(@"u2f-x-callback://test",
+                                      /*target_frame_is_main=*/true,
+                                      /*has_user_gesture=*/false));
+  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
 }
+}  // namespace

ios/chrome/browser/prerender/preload_controller.mm

@@ -500,16 +500,6 @@ bool IsPrerenderTabEvictionExperimentalGroup() {
   [self schedulePrerenderCancel];
 }
 
-#pragma mark - CRWWebDelegate protocol
-
-- (BOOL)openExternalURL:(const GURL&)URL
-              sourceURL:(const GURL&)sourceURL
-            linkClicked:(BOOL)linkClicked {
-  DCHECK(webState_);
-  Tab* tab = LegacyTabHelper::GetTabForWebState(webState_.get());
-  return [tab openExternalURL:URL sourceURL:sourceURL linkClicked:linkClicked];
-}
-
 #pragma mark - ManageAccountsDelegate
 
 - (void)onManageAccounts {

ios/chrome/browser/tabs/tab.h

@@ -162,8 +162,15 @@ extern NSString* const kProxyPassthroughHeaderValue;
 // Evaluates U2F result.
 - (void)evaluateU2FResultFromURL:(const GURL&)url;
 
+// Generates a GURL compliant with the x-callback-url specs for FIDO Universal
+// 2nd Factory (U2F) requests. Returns empty GURL if origin is not secure.
+// See http://x-callback-url.com/specifications/ for specifications.
+- (GURL)XCallbackFromRequestURL:(const GURL&)requestURL
+                      originURL:(const GURL&)originURL;
+
 // Sends a notification to indicate that |url| is going to start loading.
 - (void)notifyTabOfUrlMayStartLoading:(const GURL&)url;
+
 @end
 
 #endif  // IOS_CHROME_BROWSER_TABS_TAB_H_

ios/chrome/browser/tabs/tab.mm

@@ -38,7 +38,6 @@
 #include "components/search_engines/template_url_service.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/url_formatter/url_formatter.h"
-#import "ios/chrome/browser/app_launcher/app_launcher_tab_helper.h"
 #include "ios/chrome/browser/application_context.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
 #include "ios/chrome/browser/chrome_url_constants.h"
@@ -402,6 +401,18 @@ NSString* const kTabUrlKey = @"url";
                                               webState:self.webState];
 }
 
+- (GURL)XCallbackFromRequestURL:(const GURL&)requestURL
+                      originURL:(const GURL&)originURL {
+  // Create U2FController object lazily.
+  if (!_secondFactorController)
+    _secondFactorController = [[U2FController alloc] init];
+  return [_secondFactorController
+      XCallbackFromRequestURL:requestURL
+                    originURL:originURL
+                       tabURL:self.webState->GetLastCommittedURL()
+                        tabID:self.tabId];
+}
+
 #pragma mark - CRWWebStateObserver protocol
 
 - (void)webState:(web::WebState*)webState
@@ -447,69 +458,6 @@ NSString* const kTabUrlKey = @"url";
   _webStateImpl = nullptr;
 }
 
-#pragma mark - CRWWebDelegate protocol
-
-- (BOOL)openExternalURL:(const GURL&)url
-              sourceURL:(const GURL&)sourceURL
-            linkClicked:(BOOL)linkClicked {
-  // Make a local url copy for possible modification.
-  GURL finalURL = url;
-
-  // Check if it's a direct FIDO U2F x-callback call. If so, do not open it, to
-  // prevent pages from spoofing requests with different origins.
-  if (finalURL.SchemeIs("u2f-x-callback"))
-    return NO;
-
-  // Block attempts to open this application's settings in the native system
-  // settings application.
-  if (finalURL.SchemeIs("app-settings"))
-    return NO;
-
-  // Check if it's a FIDO U2F call.
-  if (finalURL.SchemeIs("u2f")) {
-    // Create U2FController object lazily.
-    if (!_secondFactorController)
-      _secondFactorController = [[U2FController alloc] init];
-
-    DCHECK([self navigationManager]);
-    GURL origin =
-        [self navigationManager]->GetLastCommittedItem()->GetURL().GetOrigin();
-
-    // Compose u2f-x-callback URL and update urlToOpen.
-    finalURL = [_secondFactorController
-        XCallbackFromRequestURL:finalURL
-                      originURL:origin
-                         tabURL:self.webState->GetLastCommittedURL()
-                          tabID:self.tabId];
-
-    if (!finalURL.is_valid())
-      return NO;
-  }
-
-  AppLauncherTabHelper* appLauncherTabHelper =
-      AppLauncherTabHelper::FromWebState(self.webState);
-  if (appLauncherTabHelper->RequestToLaunchApp(finalURL, sourceURL,
-                                               linkClicked)) {
-    // Clears pending navigation history after successfully launching the
-    // external app.
-    DCHECK([self navigationManager]);
-    [self navigationManager]->DiscardNonCommittedItems();
-    // Ensure the UI reflects the current entry, not the just-discarded pending
-    // entry.
-    [_parentTabModel notifyTabChanged:self];
-
-    if (sourceURL.is_valid()) {
-      ReadingListModel* model =
-          ReadingListModelFactory::GetForBrowserState(_browserState);
-      if (model && model->loaded())
-        model->SetReadStatus(sourceURL, true);
-    }
-
-    return YES;
-  }
-  return NO;
-}
-
 #pragma mark - Private methods
 
 - (OpenInController*)openInController {

ios/chrome/browser/tabs/tab_unittest.mm

@@ -68,7 +68,6 @@
 #undef catch
 
 namespace {
-const char kAppSettingsUrl[] = "app-settings://";
 const char kNewTabUrl[] = "chrome://newtab/";
 const char kGoogleUserUrl[] = "http://google.com";
 const char kGoogleRedirectUrl[] = "http://www.google.fr/";
@@ -317,13 +316,6 @@ TEST_F(TabTest, AddToHistoryWithRedirect) {
   CheckCurrentItem(results[0]);
 }
 
-TEST_F(TabTest, FailToOpenAppSettings) {
-  GURL app_settings_url = GURL(kAppSettingsUrl);
-  BOOL will_open_app_settings =
-      [tab_ openExternalURL:app_settings_url sourceURL:GURL() linkClicked:YES];
-  EXPECT_FALSE(will_open_app_settings);
-}
-
 // TODO(crbug.com/378098): Disabled because forward/back is now implemented in
 // CRWWebController, so this test cannot function with a mock CRWWebController.
 // Rewrite and re-enable this test when it becomes a CRWWebController or

ios/web/public/web_state/ui/crw_web_delegate.h

@@ -25,6 +25,8 @@ class GURL;
 // TODO(crbug.com/674991): Remove this protocol.
 @protocol CRWWebDelegate<NSObject>
 
+@optional
+
 // Called when an external app needs to be opened, it also passes |linkClicked|
 // to track if this call was a result of user action or not. Returns YES iff
 // |URL| is launched in an external app.

ios/web/web_state/ui/crw_web_controller.mm

@@ -2909,23 +2909,9 @@ registerLoadRequestForURL:(const GURL&)requestURL
   }
 }
 
-// TODO(stuartmorgan): This is mostly logic from the original UIWebView delegate
-// method, which provides less information than the WKWebView version. Audit
-// this for things that should be handled in the subclass instead.
 - (BOOL)shouldAllowLoadWithNavigationAction:(WKNavigationAction*)action {
-  // Skip the logic in this method and always allow load if |_delegate| is nil.
-  // This is a temporary workaround for https://crbug.com/809795 until we move
-  // this logic out of this class. This doesn't affect Chromium app because
-  // |_delegate| is always set in Chromium app.
-  if (!_delegate) {
-    return YES;
-  }
-
-  // The WebDelegate may instruct the CRWWebController to stop loading, and
-  // instead instruct the next page to be loaded in an animation.
-  NSURLRequest* request = action.request;
-  GURL requestURL = net::GURLWithNSURL(request.URL);
-  GURL mainDocumentURL = net::GURLWithNSURL(request.mainDocumentURL);
+  GURL requestURL = net::GURLWithNSURL(action.request.URL);
+  GURL mainDocumentURL = net::GURLWithNSURL(action.request.mainDocumentURL);
   DCHECK(_webView);
 
   // App specific pages have elevated privileges and WKWebView uses the same
@@ -2937,18 +2923,13 @@ registerLoadRequestForURL:(const GURL&)requestURL
     return NO;
   }
 
-  // If the URL doesn't look like one that can be shown as a web page, try to
-  // open the link with an external application.
-  // TODO(droger):  Check transition type before opening an external
-  // application? For example, only allow it for TYPED and LINK transitions.
+  // If the URL doesn't look like one that can be shown as a web page, it may
+  // handled by the embedder. In that case, update the web controller to
+  // correctly reflect the current state.
   if (![CRWWebController webControllerCanShow:requestURL]) {
     if (!_webStateImpl->ShouldAllowAppLaunching()) {
       return NO;
     }
-    web::NavigationItem* item = self.currentNavItem;
-    const GURL& sourceURL =
-        item ? item->GetOriginalRequestURL() : GURL::EmptyGURL();
-
     // Stop load if navigation is believed to be happening on the main frame.
     if ([self isMainFrameNavigationAction:action])
       [self stopLoading];
@@ -2967,31 +2948,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
         [self setDocumentURL:lastCommittedURL];
       }
     }
-
-    // TODO(crbug.com/820201): Launching External Applications shouldn't happen
-    // here.
-    // External application launcher needs |isNavigationTypeLinkActivated| to
-    // decide if the user intended to open the application by clicking on a
-    // link.
-    BOOL isNavigationTypeLinkActivated =
-        action.navigationType == WKNavigationTypeLinkActivated;
-    if ([_delegate openExternalURL:requestURL
-                         sourceURL:sourceURL
-                       linkClicked:isNavigationTypeLinkActivated]) {
-      if ([self shouldClosePageOnNativeApplicationLoad])
-        _webStateImpl->CloseWebState();
-    }
-    return NO;
   }
-
-  if ([[request HTTPMethod] isEqualToString:@"POST"]) {
-    web::NavigationItemImpl* item = self.currentNavItem;
-    // TODO(crbug.com/570699): Remove this check once it's no longer possible to
-    // have no current entries.
-    if (item)
-      [self cachePOSTDataForRequest:request inNavigationItem:item];
-  }
-
   return YES;
 }
 
@@ -4327,28 +4284,46 @@ registerLoadRequestForURL:(const GURL&)requestURL
   web::NavigationItem* item = self.currentNavItem;
   const GURL& sourceURL =
       item ? item->GetOriginalRequestURL() : GURL::EmptyGURL();
-
   web::WebStatePolicyDecider::RequestInfo requestInfo(
-      transition, sourceURL, [self isMainFrameNavigationAction:action],
+      transition, sourceURL, isMainFrameNavigationAction,
       userInteractedWithRequestMainFrame);
+  // First check if the navigation action should be blocked by the controller
+  // and make sure to update the controller in the case that the controller
+  // can't handle the request URL. Then use the embedders' policyDeciders to
+  // either: 1- Handle the URL it self and return false to stop the controller
+  // from proceeding with the navigation if needed. or 2- return true to allow
+  // the navigation to be proceeded by the web controller.
   BOOL allowLoad =
+      [self shouldAllowLoadWithNavigationAction:action] &&
       self.webStateImpl->ShouldAllowRequest(action.request, requestInfo);
-  if (!allowLoad && action.targetFrame.mainFrame) {
-    [_pendingNavigationInfo setCancelled:YES];
-    // Discard the pending item to ensure that the current URL is not
-    // different from what is displayed on the view.
-    [self discardNonCommittedItemsIfLastCommittedWasNotNativeView];
-  }
 
-  if (allowLoad)
-    allowLoad = [self shouldAllowLoadWithNavigationAction:action];
+  if (allowLoad) {
+    if ([[action.request HTTPMethod] isEqualToString:@"POST"]) {
+      web::NavigationItemImpl* item = self.currentNavItem;
+      // TODO(crbug.com/570699): Remove this check once it's no longer possible
+      // to have no current entries.
+      if (item)
+        [self cachePOSTDataForRequest:action.request inNavigationItem:item];
+    }
+  } else {
+    if (action.targetFrame.mainFrame) {
+      [_pendingNavigationInfo setCancelled:YES];
+      // Discard the pending item to ensure that the current URL is not
+      // different from what is displayed on the view.
+      [self discardNonCommittedItemsIfLastCommittedWasNotNativeView];
+      if (!_isBeingDestroyed && [self shouldClosePageOnNativeApplicationLoad])
+        _webStateImpl->CloseWebState();
+    }
 
-  if (!allowLoad && !_isBeingDestroyed) {
-    // Loading was started for user initiated navigations and should be stopped
-    // because no other WKWebView callbacks are called. TODO(crbug.com/767092):
-    // Loading should not start until webView.loading is changed to YES.
-    _webStateImpl->SetIsLoading(false);
+    if (!_isBeingDestroyed) {
+      // Loading was started for user initiated navigations and should be
+      // stopped because no other WKWebView callbacks are called.
+      // TODO(crbug.com/767092): Loading should not start until webView.loading
+      // is changed to YES.
+      _webStateImpl->SetIsLoading(false);
+    }
   }
+
   decisionHandler(allowLoad ? WKNavigationActionPolicyAllow
                             : WKNavigationActionPolicyCancel);
 }