Skip to content

Commit

Permalink
[Desktop PWAs] Remove force_shortcut_app.
Browse files Browse the repository at this point in the history
This CL refactors away the force_shortcut_app boolean that is forwarded
through all of WebAppInstallTask in favor of a CreateWebAppFlow enum
which describes the UI experience that is desired based on the button
that led to the install.

This CL should have no functional changes and is a precursor for
https://chromium-review.googlesource.com/c/chromium/src/+/3508988/.

Bug: 1216457
Change-Id: I17497af0349fc5b8d9697944bb36ed9d8520e7c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3509342
Reviewed-by: Mike Wasserman <msw@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Commit-Queue: Christopher Lam <calamity@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984166}
  • Loading branch information
nik3daz authored and Chromium LUCI CQ committed Mar 23, 2022
1 parent b0be273 commit 3fdfb4b
Show file tree
Hide file tree
Showing 17 changed files with 125 additions and 131 deletions.
Expand Up @@ -112,7 +112,7 @@ class TwoClientWebAppsBMOSyncTest : public WebAppsSyncTestBase {
->install_manager()
.InstallWebAppFromManifestWithFallback(
browser->tab_strip_model()->GetActiveWebContents(),
/*force_shortcut_app=*/false, source,
WebAppInstallManager::WebAppInstallFlow::kInstallSite, source,
base::BindOnce(test::TestAcceptDialogCallback),
base::BindLambdaForTesting([&](const AppId& new_app_id,
webapps::InstallResultCode code) {
Expand Down
7 changes: 5 additions & 2 deletions chrome/browser/ui/browser_command_controller.cc
Expand Up @@ -47,6 +47,7 @@
#include "chrome/browser/ui/web_applications/web_app_dialog_utils.h"
#include "chrome/browser/ui/web_applications/web_app_launch_utils.h"
#include "chrome/browser/ui/webui/inspect_ui.h"
#include "chrome/browser/web_applications/web_app_install_manager.h"
#include "chrome/common/chrome_features.h"
#include "chrome/common/content_restriction.h"
#include "chrome/common/pref_names.h"
Expand Down Expand Up @@ -688,12 +689,14 @@ bool BrowserCommandController::ExecuteCommandWithDisposition(
case IDC_CREATE_SHORTCUT:
base::RecordAction(base::UserMetricsAction("CreateShortcut"));
web_app::CreateWebAppFromCurrentWebContents(
browser_, true /* force_shortcut_app */);
browser_,
web_app::WebAppInstallManager::WebAppInstallFlow::kCreateShortcut);
break;
case IDC_INSTALL_PWA:
base::RecordAction(base::UserMetricsAction("InstallWebAppFromMenu"));
web_app::CreateWebAppFromCurrentWebContents(
browser_, false /* force_shortcut_app */);
browser_,
web_app::WebAppInstallManager::WebAppInstallFlow::kInstallSite);
break;
case IDC_DEV_TOOLS:
ToggleDevToolsWindow(browser_, DevToolsToggleAction::Show(),
Expand Down
Expand Up @@ -147,13 +147,12 @@ IN_PROC_BROWSER_TEST_F(WebAppTabStripBrowserTest, PopOutTabOnInstall) {
test::WaitUntilReady(provider);
provider->install_manager().InstallWebAppFromManifestWithFallback(
browser()->tab_strip_model()->GetActiveWebContents(),
/*force_shortcut_app=*/false,
WebAppInstallManager::WebAppInstallFlow::kInstallSite,
webapps::WebappInstallSource::MENU_BROWSER_TAB,
/*dialog_callback=*/
base::BindLambdaForTesting(
[](content::WebContents*,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite,
WebAppInstallationAcceptanceCallback acceptance_callback) {
web_app_info->user_display_mode = DisplayMode::kTabbed;
std::move(acceptance_callback)
Expand Down
Expand Up @@ -87,7 +87,6 @@ namespace {
void AutoAcceptDialogCallback(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback) {
web_app_info->user_display_mode = DisplayMode::kStandalone;
std::move(acceptance_callback)
Expand Down Expand Up @@ -137,7 +136,7 @@ AppId InstallWebAppFromPage(Browser* browser, const GURL& app_url) {
test::WaitUntilReady(provider);
provider->install_manager().InstallWebAppFromManifestWithFallback(
browser->tab_strip_model()->GetActiveWebContents(),
/*force_shortcut_app=*/false,
WebAppInstallManager::WebAppInstallFlow::kInstallSite,
webapps::WebappInstallSource::MENU_BROWSER_TAB,
base::BindOnce(&AutoAcceptDialogCallback),
base::BindLambdaForTesting(
Expand Down Expand Up @@ -166,7 +165,7 @@ AppId InstallWebAppFromManifest(Browser* browser, const GURL& app_url) {
test::WaitUntilReady(provider);
provider->install_manager().InstallWebAppFromManifestWithFallback(
browser->tab_strip_model()->GetActiveWebContents(),
/*force_shortcut_app=*/false,
WebAppInstallManager::WebAppInstallFlow::kInstallSite,
webapps::WebappInstallSource::MENU_BROWSER_TAB,
base::BindOnce(&AutoAcceptDialogCallback),
base::BindLambdaForTesting(
Expand Down
50 changes: 31 additions & 19 deletions chrome/browser/ui/web_applications/web_app_dialog_utils.cc
Expand Up @@ -30,26 +30,35 @@

namespace web_app {

using WebAppInstallFlow = WebAppInstallManager::WebAppInstallFlow;

namespace {

void OnWebAppInstallShowInstallDialog(
WebAppInstallFlow flow,
webapps::WebappInstallSource install_source,
chrome::PwaInProductHelpState iph_state,
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback web_app_acceptance_callback) {
DCHECK(web_app_info);
if (for_installable_site == ForInstallableSite::kYes) {
web_app_info->user_display_mode = blink::mojom::DisplayMode::kStandalone;
chrome::ShowPWAInstallBubble(
initiator_web_contents, std::move(web_app_info),
std::move(web_app_acceptance_callback), iph_state);
} else {
chrome::ShowWebAppInstallDialog(initiator_web_contents,
std::move(web_app_info),
std::move(web_app_acceptance_callback));

switch (flow) {
case WebAppInstallFlow::kInstallSite:
web_app_info->user_display_mode = blink::mojom::DisplayMode::kStandalone;
chrome::ShowPWAInstallBubble(
initiator_web_contents, std::move(web_app_info),
std::move(web_app_acceptance_callback), iph_state);
return;
case WebAppInstallFlow::kCreateShortcut:
chrome::ShowWebAppInstallDialog(initiator_web_contents,
std::move(web_app_info),
std::move(web_app_acceptance_callback));
return;
case WebAppInstallFlow::kUnknown:
NOTREACHED();
}
NOTREACHED();
}

WebAppInstalledCallback& GetInstalledCallbackForTesting() {
Expand Down Expand Up @@ -93,8 +102,9 @@ bool CanPopOutWebApp(Profile* profile) {
!profile->IsOffTheRecord();
}

void CreateWebAppFromCurrentWebContents(Browser* browser,
bool force_shortcut_app) {
void CreateWebAppFromCurrentWebContents(
Browser* browser,
WebAppInstallManager::WebAppInstallFlow flow) {
DCHECK(CanCreateWebApp(browser));

content::WebContents* web_contents =
Expand All @@ -107,15 +117,16 @@ void CreateWebAppFromCurrentWebContents(Browser* browser,

webapps::WebappInstallSource install_source =
webapps::InstallableMetrics::GetInstallSource(
web_contents, force_shortcut_app
? webapps::InstallTrigger::CREATE_SHORTCUT
: webapps::InstallTrigger::MENU);
web_contents,
flow == WebAppInstallManager::WebAppInstallFlow::kCreateShortcut
? webapps::InstallTrigger::CREATE_SHORTCUT
: webapps::InstallTrigger::MENU);

WebAppInstalledCallback callback = base::DoNothing();

provider->install_manager().InstallWebAppFromManifestWithFallback(
web_contents, force_shortcut_app, install_source,
base::BindOnce(OnWebAppInstallShowInstallDialog, install_source,
web_contents, flow, install_source,
base::BindOnce(OnWebAppInstallShowInstallDialog, flow, install_source,
chrome::PwaInProductHelpState::kNotShown),
base::BindOnce(OnWebAppInstalled, std::move(callback)));
}
Expand All @@ -131,8 +142,9 @@ bool CreateWebAppFromManifest(content::WebContents* web_contents,

provider->install_manager().InstallWebAppFromManifest(
web_contents, bypass_service_worker_check, install_source,
base::BindOnce(OnWebAppInstallShowInstallDialog, install_source,
iph_state),
base::BindOnce(OnWebAppInstallShowInstallDialog,
WebAppInstallManager::WebAppInstallFlow::kInstallSite,
install_source, iph_state),
base::BindOnce(OnWebAppInstalled, std::move(installed_callback)));
return true;
}
Expand Down
9 changes: 4 additions & 5 deletions chrome/browser/ui/web_applications/web_app_dialog_utils.h
Expand Up @@ -8,6 +8,7 @@
#include "base/callback_forward.h"
#include "chrome/browser/ui/browser_dialogs.h"
#include "chrome/browser/web_applications/web_app_id.h"
#include "chrome/browser/web_applications/web_app_install_manager.h"

class Browser;
class Profile;
Expand Down Expand Up @@ -38,11 +39,9 @@ using WebAppInstalledCallback =
webapps::InstallResultCode code)>;

// Initiates user install of a WebApp for the current page.
// If |force_shortcut_app| is true, the current page will be installed even if
// the site does not meet installability requirements (see
// |AppBannerManager::PerformInstallableCheck|).
void CreateWebAppFromCurrentWebContents(Browser* browser,
bool force_shortcut_app);
void CreateWebAppFromCurrentWebContents(
Browser* browser,
WebAppInstallManager::WebAppInstallFlow flow);

// Starts install of a WebApp for a given |web_contents|, initiated from
// a promotional banner or omnibox install icon.
Expand Down
Expand Up @@ -401,7 +401,7 @@ class ManifestUpdateManagerBrowserTest : public InProcessBrowserTest {
base::RunLoop run_loop;
GetProvider().install_manager().InstallWebAppFromManifestWithFallback(
browser()->tab_strip_model()->GetActiveWebContents(),
/*force_shortcut_app=*/false,
WebAppInstallManager::WebAppInstallFlow::kInstallSite,
webapps::WebappInstallSource::OMNIBOX_INSTALL_ICON,
base::BindOnce(test::TestAcceptDialogCallback),
base::BindLambdaForTesting(
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/web_applications/test/web_app_test_utils.cc
Expand Up @@ -500,7 +500,6 @@ std::unique_ptr<WebApp> CreateRandomWebApp(const GURL& base_url,
void TestAcceptDialogCallback(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(acceptance_callback), true /*accept*/,
Expand All @@ -510,7 +509,6 @@ void TestAcceptDialogCallback(
void TestDeclineDialogCallback(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback) {
base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(acceptance_callback),
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/web_applications/test/web_app_test_utils.h
Expand Up @@ -33,13 +33,11 @@ std::unique_ptr<WebApp> CreateRandomWebApp(const GURL& base_url, uint32_t seed);
void TestAcceptDialogCallback(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback);

void TestDeclineDialogCallback(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback);

AppId InstallPwaForCurrentUrl(Browser* browser);
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/web_applications/web_app_install_manager.cc
Expand Up @@ -146,7 +146,7 @@ void WebAppInstallManager::InstallWebAppFromManifest(

void WebAppInstallManager::InstallWebAppFromManifestWithFallback(
content::WebContents* contents,
bool force_shortcut_app,
WebAppInstallFlow flow,
webapps::WebappInstallSource install_source,
WebAppInstallDialogCallback dialog_callback,
OnceInstallCallback callback) {
Expand All @@ -156,7 +156,7 @@ void WebAppInstallManager::InstallWebAppFromManifestWithFallback(
auto task = std::make_unique<WebAppInstallTask>(
profile_, this, finalizer_, data_retriever_factory_.Run(), registrar_);
task->InstallWebAppFromManifestWithFallback(
contents, force_shortcut_app, install_source, std::move(dialog_callback),
contents, flow, install_source, std::move(dialog_callback),
base::BindOnce(&WebAppInstallManager::OnInstallTaskCompleted,
GetWeakPtr(), task.get(), std::move(callback)));

Expand Down Expand Up @@ -225,7 +225,7 @@ void WebAppInstallManager::InstallWebAppFromInfo(
}
task->InstallWebAppFromInfo(
std::move(web_application_info), overwrite_existing_manifest_fields,
for_installable_site, install_source,
install_source,
base::BindOnce(&WebAppInstallManager::OnInstallTaskCompleted,
GetWeakPtr(), task.get(), std::move(callback)));

Expand Down
18 changes: 14 additions & 4 deletions chrome/browser/web_applications/web_app_install_manager.h
Expand Up @@ -45,6 +45,18 @@ class WebAppRegistrar;
// TODO(loyso): Unify the API and merge similar InstallWebAppZZZZ functions.
class WebAppInstallManager final : public SyncInstallDelegate {
public:
// The different UI flows that exist for creating a web app.
enum class WebAppInstallFlow {
// TODO(crbug.com/1216457): This should be removed by adding all known flows
// to this enum.
kUnknown,
// The 'Create Shortcut' flow for adding the current page as a shortcut app.
kCreateShortcut,
// The 'Install Site' flow for installing the current site with an app
// experience determined by the site.
kInstallSite,
};

explicit WebAppInstallManager(Profile* profile);
WebAppInstallManager(const WebAppInstallManager&) = delete;
WebAppInstallManager& operator=(const WebAppInstallManager&) = delete;
Expand Down Expand Up @@ -73,12 +85,10 @@ class WebAppInstallManager final : public SyncInstallDelegate {

// Infers WebApp info from the blink renderer process and then retrieves a
// manifest in a way similar to |InstallWebAppFromManifest|. If the manifest
// is incomplete or missing, the inferred info is used. |force_shortcut_app|
// forces the creation of a shortcut app instead of a PWA even if installation
// is available.
// is incomplete or missing, the inferred info is used.
void InstallWebAppFromManifestWithFallback(
content::WebContents* contents,
bool force_shortcut_app,
WebAppInstallFlow flow,
webapps::WebappInstallSource install_source,
WebAppInstallDialogCallback dialog_callback,
OnceInstallCallback callback);
Expand Down
Expand Up @@ -293,7 +293,7 @@ class WebAppInstallManagerTest
InstallResult result;
base::RunLoop run_loop;
install_manager().InstallWebAppFromManifestWithFallback(
web_contents(), /*force_shortcut_app=*/false,
web_contents(), WebAppInstallManager::WebAppInstallFlow::kInstallSite,
webapps::WebappInstallSource::OMNIBOX_INSTALL_ICON,
base::BindOnce(test::TestAcceptDialogCallback),
base::BindLambdaForTesting([&](const AppId& installed_app_id,
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/web_applications/web_app_install_params.h
Expand Up @@ -43,7 +43,6 @@ using WebAppInstallationAcceptanceCallback =
using WebAppInstallDialogCallback = base::OnceCallback<void(
content::WebContents* initiator_web_contents,
std::unique_ptr<WebAppInstallInfo> web_app_info,
ForInstallableSite for_installable_site,
WebAppInstallationAcceptanceCallback acceptance_callback)>;

enum class InstallableCheckResult {
Expand Down

0 comments on commit 3fdfb4b

Please sign in to comment.