Use Asynchronous openURL to launch external apps.

Break dependency between post app launching logic and the app launching
success status, and then use Asynchronous openURL instead to launch apps.

Bug: 774736, 850760
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id51a246df9f8a7bd809025d66d987520c4001c6e
Reviewed-on: https://chromium-review.googlesource.com/1152545
Commit-Queue: Mohammad Refaat <mrefaat@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Reviewed-by: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580353}

ios/chrome/browser/BUILD.gn

@@ -116,6 +116,7 @@ source_set("browser") {
     "//components/webdata_services",
     "//google_apis",
     "//ios/chrome/app/strings",
+    "//ios/chrome/browser/app_launcher:feature_flags",
     "//ios/chrome/browser/browser_state",
     "//ios/chrome/browser/browsing_data:feature_flags",
     "//ios/chrome/browser/download",

ios/chrome/browser/about_flags.mm

@@ -41,6 +41,7 @@
 #include "components/strings/grit/components_strings.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/unified_consent/feature.h"
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
 #include "ios/chrome/browser/browsing_data/browsing_data_features.h"
 #include "ios/chrome/browser/chrome_switches.h"
 #include "ios/chrome/browser/drag_and_drop/drag_and_drop_flag.h"
@@ -371,6 +372,9 @@ const flags_ui::FeatureEntry kFeatureEntries[] = {
      flag_descriptions::kNewPasswordFormParsingName,
      flag_descriptions::kNewPasswordFormParsingDescription, flags_ui::kOsIos,
      FEATURE_VALUE_TYPE(password_manager::features::kNewPasswordFormParsing)},
+    {"app-launcher-refresh", flag_descriptions::kAppLauncherRefreshName,
+     flag_descriptions::kAppLauncherRefreshDescription, flags_ui::kOsIos,
+     FEATURE_VALUE_TYPE(kAppLauncherRefresh)},
 };
 
 // Add all switches from experimental flags to |command_line|.

ios/chrome/browser/app_launcher/BUILD.gn

@@ -16,6 +16,7 @@ source_set("app_launcher") {
     "app_launching_state.mm",
   ]
   deps = [
+    ":feature_flags",
     "//base",
     "//components/reading_list/core:core",
     "//ios/chrome/browser",
@@ -26,6 +27,17 @@ source_set("app_launcher") {
   ]
 }
 
