glic: Fix GlicWindowController open animation state transition bugs.
The glic window should be shown after the open animation is finished and
glic is ready. Prior to this CL, this state transition was not correctly
being managed.
Bug: 391402352
Change-Id: I173cae0e6d0ff7c26d15933546081376b45e9363
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6188326
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1409773}
diff --git a/chrome/browser/glic/glic_window_controller.cc b/chrome/browser/glic/glic_window_controller.cc
index f16469b..1ba38a1 100644
--- a/chrome/browser/glic/glic_window_controller.cc
+++ b/chrome/browser/glic/glic_window_controller.cc
@@ -176,7 +176,6 @@
GlicWindowController::~GlicWindowController() = default;
void GlicWindowController::WebClientInitializeFailed() {
- // TODO(crbug.com/391402352): Set state.
if (state_ == State::kOpenAnimation ||
state_ == State::kWaitingForGlicToLoad) {
// TODO(crbug.com/388328847): The web client failed to initialize. Decide
@@ -201,11 +200,19 @@
}
void GlicWindowController::SetWebClient(GlicWebClientAccess* web_client) {
+ // If state_ == kClosed, then store web_client_ for a future call to Open().
+ // Once we get crash/error flows, this can theoretically happen with state_ ==
+ // kOpen, but those will those need to handled alongside the crash/error
+ // flows.
web_client_ = web_client;
+
+ // Always reset `glic_loaded_` since the web client has changed.
+ glic_loaded_ = false;
+
if (state_ == State::kOpenAnimation ||
state_ == State::kWaitingForGlicToLoad) {
if (web_client_) {
- ShowPhase2();
+ WaitForGlicToLoad();
} else {
// TODO(crbug.com/388328847): The web client could disconnect without a
// WebClientInitializeFailed() call, for example, if the renderer crashes.
@@ -297,18 +304,18 @@
views::CreateRoundedRectBackground(SK_ColorBLACK, 12));
// If there's a browser, then animate.
- AnimateBounds(
- target_bounds, base::Milliseconds(kEntryDurationMs),
- base::BindOnce(&GlicWindowController::SetWebContents, GetWeakPtr()));
+ AnimateBounds(target_bounds, base::Milliseconds(kEntryDurationMs),
+ base::BindOnce(&GlicWindowController::OpenAnimationFinished,
+ GetWeakPtr()));
} else {
// Otherwise, skip straight to waiting for glic to load.
- SetWebContents();
+ state_ = State::kWaitingForGlicToLoad;
+ GetGlicView()->web_view()->SetWebContents(contents_->web_contents());
}
- // If the web client is already initialized, go to phase 2. Otherwise, wait
- // for the web client to initialize.
+ // If the web client is already initialized, wait for it to load in parallel.
if (web_client_) {
- ShowPhase2();
+ WaitForGlicToLoad();
} else if (login_page_committed_) {
// This indicates that we've warmed the web client and it has hit a login
// page. See LoginPageCommitted.
@@ -316,16 +323,33 @@
}
}
-// Phase 2 of showing the widget. This happens after the web client is
-// initialized. It signals the web client that it will be shown, and waits for
-// the response before actually showing the widget.
-void GlicWindowController::ShowPhase2() {
+// This happens after the web client is initialized. It signals the web client
+// that it will be shown, and waits for the response before actually showing the
+// widget.
+void GlicWindowController::WaitForGlicToLoad() {
DCHECK(web_client_);
// Notify the web client that the panel will open, and wait for the response
// to actually show the window.
web_client_->PanelWillOpen(
CreatePanelState(true, attached_browser_),
- base::BindOnce(&GlicWindowController::ShowFinish, GetWeakPtr()));
+ base::BindOnce(&GlicWindowController::GlicLoaded, GetWeakPtr()));
+}
+
+void GlicWindowController::GlicLoaded() {
+ glic_loaded_ = true;
+ if (state_ == State::kWaitingForGlicToLoad) {
+ ShowFinish();
+ }
+}
+
+void GlicWindowController::OpenAnimationFinished() {
+ if (state_ == State::kOpenAnimation) {
+ state_ = State::kWaitingForGlicToLoad;
+ GetGlicView()->web_view()->SetWebContents(contents_->web_contents());
+ if (glic_loaded_) {
+ ShowFinish();
+ }
+ }
}
void GlicWindowController::ShowFinish() {
@@ -355,11 +379,6 @@
{{0, 0, final_widget_bounds_.width(), kWidgetTopBarHeight}});
}
-void GlicWindowController::SetWebContents() {
- GetGlicView()->web_view()->SetWebContents(contents_->web_contents());
- state_ = State::kWaitingForGlicToLoad;
-}
-
GlicView* GlicWindowController::GetGlicView() {
if (!glic_window_widget_) {
return nullptr;
diff --git a/chrome/browser/glic/glic_window_controller.h b/chrome/browser/glic/glic_window_controller.h
index 4e25bf1..011033d 100644
--- a/chrome/browser/glic/glic_window_controller.h
+++ b/chrome/browser/glic/glic_window_controller.h
@@ -141,17 +141,19 @@
views::Widget* GetGlicWidgetForTesting() { return glic_window_widget_.get(); }
private:
- // TODO(crbug.com/391402352): This method is misnamed. It's used to send a
- // message to glic indicating that the window is ready to show.
- void ShowPhase2();
+ // This sends a message to glic to get ready to show. This will eventually
+ // result in the callback GlicLoaded().
+ void WaitForGlicToLoad();
+ void GlicLoaded();
+
+ // Called when the open animation is finished.
+ void OpenAnimationFinished();
// TODO(crbug.com/391402352): This method is misnamed. It's used to send
// coordinate showing the window when glic and this class are both ready.
// However this class already shows the window via animation.
void ShowFinish();
- void SetWebContents();
-
// Determines the correct position for the glic window when attached to a
// browser window.
gfx::Point GetTopRightPositionForAttachedGlicWindow(
@@ -289,6 +291,9 @@
// window, or standalone. That is tracked by this member.
raw_ptr<Browser> attached_browser_ = nullptr;
+ // Set to true when glic is ready.
+ bool glic_loaded_ = false;
+
base::ObserverList<StateObserver> state_observers_;
base::WeakPtrFactory<GlicWindowController> weak_ptr_factory_{this};
