apps: Update ranking for apps installed internally.

App ranking system uses install time in case launch time is not
available to rank apps. We don't want apps, installed by policy or by
default appear in recent app. This CL extends app ranking system by
handling such apps. Previously we had ARC apps folled by this approach
by not-setting install time installed internally apps. However this was
not good once install time should not depend on ranking system. So this
CL makes ranking centralized and switch ARC and Extension apps to it.

TEST=Manually + unit tests added.
BUG=757639

Change-Id: If567e7e72908c0186e3096b1e83fa917df47d082
Reviewed-on: https://chromium-review.googlesource.com/1188591
Commit-Queue: Yury Khmel <khmel@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586071}

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

@@ -740,6 +740,11 @@ bool ArcAppListPrefs::IsShortcut(const std::string& app_id) const {
   return app_info && app_info->shortcut;
 }
 
+bool ArcAppListPrefs::IsControlledByPolicy(
+    const std::string& package_name) const {
+  return packages_by_policy_.count(package_name);
+}
+
 void ArcAppListPrefs::SetLastLaunchTime(const std::string& app_id) {
   if (!IsRegistered(app_id)) {
     NOTREACHED();
@@ -1045,7 +1050,7 @@ void ArcAppListPrefs::AddAppAndShortcut(const std::string& name,
 
   // Note the install time is the first time the Chrome OS sees the app, not the
   // actual install time in Android side.
-  if (GetInstallTime(app_id).is_null() && NeedSetInstallTime(package_name)) {
+  if (GetInstallTime(app_id).is_null()) {
     std::string install_time_str =
         base::Int64ToString(base::Time::Now().ToInternalValue());
     app_dict->SetString(kInstallTime, install_time_str);
@@ -1285,24 +1290,6 @@ void ArcAppListPrefs::InvalidatePackageIcons(const std::string& package_name) {
     InvalidateAppIcons(app_id);
 }
 
-bool ArcAppListPrefs::NeedSetInstallTime(
-    const std::string& package_name) const {
-  // If checked package is in active default list that means it is installed by
-  // PAI and install time should not be recorded. Once package is not in active
-  // default list then this package was removed from default and user installs
-  // it manually. In last case we have to record install time.
-  if (default_apps_.GetActivePackages().count(package_name))
-    return false;
-
-  // Check if package is installed by policy. In this case don't set install
-  // time.
-  if (packages_by_policy_.count(package_name))
-    return false;
-
-  // TODO(b/34248841) - Handle apps, installed by sync.
-  return true;
-}
-
 void ArcAppListPrefs::ScheduleAppFolderDeletion(const std::string& app_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   base::PostTaskWithTraits(

chrome/browser/ui/app_list/arc/arc_app_list_prefs.h

@@ -260,6 +260,8 @@ class ArcAppListPrefs : public KeyedService,
   bool IsOem(const std::string& app_id) const;
   // Returns true if app is a shortcut
   bool IsShortcut(const std::string& app_id) const;
+  // Returns true if package is controlled by policy.
+  bool IsControlledByPolicy(const std::string& package_name) const;
 
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
@@ -467,12 +469,6 @@ class ArcAppListPrefs : public KeyedService,
   // Marks package icons as invalidated and request icons updated.
   void InvalidatePackageIcons(const std::string& package_name);
 
-  // Returns true if install time has to be set to current time for the newly
-  // installed app from the |package_name|. App launcher uses install time to
-  // rank apps. Do not set install time for apps, installed by default or by
-  // policy.
-  bool NeedSetInstallTime(const std::string& package_name) const;
-
   // Extracts app info from the prefs without any ARC availability check.
   // Returns null if app is not registered.
   std::unique_ptr<AppInfo> GetAppFromPrefs(const std::string& app_id) const;

chrome/browser/ui/app_list/arc/arc_app_unittest.cc

@@ -1256,30 +1256,6 @@ TEST_P(ArcAppModelBuilderTest, InstallTime) {
   EXPECT_LE(app_info->install_time, base::Time::Now());
 }
 
-// Makes sure that install time is not set when installed by policy.
-TEST_P(ArcAppModelBuilderTest, InstallTimeForPolicyApps) {
-  ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
-  ASSERT_TRUE(prefs);
-
-  ASSERT_TRUE(fake_apps().size());
-
-  const std::string app_id = ArcAppTest::GetAppId(fake_apps()[0]);
-  EXPECT_FALSE(prefs->GetApp(app_id));
-
-  const std::string policy = base::StringPrintf(
-      "{\"applications\":[{\"installType\":\"FORCE_INSTALLED\",\"packageName\":"
-      "\"%s\"}]}",
-      fake_apps()[0].package_name.c_str());
-  prefs->OnPolicySent(policy);
-
-  app_instance()->RefreshAppList();
-  app_instance()->SendRefreshAppList(fake_apps());
-
-  std::unique_ptr<ArcAppListPrefs::AppInfo> app_info = prefs->GetApp(app_id);
-  ASSERT_TRUE(app_info);
-  EXPECT_EQ(base::Time(), app_info->install_time);
-}
-
 TEST_P(ArcAppModelBuilderTest, AppLifeCycleEventsOnOptOut) {
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_.get());
   ASSERT_TRUE(prefs);
@@ -2191,6 +2167,7 @@ TEST_P(ArcDefaulAppTest, DefaultApps) {
         prefs->GetApp(ArcAppTest::GetAppId(default_app));
     ASSERT_TRUE(app_info);
     EXPECT_FALSE(app_info->ready);
+    EXPECT_NE(base::Time(), app_info->install_time);
   }
 
   // Install default apps.
@@ -2200,11 +2177,10 @@ TEST_P(ArcDefaulAppTest, DefaultApps) {
     app_instance()->SendPackageAppListRefreshed(default_app.package_name,
                                                 package_apps);
 
-    // Install time is not set for installed default apps.
     std::unique_ptr<ArcAppListPrefs::AppInfo> app_info =
         prefs->GetApp(ArcAppTest::GetAppId(default_app));
     ASSERT_TRUE(app_info);
-    EXPECT_EQ(base::Time(), app_info->install_time);
+    EXPECT_NE(base::Time(), app_info->install_time);
   }
 
   // And now default apps are ready.

chrome/browser/ui/app_list/search/app_search_provider.cc

@@ -119,12 +119,14 @@ class AppSearchProvider::App {
       const std::string& id,
       const std::string& name,
       const base::Time& last_launch_time,
-      const base::Time& install_time)
+      const base::Time& install_time,
+      bool installed_internally)
       : data_source_(data_source),
         id_(id),
         name_(base::UTF8ToUTF16(name)),
         last_launch_time_(last_launch_time),
-        install_time_(install_time) {}
+        install_time_(install_time),
+        installed_internally_(installed_internally) {}
   ~App() = default;
 
   struct CompareByLastActivityTime {
@@ -143,8 +145,12 @@ class AppSearchProvider::App {
     return tokenized_indexed_name_.get();
   }
 
-  const base::Time& GetLastActivityTime() const {
-    return last_launch_time_.is_null() ? install_time_ : last_launch_time_;
+  base::Time GetLastActivityTime() const {
+    if (!last_launch_time_.is_null())
+      return last_launch_time_;
+    if (!installed_internally_)
+      return install_time_;
+    return base::Time();
   }
 
   bool MatchSearchableText(const TokenizedString& query) {
@@ -180,6 +186,8 @@ class AppSearchProvider::App {
     require_exact_match_ = require_exact_match;
   }
 
+  bool installed_internally() const { return installed_internally_; }
+
  private:
   AppSearchProvider::DataSource* data_source_;
   std::unique_ptr<TokenizedString> tokenized_indexed_name_;
@@ -192,6 +200,9 @@ class AppSearchProvider::App {
   bool searchable_ = true;
   base::string16 searchable_text_;
   bool require_exact_match_ = false;
+  // Set to true in case app was installed internally, by sync, policy or as a
+  // default app.
+  const bool installed_internally_;
 
   DISALLOW_COPY_AND_ASSIGN(App);
 };
@@ -282,7 +293,12 @@ class ExtensionDataSource : public AppSearchProvider::DataSource,
       apps->emplace_back(std::make_unique<AppSearchProvider::App>(
           this, extension->id(), extension->short_name(),
           prefs->GetLastLaunchTime(extension->id()),
-          prefs->GetInstallTime(extension->id())));
+          prefs->GetInstallTime(extension->id()),
+          extension->was_installed_by_default() ||
+              extension->was_installed_by_oem() ||
+              extensions::Manifest::IsComponentLocation(
+                  extension->location()) ||
+              extensions::Manifest::IsPolicyLocation(extension->location())));
     }
   }
 
@@ -324,7 +340,9 @@ class ArcDataSource : public AppSearchProvider::DataSource,
 
       apps->emplace_back(std::make_unique<AppSearchProvider::App>(
           this, app_id, app_info->name, app_info->last_launch_time,
-          app_info->install_time));
+          app_info->install_time,
+          arc_prefs->IsDefault(app_id) ||
+              arc_prefs->IsControlledByPolicy(app_info->package_name)));
     }
   }
 
@@ -380,7 +398,6 @@ class InternalDataSource : public AppSearchProvider::DataSource,
 
   // AppSearchProvider::DataSource overrides:
   void AddApps(AppSearchProvider::Apps* apps) override {
-    const base::Time time;
     for (const auto& internal_app :
          GetInternalAppList(profile()->IsGuestSession())) {
       if (!std::strcmp(internal_app.app_id, kInternalAppIdContinueReading) &&
@@ -392,8 +409,9 @@ class InternalDataSource : public AppSearchProvider::DataSource,
 
       apps->emplace_back(std::make_unique<AppSearchProvider::App>(
           this, internal_app.app_id,
-          l10n_util::GetStringUTF8(internal_app.name_string_resource_id), time,
-          time));
+          l10n_util::GetStringUTF8(internal_app.name_string_resource_id),
+          base::Time() /* last_launch_time */, base::Time() /* install_time */,
+          true /* installed_internally */));
       apps->back()->set_recommendable(internal_app.recommendable);
       apps->back()->set_searchable(internal_app.searchable);
       if (internal_app.searchable_string_resource_id != 0) {
@@ -447,7 +465,7 @@ class CrostiniDataSource : public AppSearchProvider::DataSource,
       // the 'Keywords' desktop entry field and the executable file name.
       apps->emplace_back(std::make_unique<AppSearchProvider::App>(
           this, app_id, registration.Name(), registration.LastLaunchTime(),
-          registration.InstallTime()));
+          registration.InstallTime(), false /* installed_internally */));
 
       // Until it's been installed, the Terminal is hidden unless you search
       // for 'Terminal' exactly (case insensitive).

chrome/browser/ui/app_list/search/tests/app_search_provider_unittest.cc

@@ -30,6 +30,7 @@
 #include "chrome/common/chrome_constants.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/arc/test/fake_app_instance.h"
+#include "components/crx_file/id_util.h"
 #include "components/sync/model/string_ordinal.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/uninstall_reason.h"
@@ -49,8 +50,29 @@ constexpr char kGmailArcPackage[] = "com.google.android.gm";
 constexpr char kGmailArcActivity[] =
     "com.google.android.gm.ConversationListActivityGmail";
 constexpr char kKeyboardShortcutHelperInternalName[] = "Shortcuts";
+
+constexpr char kRankingAppQuery[] = "testRankingApp";
+
+// Activity and package should match
+// chrome/test/data/arc_default_apps/test_app1.json
+constexpr char kRankingInternalAppActivity[] = "test.app1.activity";
+constexpr char kRankingInternalAppName[] = "testRankingAppInternal";
+constexpr char kRankingInternalAppPackageName[] = "test.app1";
+
+constexpr char kRankingNormalAppActivity[] = "test.ranking.app.normal.activity";
+constexpr char kRankingNormalAppName[] = "testRankingAppNormal";
+constexpr char kRankingNormalAppPackageName[] = "test.ranking.app.normal";
+
 constexpr char kSettingsInternalName[] = "Settings";
 
+// Waits for base::Time::Now() is updated.
+void WaitTimeUpdated() {
+  base::RunLoop run_loop;
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(), base::TimeDelta::FromMilliseconds(1));
+  run_loop.Run();
+}
+
 }  // namespace
 
 const base::Time kTestCurrentTime = base::Time::FromInternalValue(100000);
