Skip to content

Commit

Permalink
Add the single web app permission dialog case
Browse files Browse the repository at this point in the history
Add the single web app permission dialog case for web app protocol
handler intent picker dialog. This cl adds the startup launch logic to
show the dialog with the right conditions.

This cl does not remember the choices the user makes so they will see
the dialog every time we launch from protocol.

This cl also removes the multi app picker dialog from the code.
The decision to do this is following our new design to support
multi-profile protocol handling for web applications. Design doc link
below.

One-pager: https://docs.google.com/document/d/1ksfUr31h1YYNdsAswwQ5_-567RukXP_i3uQ2c4kJYYo/edit?usp=sharing&resourcekey=0-Lve5NUPuyZJ_ZPcE8g0HEw

New screenshots of the dialog can be seen at this patchset:
https://chromium-review.googlesource.com/c/chromium/src/+/2800473/20

To test this change:
- Launch the browser on Windows/Linux/Mac with
--enable-blink-features=ParseUrlProtocolHandler
- Install http://fabiorocha.github.io/pwa
- From cmd run "start web+github://testvalue"
- Observe the dialog and select "Allow".
- Use the DevTools to verify that the PWA was launched with the fully
translated URL: https://fabiorocha.github.io/pwa/?github=web%2Bgithub%3A%2F%2Ftestvalue%2F

Bug: 1105257
Change-Id: I2802da85e0adee1e8565e88708b54ae543f2a3e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2800473
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Commit-Queue: Samuel Tang <samtan@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#880092}
  • Loading branch information
samtan-msft authored and Chromium LUCI CQ committed May 6, 2021
1 parent 9499ad9 commit a5fc44e
Show file tree
Hide file tree
Showing 18 changed files with 238 additions and 256 deletions.
14 changes: 1 addition & 13 deletions chrome/app/protocol_handler_intent_picker_strings.grdp
@@ -1,26 +1,14 @@
<?xml version="1.0" encoding="utf-8"?>
<!-- Protocol-Handler-Intent-Picker strings (included from generated_resources.grd). -->
<grit-part>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_MULTI_TITLE" desc="Title for the protocol handler intent picker when there is more than one web app choice">
Which application do you want to use?
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_SINGLE_TITLE" desc="Title for the protocol handler intent picker when there is one web app choice">
Do you want to use this application?
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_REMEMBER_SELECTION" desc="Label for the checkbox in the protocol handler intent picker to save the current selection so that the protocol handler intent picker will not be shown again.">
Remember my choice
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_MULTI_OK_BUTTON_TEXT" desc="Label for the button in the protocol handler intent picker dialog that dismisses the dialog and launches an application when there is more than one web app choice">
Open
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_SINGLE_OK_BUTTON_TEXT" desc="Label for the button in the protocol handler intent picker dialog that dismisses the dialog and launches an application when there is one web app choice">
Allow
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_MULTI_CANCEL_BUTTON_TEXT" desc="Label for the button in the protocol handler intent picker dialog that dismisses the dialog and does not launch an application">
Cancel
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_SINGLE_CANCEL_BUTTON_TEXT" desc="Label for the button in the protocol handler intent picker dialog that dismisses the dialog and does not launch an application when there is one web app choice">
Block
Cancel
</message>
<message name="IDS_PROTOCOL_HANDLER_INTENT_PICKER_APP_ORIGIN_LABEL" desc="Label for the url origin of each item in the web app list in the protocol handler intent picker dialog">
Publisher: <ph name="APP_ORIGIN">$1<ex>google.com</ex></ph>
Expand Down
@@ -1 +1 @@
04fafe5e2e86111e0b4532e2a28d64a6b08cb860
af4a036812ba61536eea9af5e7ed7b36f69f7859

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