+source_set("feature_flags") {
+  configs += [ "//build/config/compiler:enable_arc" ]
+  sources = [
+    "app_launcher_flags.h",
+    "app_launcher_flags.mm",
+  ]
+  deps = [
+    "//base",
+  ]
+}
+
 source_set("unit_tests") {
   configs += [ "//build/config/compiler:enable_arc" ]
   testonly = true
@@ -36,9 +48,15 @@ source_set("unit_tests") {
   ]
   deps = [
     ":app_launcher",
+    ":feature_flags",
     "//base",
     "//base/test:test_support",
+    "//components/reading_list/core:core",
     "//ios/chrome/browser",
+    "//ios/chrome/browser/browser_state:test_support",
+    "//ios/chrome/browser/reading_list",
+    "//ios/chrome/browser/tabs",
+    "//ios/chrome/browser/web:tab_id_tab_helper",
     "//ios/chrome/browser/web:web_internal",
     "//ios/web/public/test/fakes",
     "//testing/gtest",

ios/chrome/browser/app_launcher/app_launcher_flags.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_APP_LAUNCHER_APP_LAUNCHER_FLAGS_H_
+#define IOS_CHROME_BROWSER_APP_LAUNCHER_APP_LAUNCHER_FLAGS_H_
+
+#include "base/feature_list.h"
+
+// Feature flag to enable the new app launcher logic.
+extern const base::Feature kAppLauncherRefresh;
+
+#endif  // IOS_CHROME_BROWSER_APP_LAUNCHER_APP_LAUNCHER_FLAGS_H_

ios/chrome/browser/app_launcher/app_launcher_flags.mm

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+const base::Feature kAppLauncherRefresh{"AppLauncherRefresh",
+                                        base::FEATURE_DISABLED_BY_DEFAULT};

ios/chrome/browser/app_launcher/app_launcher_tab_helper.mm

@@ -11,6 +11,7 @@
 #import "base/strings/sys_string_conversions.h"
 #include "components/reading_list/core/reading_list_model.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_abuse_detector.h"
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_tab_helper_delegate.h"
 #import "ios/chrome/browser/chrome_url_util.h"
 #include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
@@ -189,7 +190,7 @@ bool AppLauncherTabHelper::ShouldAllowRequest(
 
   Tab* tab = LegacyTabHelper::GetTabForWebState(web_state_);
 
-  // If this is a Universal 2nd Factory (U2F) call, the origin needs to be
+  // If this is a Universal 2nd Factor (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")) {
@@ -203,6 +204,23 @@ bool AppLauncherTabHelper::ShouldAllowRequest(
   const GURL& source_url = request_info.source_url;
   bool is_link_transition = ui::PageTransitionTypeIncludingQualifiersIs(
       request_info.transition_type, ui::PAGE_TRANSITION_LINK);
+
+  if (base::FeatureList::IsEnabled(kAppLauncherRefresh)) {
+    if (!is_link_transition && source_url.is_valid()) {
+      // At this stage the navigation will be canceled in all cases. If this
+      // was a redirection, the |source_url| may not have been reported to
+      // ReadingListWebStateObserver. Report it to mark as read if needed.
+      ReadingListModel* model =
+          ReadingListModelFactory::GetForBrowserState(tab.browserState);
+      if (model && model->loaded())
+        model->SetReadStatus(source_url, true);
+    }
+    if (IsValidAppUrl(request_url)) {
+      RequestToLaunchApp(request_url, source_url, is_link_transition);
+    }
+    return false;
+  }
+
   if (IsValidAppUrl(request_url) &&
       RequestToLaunchApp(request_url, source_url, is_link_transition)) {
     // Clears pending navigation history after successfully launching the

ios/chrome/browser/app_launcher/app_launcher_tab_helper_delegate.h

@@ -14,9 +14,11 @@ class GURL;
 @protocol AppLauncherTabHelperDelegate
 
 // Launches application that has |URL| if possible (optionally after confirming
-// via dialog in case the user didn't interact using |linkTapped| or if the
-// application is facetime). Returns NO if there is no such application
-// available.
+// via dialog in case that the navigation transition type was link based on
+// |linkTapped| or if the application is facetime).
+// Returns NO if there is no such application available.
+// TODO(crbug.com/850760): Change this method return to void, once the new
+// AppLauncherRefresh logic is always enabled.
 - (BOOL)appLauncherTabHelper:(AppLauncherTabHelper*)tabHelper
             launchAppWithURL:(const GURL&)URL
                   linkTapped:(BOOL)linkTapped;

ios/chrome/browser/app_launcher/app_launcher_tab_helper_unittest.mm

@@ -7,9 +7,19 @@
 #include <memory>
 
 #include "base/compiler_specific.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/time/default_clock.h"
+#include "components/reading_list/core/reading_list_entry.h"
+#include "components/reading_list/core/reading_list_model_impl.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_abuse_detector.h"
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
 #import "ios/chrome/browser/app_launcher/app_launcher_tab_helper_delegate.h"
+#include "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
 #import "ios/chrome/browser/chrome_url_util.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/web/tab_id_tab_helper.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"
@@ -81,6 +91,16 @@ class FakeNavigationManager : public web::TestNavigationManager {
   DISALLOW_COPY_AND_ASSIGN(FakeNavigationManager);
 };
 
+std::unique_ptr<KeyedService> BuildReadingListModel(
+    web::BrowserState* context) {
+  ios::ChromeBrowserState* browser_state =
+      ios::ChromeBrowserState::FromBrowserState(context);
+  std::unique_ptr<ReadingListModelImpl> reading_list_model(
+      new ReadingListModelImpl(nullptr, browser_state->GetPrefs(),
+                               base::DefaultClock::GetInstance()));
+  return reading_list_model;
+}
+
 // Test fixture for AppLauncherTabHelper class.
 class AppLauncherTabHelperTest : public PlatformTest {
  protected:
@@ -107,9 +127,58 @@ class AppLauncherTabHelperTest : public PlatformTest {
                                            request_info);
   }
 
+  // Initialize reading list model and its required tab helpers.
+  void InitializeReadingListModel() {
+    TestChromeBrowserState::Builder test_cbs_builder;
+    chrome_browser_state_ = test_cbs_builder.Build();
+    web_state_.SetBrowserState(chrome_browser_state_.get());
+    ReadingListModelFactory::GetInstance()->SetTestingFactoryAndUse(
+        chrome_browser_state_.get(), &BuildReadingListModel);
+    TabIdTabHelper::CreateForWebState(&web_state_);
+    LegacyTabHelper::CreateForWebState(&web_state_);
+    is_reading_list_initialized_ = true;
+  }
+
+  // Returns true if the |expected_read_status| matches the read status for any
+  // non empty source URL based on the transition type and the app policy.
+  bool TestReadingListUpdate(bool is_app_blocked,
+                             bool is_link_transition,
+                             bool expected_read_status) {
+    // Make sure reading list model is initialized.
+    if (!is_reading_list_initialized_)
+      InitializeReadingListModel();
+
+    GURL test_source_url("http://google.com");
+    ReadingListModel* model = ReadingListModelFactory::GetForBrowserState(
+        chrome_browser_state_.get());
+    EXPECT_TRUE(model->DeleteAllEntries());
+    model->AddEntry(test_source_url, "unread",
+                    reading_list::ADDED_VIA_CURRENT_APP);
+    abuse_detector_.policy = is_app_blocked ? ExternalAppLaunchPolicyBlock
+                                            : ExternalAppLaunchPolicyAllow;
+    ui::PageTransition transition_type =
+        is_link_transition
+            ? ui::PageTransition::PAGE_TRANSITION_LINK
+            : ui::PageTransition::PAGE_TRANSITION_CLIENT_REDIRECT;
+
+    NSURL* url = [NSURL
+        URLWithString:@"itms-apps://itunes.apple.com/us/app/appname/id123"];
+    web::WebStatePolicyDecider::RequestInfo request_info(
+        transition_type, test_source_url,
+        /*target_frame_is_main=*/true, /*has_user_gesture=*/true);
+    EXPECT_FALSE(tab_helper_->ShouldAllowRequest(
+        [NSURLRequest requestWithURL:url], request_info));
+
+    const ReadingListEntry* entry = model->GetEntryByURL(test_source_url);
+    return entry->IsRead() == expected_read_status;
+  }
+
+  base::test::ScopedTaskEnvironment scoped_task_environment;
   web::TestWebState web_state_;
+  std::unique_ptr<TestChromeBrowserState> chrome_browser_state_ = nil;
   FakeAppLauncherAbuseDetector* abuse_detector_ = nil;
   FakeAppLauncherTabHelperDelegate* delegate_ = nil;
+  bool is_reading_list_initialized_ = false;
   AppLauncherTabHelper* tab_helper_;
 };
 
@@ -201,7 +270,7 @@ TEST_F(AppLauncherTabHelperTest, ShouldAllowRequestWithNonAppUrl) {
 
 // Tests that invalid Urls are completely blocked.
 TEST_F(AppLauncherTabHelperTest, InvalidUrls) {
-  EXPECT_FALSE(TestShouldAllowRequest(@"",
+  EXPECT_FALSE(TestShouldAllowRequest(/*url_string=*/@"",
                                       /*target_frame_is_main=*/true,
                                       /*has_user_gesture=*/false));
   EXPECT_FALSE(TestShouldAllowRequest(@"invalid",
@@ -240,7 +309,62 @@ TEST_F(AppLauncherTabHelperTest, ChromeBundleUrlScheme) {
   EXPECT_FALSE(TestShouldAllowRequest(url,
                                       /*target_frame_is_main=*/true,
                                       /*has_user_gesture=*/false));
+  EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
+}
+
+// Tests that ShouldAllowRequest updates the reading list correctly, when there
+// is a valid app URL to be launches successfully.
+// TODO(crbug.com/850760): Remove this test, once the new AppLauncherRefresh
+// logic is always enabled.
+TEST_F(AppLauncherTabHelperTest, UpdatingTheReadingList) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(kAppLauncherRefresh);
+  // Reading list isn't expected to be updated if there was no app launch.
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/true,
+                                    /*is_link_transition*/ false,
+                                    /*expected_read_status*/ false));
+  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
 
+  // Reading list to be updated when app launch is successful.
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/false,
+                                    /*is_link_transition*/ false,
+                                    /*expected_read_status*/ true));
   EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
+
+  // Transition type doesn't affect the reading list status
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/false,
+                                    /*is_link_transition*/ true,
+                                    /*expected_read_status*/ true));
+  EXPECT_EQ(2U, delegate_.countOfAppsLaunched);
 }
+
+// Tests that ShouldAllowRequest updates the reading list correctly for non-link
+// transitions regardless of the app launching success when AppLauncherRefresh
+// flag is enabled.
+TEST_F(AppLauncherTabHelperTest, UpdatingTheReadingListWithAppLauncherRefresh) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kAppLauncherRefresh);
+  // Update reading list if the transition is not a link transition.
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/true,
+                                    /*is_link_transition*/ false,
+                                    /*expected_read_status*/ true));
+  EXPECT_EQ(0U, delegate_.countOfAppsLaunched);
+
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/false,
+                                    /*is_link_transition*/ false,
+                                    /*expected_read_status*/ true));
+  EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
+
+  // Don't update reading list if the transition is a link transition.
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/true,
+                                    /*is_link_transition*/ true,
+                                    /*expected_read_status*/ false));
+  EXPECT_EQ(1U, delegate_.countOfAppsLaunched);
+
+  EXPECT_TRUE(TestReadingListUpdate(/*is_app_blocked=*/false,
+                                    /*is_link_transition*/ true,
+                                    /*expected_read_status*/ false));
+  EXPECT_EQ(2U, delegate_.countOfAppsLaunched);
+}
+
 }  // namespace

