Log MobileDownloadFileUIInstallGoogleDrive in New Download Manager.

This CL also changes InstallationNotifier class to be compatible
with OCMock (UIApplication shared instance is not stored in ivar anymore)
and adds resetDispatcher method to allow restoring InstallationNotifier
to its default state.

Bug: 791806
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Iaee0d80938639d23862b254732eb49c97ddd4e86
Reviewed-on: https://chromium-review.googlesource.com/958123
Commit-Queue: Eugene But <eugenebut@chromium.org>
Reviewed-by: Peter Lee <pkl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543515}

ios/chrome/browser/BUILD.gn

@@ -248,6 +248,7 @@ source_set("unit_tests") {
     "//net",
     "//net:test_support",
     "//testing/gtest",
+    "//third_party/ocmock",
     "//url",
   ]
 }

ios/chrome/browser/download/BUILD.gn

@@ -10,6 +10,8 @@ source_set("download") {
     "browser_download_service_factory.mm",
     "download_directory_util.cc",
     "download_directory_util.h",
+    "download_manager_metric_names.cc",
+    "download_manager_metric_names.h",
     "download_manager_tab_helper.h",
     "download_manager_tab_helper.mm",
     "download_manager_tab_helper_delegate.h",

ios/chrome/browser/download/download_manager_metric_names.cc

+// 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/download/download_manager_metric_names.h"
+
+const char kDownloadManagerGoogleDriveInstalled[] =
+    "MobileDownloadFileUIInstallGoogleDrive";

ios/chrome/browser/download/download_manager_metric_names.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_DOWNLOAD_DOWNLOAD_MANAGER_METRIC_NAMES_H_
+#define IOS_CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_METRIC_NAMES_H_
+
+// MobileDownloadFileUIInstallGoogleDrive UMA action. Logged when Google Drive
+// app is installed after presenting Store Kit dialog from the Download Manager.
+extern const char kDownloadManagerGoogleDriveInstalled[];
+
+#endif  // IOS_CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_METRIC_NAMES_H_

ios/chrome/browser/installation_notifier.mm

@@ -59,9 +59,6 @@ const net::BackoffEntry::Policy kPollingBackoffPolicy = {
 @interface InstallationNotifier (Testing)
 // Sets the dispatcher.
 - (void)setDispatcher:(id<DispatcherProtocol>)dispatcher;
-// Sets the UIApplication used to determine if a scheme can be opened by an
-// application.
-- (void)setSharedApplication:(UIApplication*)sharedApplication;
 @end
 
 @implementation InstallationNotifier {
@@ -89,7 +86,6 @@ const net::BackoffEntry::Policy kPollingBackoffPolicy = {
     _dispatcher = [[DefaultDispatcher alloc] init];
     _installedAppObservers = [[NSMutableDictionary alloc] init];
     _notificationCenter = [NSNotificationCenter defaultCenter];
-    sharedApplication_ = [UIApplication sharedApplication];
     _backoffEntry.reset(new net::BackoffEntry([self backOffPolicy]));
   }
   return self;
@@ -186,7 +182,7 @@ const net::BackoffEntry::Policy kPollingBackoffPolicy = {
     DCHECK([observers count] > 0);
     NSURL* testSchemeURL =
         [NSURL URLWithString:[NSString stringWithFormat:@"%@:", scheme]];
-    if ([sharedApplication_ canOpenURL:testSchemeURL]) {
+    if ([[UIApplication sharedApplication] canOpenURL:testSchemeURL]) {
       [_notificationCenter postNotificationName:scheme object:self];
       for (id weakReferenceToObserver in observers) {
         id observer = [weakReferenceToObserver nonretainedObjectValue];
@@ -216,11 +212,8 @@ const net::BackoffEntry::Policy kPollingBackoffPolicy = {
   _dispatcher = dispatcher;
 }
 
-- (void)setSharedApplication:(id)sharedApplication {
-  // Verify that the test application object responds to all the selectors that
-  // will be called on it.
-  CHECK([sharedApplication respondsToSelector:@selector(canOpenURL:)]);
-  sharedApplication_ = (UIApplication*)sharedApplication;
+- (void)resetDispatcher {
+  _dispatcher = [[DefaultDispatcher alloc] init];
 }
 
 @end

ios/chrome/browser/installation_notifier_unittest.mm

@@ -13,6 +13,7 @@
 #include "ios/web/public/test/test_web_thread.h"
 #include "net/base/backoff_entry.h"
 #include "testing/platform_test.h"
+#import "third_party/ocmock/OCMock/OCMock.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
@@ -78,27 +79,9 @@
 
 @end
 
-@interface MockUIApplication : NSObject
-// Mocks UIApplication's canOpenURL.
-@end
-
-@implementation MockUIApplication {
-  BOOL canOpen_;
-}
-
-- (void)setCanOpenURL:(BOOL)canOpen {
-  canOpen_ = canOpen;
-}
-
-- (BOOL)canOpenURL:(NSURL*)url {
-  return canOpen_;
-}
-
-@end
-
 @interface InstallationNotifier (Testing)
 - (void)setDispatcher:(id<DispatcherProtocol>)dispatcher;
-- (void)setSharedApplication:(id)sharedApplication;
+- (void)resetDispatcher;
 - (void)dispatchInstallationNotifierBlock;
 - (void)registerForInstallationNotifications:(id)observer
                                 withSelector:(SEL)notificationSelector
@@ -120,12 +103,16 @@ class InstallationNotifierTest : public PlatformTest {
     dispatcher_ = dispatcher;
     notificationReceiver1_ = ([[MockNotificationReceiver alloc] init]);
     notificationReceiver2_ = ([[MockNotificationReceiver alloc] init]);
-    sharedApplication_ = [[MockUIApplication alloc] init];
-    [installationNotifier_ setSharedApplication:sharedApplication_];
+    application_ = OCMClassMock([UIApplication class]);
+    OCMStub([application_ sharedApplication]).andReturn(application_);
     [installationNotifier_ setDispatcher:dispatcher_];
     histogramTester_.reset(new base::HistogramTester());
   }
 
+  ~InstallationNotifierTest() override {
+    [installationNotifier_ resetDispatcher];
+  }
+
   void VerifyHistogramValidity(int expectedYes, int expectedNo) {
     histogramTester_->ExpectTotalCount("NativeAppLauncher.InstallationDetected",
                                        expectedYes + expectedNo);
@@ -154,12 +141,12 @@ class InstallationNotifierTest : public PlatformTest {
   __weak FakeDispatcher* dispatcher_;
   MockNotificationReceiver* notificationReceiver1_;
   MockNotificationReceiver* notificationReceiver2_;
-  MockUIApplication* sharedApplication_;
+  id application_;
   std::unique_ptr<base::HistogramTester> histogramTester_;
 };
 
 TEST_F(InstallationNotifierTest, RegisterWithAppAlreadyInstalled) {
-  [sharedApplication_ setCanOpenURL:YES];
+  OCMStub([application_ canOpenURL:[OCMArg any]]).andReturn(YES);
   [installationNotifier_
       registerForInstallationNotifications:notificationReceiver1_
                               withSelector:@selector(receivedNotification)
@@ -174,11 +161,11 @@ TEST_F(InstallationNotifierTest, RegisterWithAppAlreadyInstalled) {
 }
 
 TEST_F(InstallationNotifierTest, RegisterWithAppInstalledAfterSomeTime) {
-  [sharedApplication_ setCanOpenURL:NO];
-  [dispatcher_ executeAfter:10
-                      block:^{
-                        [sharedApplication_ setCanOpenURL:YES];
-                      }];
+  [dispatcher_
+      executeAfter:10
+             block:^{
+               OCMStub([application_ canOpenURL:[OCMArg any]]).andReturn(YES);
+             }];
   [installationNotifier_
       registerForInstallationNotifications:notificationReceiver1_
                               withSelector:@selector(receivedNotification)
@@ -188,11 +175,11 @@ TEST_F(InstallationNotifierTest, RegisterWithAppInstalledAfterSomeTime) {
 }
 
 TEST_F(InstallationNotifierTest, RegisterForTwoInstallations) {
-  [sharedApplication_ setCanOpenURL:NO];
-  [dispatcher_ executeAfter:10
-                      block:^{
-                        [sharedApplication_ setCanOpenURL:YES];
-                      }];
+  [dispatcher_
+      executeAfter:10
+             block:^{
+               OCMStub([application_ canOpenURL:[OCMArg any]]).andReturn(YES);
+             }];
   [installationNotifier_
       registerForInstallationNotifications:notificationReceiver1_
                               withSelector:@selector(receivedNotification)
@@ -215,7 +202,7 @@ TEST_F(InstallationNotifierTest, RegisterForTwoInstallations) {
 }
 
 TEST_F(InstallationNotifierTest, RegisterAndThenUnregister) {
-  [sharedApplication_ setCanOpenURL:NO];
+  OCMStub([application_ canOpenURL:[OCMArg any]]).andReturn(NO);
   [dispatcher_ executeAfter:10
                       block:^{
                         [installationNotifier_
@@ -230,7 +217,7 @@ TEST_F(InstallationNotifierTest, RegisterAndThenUnregister) {
 }
 
 TEST_F(InstallationNotifierTest, TestExponentialBackoff) {
-  [sharedApplication_ setCanOpenURL:NO];
+  OCMStub([application_ canOpenURL:[OCMArg any]]).andReturn(NO);
   // Making sure that delay is multiplied by |multiplyFactor| every time.
   [dispatcher_ executeAfter:0
                       block:^{

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

@@ -79,6 +79,7 @@ source_set("unit_tests") {
     "//ios/chrome/browser/ui:ui_util",
     "//ios/chrome/browser/web:test_support",
     "//ios/chrome/test:test_support",
+    "//ios/chrome/test/app:test_support",
     "//ios/chrome/test/fakes",
     "//ios/testing:ios_test_support",
     "//ios/web/public/test",

ios/chrome/browser/ui/download/download_manager_coordinator.mm

@@ -7,10 +7,14 @@
 #include <memory>
 
 #import "base/logging.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
 #include "base/strings/sys_string_conversions.h"
 #include "components/strings/grit/components_strings.h"
+#include "ios/chrome/browser/download/download_manager_metric_names.h"
 #import "ios/chrome/browser/download/download_manager_tab_helper.h"
 #import "ios/chrome/browser/download/google_drive_app_util.h"
+#import "ios/chrome/browser/installation_notifier.h"
 #import "ios/chrome/browser/store_kit/store_kit_coordinator.h"
 #import "ios/chrome/browser/ui/download/download_manager_mediator.h"
 #import "ios/chrome/browser/ui/download/download_manager_view_controller.h"
@@ -45,6 +49,10 @@
 @synthesize animatesPresentation = _animatesPresentation;
 @synthesize downloadTask = _downloadTask;
 
+- (void)dealloc {
+  [[InstallationNotifier sharedInstance] unregisterForNotifications:self];
+}
+
 - (void)start {
   DCHECK(self.presenter);
 
@@ -171,6 +179,11 @@
   }
   [_storeKitCoordinator start];
   [controller setInstallDriveButtonVisible:NO animated:YES];
+
+  [[InstallationNotifier sharedInstance]
+      registerForInstallationNotifications:self
+                              withSelector:@selector(didInstallGoogleDriveApp)
+                                 forScheme:kGoogleDriveAppURLScheme];
 }
 
 - (void)downloadManagerViewControllerDidStartDownload:
@@ -236,4 +249,10 @@
                                       completion:nil];
 }
 
+// Called when Google Drive app is installed after starting StoreKitCoordinator.
+- (void)didInstallGoogleDriveApp {
+  base::RecordAction(
+      base::UserMetricsAction(kDownloadManagerGoogleDriveInstalled));
+}
+
 @end

ios/chrome/browser/ui/download/download_manager_coordinator_unittest.mm

@@ -11,8 +11,11 @@
 #include "base/mac/foundation_util.h"
 #include "base/run_loop.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/test/user_action_tester.h"
 #include "ios/chrome/browser/download/download_directory_util.h"
+#include "ios/chrome/browser/download/download_manager_metric_names.h"
 #import "ios/chrome/browser/download/download_manager_tab_helper.h"
+#import "ios/chrome/browser/download/google_drive_app_util.h"
 #import "ios/chrome/browser/ui/download/download_manager_view_controller.h"
 #import "ios/chrome/test/fakes/fake_contained_presenter.h"
 #import "ios/chrome/test/fakes/fake_document_interaction_controller.h"
@@ -78,15 +81,22 @@ class DownloadManagerCoordinatorTest : public PlatformTest {
   }
   ~DownloadManagerCoordinatorTest() override {
     [document_interaction_controller_class_ stopMocking];
+    [application_ stopMocking];
   }
 
+  web::TestWebThreadBundle thread_bundle_;
   FakeContainedPresenter* presenter_;
   UIViewController* base_view_controller_;
   ScopedKeyWindow scoped_key_window_;
   web::TestWebState web_state_;
   id document_interaction_controller_class_;
   StubTabHelper tab_helper_;
+  // Application can be lazily created by tests, but it has to be OCMock.
+  // Destructor will call -stopMocking on this object to make sure that
+  // UIApplication is not mocked after these test finish running.
+  id application_;
   DownloadManagerCoordinator* coordinator_;
+  base::UserActionTester user_action_tester_;
 };
 
 // Tests starting the coordinator. Verifies that view controller is presented
@@ -274,8 +284,11 @@ TEST_F(DownloadManagerCoordinatorTest, Close) {
   DownloadManagerViewController* viewController =
       base_view_controller_.childViewControllers.firstObject;
   ASSERT_EQ([DownloadManagerViewController class], [viewController class]);
-  [viewController.delegate
-      downloadManagerViewControllerDidClose:viewController];
+  @autoreleasepool {
+    // This call will retain coordinator, which should outlive thread bundle.
+    [viewController.delegate
+        downloadManagerViewControllerDidClose:viewController];
+  }
 
   // Verify that child view controller is removed, download task is set to null
   // and download task is cancelled.
@@ -305,9 +318,11 @@ TEST_F(DownloadManagerCoordinatorTest, InstallDrive) {
   // button changes it's alpha.
   ASSERT_EQ(1.0f, viewController.installDriveButton.superview.alpha);
 
-  [viewController.delegate
-      installDriveForDownloadManagerViewController:viewController];
-
+  @autoreleasepool {
+    // This call will retain coordinator, which should outlive thread bundle.
+    [viewController.delegate
+        installDriveForDownloadManagerViewController:viewController];
+  }
   // Verify that Store Kit dialog was presented.
   EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForUIElementTimeout, ^{
     return [base_view_controller_.presentedViewController class] ==
@@ -319,14 +334,27 @@ TEST_F(DownloadManagerCoordinatorTest, InstallDrive) {
     return viewController.installDriveButton.superview.alpha == 0.0f;
   }));
 
+  // Simulate Google Drive app installation and verify that expected user action
+  // has been recorded.
+  ASSERT_EQ(0, user_action_tester_.GetActionCount(
+                   kDownloadManagerGoogleDriveInstalled));
+  // SKStoreProductViewController uses UIApplication, so it's not possible to
+  // install the mock before the test run.
+  application_ = OCMClassMock([UIApplication class]);
+  OCMStub([application_ sharedApplication]).andReturn(application_);
+  OCMStub([application_ canOpenURL:GetGoogleDriveAppUrl()]).andReturn(YES);
+  EXPECT_TRUE(WaitUntilConditionOrTimeout(testing::kWaitForActionTimeout, ^{
+    base::RunLoop().RunUntilIdle();
+    return user_action_tester_.GetActionCount(
+               kDownloadManagerGoogleDriveInstalled) == 1;
+  }));
+
   // Stop to avoid holding a dangling pointer to destroyed task.
   [coordinator_ stop];
 }
 
 // Tests presenting Open In... menu.
 TEST_F(DownloadManagerCoordinatorTest, OpenIn) {
-  web::TestWebThreadBundle thread_bundle;
-
   web::FakeDownloadTask task(GURL(kTestUrl), kTestMimeType);
   task.SetSuggestedFilename(base::SysNSStringToUTF16(kTestSuggestedFileName));
   coordinator_.downloadTask = &task;
@@ -357,8 +385,11 @@ TEST_F(DownloadManagerCoordinatorTest, OpenIn) {
   UIView* view = [[UIView alloc] init];
   [view addLayoutGuide:guide];
   ASSERT_FALSE(document_interaction_controller.presentedOpenInMenu);
-  [viewController.delegate downloadManagerViewController:viewController
-                        presentOpenInMenuWithLayoutGuide:guide];
+  @autoreleasepool {
+    // This call will retain coordinator, which should outlive thread bundle.
+    [viewController.delegate downloadManagerViewController:viewController
+                          presentOpenInMenuWithLayoutGuide:guide];
+  }
   ASSERT_TRUE(document_interaction_controller.presentedOpenInMenu);
   ASSERT_TRUE(CGRectEqualToRect(
       CGRectZero, document_interaction_controller.presentedOpenInMenu.rect));
@@ -378,9 +409,11 @@ TEST_F(DownloadManagerCoordinatorTest, CloseInProgressDownload) {
   DownloadManagerViewController* viewController =
       base_view_controller_.childViewControllers.firstObject;
   ASSERT_EQ([DownloadManagerViewController class], [viewController class]);
-  [viewController.delegate
-      downloadManagerViewControllerDidClose:viewController];
-
+  @autoreleasepool {
+    // This call will retain coordinator, which should outlive thread bundle.
+    [viewController.delegate
+        downloadManagerViewControllerDidClose:viewController];
+  }
   // Verify that UIAlert is presented.
   ASSERT_TRUE([base_view_controller_.presentedViewController
       isKindOfClass:[UIAlertController class]]);
@@ -428,8 +461,6 @@ TEST_F(DownloadManagerCoordinatorTest, DecidePolicyForDownload) {
 // Tests starting the download. Verifies that download task is started and its
 // file writer is configured to write into download directory.
 TEST_F(DownloadManagerCoordinatorTest, StartDownload) {
-  web::TestWebThreadBundle thread_bundle;
-
   web::FakeDownloadTask task(GURL(kTestUrl), kTestMimeType);
   task.SetSuggestedFilename(base::SysNSStringToUTF16(kTestSuggestedFileName));
   web::DownloadTask* task_ptr = &task;
@@ -439,8 +470,11 @@ TEST_F(DownloadManagerCoordinatorTest, StartDownload) {
   DownloadManagerViewController* viewController =
       base_view_controller_.childViewControllers.firstObject;
   ASSERT_EQ([DownloadManagerViewController class], [viewController class]);
-  [viewController.delegate
-      downloadManagerViewControllerDidStartDownload:viewController];
+  @autoreleasepool {
+    // This call will retain coordinator, which should outlive thread bundle.
+    [viewController.delegate
+        downloadManagerViewControllerDidStartDownload:viewController];
+  }
 
   // Starting download is async for model.
   ASSERT_TRUE(WaitUntilConditionOrTimeout(testing::kWaitForDownloadTimeout, ^{

ios/chrome/browser/ui/main/browser_view_wrangler_unittest.mm

@@ -30,30 +30,33 @@ class BrowserViewWranglerTest : public PlatformTest {
 };
 
 TEST_F(BrowserViewWranglerTest, TestInitNilObserver) {
-  BrowserViewWrangler* wrangler = [[BrowserViewWrangler alloc]
-            initWithBrowserState:chrome_browser_state_.get()
-
-                tabModelObserver:nil
-      applicationCommandEndpoint:(id<ApplicationCommands>)nil];
-
-  // Test that BVC and tab model are created on demand.
-  BrowserViewController* bvc = [wrangler mainBVC];
-  EXPECT_NE(bvc, nil);
-
-  TabModel* tabModel = [wrangler mainTabModel];
-  EXPECT_NE(tabModel, nil);
-
-  // Test that once created the BVC and tab model aren't re-created.
-  EXPECT_EQ(bvc, [wrangler mainBVC]);
-  EXPECT_EQ(tabModel, [wrangler mainTabModel]);
-
-  // Test that the OTR objects are (a) OTR and (b) not the same as the non-OTR
-  // objects.
-  EXPECT_NE(bvc, [wrangler otrBVC]);
-  EXPECT_NE(tabModel, [wrangler otrTabModel]);
-  EXPECT_TRUE([wrangler otrTabModel].browserState->IsOffTheRecord());
-
-  [wrangler shutdown];
+  // |thread_bundle_| must outlive all objects created by BVC, because those
+  // objects may rely on threading API in dealloc.
+  @autoreleasepool {
+    BrowserViewWrangler* wrangler = [[BrowserViewWrangler alloc]
+              initWithBrowserState:chrome_browser_state_.get()
+
+                  tabModelObserver:nil
+        applicationCommandEndpoint:(id<ApplicationCommands>)nil];
+    // Test that BVC and tab model are created on demand.
+    BrowserViewController* bvc = [wrangler mainBVC];
+    EXPECT_NE(bvc, nil);
+
+    TabModel* tabModel = [wrangler mainTabModel];
+    EXPECT_NE(tabModel, nil);
+
+    // Test that once created the BVC and tab model aren't re-created.
+    EXPECT_EQ(bvc, [wrangler mainBVC]);
+    EXPECT_EQ(tabModel, [wrangler mainTabModel]);
+
+    // Test that the OTR objects are (a) OTR and (b) not the same as the non-OTR
+    // objects.
+    EXPECT_NE(bvc, [wrangler otrBVC]);
+    EXPECT_NE(tabModel, [wrangler otrTabModel]);
+    EXPECT_TRUE([wrangler otrTabModel].browserState->IsOffTheRecord());
+
+    [wrangler shutdown];
+  }
 }
 
 }  // namespace