Adjust Default App Order

This CL changes the default app order according to
PRD at go/default-apps.
It also introduces a mechanism to add default page breaks
that are part of the default sort order so that we're
able to make default apps span multiple pages if desired.
Default page breaks should only be installed for first-time
users, or users who already have them in their remote sync
data.

BUG=880506
TEST=Manually by installing an image on device, also added a test.

Change-Id: I4882e3a6b76b81862686b2eba0d1bd76d0ec4b35
Reviewed-on: https://chromium-review.googlesource.com/1212704
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595681}

chrome/browser/chromeos/extensions/default_app_order.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "ash/public/cpp/app_list/internal_app_id_constants.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_path.h"
@@ -17,6 +18,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
+#include "chrome/browser/ui/app_list/page_break_constants.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chromeos/chromeos_paths.h"
 #include "extensions/common/constants.h"
@@ -40,12 +42,12 @@ const char* const kDefaultAppOrder[] = {
     arc::kPlayStoreAppId,
     extension_misc::kFilesManagerAppId,
     extension_misc::kGmailAppId,
-    extension_misc::kCalendarAppId,
     extension_misc::kGoogleDocAppId,
     extension_misc::kGoogleSlidesAppId,
     extension_misc::kGoogleSheetsAppId,
+    extension_misc::kDriveHostedAppId,
     extension_misc::kGoogleKeepAppId,
-    extension_misc::kGooglePhotosAppId,
+    extension_misc::kCalendarAppId,
     extension_misc::kYoutubeAppId,
     arc::kPlayMoviesAppId,                   // Play Movies & TV ARC app
     extension_misc::kGooglePlayMoviesAppId,  // Play Movies & TV Chrome app
@@ -54,15 +56,22 @@ const char* const kDefaultAppOrder[] = {
     arc::kPlayGamesAppId,
     arc::kPlayBooksAppId,                   // Play Books ARC app
     extension_misc::kGooglePlayBooksAppId,  // Play Books Chrome app
-    extension_misc::kGoogleMapsAppId,
-    extension_misc::kDriveHostedAppId,
+    app_list::kInternalAppIdCamera,
     extension_misc::kCameraAppId,
+    extension_misc::kGooglePhotosAppId,
+    app_list::kDefaultPageBreak1,  // First default page break
+    extension_misc::kGoogleMapsAppId,
+    app_list::kInternalAppIdSettings,
+    app_list::kInternalAppIdDiscover,
+    extension_misc::kGeniusAppId,
     extension_misc::kCalculatorAppId,
     extension_misc::kTextEditorAppId,
+    arc::kGoogleDuo,
+    arc::kLightRoom,
+    arc::kInfinitePainter,
     extension_misc::kGooglePlusAppId,
     extension_misc::kChromeRemoteDesktopAppId,
     extensions::kWebStoreAppId,
-    extension_misc::kGeniusAppId,
 };
 
 // Reads external ordinal json file and returned the parsed value. Returns NULL

chrome/browser/extensions/chrome_app_sorting.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/extension_sync_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/extensions/extension_constants.h"
 #include "content/public/browser/notification_service.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
@@ -23,6 +24,7 @@
 
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/extensions/default_app_order.h"
+#include "chrome/browser/ui/app_list/page_break_constants.h"
 #endif
 
 namespace extensions {
@@ -583,6 +585,15 @@ void ChromeAppSorting::CreateDefaultOrdinals() {
     default_ordinals_[extension_id].page_ordinal = page_ordinal;
     default_ordinals_[extension_id].app_launch_ordinal = app_launch_ordinal;
     app_launch_ordinal = app_launch_ordinal.CreateAfter();
+#if defined(OS_CHROMEOS)
+    // Default page breaks are installed by default for first-time users so that
+    // we can make default apps span multiple pages in the Launcher without
+    // fully filling those pages. If |extension_id| is of a default page break,
+    // then apps that follow it in the order should have an incremented page
+    // ordinal.
+    if (app_list::IsDefaultPageBreakItem(extension_id))
+      page_ordinal = page_ordinal.CreateAfter();
+#endif  // defined(OS_CHROMEOS)
   }
 }
 

chrome/browser/sync/test/integration/single_client_app_list_sync_test.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/browser/ui/app_list/internal_app/internal_app_metadata.h"
+#include "chrome/browser/ui/app_list/page_break_constants.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "extensions/browser/extension_system.h"
 
@@ -124,10 +125,13 @@ IN_PROC_BROWSER_TEST_F(SingleClientAppListSyncTest, AppListSomeApps) {
   app_list::AppListSyncableService* service =
       app_list::AppListSyncableServiceFactory::GetForProfile(verifier());
 
-  // Default apps: chrome + web store + internal apps.
+  // Default apps: chrome + web store + internal apps + number of default page
+  // breaks.
   const size_t kNumDefaultApps =
-      2u + app_list::GetNumberOfInternalAppsShowInLauncherForTest(
-               /*apps_name=*/nullptr, GetProfile(0));
+      2u +
+      app_list::GetNumberOfInternalAppsShowInLauncherForTest(
+          /*apps_name=*/nullptr, GetProfile(0)) +
+      app_list::kDefaultPageBreakAppIdsLength;
   ASSERT_EQ(kNumApps + kNumDefaultApps, service->GetNumSyncItemsForTest());
 
   ASSERT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());

chrome/browser/sync/test/integration/sync_app_list_helper.cc

@@ -42,6 +42,15 @@ void SyncAppListHelper::SetupIfNecessary(SyncTest* test) {
   for (auto* profile : test_->GetAllProfiles()) {
     extensions::ExtensionSystem::Get(profile)->InitForRegularProfile(
         true /* extensions_enabled */);
+    if (test_->use_verifier() && profile == test_->verifier()) {
+      // The default page break items are only installed for first-time users.
+      // The verifier() profile doesn't get initialized with remote sync data,
+      // and hence the default page breaks are not installed for it. We have to
+      // install them manually to avoid a mismatch when comparing the verifier()
+      // against other client profiles.
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile)
+          ->InstallDefaultPageBreaksForTest();
+    }
   }
 
   setup_completed_ = true;

chrome/browser/ui/BUILD.gn

@@ -3318,6 +3318,10 @@ jumbo_split_static_library("ui") {
         "app_list/internal_app/internal_app_metadata.h",
         "app_list/internal_app/internal_app_model_builder.cc",
         "app_list/internal_app/internal_app_model_builder.h",
+        "app_list/page_break_app_item.cc",
+        "app_list/page_break_app_item.h",
+        "app_list/page_break_constants.cc",
+        "app_list/page_break_constants.h",
         "app_list/search/arc_app_result.cc",
         "app_list/search/arc_app_result.h",
         "app_list/search/crostini_app_result.cc",

chrome/browser/ui/app_list/app_list_syncable_service.cc

@@ -30,6 +30,8 @@
 #include "chrome/browser/ui/app_list/extension_app_item.h"
 #include "chrome/browser/ui/app_list/extension_app_model_builder.h"
 #include "chrome/browser/ui/app_list/internal_app/internal_app_model_builder.h"
+#include "chrome/browser/ui/app_list/page_break_app_item.h"
+#include "chrome/browser/ui/app_list/page_break_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/pref_names.h"
@@ -743,6 +745,10 @@ void AppListSyncableService::PruneEmptySyncFolders() {
 
 // AppListSyncableService syncer::SyncableService
 
+void AppListSyncableService::InstallDefaultPageBreaksForTest() {
+  InstallDefaultPageBreaks();
+}
+
 syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
@@ -752,6 +758,16 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
   DCHECK(sync_processor.get());
   DCHECK(error_handler.get());
 
+  const bool first_time_user = initial_sync_data.empty();
+  if (first_time_user) {
+    // Post a task to avoid adding the default page break items which can cause
+    // sync changes during sync startup.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(&AppListSyncableService::InstallDefaultPageBreaks,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
+
   HandleUpdateStarted();
 
   // Reset local state and recreate from sync info.
@@ -983,12 +999,12 @@ void AppListSyncableService::ProcessNewSyncItem(SyncItem* sync_item) {
       return;
     }
     case sync_pb::AppListSpecifics::TYPE_PAGE_BREAK: {
-      std::unique_ptr<ChromeAppListItem> page_break_item =
-          std::make_unique<ChromeAppListItem>(profile_, sync_item->item_id,
-                                              model_updater_.get());
-      page_break_item->SetPosition(sync_item->item_ordinal);
-      page_break_item->SetIsPageBreak(true);
-      model_updater_->AddItem(std::move(page_break_item));
+      // This is can be either a default page break item that was installed by
+      // default for new users, or a non-default page-break item that was added
+      // by the user. the ctor of PageBreakAppItem will update the newly-created
+      // item from its |sync_item|.
+      model_updater_->AddItem(std::make_unique<PageBreakAppItem>(
+          profile_, model_updater_.get(), sync_item, sync_item->item_id));
       return;
     }
   }
@@ -1192,4 +1208,23 @@ void AppListSyncableService::PruneRedundantPageBreakItems() {
   }
 }
 
+void AppListSyncableService::InstallDefaultPageBreaks() {
+  for (size_t i = 0; i < app_list::kDefaultPageBreakAppIdsLength; ++i) {
+    auto* const id = app_list::kDefaultPageBreakAppIds[i];
+    auto* sync_item = GetSyncItem(id);
+    if (sync_item) {
+      // The user may have cleared their sync from
+      // https://chrome.google.com/sync, so it may appear here that it's a new
+      // user, while in fact on this device, it's not. We don't want to recreate
+      // and re-add an already existing default page break item.
+      continue;
+    }
+
+    auto page_break_item = std::make_unique<PageBreakAppItem>(
+        profile(), model_updater_.get(), nullptr /* sync_item */, id);
+    page_break_item->SetName("__default_page_break__");
+    AddItem(std::move(page_break_item));
+  }
+}
+
 }  // namespace app_list

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -148,6 +148,8 @@ class AppListSyncableService : public syncer::SyncableService,
     return oem_folder_name_;
   }
 