ios/chrome/browser/ios_chrome_flag_descriptions.cc

@@ -43,6 +43,12 @@ const char kWalletServiceUseSandboxName[] = "Use Google Payments sandbox";
 const char kWalletServiceUseSandboxDescription[] =
     "Uses the sandbox service for Google Payments API calls.";
 
+const char kAppLauncherRefreshName[] = "Enable the new AppLauncher logic";
+const char kAppLauncherRefreshDescription[] =
+    "AppLauncher will always prompt if there is no direct link navigation, "
+    "also Apps will launch asynchronously and there will be no logic that"
+    "depends on the success or the failure of launching an app.";
+
 const char kAutofillDynamicFormsName[] = "Autofill dynamic forms";
 const char kAutofillDynamicFormsDescription[] =
     "Refills forms that dynamically change after an initial fill";

ios/chrome/browser/ios_chrome_flag_descriptions.h

@@ -36,6 +36,10 @@ extern const char kSyncSandboxDescription[];
 extern const char kWalletServiceUseSandboxName[];
 extern const char kWalletServiceUseSandboxDescription[];
 
+// Title and description for the flag to control the new app launcher.
+extern const char kAppLauncherRefreshName[];
+extern const char kAppLauncherRefreshDescription[];
+
 // Title and description for the flag to control the dynamic autofill.
 extern const char kAutofillDynamicFormsName[];
 extern const char kAutofillDynamicFormsDescription[];

