Don't log TabMetrics event when closing browser window

BrowserView::CanClose() hides the browser frame before closing the
browser. This causes the active WebContents to log an extraneous
TabManager.TabMetrics event, thinking the tab was backgrounded.

Unless the window was actually minimized, the active WebContents
shouldn't log a backgrounded event when hidden.

Bug: 784639
Change-Id: I211e37bf169112982c1130dffb7c502d83c5e9db
Reviewed-on: https://chromium-review.googlesource.com/905095
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535974}

chrome/browser/resource_coordinator/tab_activity_watcher.cc

@@ -7,6 +7,8 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/resource_coordinator/tab_metrics_logger_impl.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/tabs/window_activity_watcher.h"
 #include "content/public/browser/browser_context.h"
@@ -137,6 +139,21 @@ bool TabActivityWatcher::ShouldTrackBrowser(Browser* browser) {
 void TabActivityWatcher::OnWasHidden(content::WebContents* web_contents) {
   DCHECK(web_contents);
 
+  Browser* browser = chrome::FindBrowserWithWebContents(web_contents);
+  if (!browser)
+    return;
+
+  if (browser->tab_strip_model()->GetActiveWebContents() == web_contents &&
+      !browser->window()->IsMinimized()) {
+    // The active tab is considered to be in the foreground unless its window is
+    // minimized. It might still get hidden, e.g. when the browser is about to
+    // close, but that shouldn't count as a backgrounded event.
+    //
+    // TODO(michaelpg): On Mac, hiding the application (e.g. via Cmd+H) should
+    // log tabs as backgrounded. Check NSApplication's isHidden property.
+    return;
+  }
+
   MaybeLogTab(web_contents);
 }
 

chrome/browser/resource_coordinator/tab_activity_watcher_unittest.cc

@@ -327,12 +327,16 @@ TEST_F(TabActivityWatcherTest, InputEvents) {
   widget_1->ForwardMouseEvent(CreateMouseEvent(WebInputEvent::kMouseMove));
   widget_1->ForwardMouseEvent(CreateMouseEvent(WebInputEvent::kMouseMove));
   expected_metrics_1[TabManager_TabMetrics::kMouseEventCountName] = 5;
-  test_contents_1->WasHidden();
+  tab_activity_simulator_.SwitchToTabAt(tab_strip_model, 1);
   {
     SCOPED_TRACE("");
     ExpectNewEntry(kTestUrls[0], expected_metrics_1);
   }
-  test_contents_1->WasShown();
+  tab_activity_simulator_.SwitchToTabAt(tab_strip_model, 0);
+  {
+    SCOPED_TRACE("");
+    ExpectNewEntry(kTestUrls[1], expected_metrics_2);
+  }
 
   // After a navigation, test that the counts are reset.
   WebContentsTester::For(test_contents_1)->NavigateAndCommit(kTestUrls[2]);
@@ -340,7 +344,7 @@ TEST_F(TabActivityWatcherTest, InputEvents) {
   widget_1 = test_contents_1->GetRenderViewHost()->GetWidget();
   widget_1->ForwardMouseEvent(CreateMouseEvent(WebInputEvent::kMouseMove));
   expected_metrics_1[TabManager_TabMetrics::kMouseEventCountName] = 1;
-  test_contents_1->WasHidden();
+  tab_activity_simulator_.SwitchToTabAt(tab_strip_model, 1);
   {
     SCOPED_TRACE("");
     ExpectNewEntry(kTestUrls[2], expected_metrics_1);
@@ -349,9 +353,9 @@ TEST_F(TabActivityWatcherTest, InputEvents) {
   tab_strip_model->CloseAllTabs();
 }
 
-// Tests that logging happens when the browser window is hidden (even if the
-// WebContents is still the active tab).
-TEST_F(TabActivityWatcherTest, HideWindow) {
+// Tests that logging doesn't occur when the WebContents is hidden while still
+// the active tab, e.g. when the browser window hides before closing.
+TEST_F(TabActivityWatcherTest, HideWebContents) {
   Browser::CreateParams params(profile(), true);
   std::unique_ptr<Browser> browser =
       CreateBrowserWithTestWindowForParams(&params);
@@ -362,14 +366,12 @@ TEST_F(TabActivityWatcherTest, HideWindow) {
                                                         GURL(kTestUrls[0]));
   tab_strip_model->ActivateTabAt(0, false);
 
-  // Hiding the window triggers the log.
+  // Hiding the window doesn't trigger a log entry, unless the window was
+  // minimized.
+  // TODO(michaelpg): Test again with the window minimized using the
+  // FakeBrowserWindow from window_activity_watcher_unittest.cc.
   test_contents->WasHidden();
-  {
-    SCOPED_TRACE("");
-    ExpectNewEntry(kTestUrls[0], kBasicMetricValues);
-  }
-
-  // Showing the window does not.
+  EXPECT_EQ(0, ukm_entry_checker_.NumNewEntriesRecorded(kEntryName));
   test_contents->WasShown();
   EXPECT_EQ(0, ukm_entry_checker_.NumNewEntriesRecorded(kEntryName));