+  void InstallDefaultPageBreaksForTest();
+
   const SyncItemMap& sync_items() const { return sync_items_; }
 
   // syncer::SyncableService
@@ -263,6 +265,10 @@ class AppListSyncableService : public syncer::SyncableService,
   // level item list.
   void PruneRedundantPageBreakItems();
 
+  // Installs the default page break items. This is only called for first time
+  // users.
+  void InstallDefaultPageBreaks();
+
   Profile* profile_;
   extensions::ExtensionSystem* extension_system_;
   std::unique_ptr<AppListModelUpdater> model_updater_;

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -3,7 +3,9 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
+
 #include "ash/public/cpp/app_list/app_list_config.h"
+#include "ash/public/cpp/app_list/internal_app_id_constants.h"
 #include "base/bind.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/string_number_conversions.h"
@@ -12,6 +14,7 @@
 #include "chrome/browser/ui/app_list/app_list_model_updater.h"
 #include "chrome/browser/ui/app_list/app_list_test_util.h"
 #include "chrome/browser/ui/app_list/chrome_app_list_item.h"
+#include "chrome/browser/ui/app_list/page_break_constants.h"
 #include "chrome/browser/ui/app_list/test/fake_app_list_model_updater.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -319,6 +322,67 @@ TEST_F(AppListSyncableServiceTest, InitialMerge) {
   EXPECT_EQ("ordinal", GetSyncItem(kItemId2)->item_ordinal.ToDebugString());
   EXPECT_EQ("pinordinal",
             GetSyncItem(kItemId2)->item_pin_ordinal.ToDebugString());