@@ -111,7 +133,10 @@ class AppSearchProviderTest : public AppListTestBase {
     return ArcAppListPrefs::GetAppId(package, activity);
   }
 
-  void AddExtension(const std::string& id, const std::string& name) {
+  void AddExtension(const std::string& id,
+                    const std::string& name,
+                    extensions::Manifest::Location location,
+                    int extra_flags) {
     scoped_refptr<extensions::Extension> extension =
         extensions::ExtensionBuilder()
             .SetManifest(
@@ -135,9 +160,16 @@ class AppSearchProviderTest : public AppListTestBase {
                                       .Build())
                              .Build())
                     .Build())
+            .SetLocation(location)
+            .AddFlags(extra_flags)
             .SetID(id)
             .Build();
     service()->AddExtension(extension.get());
+    // This sets install time and other extension parameters.
+    extensions::ExtensionPrefs::Get(profile())->OnExtensionInstalled(
+        extension.get(), extensions::Extension::ENABLED,
+        syncer::StringOrdinal::CreateInitialOrdinal() /* page_ordinal */,
+        std::string() /* install_parameter */);
   }
 
   const SearchProvider::Results& results() { return app_search_->results(); }
@@ -354,7 +386,9 @@ TEST_F(AppSearchProviderTest, FilterDuplicate) {
       extensions::ExtensionPrefs::Get(profile_.get());
   ASSERT_TRUE(extension_prefs);
 
-  AddExtension(extension_misc::kGmailAppId, kGmailExtensionName);
+  AddExtension(extension_misc::kGmailAppId, kGmailExtensionName,
+               extensions::Manifest::EXTERNAL_PREF_DOWNLOAD,
+               0 /* extra_flags */);
   const std::string arc_gmail_app_id =
       AddArcApp(kGmailArcName, kGmailArcPackage, kGmailArcActivity);
   arc_test().arc_app_list_prefs()->SetLastLaunchTime(arc_gmail_app_id);
@@ -394,5 +428,177 @@ TEST_F(AppSearchProviderTest, FetchInternalApp) {
   EXPECT_EQ(kSettingsInternalName, RunQuery("Set"));
 }
 
+enum class TestExtensionInstallType {
+  CONTROLLED_BY_POLICY,
+  CHROME_COMPONENT,
+  INSTALLED_BY_DEFAULT,
+  INSTALLED_BY_OEM,
+};
+
+class AppSearchProviderWithExtensionInstallType
+    : public AppSearchProviderTest,
+      public ::testing::WithParamInterface<TestExtensionInstallType> {
+ public:
+  AppSearchProviderWithExtensionInstallType() = default;
+  ~AppSearchProviderWithExtensionInstallType() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AppSearchProviderWithExtensionInstallType);
+};
+
+TEST_P(AppSearchProviderWithExtensionInstallType, InstallInernallyRanking) {
+  extensions::ExtensionPrefs* const prefs =
+      extensions::ExtensionPrefs::Get(profile());
+  ASSERT_TRUE(prefs);
+
+  // Install normal app.
+  const std::string normal_app_id =
+      crx_file::id_util::GenerateId(kRankingNormalAppName);
+  AddExtension(normal_app_id, kRankingNormalAppName,
+               extensions::Manifest::EXTERNAL_PREF_DOWNLOAD,
+               0 /* extra_flags */);
+
+  // Wait a bit to make sure time is updated.
+  WaitTimeUpdated();
+
+  // Install app internally.
+  const std::string internal_app_id =
+      crx_file::id_util::GenerateId(kRankingInternalAppName);
+  switch (GetParam()) {
+    case TestExtensionInstallType::CONTROLLED_BY_POLICY:
+      AddExtension(internal_app_id, kRankingInternalAppName,
+                   extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD,
+                   0 /* extra_flags */);
+      break;
+    case TestExtensionInstallType::CHROME_COMPONENT:
+      AddExtension(internal_app_id, kRankingInternalAppName,
+                   extensions::Manifest::COMPONENT, 0 /* extra_flags */);
+      break;
+    case TestExtensionInstallType::INSTALLED_BY_DEFAULT:
+      AddExtension(internal_app_id, kRankingInternalAppName,
+                   extensions::Manifest::EXTERNAL_PREF_DOWNLOAD,
+                   extensions::Extension::WAS_INSTALLED_BY_DEFAULT);
+      break;
+    case TestExtensionInstallType::INSTALLED_BY_OEM:
+      AddExtension(internal_app_id, kRankingInternalAppName,
+                   extensions::Manifest::EXTERNAL_PREF_DOWNLOAD,
+                   extensions::Extension::WAS_INSTALLED_BY_OEM);
+      break;
+  }
+
+  EXPECT_LT(prefs->GetInstallTime(normal_app_id),
+            prefs->GetInstallTime(internal_app_id));
+
+  // Installed internally app has runking below other apps, even if it's install
+  // time is later.
+  CreateSearch();
+  EXPECT_EQ(std::string(kRankingNormalAppName) + "," +
+                std::string(kRankingInternalAppName),
+            RunQuery(kRankingAppQuery));
+
+  // Using installed internally app moves it in ranking up.
+  WaitTimeUpdated();
+  prefs->SetLastLaunchTime(internal_app_id, base::Time::Now());
+  CreateSearch();
+  EXPECT_EQ(std::string(kRankingInternalAppName) + "," +
+                std::string(kRankingNormalAppName),
+            RunQuery(kRankingAppQuery));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,
+    AppSearchProviderWithExtensionInstallType,
+    ::testing::ValuesIn({TestExtensionInstallType::CONTROLLED_BY_POLICY,
+                         TestExtensionInstallType::CHROME_COMPONENT,
+                         TestExtensionInstallType::INSTALLED_BY_DEFAULT,
+                         TestExtensionInstallType::INSTALLED_BY_OEM}));
+
+enum class TestArcAppInstallType {
+  CONTROLLED_BY_POLICY,
+  INSTALLED_BY_DEFAULT,
+};
+
+class AppSearchProviderWithArcAppInstallType
+    : public AppSearchProviderTest,
+      public ::testing::WithParamInterface<TestArcAppInstallType> {
+ public:
+  AppSearchProviderWithArcAppInstallType() = default;
+  ~AppSearchProviderWithArcAppInstallType() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AppSearchProviderWithArcAppInstallType);
+};
+
+TEST_P(AppSearchProviderWithArcAppInstallType, InstallInernallyRanking) {
+  const bool default_app =
+      GetParam() == TestArcAppInstallType::INSTALLED_BY_DEFAULT;
+  if (default_app) {
+    ArcDefaultAppList::UseTestAppsDirectory();
+    arc_test().set_wait_default_apps(true);
+  }
+  arc_test().SetUp(profile());
+
+  ArcAppListPrefs* const prefs = arc_test().arc_app_list_prefs();
+  ASSERT_TRUE(prefs);
+
+  // Install normal app.
+  const std::string normal_app_id =
+      AddArcApp(kRankingNormalAppName, kRankingNormalAppPackageName,
+                kRankingNormalAppActivity);
+
+  // Wait a bit to make sure time is updated.
+  WaitTimeUpdated();
+
+  if (GetParam() == TestArcAppInstallType::CONTROLLED_BY_POLICY) {
+    const std::string policy = base::StringPrintf(
+        "{\"applications\":[{\"installType\":\"FORCE_INSTALLED\","
+        "\"packageName\":"
+        "\"%s\"}]}",
+        kRankingInternalAppPackageName);
+    prefs->OnPolicySent(policy);
+  }
+
+  // Reinstall default app to make install time after normall app install time.
+  if (default_app) {
+    static_cast<arc::mojom::AppHost*>(prefs)->OnPackageAppListRefreshed(
+        kRankingInternalAppPackageName, {} /* apps */);
+  }
+
+  const std::string internal_app_id =
+      AddArcApp(kRankingInternalAppName, kRankingInternalAppPackageName,
+                kRankingInternalAppActivity);
+
+  EXPECT_EQ(default_app, prefs->IsDefault(internal_app_id));
+
+  std::unique_ptr<ArcAppListPrefs::AppInfo> normal_app =
+      prefs->GetApp(normal_app_id);
+  std::unique_ptr<ArcAppListPrefs::AppInfo> internal_app =
+      prefs->GetApp(internal_app_id);
+  ASSERT_TRUE(normal_app && internal_app);
+
+  EXPECT_LT(normal_app->install_time, internal_app->install_time);
+
+  // Installed internally app has runking below other apps, even if it's install
+  // time is later.
+  CreateSearch();
+  EXPECT_EQ(std::string(kRankingNormalAppName) + "," +
+                std::string(kRankingInternalAppName),
+            RunQuery(kRankingAppQuery));
+
+  // Using installed internally app moves it in ranking up.
+  WaitTimeUpdated();
+  prefs->SetLastLaunchTime(internal_app_id);
+  CreateSearch();
+  EXPECT_EQ(std::string(kRankingInternalAppName) + "," +
+                std::string(kRankingNormalAppName),
+            RunQuery(kRankingAppQuery));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ,
+    AppSearchProviderWithArcAppInstallType,
+    ::testing::ValuesIn({TestArcAppInstallType::CONTROLLED_BY_POLICY,
+                         TestArcAppInstallType::INSTALLED_BY_DEFAULT}));
+
 }  // namespace test
 }  // namespace app_list