Skip to content

Commit

Permalink
[Sheriff] Revert "[Office upload] Introduce CloudOpenTask to cloud_up…
Browse files Browse the repository at this point in the history
…load_dialog"

This reverts commit 46db1a5.

Reason for revert: Multiple failures in
https://ci.chromium.org/p/chromium/builders/ci/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29

Original change's description:
> [Office upload] Introduce CloudOpenTask to cloud_upload_dialog
>
> Creates a new class CloudOpenTask that holds information needed
> to execute the current task like profile, urls and cloud_provider.
>
> This was done to reduce the need to bind these parameters into
> every static function like we did previously. This is also needed
> for an upcoming CL which needs to create and store additional state
> about windows, but had no place to stash the state other than
> in a very complicated way in callbacks.
>
> This CL also splits the business logic of when to run setup and what
> page of the dialog we need to show next, from the WebUI dialog wiring
> necessary to launch a WebUI. CloudOpenTask handles the former, and
> CloudUploadDialog handles the latter.
>
> Using CloudOpenTask::Status it's much easier to reason about
> lifetimes rather than having them implicitly hidden in bound
> callbacks.
>
> Bug: b:269546410
> Change-Id: I070490d4301eab99998d0bbf045200b4cf1a5e5b
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4252827
> Reviewed-by: Jeremie Boulic <jboulic@chromium.org>
> Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
> Reviewed-by: Cassy Chun-Crogan <cassycc@google.com>
> Commit-Queue: Peter Marshall <petermarshall@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1108789}

Bug: b:269546410
Change-Id: Ie5093987fdcfa0d5113684f7d58eb3961925f16a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4284946
Commit-Queue: Renato Silva <rrsilva@google.com>
Owners-Override: Renato Silva <rrsilva@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1108885}
  • Loading branch information
