Commit 77a9f6d7 authored by Tim Sergeant's avatar Tim Sergeant Committed by Commit Bot

Reland "Reland "CrOS: Add Files app to default shelf pins in second position""

This reverts commit 5243d913.

Reason for revert: Chrome OS LKGM has been manually updated in crrev.com/c/2544964, bots using simplechrome should now have the fixed tast test. 

Original change's description:
> Revert "Reland "CrOS: Add Files app to default shelf pins in second position""
>
> This reverts commit 527ae681.
>
> Reason for revert: suspect causing chrome_all_tast_tests failure on chromeos-kevin-chrome
> - ui.ShelfLaunchedApps
>
> First detected build:
> https://ci.chromium.org/p/chrome/builders/ci/chromeos-kevin-chrome/10864
>
> Sample log:
> https://logs.chromium.org/logs/chrome/buildbucket/cr-buildbucket.appspot.com/8863425670685038096/+/steps/chrome_all_tast_tests_on_ChromeOS/0/logs/Deterministic_failure:_ui.ShelfLaunchedApps__status_FAILURE_/0
> --
> Unexpected apps in the shelf. Expected only Chrome: [0x2f76930 0x2f76960]
> --
>
> Original change's description:
> > Reland "CrOS: Add Files app to default shelf pins in second position"
> >
> > This is a reland of 5d6ecbfb
> >
> > The failing Tast test (ui.ShelfLaunchedApps) has been updated to
> > ensure it continues passing after this CL is landed.
> >
> > This is the second of a chain of 3 CLs:
> >
> > 1. crrev.com/c/2538305: Allow Files app to be pinned by default in
> >    ui.ShelfLaunchedApps
> > 2. crrev.com/c/2538857: Add Files app to default shelf pins in second
> >    position
> > 3. crrev.com/c/2538307: Update ui.ShelfLaunchedApps to ensure Chrome and
> >    Files are always pinned
> >
> > Original change's description:
> > > CrOS: Add Files app to default shelf pins in second position
> > >
> > > This adds the Files app to the spot after the Chrome icon, for new users
> > > only. There's also some variations of the default pin layout controlled
> > > by a Finch experiment, I've moved the Files app up in those layouts for
> > > consistency.
> > >
> > > This change was requested for M88 and will be merged back.
> > >
> > > Bug: 1148519
> > > Change-Id: Iae54561fec599b813ba97e82a5678fa834a02db2
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2537214
> > > Reviewed-by: Dominick Ng <dominickn@chromium.org>
> > > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> > > Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> > > Auto-Submit: Tim Sergeant <tsergeant@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#827272}
> >
> > Bug: 1148519
> > Change-Id: I9186b43a651dea4c4de9e46f849d35da9cc6062c
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2538857
> > Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> > Commit-Queue: Tim Sergeant <tsergeant@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#827981}
>
> TBR=xiyuan@chromium.org,tsergeant@chromium.org
>
> Change-Id: I36e3778d368973d250824541c422d68b1679babd
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1148519
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2543883
> Reviewed-by: Takashi Sakamoto <tasak@google.com>
> Commit-Queue: Takashi Sakamoto <tasak@google.com>
> Cr-Commit-Position: refs/heads/master@{#828095}

CQ_INCLUDE_TRYBOTS=luci.chrome.try:chromeos-kevin-chrome
TBR=xiyuan@chromium.org,tasak@google.com,tsergeant@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1148519
Change-Id: I113ab25e648699fb0a40c0e56e3205bb3cf00a94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547646
Commit-Queue: Tim Sergeant <tsergeant@chromium.org>
Reviewed-by: default avatarTim Sergeant <tsergeant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829096}
parent b5189196
...@@ -49,27 +49,28 @@ namespace { ...@@ -49,27 +49,28 @@ namespace {
// Chrome is pinned explicitly. // Chrome is pinned explicitly.
const char* kDefaultPinnedApps[] = { const char* kDefaultPinnedApps[] = {
extension_misc::kGmailAppId, extension_misc::kGoogleDocAppId, extension_misc::kFilesManagerAppId, extension_misc::kGmailAppId,
extension_misc::kYoutubeAppId, arc::kPlayStoreAppId}; extension_misc::kGoogleDocAppId, extension_misc::kYoutubeAppId,
arc::kPlayStoreAppId};
const char* kDefaultPinnedApps7Apps[] = { const char* kDefaultPinnedApps7Apps[] = {
extension_misc::kGmailAppId, extension_misc::kGoogleDocAppId, extension_misc::kFilesManagerAppId, extension_misc::kGmailAppId,
extension_misc::kGooglePhotosAppId, extension_misc::kFilesManagerAppId, extension_misc::kGoogleDocAppId, extension_misc::kGooglePhotosAppId,
extension_misc::kYoutubeAppId, arc::kPlayStoreAppId}; extension_misc::kYoutubeAppId, arc::kPlayStoreAppId};
const char* kDefaultPinnedApps10Apps[] = {extension_misc::kGmailAppId, const char* kDefaultPinnedApps10Apps[] = {extension_misc::kFilesManagerAppId,
extension_misc::kGmailAppId,
extension_misc::kCalendarAppId, extension_misc::kCalendarAppId,
extension_misc::kGoogleDocAppId, extension_misc::kGoogleDocAppId,
extension_misc::kGoogleSheetsAppId, extension_misc::kGoogleSheetsAppId,
extension_misc::kGoogleSlidesAppId, extension_misc::kGoogleSlidesAppId,
extension_misc::kFilesManagerAppId,
extension_misc::kCameraAppId, extension_misc::kCameraAppId,
extension_misc::kGooglePhotosAppId, extension_misc::kGooglePhotosAppId,
arc::kPlayStoreAppId}; arc::kPlayStoreAppId};
const char* kTabletFormFactorDefaultPinnedApps[] = { const char* kTabletFormFactorDefaultPinnedApps[] = {
arc::kGmailAppId, extension_misc::kGoogleDocAppId, arc::kYoutubeAppId, extension_misc::kFilesManagerAppId, arc::kGmailAppId,
arc::kPlayStoreAppId}; extension_misc::kGoogleDocAppId, arc::kYoutubeAppId, arc::kPlayStoreAppId};
const char kDefaultPinnedAppsKey[] = "default"; const char kDefaultPinnedAppsKey[] = "default";
const char kDefaultPinnedApps7AppsKey[] = "7apps"; const char kDefaultPinnedApps7AppsKey[] = "7apps";
......
...@@ -417,6 +417,11 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest { ...@@ -417,6 +417,11 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
base::FilePath(), Manifest::UNPACKED, manifest, Extension::NO_FLAGS, base::FilePath(), Manifest::UNPACKED, manifest, Extension::NO_FLAGS,
extension_misc::kYoutubeAppId, &error); extension_misc::kYoutubeAppId, &error);
// Fake File Manager app.
extension_files_app_ = Extension::Create(
base::FilePath(), Manifest::UNPACKED, manifest, Extension::NO_FLAGS,
extension_misc::kFilesManagerAppId, &error);
MaybeStartWebAppProvider(); MaybeStartWebAppProvider();
} }
...@@ -788,6 +793,8 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest { ...@@ -788,6 +793,8 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
result += "Doc"; result += "Doc";
} else if (app == extension_youtube_app_->id()) { } else if (app == extension_youtube_app_->id()) {
result += "Youtube"; result += "Youtube";
} else if (app == extension_files_app_->id()) {
result += "Files";
} else if (app == extension_platform_app_->id()) { } else if (app == extension_platform_app_->id()) {
result += "Platform_App"; result += "Platform_App";
} else if (app == arc_support_host_->id()) { } else if (app == arc_support_host_->id()) {
...@@ -962,6 +969,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest { ...@@ -962,6 +969,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
scoped_refptr<Extension> extension_gmail_app_; scoped_refptr<Extension> extension_gmail_app_;
scoped_refptr<Extension> extension_doc_app_; scoped_refptr<Extension> extension_doc_app_;
scoped_refptr<Extension> extension_youtube_app_; scoped_refptr<Extension> extension_youtube_app_;
scoped_refptr<Extension> extension_files_app_;
scoped_refptr<Extension> extension_platform_app_; scoped_refptr<Extension> extension_platform_app_;
scoped_refptr<Extension> arc_support_host_; scoped_refptr<Extension> arc_support_host_;
...@@ -1085,6 +1093,7 @@ class ChromeLauncherControllerExtendedShelfTest ...@@ -1085,6 +1093,7 @@ class ChromeLauncherControllerExtendedShelfTest
extension_service_->AddExtension(extension_gmail_app_.get()); extension_service_->AddExtension(extension_gmail_app_.get());
extension_service_->AddExtension(extension_doc_app_.get()); extension_service_->AddExtension(extension_doc_app_.get());
extension_service_->AddExtension(extension_youtube_app_.get()); extension_service_->AddExtension(extension_youtube_app_.get());
extension_service_->AddExtension(extension_files_app_.get());
extension_service_->AddExtension(arc_support_host_.get()); extension_service_->AddExtension(arc_support_host_.get());
std::string error; std::string error;
...@@ -1102,7 +1111,6 @@ class ChromeLauncherControllerExtendedShelfTest ...@@ -1102,7 +1111,6 @@ class ChromeLauncherControllerExtendedShelfTest
{extension_misc::kCalendarAppId, "Calendar"}, {extension_misc::kCalendarAppId, "Calendar"},
{extension_misc::kGoogleSheetsAppId, "Sheets"}, {extension_misc::kGoogleSheetsAppId, "Sheets"},
{extension_misc::kGoogleSlidesAppId, "Slides"}, {extension_misc::kGoogleSlidesAppId, "Slides"},
{extension_misc::kFilesManagerAppId, "Files"},
{extension_misc::kCameraAppId, "Camera"}, {extension_misc::kCameraAppId, "Camera"},
{extension_misc::kGooglePhotosAppId, "Photos"}, {extension_misc::kGooglePhotosAppId, "Photos"},
}; };
...@@ -1364,6 +1372,8 @@ TEST_F(ChromeLauncherControllerTest, DefaultApps) { ...@@ -1364,6 +1372,8 @@ TEST_F(ChromeLauncherControllerTest, DefaultApps) {
EXPECT_EQ("Chrome, Doc, Youtube, App1", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Doc, Youtube, App1", GetPinnedAppStatus());
AddExtension(extension_gmail_app_.get()); AddExtension(extension_gmail_app_.get());
EXPECT_EQ("Chrome, Gmail, Doc, Youtube, App1", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Gmail, Doc, Youtube, App1", GetPinnedAppStatus());
AddExtension(extension_files_app_.get());
EXPECT_EQ("Chrome, Files, Gmail, Doc, Youtube, App1", GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerSplitSettingsSyncTest, DefaultApps) { TEST_F(ChromeLauncherControllerSplitSettingsSyncTest, DefaultApps) {
...@@ -1390,28 +1400,29 @@ TEST_F(ChromeLauncherControllerLacrosTest, LacrosPinnedByDefault) { ...@@ -1390,28 +1400,29 @@ TEST_F(ChromeLauncherControllerLacrosTest, LacrosPinnedByDefault) {
EXPECT_EQ("Chrome, Lacros", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Lacros", GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShefDefault) { TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShelfDefault) {
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeatureWithParameters( scoped_feature_list.InitAndEnableFeatureWithParameters(
kEnableExtendedShelfLayout, kEnableExtendedShelfLayout,
{std::pair<std::string, std::string>("app_count", "0")}); {std::pair<std::string, std::string>("app_count", "0")});
InitLauncherController(); InitLauncherController();
EXPECT_EQ("Chrome, Gmail, Doc, Youtube, Play Store", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Files, Gmail, Doc, Youtube, Play Store",
GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShef7Apps) { TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShelf7Apps) {
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeatureWithParameters( scoped_feature_list.InitAndEnableFeatureWithParameters(
kEnableExtendedShelfLayout, kEnableExtendedShelfLayout,
{std::pair<std::string, std::string>("app_count", "7")}); {std::pair<std::string, std::string>("app_count", "7")});
InitLauncherController(); InitLauncherController();
EXPECT_EQ("Chrome, Gmail, Doc, Photos, Files, Youtube, Play Store", EXPECT_EQ("Chrome, Files, Gmail, Doc, Photos, Youtube, Play Store",
GetPinnedAppStatus()); GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShef10Apps) { TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShelf10Apps) {
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeatureWithParameters( scoped_feature_list.InitAndEnableFeatureWithParameters(
kEnableExtendedShelfLayout, kEnableExtendedShelfLayout,
...@@ -1419,14 +1430,15 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShef10Apps) { ...@@ -1419,14 +1430,15 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, ExtendedShef10Apps) {
InitLauncherController(); InitLauncherController();
EXPECT_EQ( EXPECT_EQ(
"Chrome, Gmail, Calendar, Doc, Sheets, Slides, Files, Camera, Photos, " "Chrome, Files, Gmail, Calendar, Doc, Sheets, Slides, Camera, Photos, "
"Play Store", "Play Store",
GetPinnedAppStatus()); GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerExtendedShelfTest, UpgradeFromDefault) { TEST_F(ChromeLauncherControllerExtendedShelfTest, UpgradeFromDefault) {
InitLauncherController(); InitLauncherController();
EXPECT_EQ("Chrome, Gmail, Doc, Youtube, Play Store", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Files, Gmail, Doc, Youtube, Play Store",
GetPinnedAppStatus());
// Upgrade happens only in case default layout is active. // Upgrade happens only in case default layout is active.
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
...@@ -1438,15 +1450,15 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, UpgradeFromDefault) { ...@@ -1438,15 +1450,15 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, UpgradeFromDefault) {
AddExtension(extension1_.get()); AddExtension(extension1_.get());
EXPECT_EQ( EXPECT_EQ(
"Chrome, Gmail, Calendar, Doc, Sheets, Slides, Files, Camera, Photos, " "Chrome, Files, Gmail, Calendar, Doc, Sheets, Slides, Camera, Photos, "
"Play Store", "Play Store",
GetPinnedAppStatus()); GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerExtendedShelfTest, NoDefaultAfterExperemental) { TEST_F(ChromeLauncherControllerExtendedShelfTest, NoDefaultAfterExperimental) {
const std::string expectations = const std::string expectations =
"Chrome, Gmail, Calendar, Doc, Sheets, " "Chrome, Files, Gmail, Calendar, Doc, Sheets, "
"Slides, Files, Camera, Photos, Play Store"; "Slides, Camera, Photos, Play Store";
{ {
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeatureWithParameters( scoped_feature_list.InitAndEnableFeatureWithParameters(
...@@ -1473,7 +1485,7 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, NoDefaultAfterExperemental) { ...@@ -1473,7 +1485,7 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, NoDefaultAfterExperemental) {
TEST_F(ChromeLauncherControllerExtendedShelfTest, NoUpgradeFromNonDefault) { TEST_F(ChromeLauncherControllerExtendedShelfTest, NoUpgradeFromNonDefault) {
InitLauncherController(); InitLauncherController();
launcher_controller_->UnpinAppWithID(extension_misc::kYoutubeAppId); launcher_controller_->UnpinAppWithID(extension_misc::kYoutubeAppId);
EXPECT_EQ("Chrome, Gmail, Doc, Play Store", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Files, Gmail, Doc, Play Store", GetPinnedAppStatus());
// Upgrade does not happen due to default pin layout change. // Upgrade does not happen due to default pin layout change.
base::test::ScopedFeatureList scoped_feature_list; base::test::ScopedFeatureList scoped_feature_list;
...@@ -1484,7 +1496,7 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, NoUpgradeFromNonDefault) { ...@@ -1484,7 +1496,7 @@ TEST_F(ChromeLauncherControllerExtendedShelfTest, NoUpgradeFromNonDefault) {
// Trigger layout update, app_id does not matter. // Trigger layout update, app_id does not matter.
AddExtension(extension1_.get()); AddExtension(extension1_.get());
EXPECT_EQ("Chrome, Gmail, Doc, Play Store", GetPinnedAppStatus()); EXPECT_EQ("Chrome, Files, Gmail, Doc, Play Store", GetPinnedAppStatus());
} }
TEST_F(ChromeLauncherControllerWithArcTest, ArcAppPinCrossPlatformWorkflow) { TEST_F(ChromeLauncherControllerWithArcTest, ArcAppPinCrossPlatformWorkflow) {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment