Skip to content

Commit

Permalink
Revert "CrOS media-app: Launch PDFs in distinct windows when multiple…
Browse files Browse the repository at this point in the history 
… selected."

This reverts commit 4a03b1e.

Reason for revert: Flipped a flag in the test harness which collided 
with https://crrev.com/c/3565223 causing test failures since 
https://ci.chromium.org/ui/p/chromium/builders/ci/linux-chromeos-rel/55967

Original change's description:
> CrOS media-app: Launch PDFs in distinct windows when multiple selected.
>
> This is similar to current behaviour where a multi-selection of PDF
> files in the files app will launch to multiple tabs simultaneously.
>
> The current "pdf" extension entry is moved to its own handler to ensure
> multi-selections that include a mix of pdf and other files will not
> see the media-app as a handler.
>
> Fixed: b/207802912
> Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
> Change-Id: I1fbd9c1d73cf445f3a0c8a0ac5444412e0446376
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3565222
> Reviewed-by: Patti Lor <patricialor@chromium.org>
> Commit-Queue: Trent Apted <tapted@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#988377}

Change-Id: Ie6106642b158a92c830e012629e1a32358b79b16
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3567853
Auto-Submit: Trent Apted <tapted@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#988405}
  • Loading branch information
@tapted
tapted authored and Chromium LUCI CQ committed Apr 4, 2022
1 parent 7027b3b commit 26e1e1d
Show file tree
Hide file tree
Showing 4 changed files with 3 additions and 109 deletions.
1 change: 0 additions & 1 deletion ash/webui/media_app_ui/resources/mock/js/app_main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ class BacklightApp extends HTMLElement {

this.replaceChild(child, this.currentMedia);
this.currentMedia = child;
this.delegate.notifyCurrentFile(file.name, mimeType);
}

updateHandler() {
Expand Down
54 changes: 0 additions & 54 deletions chrome/browser/ash/web_applications/media_app/media_app_integration_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,6 @@ constexpr char kFileVideoVP9[] = "world.webm";
// A 5-second long 96kb/s Ogg-Vorbis 44.1kHz mono audio file.
constexpr char kFileAudioOgg[] = "music.ogg";

// A 1-page (8.5" x 11") PDF with some text and metadata.
constexpr char kFilePdfTall[] = "tall.pdf";

// A small square image PDF created by a camera.
constexpr char kFilePdfImg[] = "img.pdf";

constexpr char kUnhandledRejectionScript[] =
"window.dispatchEvent("
"new CustomEvent('simulate-unhandled-rejection-for-test'));";
Expand All @@ -100,10 +94,6 @@ constexpr char kDomExceptionScript[] =

class MediaAppIntegrationTest : public SystemWebAppIntegrationTest {
public:
MediaAppIntegrationTest() {
feature_list_.InitAndEnableFeature(ash::features::kMediaAppHandlesPdf);
}

void MediaAppLaunchWithFile(bool audio_enabled);
void MediaAppWithLaunchSystemWebAppAsync(bool audio_enabled);
void MediaAppEligibleOpenTask(bool audio_enabled);
Expand All @@ -115,7 +105,6 @@ class MediaAppIntegrationTest : public SystemWebAppIntegrationTest {
content::WebContents* LaunchWithNoFiles();

private:
base::test::ScopedFeatureList feature_list_;
std::unique_ptr<file_manager::test::FolderInMyFiles> launch_folder_;
};

Expand Down Expand Up @@ -425,49 +414,6 @@ IN_PROC_BROWSER_TEST_P(MediaAppIntegrationAudioDisabledTest,
MediaAppWithLaunchSystemWebAppAsync(false);
}

// Test that the Media App launches a single window for images.
IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppLaunchImageMulti) {
WaitForTestSystemAppInstall();
web_app::SystemAppLaunchParams image_params;
image_params.launch_paths = {TestFile(kFilePng800x600),
TestFile(kFileJpeg640x480)};

web_app::LaunchSystemWebAppAsync(browser()->profile(),
web_app::SystemAppType::MEDIA, image_params);
web_app::FlushSystemWebAppLaunchesForTesting(browser()->profile());

const BrowserList* browser_list = BrowserList::GetInstance();
EXPECT_EQ(2u, browser_list->size()); // 1 extra for the browser test browser.

content::TitleWatcher watcher(
browser_list->get(1)->tab_strip_model()->GetActiveWebContents(),
u"image.png");
EXPECT_EQ(u"image.png", watcher.WaitAndGetTitle());
}

// Test that the Media App launches multiple windows for PDFs.
IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppLaunchPdfMulti) {
WaitForTestSystemAppInstall();
web_app::SystemAppLaunchParams pdf_params;
pdf_params.launch_paths = {TestFile(kFilePdfTall), TestFile(kFilePdfImg)};

web_app::LaunchSystemWebAppAsync(browser()->profile(),
web_app::SystemAppType::MEDIA, pdf_params);
web_app::FlushSystemWebAppLaunchesForTesting(browser()->profile());

const BrowserList* browser_list = BrowserList::GetInstance();
EXPECT_EQ(3u, browser_list->size()); // 1 extra for the browser test browser.

content::TitleWatcher watcher1(
browser_list->get(1)->tab_strip_model()->GetActiveWebContents(),
u"tall.pdf");
content::TitleWatcher watcher2(
browser_list->get(2)->tab_strip_model()->GetActiveWebContents(),
u"img.pdf");
EXPECT_EQ(u"tall.pdf", watcher1.WaitAndGetTitle());
EXPECT_EQ(u"img.pdf", watcher2.WaitAndGetTitle());
}