Renato Silva authored and Chromium LUCI CQ committed Feb 23, 2023
1 parent a3a0b95 commit ade0487
Show file tree
Hide file tree
Showing 6 changed files with 414 additions and 645 deletions.
10 changes: 5 additions & 5 deletions chrome/browser/ash/file_manager/file_tasks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ bool ExecuteWebDriveOfficeTask(Profile* profile,
drive::DriveIntegrationServiceFactory::FindForProfile(profile);
if (integration_service && integration_service->IsMounted() &&
integration_service->GetDriveFsInterface()) {
return ash::cloud_upload::CloudOpenTask::Execute(
return ash::cloud_upload::OpenFilesWithCloudProvider(
profile, file_urls, ash::cloud_upload::CloudProvider::kGoogleDrive);
} else {
UMA_HISTOGRAM_ENUMERATION(kDriveErrorMetricName,
Expand All @@ -444,7 +444,7 @@ bool ExecuteOpenInOfficeTask(Profile* profile,
// TODO(petermarshall): UMAs.
}

return ash::cloud_upload::CloudOpenTask::Execute(
return ash::cloud_upload::OpenFilesWithCloudProvider(
profile, file_urls, ash::cloud_upload::CloudProvider::kOneDrive);
}

Expand Down Expand Up @@ -1120,7 +1120,7 @@ std::string ToSwaActionId(const std::string& action_id) {

} // namespace

void SetWordFileHandler(Profile* profile, const TaskDescriptor& task) {
void SetWordFileHandler(Profile* profile, TaskDescriptor& task) {
UpdateDefaultTask(
profile, task, {".doc", ".docx"},
{"application/msword",
Expand All @@ -1134,7 +1134,7 @@ void SetWordFileHandlerToFilesSWA(Profile* profile,
SetWordFileHandler(profile, task);
}

void SetExcelFileHandler(Profile* profile, const TaskDescriptor& task) {
void SetExcelFileHandler(Profile* profile, TaskDescriptor& task) {
UpdateDefaultTask(
profile, task, {".xls", ".xlsx"},
{"application/vnd.ms-excel",
Expand All @@ -1148,7 +1148,7 @@ void SetExcelFileHandlerToFilesSWA(Profile* profile,
SetExcelFileHandler(profile, task);
}

void SetPowerPointFileHandler(Profile* profile, const TaskDescriptor& task) {
void SetPowerPointFileHandler(Profile* profile, TaskDescriptor& task) {
UpdateDefaultTask(
profile, task, {".ppt", ".pptx"},
{"application/vnd.ms-powerpoint",
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ash/file_manager/file_tasks.h
Original file line number Diff line number Diff line change
Expand Up @@ -403,9 +403,9 @@ bool IsHtmlFile(const base::FilePath& path);
bool IsOfficeFile(const base::FilePath& path);

// Updates the default task for each of the office file types.
void SetWordFileHandler(Profile* profile, const TaskDescriptor& task);
void SetExcelFileHandler(Profile* profile, const TaskDescriptor& task);
void SetPowerPointFileHandler(Profile* profile, const TaskDescriptor& task);
void SetWordFileHandler(Profile* profile, TaskDescriptor& task);
void SetExcelFileHandler(Profile* profile, TaskDescriptor& task);
void SetPowerPointFileHandler(Profile* profile, TaskDescriptor& task);

// TODO(petermarshall): Move these to a new file office_file_tasks.cc/h
// Updates the default task for each of the office file types to a Files
Expand Down
42 changes: 17 additions & 25 deletions chrome/browser/ash/file_manager/file_tasks_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
#include "base/files/file_util.h"
#include "base/functional/bind.h"
#include "base/functional/callback_helpers.h"
#include "base/memory/scoped_refptr.h"
#include "base/path_service.h"
#include "base/ranges/algorithm.h"
#include "base/strings/strcat.h"
Expand Down Expand Up @@ -1124,8 +1123,8 @@ IN_PROC_BROWSER_TEST_F(DriveTest, OfficeFallbackTryAgain) {
}
#endif // BUILDFLAG(GOOGLE_CHROME_BRANDING)

// Test that CloudOpenTask::Execute() will open a DriveFs office file when the
// cloud provider specified is Google Drive.
// Test that OpenOrMoveFiles() will open a DriveFs office file when the cloud
// provider specified is Google Drive.
IN_PROC_BROWSER_TEST_F(DriveTest, OpenFileInDrive) {
// Add test file to fake DriveFs.
SetUpTest();
Expand All @@ -1140,9 +1139,8 @@ IN_PROC_BROWSER_TEST_F(DriveTest, OpenFileInDrive) {
expected_web_drive_office_url);
navigation_observer_office.StartWatchingNewWebContents();

auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
profile(), file_urls, ash::cloud_upload::CloudProvider::kGoogleDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::OpenOrMoveFiles(
profile(), file_urls, ash::cloud_upload::CloudProvider::kGoogleDrive);

// Wait for file to open in web drive office.
navigation_observer_office.Wait();
Expand Down Expand Up @@ -1516,7 +1514,7 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest, OfficeFallbackTryAgain) {
SetConnectionOnline();

// Run dialog callback, simulate user choosing to "try-again". Will succeed
// because system is online, and the file doesn't need to be moved.
// because system is online.
OnDialogChoiceReceived(profile(), open_in_office_task, file_urls,
ash::office_fallback::kDialogChoiceTryAgain);

Expand Down Expand Up @@ -1583,9 +1581,8 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest, OpenFileFromODFS) {
web_app_publisher_->ClearPastLaunches();

// Open file directly from ODFS.
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
profile(), file_urls, ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::OpenOrMoveFiles(
profile(), file_urls, ash::cloud_upload::CloudProvider::kOneDrive);

auto launches = web_app_publisher_->GetLaunches();
ASSERT_EQ(1u, launches.size());
Expand All @@ -1607,9 +1604,8 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest, OpenFileNotFromODFS) {
navigation_observer_dialog.StartWatchingNewWebContents();

// Triggers Move Confirmation dialog.
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
profile(), file_urls, ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::OpenOrMoveFiles(
profile(), file_urls, ash::cloud_upload::CloudProvider::kOneDrive);

// Wait for setup flow dialog to open.
navigation_observer_dialog.Wait();
Expand Down Expand Up @@ -1637,10 +1633,9 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest, OpenFileFromAndroidOneDriveViaODFS) {
web_app_publisher_->ClearPastLaunches();

// Open the file indirectly from Android OneDrive (via ODFS).
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
ash::cloud_upload::OpenOrMoveFiles(
profile(), {android_onedrive_url},
ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::CloudProvider::kOneDrive);

auto launches = web_app_publisher_->GetLaunches();
ASSERT_EQ(1u, launches.size());
Expand Down Expand Up @@ -1674,10 +1669,9 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest,

// Attempt to open the file indirectly from Android OneDrive (via ODFS). It
// will fail as the email accounts don't match.
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
ash::cloud_upload::OpenOrMoveFiles(
profile(), {android_onedrive_url},
ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::CloudProvider::kOneDrive);

auto launches = web_app_publisher_->GetLaunches();
ASSERT_EQ(0u, launches.size());
Expand Down Expand Up @@ -1706,10 +1700,9 @@ IN_PROC_BROWSER_TEST_F(OneDriveTest,

// Attempt to open the file indirectly from Android OneDrive (via ODFS). It
// will fail as there is not an equivalent ODFS file path.
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
ash::cloud_upload::OpenOrMoveFiles(
profile(), {android_onedrive_url},
ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::CloudProvider::kOneDrive);

auto launches = web_app_publisher_->GetLaunches();
ASSERT_EQ(0u, launches.size());
Expand Down Expand Up @@ -1742,10 +1735,9 @@ IN_PROC_BROWSER_TEST_F(

// Attempt to open the file indirectly from Android OneDrive (via ODFS). It
// will fail as there is not an equivalent ODFS file path.
auto task = base::WrapRefCounted(new ash::cloud_upload::CloudOpenTask(
ash::cloud_upload::OpenOrMoveFiles(
profile(), {android_onedrive_url},
ash::cloud_upload::CloudProvider::kOneDrive));
task->OpenOrMoveFiles();
ash::cloud_upload::CloudProvider::kOneDrive);

auto launches = web_app_publisher_->GetLaunches();
ASSERT_EQ(0u, launches.size());
Expand Down

0 comments on commit ade0487

Please sign in to comment.