+
+  // Default page breaks are not installed for non-first time users that don't
+  // have them in their sync.
+  EXPECT_FALSE(GetSyncItem(app_list::kDefaultPageBreak1));
+}
+
+TEST_F(AppListSyncableServiceTest, ExistingDefaultPageBreak) {
+  // Non-first time users have items in their remote sync data.
+  syncer::SyncDataList sync_list;
+  sync_list.push_back(CreateAppRemoteData(
+      app_list::kDefaultPageBreak1, "page_break_1", "", "ordinal", "pinordinal",
+      sync_pb::AppListSpecifics_AppListItemType_TYPE_PAGE_BREAK));
+
+  app_list_syncable_service()->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::make_unique<syncer::FakeSyncChangeProcessor>(),
+      std::make_unique<syncer::SyncErrorFactoryMock>());
+  content::RunAllTasksUntilIdle();
+
+  // Existing page break item in remote sync will be added, and its data will be
+  // updated with the item's remote sync data.
+  auto* page_break_sync_item = GetSyncItem(app_list::kDefaultPageBreak1);
+  ASSERT_TRUE(page_break_sync_item);
+  EXPECT_EQ(page_break_sync_item->item_type,
+            sync_pb::AppListSpecifics::TYPE_PAGE_BREAK);
+  EXPECT_EQ("page_break_1", page_break_sync_item->item_name);
+  EXPECT_EQ("", page_break_sync_item->parent_id);
+  EXPECT_EQ("ordinal", page_break_sync_item->item_ordinal.ToDebugString());
+  EXPECT_EQ("pinordinal",
+            page_break_sync_item->item_pin_ordinal.ToDebugString());
+}
+
+TEST_F(AppListSyncableServiceTest, DefaultPageBreakFirstTimeUser) {
+  // Empty sync list simulates a first time user.
+  syncer::SyncDataList sync_list;
+
+  app_list_syncable_service()->MergeDataAndStartSyncing(
+      syncer::APP_LIST, sync_list,
+      std::make_unique<syncer::FakeSyncChangeProcessor>(),
+      std::make_unique<syncer::SyncErrorFactoryMock>());
+  content::RunAllTasksUntilIdle();
+
+  auto* page_break_sync_item = GetSyncItem(app_list::kDefaultPageBreak1);
+  ASSERT_TRUE(page_break_sync_item);
+  EXPECT_EQ(page_break_sync_item->item_type,
+            sync_pb::AppListSpecifics::TYPE_PAGE_BREAK);
+
+  // Since internal apps are added by default, we'll use the camera and the
+  // settings apps to test the ordering.
+  auto* settings_app_sync_item = GetSyncItem(app_list::kInternalAppIdSettings);
+  auto* camera_app_sync_item = GetSyncItem(app_list::kInternalAppIdCamera);
+  ASSERT_TRUE(settings_app_sync_item);
+  ASSERT_TRUE(camera_app_sync_item);
+
+  // The default page break should be between the camera app, and the settings
+  // app; i.e. the camera app is in the first page, and the settings app is in
+  // the second page.
+  EXPECT_TRUE(page_break_sync_item->item_ordinal.LessThan(
+      settings_app_sync_item->item_ordinal));
+  EXPECT_TRUE(page_break_sync_item->item_ordinal.GreaterThan(
+      camera_app_sync_item->item_ordinal));
 }
 
 TEST_F(AppListSyncableServiceTest, InitialMerge_BadData) {

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

@@ -181,6 +181,9 @@ int64_t GetValidDisplayId(int64_t display_id) {
 
 }  // namespace
 
+const char kGoogleDuo[] = "djkcbcmkefiiphjkonbeknmcgiheajce";
+const char kInfinitePainter[] = "afihfgfghkmdmggakhkgnfhlikhdpima";
+const char kLightRoom[] = "fpegfnbgomakooccabncdaelhfppceni";
 const char kPlayStoreAppId[] = "cnbgggchhmkkdmeppjobngjoejnihlei";
 const char kPlayBooksAppId[] = "cafegjnmmjpfibnlddppihpnkbkgicbg";
 const char kPlayGamesAppId[] = "nplnnjkbeijcggmpdcecpabgbjgeiedc";

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

@@ -23,6 +23,9 @@ class BrowserContext;
 
 namespace arc {
 
+extern const char kGoogleDuo[];
+extern const char kInfinitePainter[];
+extern const char kLightRoom[];
 extern const char kPlayStoreAppId[];
 extern const char kPlayBooksAppId[];
 extern const char kPlayGamesAppId[];

chrome/browser/ui/app_list/page_break_app_item.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 "chrome/browser/ui/app_list/page_break_app_item.h"
+
+// static
+const char PageBreakAppItem::kItemType[] = "DefaultPageBreak";
+
+PageBreakAppItem::PageBreakAppItem(
+    Profile* profile,
+    AppListModelUpdater* model_updater,
+    const app_list::AppListSyncableService::SyncItem* sync_item,
+    const std::string& app_id)
+    : ChromeAppListItem(profile, app_id) {
+  SetIsPageBreak(true);
+
+  if (sync_item) {
+    DCHECK_EQ(sync_item->item_type, sync_pb::AppListSpecifics::TYPE_PAGE_BREAK);
+    if (sync_item->item_ordinal.IsValid()) {
+      UpdateFromSync(sync_item);
+      return;
+    }
+  }
+
+  SetDefaultPositionIfApplicable(model_updater);
+}
+
+PageBreakAppItem::~PageBreakAppItem() = default;
+
+// ChromeAppListItem:
+void PageBreakAppItem::Activate(int event_flags) {
+  NOTREACHED();
+}
+
+const char* PageBreakAppItem::GetItemType() const {
+  return kItemType;
+}

chrome/browser/ui/app_list/page_break_app_item.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 CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_APP_ITEM_H_
+#define CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_APP_ITEM_H_
+
+#include "base/macros.h"
+#include "chrome/browser/ui/app_list/chrome_app_list_item.h"
+
+// Represents a page break item in the app list.
+class PageBreakAppItem : public ChromeAppListItem {
+ public:
+  static const char kItemType[];
+
+  // If a page break item with ID |app_id| exists in the local sync items as
+  // |sync_item| (i.e. non-null), then this newly created item will be updated
+  // from |sync_item|.
+  PageBreakAppItem(Profile* profile,
+                   AppListModelUpdater* model_updater,
+                   const app_list::AppListSyncableService::SyncItem* sync_item,
+                   const std::string& app_id);
+  ~PageBreakAppItem() override;
+
+  // ChromeAppListItem:
+  void Activate(int event_flags) override;
+  const char* GetItemType() const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(PageBreakAppItem);
+};
+
+#endif  // CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_APP_ITEM_H_

chrome/browser/ui/app_list/page_break_constants.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 "chrome/browser/ui/app_list/page_break_constants.h"
+
+#include "base/stl_util.h"
+
+namespace app_list {
+
+// Generated as:
+// crx_file::id_util::GenerateId("org.chromium.default_page_break_1").
+const char kDefaultPageBreak1[] = "fdipebbchlhkdibfjgbfalhceahammim";
+
+const char* const kDefaultPageBreakAppIds[] = {
+    kDefaultPageBreak1,
+};
+
+const size_t kDefaultPageBreakAppIdsLength =
+    base::size(kDefaultPageBreakAppIds);
+
+// Returns true if |item_id| is of a default-installed page break item.
+bool IsDefaultPageBreakItem(const std::string& item_id) {
+  return base::ContainsValue(kDefaultPageBreakAppIds, item_id);
+}
+
+}  // namespace app_list

chrome/browser/ui/app_list/page_break_constants.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 CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_CONSTANTS_H_
+#define CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_CONSTANTS_H_
+
+#include <string>
+
+namespace app_list {
+
+extern const char kDefaultPageBreak1[];
+
+// List of default page break app list items that are added by default for first
+// time users. These items are added in a particular order to in
+// default_app_order.cc.
+extern const char* const kDefaultPageBreakAppIds[];
+
+extern const size_t kDefaultPageBreakAppIdsLength;
+
+// Returns true if |item_id| is of a default-installed page break item.
+bool IsDefaultPageBreakItem(const std::string& item_id);
+
+}  // namespace app_list
+
+#endif  // CHROME_BROWSER_UI_APP_LIST_PAGE_BREAK_CONSTANTS_H_