@@ -1 +1 @@
1e63215b07c3a7c383ef7169e5e680f0c6de5533
f2820a81df5f31fd9bb2647de326e7eb235baa95
@@ -1 +1 @@
2a909dbb29ddad95f32f3cad3a88040f3a6159e2
74205678aff0b85ef2da2184cb2df1704a599490
@@ -1 +1 @@
11c24a134a4eb2c84b9829c6504c5bee2d2f35cf
c93fc64068b1a5f8c068749b8ef87754c3c764d3
2 changes: 2 additions & 0 deletions chrome/browser/profiles/profile_keep_alive_types.cc
Expand Up @@ -39,6 +39,8 @@ std::ostream& operator<<(std::ostream& out,
return out << "kChromeViewsDelegate";
case ProfileKeepAliveOrigin::kDevToolsWindow:
return out << "kDevToolsWindow";
case ProfileKeepAliveOrigin::kWebAppPermissionDialogWindow:
return out << "kWebAppPermissionDialogWindow";
}
NOTREACHED();
return out << static_cast<int>(origin);
Expand Down
5 changes: 4 additions & 1 deletion chrome/browser/profiles/profile_keep_alive_types.h
Expand Up @@ -71,7 +71,10 @@ enum class ProfileKeepAliveOrigin {
// A DevTools window is open.
kDevToolsWindow = 14,

kMaxValue = kDevToolsWindow,
// A web app permission dialog window is open.
kWebAppPermissionDialogWindow = 15,

kMaxValue = kWebAppPermissionDialogWindow,
};

std::ostream& operator<<(std::ostream& out,
Expand Down
14 changes: 10 additions & 4 deletions chrome/browser/ui/browser_dialogs.h
Expand Up @@ -15,6 +15,7 @@
#include "build/build_config.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/ui/bookmarks/bookmark_editor.h"
#include "chrome/browser/web_applications/components/web_app_id.h"
#include "chrome/common/buildflags.h"
#include "content/public/browser/content_browser_client.h"
#include "extensions/buildflags/buildflags.h"
Expand All @@ -33,7 +34,6 @@ class SettingsOverriddenDialogController;
#endif

namespace base {
class CommandLine;
class FilePath;
}

Expand Down Expand Up @@ -122,13 +122,19 @@ void ShowWebAppInstallDialog(content::WebContents* web_contents,
AppInstallationAcceptanceCallback callback);

#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)
// Callback used to indicate whether a user has accepted the launch of a
// web app. The boolean parameter is true when the user accepts the dialog.
using WebAppProtocolHandlerAcceptanceCallback =
base::OnceCallback<void(bool accepted)>;

// Shows the Web App Protocol Handler Intent Picker view.
// |close_callback| may be null.
// |profile| is kept alive throughout the processing and running of
// |close_callback|. |close_callback| may be null.
void ShowWebAppProtocolHandlerIntentPicker(
const GURL& url,
Profile* profile,
const base::CommandLine& command_line,
base::OnceCallback<void(bool accepted)> close_callback);
const web_app::AppId& app_id,
WebAppProtocolHandlerAcceptanceCallback close_callback);
#endif

// Sets whether |ShowWebAppDialog| should accept immediately without any
Expand Down
91 changes: 69 additions & 22 deletions chrome/browser/ui/startup/startup_browser_creator.cc
Expand Up @@ -108,6 +108,7 @@
#endif

#if defined(OS_WIN)
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "chrome/browser/metrics/jumplist_metrics_win.h"
#include "chrome/browser/notifications/notification_platform_bridge_win.h"
Expand All @@ -124,6 +125,12 @@
#include "ui/base/ui_base_features.h"
#endif

#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)
#include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/web_applications/components/web_app_id.h"
#include "third_party/blink/public/common/custom_handlers/protocol_handler_utils.h"
#endif

#if defined(OS_WIN) || defined(OS_MAC) || \
(defined(OS_LINUX) && !BUILDFLAG(IS_CHROMEOS_LACROS))
#include "chrome/browser/web_applications/components/url_handler_launch_params.h"
Expand Down Expand Up @@ -390,54 +397,92 @@ void FinalizeWebAppLaunch(
StartupBrowserCreatorImpl::MaybeToggleFullscreen(browser);
}