// Test that the Media App appears as a handler for files in the App Service.
IN_PROC_BROWSER_TEST_P(MediaAppIntegrationTest, MediaAppHandlesIntents) {
WaitForTestSystemAppInstall();
Expand Down
52 changes: 3 additions & 49 deletions chrome/browser/ash/web_applications/media_app/media_web_app_info.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/strings/string_split.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/ash/web_applications/system_web_app_install_utils.h"
#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
#include "chrome/browser/web_applications/web_app_constants.h"
#include "chrome/browser/web_applications/web_app_install_info.h"
#include "chromeos/strings/grit/chromeos_strings.h"
Expand Down Expand Up @@ -61,6 +60,9 @@ constexpr FileHandlerConfig kFileHandlers[] = {
{"image/svg+xml", ".svg,.svgz"},
{"image/avif", ".avif"},

// PDF.
{"application/pdf", ".pdf"},

// When updating this list, `FOO_EXTENSIONS` in go/bl-launch should be
// updated as well.
};
Expand All @@ -84,11 +86,6 @@ constexpr FileHandlerConfig kAudioFileHandlers[] = {
// updated as well.
};

constexpr char kPdfExtension[] = ".pdf";
constexpr FileHandlerConfig kPdfFileHandlers[] = {
{"application/pdf", kPdfExtension},
};

// Converts a FileHandlerConfig constexpr into the type needed to populate the
// WebAppInstallInfo's `accept` property.
std::vector<apps::FileHandler::AcceptEntry> MakeFileHandlerAccept(
Expand All @@ -108,16 +105,6 @@ std::vector<apps::FileHandler::AcceptEntry> MakeFileHandlerAccept(
return result;
}

// Picks out a single file from a template launch `params`.
const apps::AppLaunchParams PickFileFromParams(
const apps::AppLaunchParams& params,
size_t index) {
return apps::AppLaunchParams(params.app_id, params.container,
params.disposition, params.launch_source,
params.display_id, {params.launch_files[index]},
params.intent ? params.intent.Clone() : nullptr);
}

} // namespace

MediaSystemAppDelegate::MediaSystemAppDelegate(Profile* profile)
Expand Down Expand Up @@ -176,21 +163,10 @@ std::unique_ptr<WebAppInstallInfo> CreateWebAppInfoForMediaWebApp() {
image_video_handler.action = GURL(ash::kChromeUIMediaAppURL);
image_video_handler.accept = MakeFileHandlerAccept(kFileHandlers);
info->file_handlers.push_back(std::move(image_video_handler));

apps::FileHandler audio_handler;
audio_handler.action = GURL(ash::kChromeUIMediaAppURL);
audio_handler.accept = MakeFileHandlerAccept(kAudioFileHandlers);
info->file_handlers.push_back(std::move(audio_handler));

apps::FileHandler pdf_handler;
pdf_handler.action = GURL(ash::kChromeUIMediaAppURL);
pdf_handler.accept = MakeFileHandlerAccept(kPdfFileHandlers);
// Note setting `apps::FileHandler::LaunchType::kMultipleClients` here has no
// effect for system web apps (see comments in
// WebAppPublisherHelper::OnFileHandlerDialogCompleted()). The PDF-specifc
// behavior to spawn multiple launches occurs in an override of
// LaunchAndNavigateSystemWebApp().
info->file_handlers.push_back(std::move(pdf_handler));
return info;
}

Expand Down Expand Up @@ -243,25 +219,3 @@ bool MediaSystemAppDelegate::ShouldReuseExistingWindow() const {
bool MediaSystemAppDelegate::ShouldHandleFileOpenIntents() const {
return true;
}

Browser* MediaSystemAppDelegate::LaunchAndNavigateSystemWebApp(
Profile* profile,
web_app::WebAppProvider* provider,
const GURL& url,
const apps::AppLaunchParams& params) const {
// For zero/single-file launches, or non-PDF launches, launch a single window.
if (params.launch_files.size() < 2 ||
!params.launch_files[0].MatchesExtension(kPdfExtension)) {
return SystemWebAppDelegate::LaunchAndNavigateSystemWebApp(
profile, provider, url, params);
}

// For PDFs, launch all but the last file from scratch. Windows will cascade.
for (size_t i = 0; i < params.launch_files.size() - 1; ++i) {
web_app::LaunchSystemWebAppImpl(profile, web_app::SystemAppType::MEDIA, url,
PickFileFromParams(params, i));
}
return SystemWebAppDelegate::LaunchAndNavigateSystemWebApp(
profile, provider, url,
PickFileFromParams(params, params.launch_files.size() - 1));
}
5 changes: 0 additions & 5 deletions chrome/browser/ash/web_applications/media_app/media_web_app_info.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ class MediaSystemAppDelegate : public web_app::SystemWebAppDelegate {
bool ShouldHandleFileOpenIntents() const override;
base::FilePath GetLaunchDirectory(
const apps::AppLaunchParams& params) const override;
Browser* LaunchAndNavigateSystemWebApp(
Profile* profile,
web_app::WebAppProvider* provider,
const GURL& url,
const apps::AppLaunchParams& params) const override;
};

// Return a WebAppInstallInfo used to install the app.
Expand Down

0 comments on commit 26e1e1d

Please sign in to comment.