Log NTP hovers in 1993 clients

chrome/browser/ui/search/instant_controller.cc

Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc
index 838654f4dc3efbed79973fa30ef68df80590627d..6b19231c111867ea47d8324364150b187403b0f4 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -43,6 +43,7 @@
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_user_data.h"
 #include "content/public/browser/web_contents_view.h"
 #include "net/base/escape.h"
 #include "net/base/network_change_notifier.h"
@@ -250,8 +251,41 @@ void DeletePageSoon(scoped_ptr<T> page) {
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, page.release());
 }
 
+class NtpLoggingUserData

[beaudoin, 2013/06/22 01:14:53]

Please document the class.

[annark1, 2013/06/25 16:01:38]

Done.

+    : public content::WebContentsUserData<NtpLoggingUserData> {
+  public:
+    virtual ~NtpLoggingUserData() {}

[beaudoin, 2013/06/22 01:14:53]

Indentation is wrong in the class:
one space before "public:" or "private:"
two spaces before methods/members.

[annark1, 2013/06/25 16:01:38]

Done.

+
+    // Called each time the mouse hovers over an iframe or title.
+    void increment_iframe_hovers(int iframe_pos) {
+      ntp_logging_data_[iframe_pos] += 1;

[beaudoin, 2013/06/22 01:14:53]

++

[annark1, 2013/06/25 16:01:38]

Done.

+    }
+
+    // Sums the values in the |ntp_logging_data_| map and logs to histogram.

[beaudoin, 2013/06/22 01:14:53]

Clarify to indicate that you are summing over every iframe.

[annark1, 2013/06/25 16:01:38]

changed to just an int.
On 2013/06/22 01:14:53, beaudoin wrote:

+    // Called when a tab is about to be deactivated.
+    void sum_values_and_log() {
+      int result = 0;
+      for (size_t i = 0; i < ntp_logging_data_.size(); ++i) {
+        result += ntp_logging_data_[i];
+      }
+      if (result > 0) {
+        HISTOGRAM_COUNTS_10000("NewTabPage.NumberOfMouseOvers", result);
+        ntp_logging_data_.clear();
+      }

[beaudoin, 2013/06/22 01:14:53]

Please check with asvitkine@ that you're using this correctly. In particular,
should you manually clamp result to 10000?

[annark1, 2013/06/25 16:01:38]

Alexei suggested just using the default UMA_HISTOGRAM_COUNTS macro.
On 2013/06/22 01:14:53, beaudoin wrote:

+    }
+
+  private:
+    explicit NtpLoggingUserData(content::WebContents* contents) {}
+    friend class content::WebContentsUserData<NtpLoggingUserData>;
+
+    // Map in which to log user data.
+    std::map<int, int> ntp_logging_data_;
+};
+
 }  // namespace
 
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(NtpLoggingUserData);
+
 InstantController::InstantController(BrowserInstantController* browser,
                                      bool extended_enabled)
     : browser_(browser),
@@ -1051,6 +1085,12 @@ void InstantController::TabDeactivated(content::WebContents* contents) {
 
   if (GetOverlayContents())
     HideOverlay();
+
+  NtpLoggingUserData* data =
+      NtpLoggingUserData::FromWebContents(contents);
+  if (data) {
+    data->sum_values_and_log();
+  }

[beaudoin, 2013/06/22 01:14:53]

Nit: No {}

[annark1, 2013/06/25 16:01:38]

Done.

 }
 
 void InstantController::SetInstantEnabled(bool instant_enabled,
@@ -1600,6 +1640,11 @@ void InstantController::ResetNTP(const std::string& instant_url) {
   ntp_->InitContents(profile(), browser_->GetActiveWebContents(),
                      base::Bind(&InstantController::ReloadStaleNTP,
                                 base::Unretained(this)));
+
+  if (ntp_->contents()) {
+    NtpLoggingUserData::CreateForWebContents(ntp_->contents());
+  }

[beaudoin, 2013/06/22 01:14:53]

Nit: no {}

[annark1, 2013/06/25 16:01:38]

Done.

+
   LOG_INSTANT_DEBUG_EVENT(this, base::StringPrintf(
       "ResetNTP: instant_url='%s'", instant_url.c_str()));
 }
@@ -1900,6 +1945,17 @@ void InstantController::PopulateInstantAutocompleteResultFromMatch(
       << result->transition <<  " " << result->autocomplete_match_index;
 }
 
+void InstantController::LogIframeHover(int pos) {
+  if (!browser_ || !browser_->GetActiveWebContents())
+    return;
+
+  NtpLoggingUserData* data =
+      NtpLoggingUserData::FromWebContents(browser_->GetActiveWebContents());
+  if (data) {
+    data->increment_iframe_hovers(pos);

[beaudoin, 2013/06/22 01:14:53]

Do you really have to keep the "pos" here? If you're only interested in total
number of hovers, you could keep a single int and keep a running sum in it.
There is not really a point in keeping it in an array...

[annark1, 2013/06/25 16:01:38]

Yes, I agree that only an int is needed to just log total number of hovers. This
setup had stemmed from a discussion with Mathieu that further down the line we
may want to know more information on a per tile basis, hence logging by
position. However, as that isn't used/implemented in this patch, I've removed
the position variable.

On 2013/06/22 01:14:53, beaudoin wrote:

+  }
+}
+
 bool InstantController::IsJavascriptEnabled() const {
   GURL instant_url(GetInstantURL());
   GURL origin(instant_url.GetOrigin());