// Tries to get the protocol url from the command line.
// If the protocol app url switch doesnt exist, checks if the passed in url
// is a potential protocol url, if it is, check the protocol handler registry
// for an entry. Return the protocol url if there are handlers for this scheme.
#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)
// Tries to launch the web app from a protocol handler url. Checks if the passed
// in url is a potential protocol url, if it is, check the protocol handler
// registry for an entry. If there is an entry, then check if we need approval
// from the user to launch this web app. If we didn't have approval from a
// previous session, launch the permission dialog to ask for approval. If we did
// get permission in the past, directly launch the web app with the translated
// url. Returns true if the command line references a valid app and also
// contains a protocol url that is specifically registered by the app.
// `launch_mode_recorder` is used if and only if this function returns true.
bool MaybeLaunchProtocolHandlerWebApp(
const base::CommandLine& command_line,
const base::FilePath& cur_dir,
Profile* profile,
std::unique_ptr<LaunchModeRecorder> launch_mode_recorder) {
// Maybe the URL passed in is a protocol URL.
std::string app_id = command_line.GetSwitchValueASCII(switches::kAppId);
// We must have a kAppId switch arg in the command line to launch.
if (app_id.empty())
return false;

GURL protocol_url;
base::CommandLine::StringVector args = command_line.GetArgs();
for (const auto& arg : args) {
#if defined(OS_WIN)
GURL potential_protocol(base::WideToUTF16(arg));
GURL potential_protocol(base::AsStringPiece16(arg));
#else
GURL potential_protocol(arg);
#endif // defined(OS_WIN)
if (potential_protocol.is_valid() && !potential_protocol.IsStandard()) {
protocol_url = potential_protocol;
// protocol_url is checked for validity later on with GetHandlersFor() where
// we consult the ProtocolHandlerRegistry and find entries that match this
// protocol.
if (potential_protocol.is_valid()) {
protocol_url = std::move(potential_protocol);
break;
}
}
if (protocol_url.is_empty())
return false;

// Check the ProtocolHandlerRegistry for an entry that matches the appId
// and the protocol_url provided in the command line. We want to make sure
// that we only handle protocol handlers that we've registered for when
// installing a web app. This check also filters out file handler url
// activation and other unregistered protocol urls that will be handled
// by other startup code paths.
// TODO::(crbug/1105257) Adjust this once we no longer use
// the ProtocolHandlerRegistry to store protocol handler
// registration info for web apps.
ProtocolHandlerRegistry* handler_registry =
ProtocolHandlerRegistryFactory::GetForBrowserContext(profile);
const std::vector<ProtocolHandler> handlers =
handler_registry->GetHandlersFor(protocol_url.scheme());

// Check that there is at least one handler with web_app_id.
// TODO(crbug/1019239): Display intent picker if there are multiple handlers.
for (const auto& handler : handlers) {
if (handler.web_app_id().has_value()) {
apps::AppServiceProxyFactory::GetForProfile(profile)
->BrowserAppLauncher()
->LaunchAppWithCallback(
handler.web_app_id().value(), command_line, cur_dir,
/*url_handler_launch_url=*/base::nullopt, protocol_url,
base::BindOnce(&FinalizeWebAppLaunch,
std::move(launch_mode_recorder)));
return true;
}
// Nothing to do if there are no handlers with a web_app_id.
if (!base::Contains(handlers, true, [](const auto& handler) {
return handler.web_app_id().has_value();
})) {
return false;
}

return false;
auto launch_callback = base::BindOnce(
[](const base::CommandLine& command_line, const base::FilePath& cur_dir,
Profile* profile, const GURL& protocol_url,
const web_app::AppId& app_id,
std::unique_ptr<LaunchModeRecorder> launch_mode_recorder,
bool accepted) {
if (accepted) {
apps::AppServiceProxyFactory::GetForProfile(profile)
->BrowserAppLauncher()
->LaunchAppWithCallback(
app_id, command_line, cur_dir,
/*url_handler_launch_url=*/base::nullopt, protocol_url,
base::BindOnce(&FinalizeWebAppLaunch,
std::move(launch_mode_recorder)));
} // else allow the process to exit without opening a browser.
},
command_line, cur_dir, profile, protocol_url, app_id,
std::move(launch_mode_recorder));
// ShowWebAppProtocolHandlerIntentPicker keeps the `profile` alive through
// running of `launch_callback`.
chrome::ShowWebAppProtocolHandlerIntentPicker(std::move(protocol_url),
profile, std::move(app_id),
std::move(launch_callback));
return true;
}
#endif // defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)

// If the process was launched with the web application command line flags,
// e.g. --app=http://www.google.com/ or --app_id=... return true.
Expand Down Expand Up @@ -1042,12 +1087,14 @@ bool StartupBrowserCreator::ProcessCmdLineImpl(
}
}

#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)
// Web app Protocol handling.
if (MaybeLaunchProtocolHandlerWebApp(
command_line, cur_dir, privacy_safe_profile,
std::make_unique<LaunchModeRecorder>())) {
return true;
}
#endif

// If we're being run as an application window or application tab, don't
// restore tabs or open initial URLs as the user has directly launched an app
Expand Down
61 changes: 50 additions & 11 deletions chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
Expand Up @@ -102,6 +102,11 @@
#include "components/policy/core/common/policy_map.h"
#include "components/policy/core/common/policy_types.h"

#if defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)
#include "ui/views/widget/any_widget_observer.h"
#include "ui/views/widget/widget.h"
#endif

#if defined(OS_WIN) || defined(OS_MAC) || \
(defined(OS_LINUX) && !BUILDFLAG(IS_CHROMEOS_LACROS))
#include "chrome/browser/ui/web_applications/app_browser_controller.h"
Expand Down Expand Up @@ -1604,9 +1609,11 @@ class StartupBrowserWebAppProtocolHandlingTest : public InProcessBrowserTest {
return app_id;
}

void SetUpCommandlineAndStart(const std::string& url) {
void SetUpCommandlineAndStart(const std::string& url,
const web_app::AppId& app_id) {
base::CommandLine command_line(base::CommandLine::NO_PROGRAM);
command_line.AppendArg(url);
command_line.AppendSwitchASCII(switches::kAppId, app_id);

std::vector<Profile*> last_opened_profiles;
StartupBrowserCreator browser_creator;
Expand All @@ -1618,8 +1625,12 @@ class StartupBrowserWebAppProtocolHandlingTest : public InProcessBrowserTest {
base::test::ScopedFeatureList scoped_feature_list_;
};

IN_PROC_BROWSER_TEST_F(StartupBrowserWebAppProtocolHandlingTest,
WebAppLaunch_WebAppIsLaunchedWithProtocolUrl) {
IN_PROC_BROWSER_TEST_F(
StartupBrowserWebAppProtocolHandlingTest,
WebAppLaunch_WebAppIsNotLaunchedWithProtocolUrlAndDialogCancel) {
views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey{},
"WebAppProtocolHandlerIntentPickerView");

// Register web app as a protocol handler that should handle the launch.
blink::Manifest::ProtocolHandler protocol_handler;
const std::string handler_url = std::string(kStartUrl) + "/testing=%s";
Expand All @@ -1628,7 +1639,35 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserWebAppProtocolHandlingTest,
web_app::AppId app_id = InstallWebAppWithProtocolHandlers({protocol_handler});

// Launch the browser via a command line with a handled protocol URL param.
SetUpCommandlineAndStart("web+test://parameterString");
SetUpCommandlineAndStart("web+test://parameterString", app_id);

// The waiter will get the dialog when it shows up and close it.
waiter.WaitIfNeededAndGet()->CloseWithReason(
views::Widget::ClosedReason::kEscKeyPressed);

// Check that no extra window is launched.
ASSERT_EQ(1u, chrome::GetBrowserCount(browser()->profile()));
}

IN_PROC_BROWSER_TEST_F(
StartupBrowserWebAppProtocolHandlingTest,
WebAppLaunch_WebAppIsLaunchedWithProtocolUrlAndDialogAccept) {
views::NamedWidgetShownWaiter waiter(views::test::AnyWidgetTestPasskey{},
"WebAppProtocolHandlerIntentPickerView");

// Register web app as a protocol handler that should handle the launch.
blink::Manifest::ProtocolHandler protocol_handler;
const std::string handler_url = std::string(kStartUrl) + "/testing=%s";
protocol_handler.url = GURL(handler_url);
protocol_handler.protocol = u"web+test";
web_app::AppId app_id = InstallWebAppWithProtocolHandlers({protocol_handler});

// Launch the browser via a command line with a handled protocol URL param.
SetUpCommandlineAndStart("web+test://parameterString", app_id);

// The waiter will get the dialog when it shows up and accepts it.
waiter.WaitIfNeededAndGet()->CloseWithReason(
views::Widget::ClosedReason::kAcceptButtonClicked);

// Wait for app launch task to complete.
content::RunAllTasksUntilIdle();
Expand All @@ -1650,7 +1689,7 @@ IN_PROC_BROWSER_TEST_F(StartupBrowserWebAppProtocolHandlingTest,

IN_PROC_BROWSER_TEST_F(
StartupBrowserWebAppProtocolHandlingTest,
WebAppLaunch_WebAppIsNotLaunchedWithUnhandledProtocolUrl) {
WebAppLaunch_WebAppIsNotTranslatedWithUnhandledProtocolUrl) {
// Register web app as a protocol handler that should *not* handle the launch.
blink::Manifest::ProtocolHandler protocol_handler;
const std::string handler_url = std::string(kStartUrl) + "/testing=%s";
Expand All @@ -1659,26 +1698,26 @@ IN_PROC_BROWSER_TEST_F(
web_app::AppId app_id = InstallWebAppWithProtocolHandlers({protocol_handler});

// Launch the browser via a command line with an unhandled protocol URL param.
SetUpCommandlineAndStart("web+unhandled://parameterString");
SetUpCommandlineAndStart("web+unhandled://parameterString", app_id);

// Wait for app launch task to complete.
content::RunAllTasksUntilIdle();

// Check an app window is not launched.
// Check an app window is launched.
ASSERT_EQ(2u, chrome::GetBrowserCount(browser()->profile()));
Browser* app_browser;
app_browser = FindOneOtherBrowser(browser());
ASSERT_TRUE(app_browser);
EXPECT_FALSE(web_app::AppBrowserController::IsWebApp(app_browser));
EXPECT_TRUE(web_app::AppBrowserController::IsForWebApp(app_browser, app_id));

// Check the browser launches to a blank new tab page.
// Check the app is launched to the home page and not the translated URL.
TabStripModel* tab_strip = app_browser->tab_strip_model();
ASSERT_EQ(1, tab_strip->count());
content::WebContents* web_contents = tab_strip->GetWebContentsAt(0);
EXPECT_EQ(chrome::kChromeUINewTabURL, web_contents->GetVisibleURL());
EXPECT_EQ(GURL(kStartUrl), web_contents->GetVisibleURL());
}

#endif // defined(OS_WIN)
#endif // defined(OS_WIN) || defined(OS_MAC) || defined(OS_LINUX)

class StartupBrowserCreatorExtensionsCheckupExperimentTest
: public extensions::ExtensionBrowserTest {
Expand Down

0 comments on commit a5fc44e

Please sign in to comment.