Skip to content

GitLab

T
tangled
Commit 62bd40d9 authored by Vasilii Sukhanov's avatar Vasilii Sukhanov Committed by Commit Bot
Browse files
Options

Revert "system-web-apps: better handle incognito in LaunchSystemWebApp" 

This reverts commit de6bbcbc.

Reason for revert: Broke SystemWebAppLaunchProfileBrowserTest tests
https://ci.chromium.org/p/chromium/builders/ci/Win7%20Tests%20%28dbg%29%281%29/85648

Original change's description:
> system-web-apps: better handle incognito in LaunchSystemWebApp
>
> LaunchSystemWebApp used whatever profile is provided in the parameter to
> create SWA windows and launching SWAs, even if the profile may not
> suitable to host a SWA window.
>
> This CL adds logic to determine the appropriate profile if the provided
> one is unsuitable. This serves as a mid-term solution for launching
> SWAs, and acts as a safety net to prevent chrome crashing if unsuitable
> profiles are provided.
>
> Specifically, it catches these two types of profiles:
> - Guest session "original" profile: launch into guest session's
>   primary off the record profile (which is used for guest session
>   browsing and SWAs).
> - Normal session incognito profile: launch into the original profile,
>   because we don't support incognito SWAs.
>
> Fixed: 1109594
> Change-Id: Ibe4a3b1bf7c811ef1ee02fd967307ad9bf070ee7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423760
> Commit-Queue: Jiewei Qian  <qjw@chromium.org>
> Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#814602}

TBR=ortuno@chromium.org,qjw@chromium.org

