Record launch time for Chrome app

Without this Chrome is pushed out of the list of suggested apps
very soon after the profile creation and never comes back even
after it is launched.

Bug: 894898
Test: manual
Change-Id: I7d12958c1522336bf831d31b9288a6084cc5f974
Reviewed-on: https://chromium-review.googlesource.com/c/1338472
Commit-Queue: Vladislav Kaznacheev <kaznacheev@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612868}

chrome/browser/ui/app_list/app_list_client_impl_browsertest.cc

@@ -38,6 +38,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/common/constants.h"
 #include "ui/aura/window.h"
 #include "ui/base/models/menu_model.h"
@@ -138,6 +139,61 @@ IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest, ShowContextMenu) {
   }
 }
 
+// Test that browser launch time is recorded is recorded in preferences.
+// This is important for suggested apps sorting.
+IN_PROC_BROWSER_TEST_F(AppListClientImplBrowserTest,
+                       BrowserLaunchTimeRecorded) {
+  AppListClientImpl* client = AppListClientImpl::GetInstance();
+  AppListControllerDelegate* controller = client;
+  ASSERT_TRUE(controller);
+
+  Profile* profile = browser()->profile();
+  Profile* profile_otr = profile->GetOffTheRecordProfile();
+
+  extensions::ExtensionPrefs* prefs = extensions::ExtensionPrefs::Get(profile);
+
+  // Starting with just one regular browser.
+  EXPECT_EQ(1U, chrome::GetBrowserCount(profile));
+  EXPECT_EQ(0U, chrome::GetBrowserCount(profile_otr));
+
+  // First browser launch time should be recorded.
+  const base::Time time_recorded1 =
+      prefs->GetLastLaunchTime(extension_misc::kChromeAppId);
+  EXPECT_NE(base::Time(), time_recorded1);
+
+  // Create an incognito browser so that we can close the regular one without
+  // exiting the test.
+  controller->CreateNewWindow(profile, true);
+  EXPECT_EQ(1U, chrome::GetBrowserCount(profile_otr));
+  // Creating incognito browser should not update the launch time.
+  EXPECT_EQ(time_recorded1,
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
+
+  // Close the regular browser.
+  CloseBrowserSynchronously(chrome::FindBrowserWithProfile(profile));
+  EXPECT_EQ(0U, chrome::GetBrowserCount(profile));
+  // Recorded the launch time should not update.
+  EXPECT_EQ(time_recorded1,
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
+
+  // Launch another regular browser.
+  const base::Time time_before_launch = base::Time::Now();
+  controller->CreateNewWindow(profile, false);
+  const base::Time time_after_launch = base::Time::Now();
+  EXPECT_EQ(1U, chrome::GetBrowserCount(profile));
+
+  const base::Time time_recorded2 =
+      prefs->GetLastLaunchTime(extension_misc::kChromeAppId);
+  EXPECT_LE(time_before_launch, time_recorded2);
+  EXPECT_GE(time_after_launch, time_recorded2);
+
+  // Creating a second regular browser should not update the launch time.
+  controller->CreateNewWindow(profile, false);
+  EXPECT_EQ(2U, chrome::GetBrowserCount(profile));
+  EXPECT_EQ(time_recorded2,
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
+}
+
 // Browser Test for AppListClient that observes search result changes.
 using AppListClientSearchResultsBrowserTest = extensions::ExtensionBrowserTest;
 

chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/settings_window_manager_chromeos.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/chromeos/login/discover/discover_window_manager.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/generated_resources.h"
@@ -32,6 +33,7 @@
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
+#include "extensions/browser/extension_prefs.h"
 #include "ui/aura/window.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -98,6 +100,58 @@ base::string16 GetBrowserListTitle(content::WebContents* web_contents) {
   return title.empty() ? l10n_util::GetStringUTF16(IDS_NEW_TAB_TITLE) : title;
 }
 
+// Returns true when the given |browser| is listed in the browser application
+// list.
+bool IsBrowserRepresentedInBrowserList(Browser* browser) {
+  // Only Ash desktop browser windows for the active user are represented.
+  if (!browser || !multi_user_util::IsProfileFromActiveUser(browser->profile()))
+    return false;
+
+  if (browser->is_app() && browser->is_type_popup()) {
+    // Crostini Terminals always have their own item.
+    // TODO(rjwright): We shouldn't need to special-case Crostini here.
+    // https://crbug.com/846546
+    if (crostini::CrostiniAppIdFromAppName(browser->app_name()))
+      return false;
+
+    // V1 App popup windows may have their own item.
+    ash::ShelfID id(web_app::GetAppIdFromApplicationName(browser->app_name()));
+    if (ChromeLauncherController::instance()->GetItem(id) != nullptr)
+      return false;
+  }
+
+  // Settings and Discover browsers have their own item; all others should be
+  // represented.
+  return !IsSettingsBrowser(browser) &&
+         !chromeos::DiscoverWindowManager::GetInstance()->IsDiscoverBrowser(
+             browser);
+}
+
+// Gets a list of active browsers.
+BrowserList::BrowserVector GetListOfActiveBrowsers() {
+  BrowserList::BrowserVector active_browsers;
+  for (auto* browser : *BrowserList::GetInstance()) {
+    // Make sure that the browser is from the current user, has a proper window,
+    // and the window was already shown.
+    if (!multi_user_util::IsProfileFromActiveUser(browser->profile()))
+      continue;
+    if (!browser->window()->GetNativeWindow()->IsVisible() &&
+        !browser->window()->IsMinimized()) {
+      continue;
+    }
+    if (!IsBrowserRepresentedInBrowserList(browser) &&
+        !browser->is_type_tabbed())
+      continue;
+    active_browsers.push_back(browser);
+  }
+  return active_browsers;
+}
+
+bool ShouldRecordLaunchTime(Browser* browser) {
+  return !browser->profile()->IsOffTheRecord() &&
+         IsBrowserRepresentedInBrowserList(browser);
+}
+
 }  // namespace
 
 BrowserShortcutLauncherItemController::BrowserShortcutLauncherItemController(
@@ -105,6 +159,7 @@ BrowserShortcutLauncherItemController::BrowserShortcutLauncherItemController(
     : ash::ShelfItemDelegate(ash::ShelfID(extension_misc::kChromeAppId)),
       shelf_model_(shelf_model),
       browser_list_observer_(this) {
+  browser_list_observer_.Add(BrowserList::GetInstance());
   // Tag all open browser windows with the appropriate shelf id property. This
   // associates each window with the shelf item for the active web contents.
   for (auto* browser : *BrowserList::GetInstance()) {
@@ -199,7 +254,6 @@ void BrowserShortcutLauncherItemController::ItemSelected(
 ash::MenuItemList BrowserShortcutLauncherItemController::GetAppMenuItems(
     int event_flags) {
   browser_menu_items_.clear();
-  browser_list_observer_.RemoveAll();
 
   ash::MenuItemList items;
   bool found_tabbed_browser = false;
@@ -231,15 +285,12 @@ ash::MenuItemList BrowserShortcutLauncherItemController::GetAppMenuItems(
       }
     }
     browser_menu_items_.push_back(browser);
-    if (!browser_list_observer_.IsObservingSources())
-      browser_list_observer_.Add(BrowserList::GetInstance());
   }
   // If only windowed applications are open, we return an empty list to
   // enforce the creation of a new browser.
   if (!found_tabbed_browser) {
     items.clear();
     browser_menu_items_.clear();
-    browser_list_observer_.RemoveAll();
   }
   return items;
 }
@@ -286,7 +337,6 @@ void BrowserShortcutLauncherItemController::ExecuteCommand(
   }
 
   browser_menu_items_.clear();
-  browser_list_observer_.RemoveAll();
 }
 
 void BrowserShortcutLauncherItemController::Close() {
@@ -294,6 +344,7 @@ void BrowserShortcutLauncherItemController::Close() {
     browser->window()->Close();
 }
 
+// static
 bool BrowserShortcutLauncherItemController::IsListOfActiveBrowserEmpty() {
   return GetListOfActiveBrowsers().empty();
 }
@@ -346,47 +397,21 @@ BrowserShortcutLauncherItemController::ActivateOrAdvanceToNextBrowser() {
   return ash::SHELF_ACTION_WINDOW_ACTIVATED;
 }
 
-bool BrowserShortcutLauncherItemController::IsBrowserRepresentedInBrowserList(
-    Browser* browser) {
-  // Only Ash desktop browser windows for the active user are represented.
-  if (!browser || !multi_user_util::IsProfileFromActiveUser(browser->profile()))
-    return false;
-
-  if (browser->is_app() && browser->is_type_popup()) {
-    // Crostini Terminals always have their own item.
-    // TODO(rjwright): We shouldn't need to special-case Crostini here.
-    // https://crbug.com/846546
-    if (crostini::CrostiniAppIdFromAppName(browser->app_name()))
-      return false;
-
-    // V1 App popup windows may have their own item.
-    ash::ShelfID id(web_app::GetAppIdFromApplicationName(browser->app_name()));
-    if (ChromeLauncherController::instance()->GetItem(id) != nullptr)
-      return false;
-  }
-
-  // Settings browsers have their own item; all others should be represented.
-  return !IsSettingsBrowser(browser);
-}
+void BrowserShortcutLauncherItemController::OnBrowserAdded(Browser* browser) {
+  if (!ShouldRecordLaunchTime(browser))
+    return;
 
-BrowserList::BrowserVector
-BrowserShortcutLauncherItemController::GetListOfActiveBrowsers() {
-  BrowserList::BrowserVector active_browsers;
-  for (auto* browser : *BrowserList::GetInstance()) {
-    // Make sure that the browser is from the current user, has a proper window,
-    // and the window was already shown.
-    if (!multi_user_util::IsProfileFromActiveUser(browser->profile()))
-      continue;
-    if (!browser->window()->GetNativeWindow()->IsVisible() &&
-        !browser->window()->IsMinimized()) {
-      continue;
-    }
-    if (!IsBrowserRepresentedInBrowserList(browser) &&
-        !browser->is_type_tabbed())
+  const BrowserList* browser_list = BrowserList::GetInstance();
+  for (BrowserList::const_iterator it = browser_list->begin();
+       it != browser_list->end(); ++it) {
+    if (*it == browser)
       continue;
-    active_browsers.push_back(browser);
+    if (ShouldRecordLaunchTime(*it))
+      return;
   }
-  return active_browsers;
+
+  extensions::ExtensionPrefs::Get(browser->profile())
+      ->SetLastLaunchTime(shelf_id().app_id, base::Time::Now());
 }
 
 void BrowserShortcutLauncherItemController::OnBrowserClosing(Browser* browser) {

chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.h

@@ -40,7 +40,7 @@ class BrowserShortcutLauncherItemController : public ash::ShelfItemDelegate,
                                           content::WebContents* web_contents);
 
   // Check if there is any active browsers windows.
-  bool IsListOfActiveBrowserEmpty();
+  static bool IsListOfActiveBrowserEmpty();
 
   // ash::ShelfItemDelegate overrides:
   void ItemSelected(std::unique_ptr<ui::Event> event,
@@ -62,14 +62,8 @@ class BrowserShortcutLauncherItemController : public ash::ShelfItemDelegate,
   // SHELF_ACTION_WINDOW_ACTIVATED, or SHELF_ACTION_NEW_WINDOW_CREATED.
   ash::ShelfAction ActivateOrAdvanceToNextBrowser();
 
-  // Returns true when the given |browser| is listed in the browser application
-  // list.
-  bool IsBrowserRepresentedInBrowserList(Browser* browser);
-
-  // Get a list of active browsers.
-  BrowserList::BrowserVector GetListOfActiveBrowsers();
-
   // BrowserListObserver:
+  void OnBrowserAdded(Browser* browser) override;
   void OnBrowserClosing(Browser* browser) override;
 
   ash::ShelfModel* shelf_model_;
@@ -77,7 +71,7 @@ class BrowserShortcutLauncherItemController : public ash::ShelfItemDelegate,
   // The cached list of open browser windows shown in an application menu.
   BrowserList::BrowserVector browser_menu_items_;
 
-  // Observer for browser windows closing events.
+  // Observer for browser windows adding and closing events.
   ScopedObserver<BrowserList, BrowserListObserver> browser_list_observer_;
 
   std::unique_ptr<LauncherContextMenu> context_menu_;

chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc

@@ -62,6 +62,7 @@
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/app_window/native_app_window.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/switches.h"
@@ -1691,17 +1692,30 @@ IN_PROC_BROWSER_TEST_F(ShelfAppBrowserTestNoDefaultBrowser,
   const ash::ShelfID browser_id = item_controller->shelf_id();
   EXPECT_EQ(extension_misc::kChromeAppId, browser_id.app_id);
 
+  extensions::ExtensionPrefs* prefs =
+      extensions::ExtensionPrefs::Get(profile());
+
   // Get the number of browsers.
   size_t running_browser = chrome::GetTotalBrowserCount();
   EXPECT_EQ(0u, running_browser);
   EXPECT_FALSE(controller_->IsOpen(browser_id));
+  // No launch time recorded for Chrome yet.
+  EXPECT_EQ(base::Time(),
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
 
   // Activate. This creates new browser
+  base::Time time_before_launch = base::Time::Now();
   SelectItem(browser_id, ui::ET_UNKNOWN);
+  base::Time time_after_launch = base::Time::Now();
   // New Window is created.
   running_browser = chrome::GetTotalBrowserCount();
   EXPECT_EQ(1u, running_browser);
   EXPECT_TRUE(controller_->IsOpen(browser_id));
+  // Valid launch time should be recorded for Chrome.
+  const base::Time time_launch =
+      prefs->GetLastLaunchTime(extension_misc::kChromeAppId);
+  EXPECT_LE(time_before_launch, time_launch);
+  EXPECT_GE(time_after_launch, time_launch);
 
   // Minimize Window.
   Browser* browser = chrome::FindLastActive();
@@ -1715,6 +1729,29 @@ IN_PROC_BROWSER_TEST_F(ShelfAppBrowserTestNoDefaultBrowser,
   EXPECT_EQ(1u, running_browser);
   EXPECT_TRUE(controller_->IsOpen(browser_id));
   EXPECT_FALSE(browser->window()->IsMinimized());
+  // Re-activation should not upate the recorded launch time.
+  EXPECT_GE(time_launch,
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
+}
+
+// Check that browser launch time is recorded when the browser is started
+// by means other than BrowserShortcutLauncherItemController.
+IN_PROC_BROWSER_TEST_F(ShelfAppBrowserTestNoDefaultBrowser,
+                       BrowserLaunchTimeRecorded) {
+  extensions::ExtensionPrefs* prefs =
+      extensions::ExtensionPrefs::Get(profile());
+
+  EXPECT_EQ(0u, chrome::GetTotalBrowserCount());
+  EXPECT_EQ(base::Time(),
+            prefs->GetLastLaunchTime(extension_misc::kChromeAppId));
+
+  base::Time time_before_launch = base::Time::Now();
+  chrome::NewEmptyWindow(profile());
+  base::Time time_after_launch = base::Time::Now();
+  const base::Time time_launch =
+      prefs->GetLastLaunchTime(extension_misc::kChromeAppId);
+  EXPECT_LE(time_before_launch, time_launch);
+  EXPECT_GE(time_after_launch, time_launch);
 }
 
 // Check that the window's ShelfID property matches that of the active tab.

chrome/browser/ui/ash/launcher/extension_launcher_context_menu.cc

@@ -193,8 +193,7 @@ void ExtensionLauncherContextMenu::BuildMenu(ui::SimpleMenuModel* menu_model) {
       AddContextMenuOption(menu_model, ash::MENU_NEW_INCOGNITO_WINDOW,
                            IDS_APP_LIST_NEW_INCOGNITO_WINDOW);
     }
-    if (!BrowserShortcutLauncherItemController(controller()->shelf_model())
-             .IsListOfActiveBrowserEmpty()) {
+    if (!BrowserShortcutLauncherItemController::IsListOfActiveBrowserEmpty()) {
       AddContextMenuOption(menu_model, ash::MENU_CLOSE,
                            IDS_LAUNCHER_CONTEXT_MENU_CLOSE);
     }