Add metrics to keep track of the tab activate/deactivate cycle

chrome/browser/metrics/tab_usage_recorder.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.

[chrisha, 2016/09/14 09:57:39]

2016?

[Patrick Monette, 2016/09/16 00:14:00]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/metrics/tab_usage_recorder.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_macros.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+namespace metrics {
+
+namespace {
+
+void RecordTabDeactivation(bool is_pinned) {
+  if (is_pinned)
+    UMA_HISTOGRAM_COUNTS("Tab.DeactivationCount.Pinned", 1);
+  else
+    UMA_HISTOGRAM_COUNTS("Tab.DeactivationCount", 1);

[chrisha, 2016/09/14 09:57:39]

Always record this, and then have a separate specialized .Pinned one when
is_pinned is true?

Or would this be better named .NotPinned?

[Patrick Monette, 2016/09/16 00:14:00]

I added a histogram suffix to get the best of both worlds.

+}
+
+void RecordTabReactivation(bool is_pinned) {
+  if (is_pinned)
+    UMA_HISTOGRAM_COUNTS("Tab.ReactivationCount.Pinned", 1);
+  else
+    UMA_HISTOGRAM_COUNTS("Tab.ReactivationCount", 1);
+}
+
+// This class is used to track the activation/deactivation cycle per
+// WebContents. It also keep tracks of the pinned state of the tab.

[Georges Khalil, 2016/09/14 16:10:16]

nit: s/keep tracks/keeps track.

[Patrick Monette, 2016/09/16 00:14:00]

Done.

+class WebContentsData : public content::WebContentsUserData<WebContentsData> {

[Georges Khalil, 2016/09/14 16:10:16]

Make this internal to TabUsageRecorder.

[Patrick Monette, 2016/09/16 00:14:00]

Done.

+ public:
+  static void CreateForWebContents(content::WebContents* contents,
+                                   bool is_pinned);
+
+  ~WebContentsData() override;
+
+  void OnTabActivating();
+  void OnTabDeactivating();
+  void OnTabClosing();
+  void OnTabPinnedStateChanging(bool is_pinned);
+
+ private:
+  friend class content::WebContentsUserData<WebContentsData>;
+
+  explicit WebContentsData(content::WebContents* contents, bool is_pinned);
+
+  // Indicates if the tab is pinned to the tab strip.
+  bool is_pinned_;
+
+  // Indicates if the tab has been deactivated before as to only count
+  // reactivations.
+  bool was_deactivated_once_;
+
+  // The deactivation metric is not recorded for closing tabs.
+  bool is_closing_;

[Georges Khalil, 2016/09/14 16:10:17]

Disallow copy.

[Patrick Monette, 2016/09/16 00:14:00]

Done.

+};
+
+// static
+void WebContentsData::CreateForWebContents(content::WebContents* contents,
+                                           bool is_pinned) {
+  DCHECK(contents);
+  if (FromWebContents(contents))
+    return;
+
+  contents->SetUserData(UserDataKey(),
+                        new WebContentsData(contents, is_pinned));

[Georges Khalil, 2016/09/14 16:10:16]

Use DEFINE_WEB_CONTENTS_USER_DATA_KEY so you don't need this method.

[Patrick Monette, 2016/09/16 00:13:59]

DEFINE_WEB_CONTENTS_USER_DATA_KEY is already used (but I moved it to the top).

This function is needed because CreateForWebContents() is only defined for
classes whose constructor take one parameter.

[Georges Khalil, 2016/09/16 18:58:35]

I missed DEFINE_WEB_CONTENTS_USER_DATA_KEY at the end, my bad. +1 to moving it
at the top.

I think this function can get confusing, regarding the expectation. The caller
expects it to create the WebContentsData with the value of |is_pinned|. However,
if the Data is already created, |is_pinned| is ignored, which would not be
honoring the caller's request.

I propose to remove this function and replace it with a GetWebContentsData which
creates the WebContentsData if need be, and returns it. The caller would start
by getting the WebContentsData then can modify the pinned state in a separate
instruction.

See an example here:
https://cs.chromium.org/chromium/src/chrome/browser/memory/tab_manager.cc?sq=...

WDYT?

[Patrick Monette, 2016/09/19 17:47:12]

I addressed the confusing expectation. The change is simple enough that I didn't
add a GetWebContentsData() function. Is it better?

+}
+
+WebContentsData::~WebContentsData() = default;
+
+void WebContentsData::OnTabActivating() {
+  if (was_deactivated_once_) {
+    RecordTabReactivation(is_pinned_);
+  }
+}
+
+void WebContentsData::OnTabDeactivating() {
+  was_deactivated_once_ = true;
+  if (!is_closing_)
+    RecordTabDeactivation(is_pinned_);
+}
+
+void WebContentsData::OnTabClosing() {
+  is_closing_ = true;
+}
+
+void WebContentsData::OnTabPinnedStateChanging(bool is_pinned) {
+  is_pinned_ = is_pinned;
+}
+
+WebContentsData::WebContentsData(content::WebContents* contents, bool is_pinned)
+    :  // content::WebContentsObserver(contents),

[Georges Khalil, 2016/09/14 16:10:16]

nit: remove.

[Patrick Monette, 2016/09/16 00:13:59]

Done.

+      is_pinned_(is_pinned),
+      was_deactivated_once_(false),
+      is_closing_(false) {}
+
+}  // namespace
+
+// This class is used to keep track of which browser owns the tab strip model.
+// The browser is necessary to check for the pinned state of the tab
+// in an efficient manner.
+class TabUsageRecorder::TabStripObserver : public TabStripModelObserver {
+ public:
+  explicit TabStripObserver(Browser* browser);
+  ~TabStripObserver();
+
+  // TabStripModelObserver:
+  void TabInsertedAt(content::WebContents* contents,
+                     int index,
+                     bool foreground) override;
+  void TabClosingAt(TabStripModel* tab_strip_model,
+                    content::WebContents* contents,
+                    int index) override;
+  void ActiveTabChanged(content::WebContents* old_contents,
+                        content::WebContents* new_contents,
+                        int index,
+                        int reason) override;
+  void TabPinnedStateChanged(content::WebContents* contents,
+                             int index) override;
+
+ private:
+  Browser* browser_;

[Georges Khalil, 2016/09/14 16:10:16]

Disallow copy.

[Patrick Monette, 2016/09/16 00:14:00]

Removed the class.

+};
+
+TabUsageRecorder::TabStripObserver::TabStripObserver(Browser* browser)
+    : browser_(browser) {
+  // Process tabs that are already open.
+  TabStripModel* tab_strip_model = browser_->tab_strip_model();
+  int active_index = tab_strip_model->active_index();
+  for (int i = 0; i < tab_strip_model->count(); ++i) {

[Georges Khalil, 2016/09/14 16:10:16]

nit: no braces.

[Patrick Monette, 2016/09/16 00:14:00]

Removed this class.

+    TabInsertedAt(tab_strip_model->GetWebContentsAt(i), i, i == active_index);
+  }
+
+  browser_->tab_strip_model()->AddObserver(this);
+}
+
+TabUsageRecorder::TabStripObserver::~TabStripObserver() {
+  browser_->tab_strip_model()->RemoveObserver(this);
+}
+
+void TabUsageRecorder::TabStripObserver::TabInsertedAt(
+    content::WebContents* contents,
+    int index,
+    bool foreground) {
+  WebContentsData::CreateForWebContents(
+      contents, browser_->tab_strip_model()->IsTabPinned(index));
+}
+
+void TabUsageRecorder::TabStripObserver::TabClosingAt(
+    TabStripModel* tab_strip_model,
+    content::WebContents* contents,
+    int index) {
+  WebContentsData::FromWebContents(contents)->OnTabClosing();
+}
+
+void TabUsageRecorder::TabStripObserver::ActiveTabChanged(
+    content::WebContents* old_contents,
+    content::WebContents* new_contents,
+    int index,
+    int reason) {
+  if (old_contents) {
+    WebContentsData::FromWebContents(old_contents)->OnTabDeactivating();
+  }
+  WebContentsData::FromWebContents(new_contents)->OnTabActivating();
+}
+
+void TabUsageRecorder::TabStripObserver::TabPinnedStateChanged(
+    content::WebContents* contents,
+    int index) {
+  WebContentsData::FromWebContents(contents)->OnTabPinnedStateChanging(
+      browser_->tab_strip_model()->IsTabPinned(index));
+}
+
+// static
+void TabUsageRecorder::Initialize() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  static TabUsageRecorder* tab_usage_metrics = new TabUsageRecorder();

[Georges Khalil, 2016/09/14 16:10:17]

This should be global and its lifetime well documented.

[Patrick Monette, 2016/09/16 00:13:59]

Done.

+}
+
+TabUsageRecorder::TabUsageRecorder() {
+  for (Browser* browser : *BrowserList::GetInstance()) {

[Georges Khalil, 2016/09/14 16:10:16]

nit: no braces.

[Patrick Monette, 2016/09/16 00:13:59]

Done.

+    OnBrowserAdded(browser);
+  }
+  BrowserList::GetInstance()->AddObserver(this);
+}
+
+TabUsageRecorder::~TabUsageRecorder() = default;
+
+void TabUsageRecorder::OnBrowserAdded(Browser* browser) {
+  tab_strip_observer_map_.emplace(browser,
+                                  base::MakeUnique<TabStripObserver>(browser));
+}
+
+void TabUsageRecorder::OnBrowserRemoved(Browser* browser) {
+  tab_strip_observer_map_.erase(browser);
+}
+
+}  // namespace metrics
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(metrics::WebContentsData);