ios/chrome/browser/ui/app_launcher/BUILD.gn

@@ -20,6 +20,7 @@ source_set("app_launcher") {
     "//ios/chrome/app/strings:ios_strings_grit",
     "//ios/chrome/browser",
     "//ios/chrome/browser/app_launcher",
+    "//ios/chrome/browser/app_launcher:feature_flags",
     "//ios/chrome/browser/mailto:feature_flags",
     "//ios/chrome/browser/ui/collection_view",
     "//ios/chrome/browser/web",
@@ -43,7 +44,9 @@ source_set("unit_tests") {
   deps = [
     ":app_launcher",
     "//base",
+    "//base/test:test_support",
     "//ios/chrome/app/strings:ios_strings_grit",
+    "//ios/chrome/browser/app_launcher:feature_flags",
     "//ios/chrome/browser/ui/collection_view:test_support",
     "//ios/chrome/browser/ui/collection_view/cells",
     "//ios/chrome/browser/web",

ios/chrome/browser/ui/app_launcher/app_launcher_coordinator.mm

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/strings/grit/components_strings.h"
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
 #import "ios/chrome/browser/mailto/features.h"
 #include "ios/chrome/browser/procedural_block_types.h"
 #import "ios/chrome/browser/ui/app_launcher/app_launcher_util.h"
@@ -214,6 +215,13 @@ void RecordUserAcceptedAppLaunchMetric(BOOL user_accepted) {
     return YES;
   }
 
+  if (base::FeatureList::IsEnabled(kAppLauncherRefresh)) {
+    [[UIApplication sharedApplication] openURL:net::NSURLWithGURL(URL)
+                                       options:@{}
+                             completionHandler:nil];
+    return YES;
+  }
+
 // If the following call returns YES, an application is about to be
 // launched and Chrome will go into the background now.
 #pragma clang diagnostic push

ios/chrome/browser/ui/app_launcher/app_launcher_coordinator_unittest.mm

@@ -7,6 +7,8 @@
 #import <UIKit/UIKit.h>
 
 #include "base/mac/foundation_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "ios/chrome/browser/app_launcher/app_launcher_flags.h"
 #include "ios/chrome/grit/ios_strings.h"
 #import "ios/chrome/test/scoped_key_window.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
@@ -75,6 +77,9 @@ TEST_F(AppLauncherCoordinatorTest, ItmsUrlShowsAlert) {
 
 // Tests that an app URL attempts to launch the application.
 TEST_F(AppLauncherCoordinatorTest, AppUrlLaunchesApp) {
+  // Make sure that the new AppLauncherRefresh logic is disabled.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(kAppLauncherRefresh);
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
   OCMExpect([application_ openURL:[NSURL URLWithString:@"some-app://1234"]]);
@@ -85,9 +90,28 @@ TEST_F(AppLauncherCoordinatorTest, AppUrlLaunchesApp) {
   [application_ verify];
 }
 
+// Tests that in the new AppLauncher, an app URL attempts to launch the
+// application.
+TEST_F(AppLauncherCoordinatorTest, AppLauncherRefreshAppUrlLaunchesApp) {
+  // Make sure that the new AppLauncherRefresh logic is enabled.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(kAppLauncherRefresh);
+  OCMExpect([application_ openURL:[NSURL URLWithString:@"some-app://1234"]
+                          options:@{}
+                completionHandler:nil]);
+  [coordinator_ appLauncherTabHelper:nullptr
+                    launchAppWithURL:GURL("some-app://1234")
+                          linkTapped:YES];
+  [application_ verify];
+}
+
 // Tests that |-appLauncherTabHelper:launchAppWithURL:linkTapped:| returns NO
 // if there is no application that corresponds to a given URL.
 TEST_F(AppLauncherCoordinatorTest, NoApplicationForUrl) {
+  // Make sure that the new AppLauncherRefresh logic is disabled.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(kAppLauncherRefresh);
+
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wdeprecated-declarations"
   OCMStub(

ios/web/web_state/ui/crw_web_controller.mm

@@ -4327,7 +4327,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
       [_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];
+      self.navigationManagerImpl->DiscardNonCommittedItems();
       if (!_isBeingDestroyed && [self shouldClosePageOnNativeApplicationLoad])
         _webStateImpl->CloseWebState();
     }