Change-Id: Ie46ed4204f9c841be9d4dff8772fd2f9af85c96f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455747Reviewed-by: default avatarVasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814626}
parent 1e693946
Hide whitespace changes
Inline Side-by-side
Showing with 13 additions and 202 deletions
chrome/browser/ui/web_applications/system_web_app_ui_utils.cc
View file @ 62bd40d9
......@@ -9,7 +9,6 @@
#include <vector>
#include "base/check_op.h"
#include "base/debug/dump_without_crashing.h"
#include "base/feature_list.h"
#include "base/files/file_path.h"
#include "base/metrics/histogram_functions.h"
......@@ -19,7 +18,6 @@
#include "chrome/browser/apps/app_service/app_service_metrics.h"
#include "chrome/browser/apps/app_service/launch_utils.h"
#include "chrome/browser/chromeos/printing/print_management/print_management_uma.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/installable/installable_params.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser_list.h"
......@@ -37,45 +35,10 @@
#include "chrome/browser/web_launch/web_launch_files_helper.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
#include "content/public/common/content_switches.h"
#include "third_party/blink/public/common/features.h"
#include "ui/base/window_open_disposition.h"
#include "ui/display/types/display_constants.h"
namespace {
// Returns the profile where we should launch System Web Apps into. It returns
// the most appropriate profile for launching, if the provided |profile| is
// unsuitable. It returns nullptr if the we can't find a suitable profile.
Profile* GetProfileForSystemWebAppLaunch(Profile* profile) {
DCHECK(profile);
// We can't launch into certain profiles, and we can't find a suitable
// alternative.
if (profile->IsSystemProfile())
return nullptr;
#if defined(OS_CHROMEOS)
if (chromeos::ProfileHelper::IsSigninProfile(profile))
return nullptr;
#endif
// For a guest sessions, launch into the primary off-the-record profile, which
// is used for browsing in guest sessions. We do this because the "original"
// profile of the guest session can't create windows.
if (profile->IsGuestSession())
return profile->GetPrimaryOTRProfile();
// We don't support launching SWA in incognito profiles, use the original
// profile if an incognito profile is provided (with the exception of guest
// session, which is implemented with an incognito profile, thus it is handled
// above).
if (profile->IsIncognitoProfile())
return profile->GetOriginalProfile();
// Use the profile provided in other scenarios.
return profile;
}
} // namespace
namespace web_app {
namespace {
......@@ -159,42 +122,20 @@ Browser* LaunchSystemWebApp(Profile* profile,
const GURL& url,
base::Optional<apps::AppLaunchParams> params,
bool* did_create) {
Profile* profile_for_launch = GetProfileForSystemWebAppLaunch(profile);
if (profile_for_launch == nullptr || profile_for_launch != profile) {
// The provided profile can't launch system web apps. Complain about this so
// we can catch the call site, and ask them to pick the right profile.
base::debug::DumpWithoutCrashing();
DVLOG(1) << "LaunchSystemWebApp is called on a profile that can't launch "
"system web apps. Please check the profile you are using is "
"correct."
<< (profile_for_launch
? "Instead, launch the app into a suitable profile "
"based on your intention."
: "Can't find a suitable profile based on the provided "
"argument. Thus ignore the launch request.");
NOTREACHED();
if (profile_for_launch == nullptr)
return nullptr;
}
auto* provider = WebAppProvider::Get(profile_for_launch);
auto* provider = WebAppProvider::Get(profile);
if (!provider)
return nullptr;
if (!params) {
params = CreateSystemWebAppLaunchParams(profile_for_launch, app_type,
params = CreateSystemWebAppLaunchParams(profile, app_type,
display::kInvalidDisplayId);
}
if (!params)
return nullptr;
params->override_url = url;
DCHECK_EQ(params->app_id,
*GetAppIdForSystemWebApp(profile_for_launch, app_type));
DCHECK_EQ(params->app_id, *GetAppIdForSystemWebApp(profile, app_type));
// TODO(crbug/1117655): The file manager records metrics directly when opening
// a file registered to an app, but can't tell if an SWA will ultimately be
......@@ -218,8 +159,7 @@ Browser* LaunchSystemWebApp(Profile* profile,
browser_type = Browser::TYPE_APP_POPUP;
if (browser_type == Browser::TYPE_APP_POPUP ||
provider->system_web_app_manager().IsSingleWindow(app_type)) {
browser =
FindSystemWebAppBrowser(profile_for_launch, app_type, browser_type);
browser = FindSystemWebAppBrowser(profile, app_type, browser_type);
}
// We create the app window if no existing browser found.
......@@ -232,10 +172,9 @@ Browser* LaunchSystemWebApp(Profile* profile,
// CCA supports responsive UI.
bool can_resize = app_type != SystemAppType::CAMERA;
if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions)) {
if (!browser) {
browser = CreateWebApplicationWindow(profile_for_launch, params->app_id,
if (!browser)
browser = CreateWebApplicationWindow(profile, params->app_id,
params->disposition, can_resize);
}
// Navigate application window to application's |url| if necessary.
// Help app always navigates because its url might not match the url inside
......@@ -247,10 +186,8 @@ Browser* LaunchSystemWebApp(Profile* profile,
browser, params->app_id, url, WindowOpenDisposition::CURRENT_TAB);
}
} else {
if (!browser) {
browser =
CreateApplicationWindow(profile_for_launch, *params, url, can_resize);
}
if (!browser)
browser = CreateApplicationWindow(profile, *params, url, can_resize);
// Navigate application window to application's |url| if necessary.
// Help app always navigates because its url might not match the url inside
......
chrome/browser/ui/web_applications/test/system_web_app_ui_browsertest.cc
View file @ 62bd40d9
......@@ -9,7 +9,6 @@
#include "base/strings/utf_string_conversions.h"
#include "chrome/app/chrome_command_ids.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h"
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/browser_tabstrip.h"
......@@ -393,19 +392,11 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
->GetLastCommittedURL());
}
// TODO(crbug.com/1135863): Decide and formalize this behavior. This test is
// disabled in DCHECK builds, because it hits a DCHECK in LaunchSystemWebApp. In
// production builds, SWA is link captured to the original profile. The goal is
// to behave reasonably, and not crashing.
#if DCHECK_IS_ON()
#define MAYBE_IncognitoBrowserOmniboxLinkCapture \
DISABLED_IncognitoBrowserOmniboxLinkCapture
#else
#define MAYBE_IncognitoBrowserOmniboxLinkCapture \
IncognitoBrowserOmniboxLinkCapture
#endif
// TODO(crbug.com/1109594): Update this when we decide if SWAs can be
// link-captured to standalone incognito windows. Currently, the SWA gets
// launched into a standalone browser window in incognito profile.
IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
MAYBE_IncognitoBrowserOmniboxLinkCapture) {
IncognitoBrowserOmniboxLinkCapture) {
WaitForTestSystemAppInstall();
Browser* incognito_browser = CreateIncognitoBrowser();
......@@ -419,10 +410,8 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
incognito_browser, maybe_installation_->GetAppUrl().spec());
observer.Wait();
// We launch SWAs into the incognito profile's original profile.
Browser* app_browser = FindSystemWebAppBrowser(
incognito_browser->profile()->GetOriginalProfile(),
maybe_installation_->GetType());
incognito_browser->profile(), maybe_installation_->GetType());
EXPECT_TRUE(app_browser);
EXPECT_EQ(app_browser, chrome::FindLastActive());
EXPECT_EQ(2U, chrome::GetTotalBrowserCount());
......@@ -430,111 +419,7 @@ IN_PROC_BROWSER_TEST_P(SystemWebAppLinkCaptureBrowserTest,
EXPECT_FALSE(app_browser->app_controller()->ShouldShowCustomTabBar());
}
using SystemWebAppLaunchProfileBrowserTest = SystemWebAppManagerBrowserTest;
IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
LaunchFromNormalSessionIncognitoProfile) {
Profile* startup_profile = browser()->profile();
ASSERT_TRUE(startup_profile->IsRegularProfile());
WaitForTestSystemAppInstall();
Profile* incognito_profile = startup_profile->GetPrimaryOTRProfile();
#if DCHECK_IS_ON()
EXPECT_DEATH_IF_SUPPORTED(
LaunchSystemWebApp(incognito_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType()))),
"");
#else
Browser* result_browser =
LaunchSystemWebApp(incognito_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType())));
EXPECT_EQ(startup_profile, result_browser->profile());
EXPECT_TRUE(result_browser->profile()->IsRegularProfile());
#endif
}
#if defined(OS_CHROMEOS)
IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileBrowserTest,
LaunchFromSignInProfile) {
WaitForTestSystemAppInstall();
Profile* signin_profile = chromeos::ProfileHelper::GetSigninProfile();
#if DCHECK_IS_ON()
EXPECT_DEATH_IF_SUPPORTED(
LaunchSystemWebApp(signin_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType()))),
"");
#else
Browser* result_browser =
LaunchSystemWebApp(signin_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType())));
EXPECT_EQ(nullptr, result_browser);
#endif
}
#endif // defined(OS_CHROMEOS)
#if defined(OS_CHROMEOS)
using SystemWebAppLaunchProfileGuestSessionBrowserTest =
SystemWebAppManagerBrowserTest;
IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileGuestSessionBrowserTest,
LaunchFromGuestSessionOriginalProfile) {
// We should start into the guest session browsing profile.
Profile* startup_profile = browser()->profile();
ASSERT_TRUE(startup_profile->IsGuestSession());
ASSERT_TRUE(startup_profile->IsPrimaryOTRProfile());
WaitForTestSystemAppInstall();
// We typically don't get the original profile as an argument, but it is a
// valid input to LaunchSystemWebApp.
Profile* original_profile = browser()->profile()->GetOriginalProfile();
#if DCHECK_IS_ON()
EXPECT_DEATH_IF_SUPPORTED(
LaunchSystemWebApp(original_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType()))),
"");
#else
Browser* result_browser =
LaunchSystemWebApp(original_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType())));
EXPECT_EQ(startup_profile, result_browser->profile());
EXPECT_TRUE(result_browser->profile()->IsGuestSession());
EXPECT_TRUE(result_browser->profile()->IsPrimaryOTRProfile());
#endif
}
IN_PROC_BROWSER_TEST_P(SystemWebAppLaunchProfileGuestSessionBrowserTest,
LaunchFromGuestSessionPrimaryOTRProfile) {
// We should start into the guest session browsing profile.
Profile* startup_profile = browser()->profile();
ASSERT_TRUE(startup_profile->IsGuestSession());
ASSERT_TRUE(startup_profile->IsPrimaryOTRProfile());
WaitForTestSystemAppInstall();
Browser* result_browser =
LaunchSystemWebApp(startup_profile, GetMockAppType(),
GetStartUrl(LaunchParamsForApp(GetMockAppType())));
EXPECT_EQ(startup_profile, result_browser->profile());
EXPECT_TRUE(result_browser->profile()->IsGuestSession());
EXPECT_TRUE(result_browser->profile()->IsPrimaryOTRProfile());
}
#endif // defined(OS_CHROMEOS)
INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P(
SystemWebAppLinkCaptureBrowserTest);
INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_ALL_INSTALL_TYPES_P(
SystemWebAppLaunchProfileBrowserTest);
#if defined(OS_CHROMEOS)
INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_GUEST_SESSION_P(
SystemWebAppLaunchProfileGuestSessionBrowserTest);
#endif
} // namespace web_app
chrome/browser/web_applications/system_web_app_manager_browsertest.h
View file @ 62bd40d9
......@@ -188,15 +188,4 @@ std::string SystemWebAppManagerTestParamsToString(
TestProfileType::kIncognito, \
TestProfileType::kGuest)))
// Instantiate |SUITE| for all install types, on a Chrome OS guest session.
#define INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_GUEST_SESSION_P(SUITE) \
INSTANTIATE_SYSTEM_WEB_APP_MANAGER_TEST_SUITE_P( \
SUITE, \
::testing::Combine( \
::testing::Values(web_app::ProviderType::kBookmarkApps, \
web_app::ProviderType::kWebApps), \
::testing::Values(web_app::InstallationType::kManifestInstall, \
web_app::InstallationType::kWebAppInfoInstall), \
::testing::Values(TestProfileType::kGuest)))
#endif // CHROME_BROWSER_WEB_APPLICATIONS_SYSTEM_WEB_APP_MANAGER_BROWSERTEST_H_
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