[iOS] Only present when the presentation context is active.
Previously, when the OverlayPresentationContext's coordinator is reset,
the presented UI was dismissed without notifying the presenter. This
CL updates the implementation to have OverlayPresenter drive the
dismissal when the coordinator is reset. This allows for the presenter
to be aware of all presentation/dismissal events so that its observers
can be notified when they occur (e.g. at the end of the tab grid =>
BVC transition animation).
To pipe this information back to OverlayPresenter, the presentation
context exposes IsActive(), which describes whether the context is
currently able to present. When a coordinator is provided to the
context implementation, it notifies the presenter via observation that
it has become active, which triggers presentation. When the coordinator
is reset to nil upon the BVC leaving the window, the presenter is
again notified via observation that the context is deactivating, which
triggers dismissal.
Bug: 941745
Change-Id: If06a9cd2aed18f34cf820423b42f24df78d22eea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1672173
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672685}
diff --git a/ios/chrome/browser/overlays/BUILD.gn b/ios/chrome/browser/overlays/BUILD.gn
index cc1dd0ea..0985ace 100644
--- a/ios/chrome/browser/overlays/BUILD.gn
+++ b/ios/chrome/browser/overlays/BUILD.gn
@@ -7,6 +7,7 @@
"public/overlay_dismissal_callback.h",
"public/overlay_modality.h",
"public/overlay_presentation_context.h",
+ "public/overlay_presentation_context_observer.h",
"public/overlay_presenter.h",
"public/overlay_presenter_observer.h",
"public/overlay_request.h",
diff --git a/ios/chrome/browser/overlays/overlay_presenter_impl.h b/ios/chrome/browser/overlays/overlay_presenter_impl.h
index e3f1fe7..7f3110c 100644
--- a/ios/chrome/browser/overlays/overlay_presenter_impl.h
+++ b/ios/chrome/browser/overlays/overlay_presenter_impl.h
@@ -10,6 +10,7 @@
#import "ios/chrome/browser/overlays/overlay_request_queue_impl.h"
#import "ios/chrome/browser/overlays/public/overlay_dismissal_callback.h"
#import "ios/chrome/browser/overlays/public/overlay_modality.h"
+#import "ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h"
#import "ios/chrome/browser/overlays/public/overlay_presenter.h"
#import "ios/chrome/browser/overlays/public/overlay_user_data.h"
#import "ios/chrome/browser/web_state_list/web_state_list_observer.h"
@@ -20,6 +21,7 @@
// - manages hiding and showing overlays for active WebState changes.
class OverlayPresenterImpl : public BrowserObserver,
public OverlayPresenter,
+ public OverlayPresentationContextObserver,
public OverlayRequestQueueImpl::Observer,
public WebStateListObserver {
public:
@@ -101,6 +103,13 @@
void QueuedRequestCancelled(OverlayRequestQueueImpl* queue,
OverlayRequest* request) override;
+ // OverlayPresentationContextObserver:
+ void OverlayPresentationContextWillChangeActivationState(
+ OverlayPresentationContext* presentation_context,
+ bool activating) override;
+ void OverlayPresentationContextDidChangeActivationState(
+ OverlayPresentationContext* presentation_context) override;
+
// WebStateListObserver:
void WebStateInsertedAt(WebStateList* web_state_list,
web::WebState* web_state,
diff --git a/ios/chrome/browser/overlays/overlay_presenter_impl.mm b/ios/chrome/browser/overlays/overlay_presenter_impl.mm
index 561b01e..9fdfe2d 100644
--- a/ios/chrome/browser/overlays/overlay_presenter_impl.mm
+++ b/ios/chrome/browser/overlays/overlay_presenter_impl.mm
@@ -82,16 +82,21 @@
// overlays in the new presentation context. Cancel overlay state from the
// previous context since this Browser's overlays will no longer be presented
// there.
- if (presentation_context_)
+ if (presentation_context_) {
CancelAllOverlayUI();
+ presentation_context_->RemoveObserver(this);
+ }
presentation_context_ = presentation_context;
// Reset |presenting| since it was tracking the status for the previous
// delegate's presentation context.
presenting_ = false;
- if (presentation_context_)
+
+ if (presentation_context_) {
+ presentation_context_->AddObserver(this);
PresentOverlayForActiveRequest();
+ }
}
void OverlayPresenterImpl::AddObserver(OverlayPresenterObserver* observer) {
@@ -178,8 +183,8 @@
// Overlays cannot be presented if one is already presented.
DCHECK(!presenting_);
- // Overlays cannot be shown without a UI delegate.
- if (!presentation_context_)
+ // Overlays cannot be shown without an active presentation context.
+ if (!presentation_context_ || !presentation_context_->IsActive())
return;
// No presentation is necessary if there is no active reqeust.
@@ -286,6 +291,26 @@
CancelOverlayUIForRequest(request);
}
+#pragma mark - OverlayPresentationContextObserver
+
+void OverlayPresenterImpl::OverlayPresentationContextWillChangeActivationState(
+ OverlayPresentationContext* presentation_context,
+ bool activating) {
+ DCHECK_EQ(presentation_context_, presentation_context);
+ // Hide the presented overlay UI if the presentation context is deactivating.
+ if (!activating && presenting_)
+ presentation_context_->HideOverlayUI(this, GetActiveRequest());
+}
+
+void OverlayPresenterImpl::OverlayPresentationContextDidChangeActivationState(
+ OverlayPresentationContext* presentation_context) {
+ DCHECK_EQ(presentation_context_, presentation_context);
+ // Attempt to present the active request's overlay UI if the context is being
+ // activated.
+ if (presentation_context_->IsActive())
+ PresentOverlayForActiveRequest();
+}
+
#pragma mark - WebStateListObserver
void OverlayPresenterImpl::WebStateInsertedAt(WebStateList* web_state_list,
diff --git a/ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm b/ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm
index be2e1648..e783381 100644
--- a/ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm
+++ b/ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm
@@ -142,6 +142,43 @@
presentation_context().GetPresentationState(request));
}
+// Tests that requested overlays are presented when the presentation context is
+// activated.
+TEST_F(OverlayPresenterImplTest, PresentAfterContextActivation) {
+ // Add a WebState to the list and add a request to that WebState's queue.
+ presentation_context().SetIsActive(false);
+ presenter().SetPresentationContext(&presentation_context());
+ web_state_list().InsertWebState(
+ /*index=*/0, std::make_unique<web::TestWebState>(),
+ WebStateList::InsertionFlags::INSERT_ACTIVATE, WebStateOpener());
+ OverlayRequest* request = AddRequest(active_web_state());
+ ASSERT_EQ(FakeOverlayPresentationContext::PresentationState::kNotPresented,
+ presentation_context().GetPresentationState(request));
+
+ // Activate the presentation context and verify that the UI is presented.
+ presentation_context().SetIsActive(true);
+ EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kPresented,
+ presentation_context().GetPresentationState(request));
+}
+
+// Tests that presented overlay UI is hidden when the presentation context is
+// deactivated.
+TEST_F(OverlayPresenterImplTest, HideAfterContextDeactivation) {
+ // Add a WebState to the list and add a request to that WebState's queue.
+ presenter().SetPresentationContext(&presentation_context());
+ web_state_list().InsertWebState(
+ /*index=*/0, std::make_unique<web::TestWebState>(),
+ WebStateList::InsertionFlags::INSERT_ACTIVATE, WebStateOpener());
+ OverlayRequest* request = AddRequest(active_web_state());
+ ASSERT_EQ(FakeOverlayPresentationContext::PresentationState::kPresented,
+ presentation_context().GetPresentationState(request));
+
+ // Deactivate the presentation context and verify that the UI is hidden.
+ presentation_context().SetIsActive(false);
+ EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kHidden,
+ presentation_context().GetPresentationState(request));
+}
+
// Tests resetting the presentation context. The UI should be cancelled in the
// previous context and presented in the new context.
TEST_F(OverlayPresenterImplTest, ResetPresentationContext) {
diff --git a/ios/chrome/browser/overlays/public/overlay_presentation_context.h b/ios/chrome/browser/overlays/public/overlay_presentation_context.h
index a5b99ab1..4ac8c4b 100644
--- a/ios/chrome/browser/overlays/public/overlay_presentation_context.h
+++ b/ios/chrome/browser/overlays/public/overlay_presentation_context.h
@@ -9,6 +9,7 @@
class OverlayPresenter;
class OverlayRequest;
+class OverlayPresentationContextObserver;
// Object that handles presenting the overlay UI for OverlayPresenter.
class OverlayPresentationContext {
@@ -16,6 +17,14 @@
OverlayPresentationContext() = default;
virtual ~OverlayPresentationContext() = default;
+ // Adds and removes |observer|.
+ virtual void AddObserver(OverlayPresentationContextObserver* observer) = 0;
+ virtual void RemoveObserver(OverlayPresentationContextObserver* observer) = 0;
+
+ // Whether the presentation context is active. Overlay UI will only be
+ // presented for active contexts.
+ virtual bool IsActive() const = 0;
+
// Called by |presenter| to show the overlay UI for |request|.
// |dismissal_callback| must be stored and called whenever the UI is finished
// being dismissed for user interaction, hiding, or cancellation.
diff --git a/ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h b/ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h
new file mode 100644
index 0000000..facfa0a
--- /dev/null
+++ b/ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h
@@ -0,0 +1,28 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_CHROME_BROWSER_OVERLAYS_PUBLIC_OVERLAY_PRESENTATION_CONTEXT_OBSERVER_H_
+#define IOS_CHROME_BROWSER_OVERLAYS_PUBLIC_OVERLAY_PRESENTATION_CONTEXT_OBSERVER_H_
+
+#include "base/observer_list_types.h"
+
+class OverlayPresentationContext;
+
+// Observer class for the ObserverPresentationContext.
+class OverlayPresentationContextObserver : public base::CheckedObserver {
+ public:
+ OverlayPresentationContextObserver() = default;
+
+ // Called before |presentation_context|'s activation state changes to
+ // |activating|.
+ virtual void OverlayPresentationContextWillChangeActivationState(
+ OverlayPresentationContext* presentation_context,
+ bool activating) {}
+
+ // Called after |presentation_context|'s activation state changes.
+ virtual void OverlayPresentationContextDidChangeActivationState(
+ OverlayPresentationContext* presentation_context) {}
+};
+
+#endif // IOS_CHROME_BROWSER_OVERLAYS_PUBLIC_OVERLAY_PRESENTATION_CONTEXT_OBSERVER_H_
diff --git a/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.cc b/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.cc
index 5047068..52ede06 100644
--- a/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.cc
+++ b/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.cc
@@ -6,6 +6,7 @@
#include "base/bind.h"
#include "base/logging.h"
+#include "ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h"
#include "ios/chrome/browser/overlays/public/overlay_request_queue.h"
FakeOverlayPresentationContext::FakeOverlayPresentationContext() = default;
@@ -34,6 +35,33 @@
std::move(overlay_callbacks_[request]).Run(reason);
}
+void FakeOverlayPresentationContext::SetIsActive(bool active) {
+ if (active_ == active)
+ return;
+
+ for (auto& observer : observers_) {
+ observer.OverlayPresentationContextWillChangeActivationState(this, active);
+ }
+ active_ = active;
+ for (auto& observer : observers_) {
+ observer.OverlayPresentationContextDidChangeActivationState(this);
+ }
+}
+
+void FakeOverlayPresentationContext::AddObserver(
+ OverlayPresentationContextObserver* observer) {
+ observers_.AddObserver(observer);
+}
+
+void FakeOverlayPresentationContext::RemoveObserver(
+ OverlayPresentationContextObserver* observer) {
+ observers_.RemoveObserver(observer);
+}
+
+bool FakeOverlayPresentationContext::IsActive() const {
+ return active_;
+}
+
void FakeOverlayPresentationContext::ShowOverlayUI(
OverlayPresenter* presenter,
OverlayRequest* request,
diff --git a/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.h b/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.h
index e16be57..b159253 100644
--- a/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.h
+++ b/ios/chrome/browser/overlays/test/fake_overlay_presentation_context.h
@@ -7,6 +7,7 @@
#include <map>
+#include "base/observer_list.h"
#include "ios/chrome/browser/overlays/public/overlay_presentation_context.h"
#include "ios/chrome/browser/overlays/public/overlay_request.h"
@@ -36,7 +37,13 @@
void SimulateDismissalForRequest(OverlayRequest* request,
OverlayDismissalReason reason);
+ // Setter for whether the context is active.
+ void SetIsActive(bool active);
+
// OverlayUIDelegate:
+ void AddObserver(OverlayPresentationContextObserver* observer) override;
+ void RemoveObserver(OverlayPresentationContextObserver* observer) override;
+ bool IsActive() const override;
void ShowOverlayUI(OverlayPresenter* presenter,
OverlayRequest* request,
OverlayDismissalCallback dismissal_callback) override;
@@ -50,6 +57,12 @@
std::map<OverlayRequest*, PresentationState> presentation_states_;
// The callbacks for each OverlayRequest.
std::map<OverlayRequest*, OverlayDismissalCallback> overlay_callbacks_;
+ // Whether the context is active.
+ bool active_ = true;
+
+ base::ObserverList<OverlayPresentationContextObserver,
+ /* check_empty= */ true>
+ observers_;
};
#endif // IOS_CHROME_BROWSER_OVERLAYS_TEST_FAKE_OVERLAY_PRESENTATION_CONTEXT_H_
diff --git a/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.h b/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.h
index 1479404..041c4b9 100644
--- a/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.h
+++ b/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.h
@@ -6,6 +6,7 @@
#define IOS_CHROME_BROWSER_UI_OVERLAYS_OVERLAY_PRESENTATION_CONTEXT_IMPL_H_
#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
#import "ios/chrome/browser/main/browser_observer.h"
#include "ios/chrome/browser/overlays/public/overlay_modality.h"
#import "ios/chrome/browser/overlays/public/overlay_presentation_context.h"
@@ -56,6 +57,9 @@
void SetCoordinator(OverlayContainerCoordinator* coordinator);
// OverlayPresentationContext:
+ void AddObserver(OverlayPresentationContextObserver* observer) override;
+ void RemoveObserver(OverlayPresentationContextObserver* observer) override;
+ bool IsActive() const override;
void ShowOverlayUI(OverlayPresenter* presenter,
OverlayRequest* request,
OverlayDismissalCallback dismissal_callback) override;
@@ -83,9 +87,6 @@
// Called when the UI for |request_| has finished being dismissed.
void OverlayUIWasDismissed();
- // Notifies the state for |request_| that its UI has finished being dismissed.
- void NotifyStateOfDismissal();
-
// Helper object that detaches the UI delegate for Browser shudown.
class BrowserShutdownHelper : public BrowserObserver {
public:
@@ -103,14 +104,15 @@
// Helper object that listens for UI dismissal events.
class OverlayDismissalHelper : public OverlayUIDismissalDelegate {
public:
- OverlayDismissalHelper(OverlayPresentationContextImpl* ui_delegate);
+ OverlayDismissalHelper(
+ OverlayPresentationContextImpl* presentation_context);
~OverlayDismissalHelper() override;
// OverlayUIDismissalDelegate:
void OverlayUIDidFinishDismissal(OverlayRequest* request) override;
private:
- OverlayPresentationContextImpl* ui_delegate_ = nullptr;
+ OverlayPresentationContextImpl* presentation_context_ = nullptr;
};
// The presenter whose UI is being handled by this delegate.
@@ -131,6 +133,9 @@
OverlayRequest* request_ = nullptr;
// Map storing the UI state for each OverlayRequest.
std::map<OverlayRequest*, std::unique_ptr<OverlayRequestUIState>> states_;
+ base::ObserverList<OverlayPresentationContextObserver,
+ /* check_empty= */ true>
+ observers_;
// Weak pointer factory.
base::WeakPtrFactory<OverlayPresentationContextImpl> weak_factory_;
};
diff --git a/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.mm b/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.mm
index 149b10c..74767b8 100644
--- a/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.mm
+++ b/ios/chrome/browser/ui/overlays/overlay_presentation_context_impl.mm
@@ -7,6 +7,7 @@
#include "base/bind.h"
#include "base/callback.h"
#import "ios/chrome/browser/main/browser.h"
+#import "ios/chrome/browser/overlays/public/overlay_presentation_context_observer.h"
#import "ios/chrome/browser/overlays/public/overlay_presenter.h"
#import "ios/chrome/browser/ui/overlays/overlay_container_coordinator.h"
#import "ios/chrome/browser/ui/overlays/overlay_coordinator_factory.h"
@@ -62,19 +63,38 @@
OverlayContainerCoordinator* coordinator) {
if (coordinator_ == coordinator)
return;
- if (coordinator_ && request_)
- DismissPresentedUI(OverlayDismissalReason::kHiding);
+
+ for (auto& observer : observers_) {
+ observer.OverlayPresentationContextWillChangeActivationState(this,
+ !!coordinator);
+ }
coordinator_ = coordinator;
// The new coordinator should be started before provided to the UI delegate.
DCHECK(!coordinator_ || coordinator_.viewController);
- ShowUIForPresentedRequest();
+ for (auto& observer : observers_) {
+ observer.OverlayPresentationContextDidChangeActivationState(this);
+ }
}
#pragma mark OverlayPresentationContext
+void OverlayPresentationContextImpl::AddObserver(
+ OverlayPresentationContextObserver* observer) {
+ observers_.AddObserver(observer);
+}
+
+void OverlayPresentationContextImpl::RemoveObserver(
+ OverlayPresentationContextObserver* observer) {
+ observers_.RemoveObserver(observer);
+}
+
+bool OverlayPresentationContextImpl::IsActive() const {
+ return !!coordinator_;
+}
+
void OverlayPresentationContextImpl::ShowOverlayUI(
OverlayPresenter* presenter,
OverlayRequest* request,
@@ -104,7 +124,7 @@
} else {
// Simulate dismissal if there is no container coordinator.
state->set_dismissal_reason(OverlayDismissalReason::kHiding);
- NotifyStateOfDismissal();
+ OverlayUIWasDismissed();
}
}
@@ -122,13 +142,11 @@
}
// If the current request is being cancelled (e.g. for WebState closures) when
- // there is no coordinator, simulate a dismissal and reset the current
- // request.
+ // there is no coordinator, simulate a dismissal.
if (!coordinator_) {
DCHECK_EQ(request_, request);
state->set_dismissal_reason(OverlayDismissalReason::kCancellation);
state->OverlayUIWasDismissed();
- SetRequest(nullptr);
return;
}
@@ -207,17 +225,6 @@
void OverlayPresentationContextImpl::OverlayUIWasDismissed() {
DCHECK(request_);
- // Overlays are dismissed without animation when the container coordinator is
- // reset, but the state should not be notified of these dismissals since the
- // same UI will be presented again once a new container coordinator is
- // provided.
- if (!coordinator_)
- return;
- NotifyStateOfDismissal();
-}
-
-void OverlayPresentationContextImpl::NotifyStateOfDismissal() {
- DCHECK(request_);
DCHECK(GetRequestUIState(request_)->has_callback());
// If there is another request in the active WebState's OverlayRequestQueue,
// executing the state's dismissal callback will trigger the presentation of
@@ -252,9 +259,9 @@
#pragma mark OverlayDismissalHelper
OverlayPresentationContextImpl::OverlayDismissalHelper::OverlayDismissalHelper(
- OverlayPresentationContextImpl* ui_delegate)
- : ui_delegate_(ui_delegate) {
- DCHECK(ui_delegate_);
+ OverlayPresentationContextImpl* presentation_context)
+ : presentation_context_(presentation_context) {
+ DCHECK(presentation_context_);
}
OverlayPresentationContextImpl::OverlayDismissalHelper::
@@ -263,6 +270,6 @@
void OverlayPresentationContextImpl::OverlayDismissalHelper::
OverlayUIDidFinishDismissal(OverlayRequest* request) {
DCHECK(request);
- DCHECK_EQ(ui_delegate_->request_, request);
- ui_delegate_->OverlayUIWasDismissed();
+ DCHECK_EQ(presentation_context_->request_, request);
+ presentation_context_->OverlayUIWasDismissed();
}