From 631f598b4b37234e5f41d8249bced976dd8520a2 Mon Sep 17 00:00:00 2001 From: hcarmona Date: Thu, 10 Sep 2015 19:01:24 -0700 Subject: [PATCH] Add ability to gather metrics to BubbleManager. Give BubbleManager the ability to collect close reason and display time for each for each bubble type. Display time is not separated by bubble type. BUG=496955 Review URL: https://codereview.chromium.org/1287323003 Cr-Commit-Position: refs/heads/master@{#348313} --- chrome/browser/ui/chrome_bubble_manager.cc | 74 ++++++++++++++++++++++++++++ chrome/browser/ui/chrome_bubble_manager.h | 15 ++++++ components/bubble/bubble_close_reason.h | 8 +-- components/bubble/bubble_controller.cc | 11 ++++- components/bubble/bubble_controller.h | 9 ++++ components/bubble/bubble_delegate.h | 5 ++ components/bubble/bubble_manager.cc | 47 +++++++++++++++--- components/bubble/bubble_manager.h | 32 ++++++++++++ components/bubble/bubble_manager_unittest.cc | 70 ++++++++++++++++++++++++++ tools/metrics/histograms/histograms.xml | 71 ++++++++++++++++++++++++++ 10 files changed, 328 insertions(+), 14 deletions(-) diff --git a/chrome/browser/ui/chrome_bubble_manager.cc b/chrome/browser/ui/chrome_bubble_manager.cc index 60d3f42d1b21..d7f3febaeb53 100644 --- a/chrome/browser/ui/chrome_bubble_manager.cc +++ b/chrome/browser/ui/chrome_bubble_manager.cc @@ -4,19 +4,75 @@ #include "chrome/browser/ui/chrome_bubble_manager.h" +#include "base/metrics/histogram_macros.h" #include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "components/bubble/bubble_controller.h" +#include "components/bubble/bubble_delegate.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/web_contents.h" +namespace { + +// Add any new enum before BUBBLE_TYPE_MAX and update BubbleType in +// tools/metrics/histograms/histograms.xml. +// Do not re-order these because they are used for collecting metrics. +enum BubbleType { + BUBBLE_TYPE_UNKNOWN, // Used for unrecognized bubble names. + BUBBLE_TYPE_MOCK, // Used for testing. + BUBBLE_TYPE_MAX, // Number of bubble types; used for metrics. +}; + +// Convert from bubble name to ID. The bubble ID will allow collecting the +// close reason for each bubble type. +static int GetBubbleId(BubbleReference bubble) { + BubbleType bubble_type = BUBBLE_TYPE_UNKNOWN; + + // Translate from bubble name to enum. + if (bubble->GetName().compare("MockBubble")) + bubble_type = BUBBLE_TYPE_MOCK; + + DCHECK_NE(bubble_type, BUBBLE_TYPE_UNKNOWN); + DCHECK_NE(bubble_type, BUBBLE_TYPE_MAX); + + return bubble_type; +} + +// Get the UMA title for this close reason. +static std::string GetBubbleCloseReasonName(BubbleCloseReason reason) { + switch (reason) { + case BUBBLE_CLOSE_FORCED: return "Bubbles.Close.Forced"; + case BUBBLE_CLOSE_FOCUS_LOST: return "Bubbles.Close.FocusLost"; + case BUBBLE_CLOSE_TABSWITCHED: return "Bubbles.Close.TabSwitched"; + case BUBBLE_CLOSE_TABDETACHED: return "Bubbles.Close.TabDetached"; + case BUBBLE_CLOSE_USER_DISMISSED: return "Bubbles.Close.UserDismissed"; + case BUBBLE_CLOSE_NAVIGATED: return "Bubbles.Close.Navigated"; + case BUBBLE_CLOSE_FULLSCREEN_TOGGLED: + return "Bubbles.Close.FullscreenToggled"; + case BUBBLE_CLOSE_ACCEPTED: return "Bubbles.Close.Accepted"; + case BUBBLE_CLOSE_CANCELED: return "Bubbles.Close.Canceled"; + } + + NOTREACHED(); + return "Bubbles.Close.Unknown"; +} + +} // namespace + ChromeBubbleManager::ChromeBubbleManager(TabStripModel* tab_strip_model) : tab_strip_model_(tab_strip_model) { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); DCHECK(tab_strip_model_); tab_strip_model_->AddObserver(this); + AddBubbleManagerObserver(&chrome_bubble_metrics_); } ChromeBubbleManager::~ChromeBubbleManager() { tab_strip_model_->RemoveObserver(this); + + // Finalize requests before removing the BubbleManagerObserver so it can + // collect metrics when closing any open bubbles. + FinalizePendingRequests(); + RemoveBubbleManagerObserver(&chrome_bubble_metrics_); } void ChromeBubbleManager::TabDetachedAt(content::WebContents* contents, @@ -48,3 +104,21 @@ void ChromeBubbleManager::NavigationEntryCommitted( const content::LoadCommittedDetails& load_details) { CloseAllBubbles(BUBBLE_CLOSE_NAVIGATED); } + +void ChromeBubbleManager::ChromeBubbleMetrics::OnBubbleNeverShown( + BubbleReference bubble) { + UMA_HISTOGRAM_ENUMERATION("Bubbles.NeverShown", GetBubbleId(bubble), + BUBBLE_TYPE_MAX); +} + +void ChromeBubbleManager::ChromeBubbleMetrics::OnBubbleClosed( + BubbleReference bubble, BubbleCloseReason reason) { + // Log the amount of time the bubble was visible. + base::TimeDelta visible_time = bubble->GetVisibleTime(); + UMA_HISTOGRAM_LONG_TIMES("Bubbles.DisplayTime.All", visible_time); + + // Log the reason the bubble was closed. + UMA_HISTOGRAM_ENUMERATION(GetBubbleCloseReasonName(reason), + GetBubbleId(bubble), + BUBBLE_TYPE_MAX); +} diff --git a/chrome/browser/ui/chrome_bubble_manager.h b/chrome/browser/ui/chrome_bubble_manager.h index 2c7dd7e96b0e..57c23d855640 100644 --- a/chrome/browser/ui/chrome_bubble_manager.h +++ b/chrome/browser/ui/chrome_bubble_manager.h @@ -32,7 +32,22 @@ class ChromeBubbleManager : public BubbleManager, const content::LoadCommittedDetails& load_details) override; private: + class ChromeBubbleMetrics : public BubbleManager::BubbleManagerObserver { + public: + ChromeBubbleMetrics() {} + ~ChromeBubbleMetrics() override {} + + // BubbleManager::BubbleManagerObserver: + void OnBubbleNeverShown(BubbleReference bubble) override; + void OnBubbleClosed(BubbleReference bubble, + BubbleCloseReason reason) override; + + private: + DISALLOW_COPY_AND_ASSIGN(ChromeBubbleMetrics); + }; + TabStripModel* tab_strip_model_; + ChromeBubbleMetrics chrome_bubble_metrics_; DISALLOW_COPY_AND_ASSIGN(ChromeBubbleManager); }; diff --git a/components/bubble/bubble_close_reason.h b/components/bubble/bubble_close_reason.h index bfd84b329145..68e112d8f88e 100644 --- a/components/bubble/bubble_close_reason.h +++ b/components/bubble/bubble_close_reason.h @@ -8,6 +8,10 @@ // List of reasons why a bubble might close. These correspond to various events // from the UI. Not all platforms will receive all events. enum BubbleCloseReason { + // The bubble WILL be closed regardless of return value for |ShouldClose|. + // Ex: The bubble's parent window is being destroyed. + BUBBLE_CLOSE_FORCED, + // Bubble was closed without any user interaction. BUBBLE_CLOSE_FOCUS_LOST, @@ -32,10 +36,6 @@ enum BubbleCloseReason { // The user selected a negative response in the bubble. BUBBLE_CLOSE_CANCELED, - - // The bubble WILL be closed regardless of return value for |ShouldClose|. - // Ex: The bubble's parent window is being destroyed. - BUBBLE_CLOSE_FORCED, }; #endif // COMPONENTS_BUBBLE_BUBBLE_CLOSE_REASON_H_ diff --git a/components/bubble/bubble_controller.cc b/components/bubble/bubble_controller.cc index 5e045390d24c..7dc65b03b35a 100644 --- a/components/bubble/bubble_controller.cc +++ b/components/bubble/bubble_controller.cc @@ -32,12 +32,20 @@ bool BubbleController::UpdateBubbleUi() { return delegate_->UpdateBubbleUi(bubble_ui_.get()); } +std::string BubbleController::GetName() const { + return delegate_->GetName(); +} + +base::TimeDelta BubbleController::GetVisibleTime() const { + return base::TimeTicks::Now() - show_time_; +} + void BubbleController::Show() { DCHECK(!bubble_ui_); bubble_ui_ = delegate_->BuildBubbleUi(); DCHECK(bubble_ui_); bubble_ui_->Show(AsWeakPtr()); - // TODO(hcarmona): log that bubble was shown. + show_time_ = base::TimeTicks::Now(); } void BubbleController::UpdateAnchorPosition() { @@ -50,7 +58,6 @@ bool BubbleController::ShouldClose(BubbleCloseReason reason) { if (delegate_->ShouldClose(reason) || reason == BUBBLE_CLOSE_FORCED) { bubble_ui_->Close(); bubble_ui_.reset(); - // TODO(hcarmona): log that bubble was hidden. return true; } return false; diff --git a/components/bubble/bubble_controller.h b/components/bubble/bubble_controller.h index 756ab34bfc3d..cdb685deb4e4 100644 --- a/components/bubble/bubble_controller.h +++ b/components/bubble/bubble_controller.h @@ -28,6 +28,12 @@ class BubbleController : public base::SupportsWeakPtr { // Returns true if the UI was updated. bool UpdateBubbleUi(); + // Used to identify a bubble for collecting metrics. + std::string GetName() const; + + // Used for collecting metrics. + base::TimeDelta GetVisibleTime() const; + private: friend class BubbleManager; @@ -49,6 +55,9 @@ class BubbleController : public base::SupportsWeakPtr { // Verify that functions that affect the UI are done on the same thread. base::ThreadChecker thread_checker_; + // To keep track of the amount of time a bubble was visible. + base::TimeTicks show_time_; + DISALLOW_COPY_AND_ASSIGN(BubbleController); }; diff --git a/components/bubble/bubble_delegate.h b/components/bubble/bubble_delegate.h index 178b54263a9c..f53b94b12d19 100644 --- a/components/bubble/bubble_delegate.h +++ b/components/bubble/bubble_delegate.h @@ -5,6 +5,8 @@ #ifndef COMPONENTS_BUBBLE_BUBBLE_DELEGATE_H_ #define COMPONENTS_BUBBLE_BUBBLE_DELEGATE_H_ +#include + #include "base/memory/scoped_ptr.h" #include "components/bubble/bubble_close_reason.h" @@ -32,6 +34,9 @@ class BubbleDelegate { // Return true to indicate the UI was updated. virtual bool UpdateBubbleUi(BubbleUi* bubble_ui); + // Used to identify a bubble for collecting metrics. + virtual std::string GetName() const = 0; + private: DISALLOW_COPY_AND_ASSIGN(BubbleDelegate); }; diff --git a/components/bubble/bubble_manager.cc b/components/bubble/bubble_manager.cc index 8bbed4614bcb..41529f980be1 100644 --- a/components/bubble/bubble_manager.cc +++ b/components/bubble/bubble_manager.cc @@ -12,13 +12,13 @@ BubbleManager::BubbleManager() : manager_state_(SHOW_BUBBLES) {} BubbleManager::~BubbleManager() { - manager_state_ = NO_MORE_BUBBLES; - CloseAllBubbles(BUBBLE_CLOSE_FORCED); + FinalizePendingRequests(); } BubbleReference BubbleManager::ShowBubble(scoped_ptr bubble) { DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(bubble); + scoped_ptr controller( new BubbleController(this, bubble.Pass())); @@ -33,9 +33,8 @@ BubbleReference BubbleManager::ShowBubble(scoped_ptr bubble) { show_queue_.push_back(controller.Pass()); break; case NO_MORE_BUBBLES: - // The controller will be cleaned up and |bubble_ref| will be invalidated. - // It's important that the controller is created even though it's - // destroyed immediately because it will collect metrics about the bubble. + FOR_EACH_OBSERVER(BubbleManagerObserver, observers_, + OnBubbleNeverShown(controller->AsWeakPtr())); break; } @@ -52,8 +51,11 @@ bool BubbleManager::CloseBubble(BubbleReference bubble, for (auto iter = controllers_.begin(); iter != controllers_.end(); ++iter) { if (*iter == bubble.get()) { bool closed = (*iter)->ShouldClose(reason); - if (closed) + if (closed) { + FOR_EACH_OBSERVER(BubbleManagerObserver, observers_, + OnBubbleClosed((*iter)->AsWeakPtr(), reason)); iter = controllers_.erase(iter); + } ShowPendingBubbles(); return closed; } @@ -75,8 +77,14 @@ void BubbleManager::CloseAllBubbles(BubbleCloseReason reason) { if (manager_state_ == SHOW_BUBBLES) manager_state_ = QUEUE_BUBBLES; - for (auto iter = controllers_.begin(); iter != controllers_.end();) - iter = (*iter)->ShouldClose(reason) ? controllers_.erase(iter) : iter + 1; + for (auto iter = controllers_.begin(); iter != controllers_.end();) { + bool closed = (*iter)->ShouldClose(reason); + if (closed) { + FOR_EACH_OBSERVER(BubbleManagerObserver, observers_, + OnBubbleClosed((*iter)->AsWeakPtr(), reason)); + } + iter = closed ? controllers_.erase(iter) : iter + 1; + } ShowPendingBubbles(); } @@ -87,6 +95,24 @@ void BubbleManager::UpdateAllBubbleAnchors() { controller->UpdateAnchorPosition(); } +void BubbleManager::AddBubbleManagerObserver(BubbleManagerObserver* observer) { + observers_.AddObserver(observer); +} + +void BubbleManager::RemoveBubbleManagerObserver( + BubbleManagerObserver* observer) { + observers_.RemoveObserver(observer); +} + +void BubbleManager::FinalizePendingRequests() { + // Return if already "Finalized". + if (manager_state_ == NO_MORE_BUBBLES) + return; + + manager_state_ = NO_MORE_BUBBLES; + CloseAllBubbles(BUBBLE_CLOSE_FORCED); +} + void BubbleManager::ShowPendingBubbles() { if (manager_state_ == QUEUE_BUBBLES) manager_state_ = SHOW_BUBBLES; @@ -100,6 +126,11 @@ void BubbleManager::ShowPendingBubbles() { show_queue_.weak_clear(); } else { + for (auto controller : show_queue_) { + FOR_EACH_OBSERVER(BubbleManagerObserver, observers_, + OnBubbleNeverShown(controller->AsWeakPtr())); + } + // Clear the queue if bubbles can't be shown. show_queue_.clear(); } diff --git a/components/bubble/bubble_manager.h b/components/bubble/bubble_manager.h index 931e74c97e53..5f0d9cf0406d 100644 --- a/components/bubble/bubble_manager.h +++ b/components/bubble/bubble_manager.h @@ -7,6 +7,7 @@ #include "base/memory/scoped_ptr.h" #include "base/memory/scoped_vector.h" +#include "base/observer_list.h" #include "base/threading/thread_checker.h" #include "components/bubble/bubble_close_reason.h" #include "components/bubble/bubble_reference.h" @@ -20,6 +21,25 @@ class BubbleDelegate; // TODO(hcarmona): Handle simultaneous bubbles. http://crbug.com/366937 class BubbleManager { public: + // This interface should be used to observe the manager. This is useful when + // collecting metrics. + class BubbleManagerObserver { + public: + BubbleManagerObserver() {} + virtual ~BubbleManagerObserver() {} + + // Called when a bubble is asked to be displayed but never shown. + // ex: a bubble chained on destruction will not be shown. + virtual void OnBubbleNeverShown(BubbleReference bubble) = 0; + + // Called when a bubble is closed. The reason for closing is provided. + virtual void OnBubbleClosed(BubbleReference bubble, + BubbleCloseReason reason) = 0; + + private: + DISALLOW_COPY_AND_ASSIGN(BubbleManagerObserver); + }; + // Should be instantiated on the UI thread. BubbleManager(); virtual ~BubbleManager(); @@ -38,6 +58,16 @@ class BubbleManager { // Notify all bubbles that their anchor or parent may have changed. void UpdateAllBubbleAnchors(); + // Add an observer for this BubbleManager. + void AddBubbleManagerObserver(BubbleManagerObserver* observer); + + // Remove an observer from this BubbleManager. + void RemoveBubbleManagerObserver(BubbleManagerObserver* observer); + + protected: + // Will close any open bubbles and prevent new ones from being shown. + void FinalizePendingRequests(); + private: enum ManagerStates { SHOW_BUBBLES, @@ -48,6 +78,8 @@ class BubbleManager { // Show any bubbles that were added to |show_queue_|. void ShowPendingBubbles(); + base::ObserverList observers_; + // Verify that functions that affect the UI are done on the same thread. base::ThreadChecker thread_checker_; diff --git a/components/bubble/bubble_manager_unittest.cc b/components/bubble/bubble_manager_unittest.cc index d426f95b7011..23e3e5b7ef7f 100644 --- a/components/bubble/bubble_manager_unittest.cc +++ b/components/bubble/bubble_manager_unittest.cc @@ -24,6 +24,9 @@ class MockBubbleUi : public BubbleUi { // To verify destructor call. MOCK_METHOD0(Destroyed, void()); + + private: + DISALLOW_COPY_AND_ASSIGN(MockBubbleUi); }; class MockBubbleDelegate : public BubbleDelegate { @@ -47,8 +50,13 @@ class MockBubbleDelegate : public BubbleDelegate { MOCK_METHOD1(UpdateBubbleUi, bool(BubbleUi*)); + MOCK_CONST_METHOD0(GetName, std::string()); + // To verify destructor call. MOCK_METHOD0(Destroyed, void()); + + private: + DISALLOW_COPY_AND_ASSIGN(MockBubbleDelegate); }; // static @@ -86,6 +94,8 @@ class DelegateChainHelper { private: BubbleManager* manager_; // Weak. scoped_ptr next_delegate_; + + DISALLOW_COPY_AND_ASSIGN(DelegateChainHelper); }; DelegateChainHelper::DelegateChainHelper( @@ -93,6 +103,18 @@ DelegateChainHelper::DelegateChainHelper( scoped_ptr next_delegate) : manager_(manager), next_delegate_(next_delegate.Pass()) {} +class MockBubbleManagerObserver : public BubbleManager::BubbleManagerObserver { + public: + MockBubbleManagerObserver() {} + ~MockBubbleManagerObserver() override {} + + MOCK_METHOD1(OnBubbleNeverShown, void(BubbleReference)); + MOCK_METHOD2(OnBubbleClosed, void(BubbleReference, BubbleCloseReason)); + + private: + DISALLOW_COPY_AND_ASSIGN(MockBubbleManagerObserver); +}; + class BubbleManagerTest : public testing::Test { public: BubbleManagerTest(); @@ -103,6 +125,9 @@ class BubbleManagerTest : public testing::Test { protected: scoped_ptr manager_; + + private: + DISALLOW_COPY_AND_ASSIGN(BubbleManagerTest); }; BubbleManagerTest::BubbleManagerTest() {} @@ -375,4 +400,49 @@ TEST_F(BubbleManagerTest, BubbleUpdatesFalse) { ASSERT_FALSE(ref->UpdateBubbleUi()); } +TEST_F(BubbleManagerTest, BubbleCloseReasonIsCalled) { + MockBubbleManagerObserver metrics; + EXPECT_CALL(metrics, OnBubbleNeverShown(testing::_)).Times(0); + EXPECT_CALL(metrics, OnBubbleClosed(testing::_, BUBBLE_CLOSE_ACCEPTED)); + manager_->AddBubbleManagerObserver(&metrics); + + BubbleReference ref = manager_->ShowBubble(MockBubbleDelegate::Default()); + ref->CloseBubble(BUBBLE_CLOSE_ACCEPTED); + + // Destroy to verify no events are sent to |metrics| in destructor. + manager_.reset(); +} + +TEST_F(BubbleManagerTest, BubbleCloseNeverShownIsCalled) { + MockBubbleManagerObserver metrics; + // |chained_delegate| should never be shown. + EXPECT_CALL(metrics, OnBubbleNeverShown(testing::_)); + // |delegate| should be forced to close when the manager is destroyed. + EXPECT_CALL(metrics, OnBubbleClosed(testing::_, BUBBLE_CLOSE_FORCED)); + manager_->AddBubbleManagerObserver(&metrics); + + // Manager will delete delegate. + MockBubbleDelegate* chained_delegate = new MockBubbleDelegate; + EXPECT_CALL(*chained_delegate, BuildBubbleUiMock()).Times(0); + EXPECT_CALL(*chained_delegate, ShouldClose(testing::_)).Times(0); + + DelegateChainHelper chain_helper(manager_.get(), + make_scoped_ptr(chained_delegate)); + + // Manager will delete delegate. + MockBubbleDelegate* delegate = new MockBubbleDelegate; + EXPECT_CALL(*delegate, BuildBubbleUiMock()) + .WillOnce(testing::Return(new MockBubbleUi)); + EXPECT_CALL(*delegate, ShouldClose(testing::_)) + .WillOnce(testing::DoAll(testing::InvokeWithoutArgs( + &chain_helper, &DelegateChainHelper::Chain), + testing::Return(true))); + + manager_->ShowBubble(make_scoped_ptr(delegate)); + manager_.reset(); + + // The manager will take the bubble, but not show it. + ASSERT_TRUE(chain_helper.BubbleWasTaken()); +} + } // namespace diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 691229915bdd..84b5e81b435e 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -3162,6 +3162,72 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + hcarmona@chromium.org + An open bubble was closed because the user accepted it. + + + + hcarmona@chromium.org + + An open bubble was closed because the user didn't accept it. + + + + + hcarmona@chromium.org + An open bubble was closed because of a focus change. + + + + hcarmona@chromium.org + An open bubble was forced to close. + + + + hcarmona@chromium.org + + An open bubble was dismissed because fullscreen was toggled. + + + + + hcarmona@chromium.org + + An open bubble was dismissed because the page was navigated. + + + + + hcarmona@chromium.org + An open bubble was closed because a tab was detached. + + + + hcarmona@chromium.org + An open bubble was closed because a tab was switched. + + + + hcarmona@chromium.org + + An open bubble was dismissed by the user without making a decission. + + + + + hcarmona@chromium.org + + Log the amount of time any bubble was visible. Only bubbles that are shown + will have a visible time. + + + + + hcarmona@chromium.org + A bubble was given to the bubble manager but not shown. + + Please list the metric's owners. Add more owner tags as needed. @@ -54176,6 +54242,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + + + + -- 2.11.4.GIT