Log NTP hovers in 1993 clients

chrome/browser/ui/search/instant_ntp.cc

Index: chrome/browser/ui/search/instant_ntp.cc
diff --git a/chrome/browser/ui/search/instant_ntp.cc b/chrome/browser/ui/search/instant_ntp.cc
index 8b5daec8bec626217914d25000e2709bb8320723..ff1931fb52ca19a4b1ac77a7fe719731ce04d8d0 100644
--- a/chrome/browser/ui/search/instant_ntp.cc
+++ b/chrome/browser/ui/search/instant_ntp.cc
@@ -4,9 +4,68 @@
 
 #include "chrome/browser/ui/search/instant_ntp.h"
 
+#include "base/metrics/histogram.h"
+#include "chrome/browser/search/search.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
+

[Alexei Svitkine (slow), 2013/06/28 18:50:29]

Nit: Remove extra blank line here.

[annark1, 2013/06/28 19:37:44]

Done.

+
+namespace {

[Alexei Svitkine (slow), 2013/06/28 18:50:29]

Nit: Add blank line after this.

[annark1, 2013/06/28 19:37:44]

Done.

+// Helper class for logging data from the NTP. Attached to each InstantNTP
+// instance.
+class NTPLoggingUserData
+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<NTPLoggingUserData> {
+ public:
+  virtual ~NTPLoggingUserData() {}
+
+  // Called each time the mouse hovers over an iframe or title.
+  void increment_number_of_mouseovers() {
+    number_of_mouseovers_++;
+  }
+
+  // Logs total number of mouseovers per NTP session to UMA histogram. Called
+  // when an NTP tab is about to be deactivated (be it by switching tabs, losing
+  // focus or closing the tab/shutting down Chrome) or when the user navigates
+  // to a URL.
+  void emit_mouseover_count() {

[Alexei Svitkine (slow), 2013/06/28 18:50:29]

This method isn't trivial so should use CamelCase convention for the name.

[annark1, 2013/06/28 19:37:44]

Done.

+    UMA_HISTOGRAM_COUNTS("NewTabPage.NumberOfMouseOvers",
+                         number_of_mouseovers_);
+    number_of_mouseovers_ = 0;
+  }
+
+  void set_instant_url(const std::string& url) {
+    instant_url_ = url;
+  }
+
+  // content::WebContentsObserver override
+  virtual void NavigationEntryCommitted OVERRIDE(
+      const content::LoadCommittedDetails& load_details) {
+    if (chrome::MatchesOriginAndPath(
+            GURL(instant_url_), load_details.previous_url))
+      emit_mouseover_count();
+  }
+
+ private:
+  explicit NTPLoggingUserData(content::WebContents* contents)
+      : content::WebContentsObserver(contents),
+        number_of_mouseovers_(0),
+        instant_url_("Needs To Be Set") {}
+  friend class content::WebContentsUserData<NTPLoggingUserData>;
+
+  int number_of_mouseovers_;
+  std::string instant_url_;

[Alexei Svitkine (slow), 2013/06/28 18:50:29]

Why not make this a GURL()? Also, remove the dummy value in the ctor, since the
default one should be fine. (You can then test .is_empty() to see if it was
initialized, if you need to.)

[annark1, 2013/06/28 19:37:44]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(NTPLoggingUserData);
+};
+
+}  // namespace
+
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(NTPLoggingUserData);
 
 InstantNTP::InstantNTP(InstantPage::Delegate* delegate,
                        const std::string& instant_url,
@@ -24,6 +83,11 @@ void InstantNTP::InitContents(Profile* profile,
   loader_.Init(GURL(instant_url()), profile, active_tab, on_stale_callback);
   SetContents(loader_.contents());
   SearchTabHelper::FromWebContents(contents())->InitForPreloadedNTP();
+
+  NTPLoggingUserData::CreateForWebContents(contents());
+  NTPLoggingUserData::FromWebContents(
+      contents())->set_instant_url(instant_url());
+
   loader_.Load();
 }
 
@@ -32,6 +96,19 @@ scoped_ptr<content::WebContents> InstantNTP::ReleaseContents() {
   return loader_.ReleaseContents();
 }
 

[Jered, 2013/06/28 18:43:09]

Put
// static
on the line above class static function definitions.

[annark1, 2013/06/28 19:37:44]

Done.

+void InstantNTP::CountMouseover(content::WebContents* contents) {
+  if (NTPLoggingUserData::FromWebContents(contents)) {

[Alexei Svitkine (slow), 2013/06/28 18:50:29]

Suggestion, instead you can do this to avoid querying it twice:

if (NTPLoggingUserData* data = NTPLoggingUserData::FromWebContents(contents))
  data->increment_number_of_mouseovers();

Here and below.

[annark1, 2013/06/28 19:37:44]

Done.

+    NTPLoggingUserData::FromWebContents(contents)->
+      increment_number_of_mouseovers();
+  }
+}
+
+void InstantNTP::EmitMouseoverCount(content::WebContents* contents) {
+  if (NTPLoggingUserData::FromWebContents(contents)) {
+    NTPLoggingUserData::FromWebContents(contents)->emit_mouseover_count();
+  }
+}
+
 void InstantNTP::OnSwappedContents() {
   SetContents(loader_.contents());
 }