[dPWA] Make the WebAppInstallDialogCoordinator be a WidgetObserver
Instead of using a raw_ptr to a bubble dialog delegate and then using
a WeakPtr to itself everywhere inside the view, the install dialog
coordinator for web apps is now a WidgetObserver. This ensures:
1. Simple lifetimes, since the view inside the coordinator is
tracked only as long as the WidgetObserver is observing the main
dialog, which is more robust than the previous implementation of
a criss-cross raw_ptr, weak_ptr interface.
2. Practices separation of concerns, and brings the implementation
closer to a MVC model, where the behavior, view and its observation
happens independent of each other, with no overlapping logic.
As a side note, this also fixes the measurement of the
WebApp.InstallConfirmation.CloseReason by moving it to a central
location to be used by both the old and new install dialogs.
Bug: b/326418546
Change-Id: I7672d82d1683aa45ce9e51a37d3fef2e1d0c0a22
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5320614
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Dibyajyoti Pal <dibyapal@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1264651}
diff --git a/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc b/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc
index 8713de6c..965cc466 100644
--- a/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc
+++ b/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.cc
@@ -18,7 +18,6 @@
#include "chrome/browser/ui/views/chrome_layout_provider.h"
#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
#include "chrome/browser/ui/views/web_apps/web_app_info_image_source.h"
-#include "chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h"
#include "chrome/browser/ui/views/web_apps/web_app_views_utils.h"
#include "chrome/browser/ui/web_applications/web_app_dialogs.h"
#include "chrome/browser/web_applications/mojom/user_display_mode.mojom.h"
@@ -93,18 +92,15 @@
web_app::AppInstallationAcceptanceCallback callback,
web_app::PwaInProductHelpState iph_state,
PrefService* prefs,
- feature_engagement::Tracker* tracker,
- base::WeakPtr<web_app::WebAppInstallDialogCoordinator> dialog_coordinator)
+ feature_engagement::Tracker* tracker)
: LocationBarBubbleDelegateView(anchor_view, web_contents.get()),
web_contents_(web_contents),
- highlight_icon_button_(highlight_icon_button),
web_app_info_(std::move(web_app_info)),
install_tracker_(std::move(install_tracker)),
callback_(std::move(callback)),
iph_state_(iph_state),
prefs_(prefs),
- tracker_(tracker),
- dialog_coordinator_(dialog_coordinator) {
+ tracker_(tracker) {
SetCloseOnMainFrameOriginNavigation(true);
DCHECK(web_app_info_);
DCHECK(prefs_);
@@ -165,7 +161,7 @@
SetDefaultButton(ui::DIALOG_BUTTON_CANCEL);
- SetHighlightedButton(highlight_icon_button_);
+ SetHighlightedButton(highlight_icon_button);
}
PWAConfirmationBubbleView::~PWAConfirmationBubbleView() = default;
@@ -180,8 +176,6 @@
bool PWAConfirmationBubbleView::OnCloseRequested(
views::Widget::ClosedReason close_reason) {
- base::UmaHistogramEnumeration("WebApp.InstallConfirmation.CloseReason",
- close_reason);
webapps::MlInstallUserResponse response;
switch (close_reason) {
case views::Widget::ClosedReason::kAcceptButtonClicked:
@@ -210,16 +204,6 @@
}
void PWAConfirmationBubbleView::WindowClosing() {
- // Stop tracking the bubble view so that the PageActionIconView update can
- // read the state of the dialog from the BrowserUserData when it gets updated.
- if (dialog_coordinator_ && dialog_coordinator_->IsShowing()) {
- dialog_coordinator_->StopTracking();
- }
-
- if (highlight_icon_button_) {
- highlight_icon_button_->Update();
- }
-
// If |web_app_info_| is populated, then the bubble was not accepted.
if (web_app_info_) {
base::RecordAction(base::UserMetricsAction("WebAppInstallCancelled"));
diff --git a/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.h b/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.h
index 5379438..34444e6 100644
--- a/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.h
+++ b/chrome/browser/ui/views/web_apps/pwa_confirmation_bubble_view.h
@@ -9,7 +9,6 @@
#include "base/memory/raw_ptr.h"
#include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h"
-#include "chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h"
#include "chrome/browser/ui/web_applications/web_app_dialogs.h"
#include "chrome/browser/web_applications/web_app_install_info.h"
#include "components/prefs/pref_service.h"
@@ -51,9 +50,7 @@
web_app::AppInstallationAcceptanceCallback callback,
web_app::PwaInProductHelpState iph_state,
PrefService* prefs,
- feature_engagement::Tracker* tracker,
- base::WeakPtr<web_app::WebAppInstallDialogCoordinator>
- dialog_coordinator);
+ feature_engagement::Tracker* tracker);
METADATA_HEADER(PWAConfirmationBubbleView, views::BubbleDialogDelegateView)
public:
PWAConfirmationBubbleView(const PWAConfirmationBubbleView&) = delete;
@@ -80,7 +77,6 @@
private:
base::WeakPtr<content::WebContents> web_contents_;
- raw_ptr<PageActionIconView> highlight_icon_button_ = nullptr;
std::unique_ptr<web_app::WebAppInstallInfo> web_app_info_;
std::unique_ptr<webapps::MlInstallOperationTracker> install_tracker_;
web_app::AppInstallationAcceptanceCallback callback_;
@@ -91,7 +87,6 @@
web_app::PwaInProductHelpState iph_state_;
raw_ptr<PrefService> prefs_;
raw_ptr<feature_engagement::Tracker> tracker_;
- base::WeakPtr<web_app::WebAppInstallDialogCoordinator> dialog_coordinator_;
};
#endif // CHROME_BROWSER_UI_VIEWS_WEB_APPS_PWA_CONFIRMATION_BUBBLE_VIEW_H_
diff --git a/chrome/browser/ui/views/web_apps/simple_install_dialog_bubble_view_browsertest.cc b/chrome/browser/ui/views/web_apps/simple_install_dialog_bubble_view_browsertest.cc
index dd2242f..8c6f123 100644
--- a/chrome/browser/ui/views/web_apps/simple_install_dialog_bubble_view_browsertest.cc
+++ b/chrome/browser/ui/views/web_apps/simple_install_dialog_bubble_view_browsertest.cc
@@ -31,9 +31,10 @@
#include "components/webapps/browser/installable/ml_installability_promoter.h"
#include "components/webapps/common/web_app_id.h"
#include "content/public/test/browser_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
#include "ui/views/test/widget_test.h"
#include "ui/views/widget/any_widget_observer.h"
-#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/widget/widget.h"
namespace web_app {
namespace {
@@ -117,7 +118,6 @@
GetInstallTracker(browser());
base::RunLoop loop;
- // Show the PWA install dialog.
ShowPWAInstallBubble(
browser()->tab_strip_model()->GetActiveWebContents(), std::move(app_info),
std::move(install_tracker),
@@ -131,13 +131,38 @@
GetBubbleView(browser())->CancelDialog();
loop.Run();
- // TOOD(b/326418546): Figure out a way to support widget close reason in new
- // install dialog.
- if (!UniversalInstallEnabled()) {
histograms.ExpectUniqueSample(
"WebApp.InstallConfirmation.CloseReason",
views::Widget::ClosedReason::kCancelButtonClicked, 1);
- }
+}
+
+IN_PROC_BROWSER_TEST_P(SimpleInstallDialogBubbleViewBrowserTest,
+ AcceptDialogReportsMetrics) {
+ auto app_info = GetAppInfo();
+
+ std::unique_ptr<webapps::MlInstallOperationTracker> install_tracker =
+ GetInstallTracker(browser());
+
+ base::RunLoop loop;
+ ShowPWAInstallBubble(
+ browser()->tab_strip_model()->GetActiveWebContents(), std::move(app_info),
+ std::move(install_tracker),
+ base::BindLambdaForTesting(
+ [&](bool accepted,
+ std::unique_ptr<WebAppInstallInfo> app_info_callback) {
+ loop.Quit();
+ }));
+
+ base::HistogramTester histogram_tester;
+ views::test::WidgetDestroyedWaiter destroyed_waiter(
+ GetBubbleView(browser())->GetWidget());
+ GetBubbleView(browser())->AcceptDialog();
+ loop.Run();
+ destroyed_waiter.Wait();
+
+ histogram_tester.ExpectUniqueSample(
+ "WebApp.InstallConfirmation.CloseReason",
+ views::Widget::ClosedReason::kAcceptButtonClicked, 1);
}
IN_PROC_BROWSER_TEST_P(SimpleInstallDialogBubbleViewBrowserTest,
@@ -146,7 +171,6 @@
GURL start_url = app_info->start_url;
base::RunLoop loop;
- // Show the PWA install dialog.
std::unique_ptr<webapps::MlInstallOperationTracker> install_tracker =
GetInstallTracker(browser());
content::WebContents* web_contents =
@@ -256,12 +280,8 @@
ASSERT_TRUE(dialog_accepted_);
ASSERT_FALSE(dialog_accepted_.value());
- // TOOD(b/326418546): Figure out a way to support widget close reason in new
- // install dialog.
- if (!UniversalInstallEnabled()) {
histograms.ExpectUniqueSample("WebApp.InstallConfirmation.CloseReason",
views::Widget::ClosedReason::kUnspecified, 1);
- }
}
INSTANTIATE_TEST_SUITE_P(All,
diff --git a/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.cc b/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.cc
index 54efedc..ad6e9ca0 100644
--- a/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.cc
+++ b/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.cc
@@ -5,18 +5,24 @@
#include "chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h"
#include "base/memory/weak_ptr.h"
+#include "base/metrics/histogram_functions.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_user_data.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
+#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
#include "ui/views/bubble/bubble_dialog_delegate_view.h"
namespace web_app {
WebAppInstallDialogCoordinator::~WebAppInstallDialogCoordinator() {
- dialog_delegate_ = nullptr;
+ if (IsShowing()) {
+ StopTracking();
+ }
}
bool WebAppInstallDialogCoordinator::IsShowing() {
- return dialog_delegate_ != nullptr;
+ return widget_observation_.IsObserving();
}
views::BubbleDialogDelegate* WebAppInstallDialogCoordinator::GetBubbleView() {
@@ -28,17 +34,44 @@
CHECK(!IsShowing()) << "Cannot track a new install dialog if an existing "
"one is already open";
dialog_delegate_ = bubble_view;
+ auto* widget = bubble_view->GetWidget();
+ widget_observation_.Observe(widget);
+}
+
+void WebAppInstallDialogCoordinator::OnWidgetDestroying(views::Widget* widget) {
+ CHECK_EQ(widget, dialog_delegate_->GetWidget());
+ base::UmaHistogramEnumeration("WebApp.InstallConfirmation.CloseReason",
+ widget->closed_reason());
+ StopTracking();
}
void WebAppInstallDialogCoordinator::StopTracking() {
CHECK(IsShowing()) << "Cannot stop tracking install dialog when it was not "
"being tracked previously";
+ widget_observation_.Reset();
dialog_delegate_ = nullptr;
+ MaybeUpdatePwaAnchorViewIfNeeded();
}
-base::WeakPtr<WebAppInstallDialogCoordinator>
-WebAppInstallDialogCoordinator::AsWeakPtr() {
- return weak_ptr_factory_.GetWeakPtr();
+void WebAppInstallDialogCoordinator::MaybeUpdatePwaAnchorViewIfNeeded() {
+ Browser* browser = &GetBrowser();
+ if (!browser) {
+ return;
+ }
+ BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser);
+
+ if (browser_view && browser_view->toolbar_button_provider()) {
+ PageActionIconView* install_icon =
+ browser_view->toolbar_button_provider()->GetPageActionIconView(
+ PageActionIconType::kPwaInstall);
+ if (install_icon) {
+ // This ensures that quick navigations in between an installable and
+ // non-installable site hides the anchor view. See the test
+ // |IconVisibilityAfterTabSwitchingWhenPWAConfirmationBubbleViewShowing|
+ // for more information on why this was needed.
+ install_icon->Update();
+ }
+ }
}
WebAppInstallDialogCoordinator::WebAppInstallDialogCoordinator(Browser* browser)
diff --git a/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h b/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h
index 3776892..bf843c0 100644
--- a/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h
+++ b/chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h
@@ -5,37 +5,37 @@
#ifndef CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_INSTALL_DIALOG_COORDINATOR_H_
#define CHROME_BROWSER_UI_VIEWS_WEB_APPS_WEB_APP_INSTALL_DIALOG_COORDINATOR_H_
-#include "base/memory/weak_ptr.h"
#include "chrome/browser/ui/browser_user_data.h"
-#include "chrome/browser/ui/views/web_apps/web_app_icon_name_and_origin_view.h"
#include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/widget/widget_observer.h"
class Browser;
namespace web_app {
-// TODO(b/326418546): Tracking using a weak_ptr and a raw_ptr to the bubble
-// dialog delegate may lead to complicated ownership issues. Move to a
-// WidgetObserver class, or store a weakptr to a BubbleDialogDelegate instead
-// (needs views/ code modification and some scoping).
class WebAppInstallDialogCoordinator
- : public BrowserUserData<WebAppInstallDialogCoordinator> {
+ : public BrowserUserData<WebAppInstallDialogCoordinator>,
+ public views::WidgetObserver {
public:
~WebAppInstallDialogCoordinator() override;
bool IsShowing();
views::BubbleDialogDelegate* GetBubbleView();
void StartTracking(views::BubbleDialogDelegate* view);
- void StopTracking();
- base::WeakPtr<WebAppInstallDialogCoordinator> AsWeakPtr();
+
+ // WidgetObserver overrides:
+ void OnWidgetDestroying(views::Widget* widget) override;
private:
explicit WebAppInstallDialogCoordinator(Browser* browser);
friend BrowserUserData;
- raw_ptr<views::BubbleDialogDelegate> dialog_delegate_ = nullptr;
+ void StopTracking();
+ void MaybeUpdatePwaAnchorViewIfNeeded();
- base::WeakPtrFactory<WebAppInstallDialogCoordinator> weak_ptr_factory_{this};
+ raw_ptr<views::BubbleDialogDelegate> dialog_delegate_ = nullptr;
+ base::ScopedObservation<views::Widget, views::WidgetObserver>
+ widget_observation_{this};
BROWSER_USER_DATA_KEY_DECL();
};
diff --git a/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.cc b/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.cc
index 983f6fb..d0f72f2 100644
--- a/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.cc
+++ b/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.cc
@@ -13,7 +13,6 @@
#include "chrome/browser/ui/views/frame/browser_view.h"
#include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
#include "chrome/browser/ui/views/page_action/page_action_icon_view.h"
-#include "chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h"
#include "chrome/browser/ui/web_applications/web_app_dialogs.h"
#include "chrome/browser/web_applications/web_app_helpers.h"
#include "chrome/browser/web_applications/web_app_install_info.h"
@@ -56,9 +55,7 @@
PwaInProductHelpState iph_state,
PrefService* prefs,
feature_engagement::Tracker* tracker,
- InstallDialogType dialog_type,
- std::optional<base::WeakPtr<web_app::WebAppInstallDialogCoordinator>>
- dialog_coordinator)
+ InstallDialogType dialog_type)
: content::WebContentsObserver(web_contents),
web_contents_(web_contents),
install_info_(std::move(web_app_info)),
@@ -67,8 +64,7 @@
iph_state_(std::move(iph_state)),
prefs_(prefs),
tracker_(tracker),
- dialog_type_(dialog_type),
- dialog_coordinator_(dialog_coordinator) {
+ dialog_type_(dialog_type) {
CHECK(install_info_);
CHECK(install_info_->manifest_id.is_valid());
CHECK(install_tracker_);
@@ -76,11 +72,6 @@
}
WebAppInstallDialogDelegate::~WebAppInstallDialogDelegate() {
- if (dialog_coordinator_.has_value() &&
- dialog_coordinator_.value()->IsShowing()) {
- dialog_coordinator_.value()->StopTracking();
- }
-
// TODO(crbug.com/1327363): move this to dialog->SetHighlightedButton.
Browser* browser = chrome::FindBrowserWithTab(web_contents_);
if (!browser) {
@@ -95,9 +86,7 @@
PageActionIconType::kPwaInstall);
if (install_icon) {
// Dehighlight the install icon when this dialog is closed.
- browser_view->toolbar_button_provider()
- ->GetPageActionIconView(PageActionIconType::kPwaInstall)
- ->SetHighlighted(false);
+ install_icon->SetHighlighted(false);
}
}
}
@@ -179,11 +168,6 @@
}
void WebAppInstallDialogDelegate::MeasureIphOnDialogClose() {
- if (dialog_coordinator_.has_value() &&
- dialog_coordinator_.value()->IsShowing()) {
- dialog_coordinator_.value()->StopTracking();
- }
-
if (callback_.is_null()) {
return;
}
diff --git a/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.h b/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.h
index cc4430e4..7fcd0f42 100644
--- a/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.h
+++ b/chrome/browser/ui/views/web_apps/web_app_install_dialog_delegate.h
@@ -9,12 +9,12 @@
#include "base/memory/raw_ptr.h"
#include "base/memory/weak_ptr.h"
-#include "chrome/browser/ui/views/web_apps/web_app_install_dialog_coordinator.h"
#include "chrome/browser/ui/web_applications/web_app_dialogs.h"
#include "chrome/browser/web_applications/web_app_install_info.h"
#include "content/public/browser/web_contents_observer.h"
#include "ui/base/models/dialog_model.h"
#include "ui/views/widget/widget.h"
+#include "ui/views/widget/widget_observer.h"
class PrefService;
@@ -45,8 +45,6 @@
: public ui::DialogModelDelegate,
public content::WebContentsObserver {
public:
- // dialog_coordinator is std::optional since the detailed install dialog uses
- // this delegate but is not tracked by the PWA action view.
WebAppInstallDialogDelegate(
content::WebContents* web_contents,
std::unique_ptr<WebAppInstallInfo> install_info,
@@ -55,9 +53,7 @@
PwaInProductHelpState iph_state,
PrefService* prefs,
feature_engagement::Tracker* tracker,
- InstallDialogType dialog_type,
- std::optional<base::WeakPtr<web_app::WebAppInstallDialogCoordinator>>
- dialog_coordinator = std::nullopt);
+ InstallDialogType dialog_type);
~WebAppInstallDialogDelegate() override;
@@ -87,8 +83,6 @@
raw_ptr<PrefService> prefs_;
raw_ptr<feature_engagement::Tracker> tracker_;
InstallDialogType dialog_type_;
- std::optional<base::WeakPtr<web_app::WebAppInstallDialogCoordinator>>
- dialog_coordinator_;
base::WeakPtrFactory<WebAppInstallDialogDelegate> weak_ptr_factory_{
this};
diff --git a/chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc b/chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc
index c600cfa..f0d592f 100644
--- a/chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc
+++ b/chrome/browser/ui/views/web_apps/web_app_simple_install_dialog.cc
@@ -110,7 +110,7 @@
auto delegate = std::make_unique<web_app::WebAppInstallDialogDelegate>(
web_contents, std::move(web_app_info), std::move(install_tracker),
std::move(callback), std::move(iph_state), prefs, tracker,
- InstallDialogType::kSimple, dialog_coordinator->AsWeakPtr());
+ InstallDialogType::kSimple);
auto delegate_weak_ptr = delegate->AsWeakPtr();
auto dialog_model =
@@ -150,7 +150,7 @@
auto* dialog_view = new PWAConfirmationBubbleView(
anchor_view, web_contents->GetWeakPtr(), icon, std::move(web_app_info),
std::move(install_tracker), std::move(callback), iph_state, prefs,
- tracker, dialog_coordinator->AsWeakPtr());
+ tracker);
if (g_dont_close_on_deactivate) {
dialog_view->set_close_on_deactivate(false);
}
@@ -158,7 +158,6 @@
views::BubbleDialogDelegateView::CreateBubble(dialog_view)->Show();
dialog_delegate = dialog_view->AsBubbleDialogDelegate();
dialog_coordinator->StartTracking(dialog_delegate);
-
if (icon) {
icon->Update();
DCHECK(icon->GetVisible());
