Add some additional metadata to Popup.Closed UKM

This CL extends the amount of useful information a popup tracker
has, and reports it to UKM on close.

This data will additionally be useful if we add spammy popup detection
+ enforcement at run time.

Bug: 825875
Change-Id: I1366b795c500cbf58730887cf4e71c3f32b945a8
Reviewed-on: https://chromium-review.googlesource.com/c/1313508Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607725}

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc

@@ -137,8 +137,9 @@ void PopupBlockerTabHelper::ShowBlockedPopup(
   Navigate(&popup->params);
 #endif
   if (popup->params.navigated_or_inserted_contents) {
-    PopupTracker::CreateForWebContents(
+    auto* tracker = PopupTracker::CreateForWebContents(
         popup->params.navigated_or_inserted_contents, web_contents());
+    tracker->set_is_trusted(true);
 
     if (popup->params.disposition == WindowOpenDisposition::NEW_POPUP) {
       content::RenderFrameHost* host =

chrome/browser/ui/blocked_content/popup_tracker.cc

@@ -15,14 +15,16 @@
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
 
-void PopupTracker::CreateForWebContents(content::WebContents* contents,
-                                        content::WebContents* opener) {
+PopupTracker* PopupTracker::CreateForWebContents(content::WebContents* contents,
+                                                 content::WebContents* opener) {
   DCHECK(contents);
   DCHECK(opener);
-  if (!FromWebContents(contents)) {
-    contents->SetUserData(UserDataKey(),
-                          base::WrapUnique(new PopupTracker(contents, opener)));
+  auto* tracker = FromWebContents(contents);
+  if (!tracker) {
+    tracker = new PopupTracker(contents, opener);
+    contents->SetUserData(UserDataKey(), base::WrapUnique(tracker));
   }
+  return tracker;
 }
 
 PopupTracker::~PopupTracker() = default;
@@ -61,10 +63,16 @@ void PopupTracker::WebContentsDestroyed() {
   }
 
   if (opener_source_id_ != ukm::kInvalidSourceId) {
+    const int kMaxInteractions = 100;
+    int capped_interactions = num_interactions_ > kMaxInteractions
+                                  ? kMaxInteractions
+                                  : num_interactions_;
     ukm::builders::Popup_Closed(opener_source_id_)
         .SetEngagementTime(ukm::GetExponentialBucketMinForUserTiming(
             total_foreground_duration.InMilliseconds()))
         .SetUserInitiatedClose(web_contents()->GetClosedByUserGesture())
+        .SetTrusted(is_trusted_)
+        .SetNumInteractions(capped_interactions)
         .Record(ukm::UkmRecorder::Get());
   }
 }
@@ -93,3 +101,10 @@ void PopupTracker::OnVisibilityChanged(content::Visibility visibility) {
   else
     visibility_tracker_.OnShown();
 }
+
+void PopupTracker::DidGetUserInteraction(
+    const blink::WebInputEvent::Type type) {
+  // TODO(csharrison): It would be nice if ctrl-W could be filtered out here,
+  // but the initial ctrl key press is registered as a kRawKeyDown.
+  num_interactions_++;
+}

chrome/browser/ui/blocked_content/popup_tracker.h

@@ -26,10 +26,12 @@ class ScopedVisibilityTracker;
 class PopupTracker : public content::WebContentsObserver,
                      public content::WebContentsUserData<PopupTracker> {
  public:
-  static void CreateForWebContents(content::WebContents* contents,
-                                   content::WebContents* opener);
+  static PopupTracker* CreateForWebContents(content::WebContents* contents,
+                                            content::WebContents* opener);
   ~PopupTracker() override;
 
+  void set_is_trusted(bool is_trusted) { is_trusted_ = is_trusted; }
+
  private:
   friend class content::WebContentsUserData<PopupTracker>;
 
@@ -40,6 +42,7 @@ class PopupTracker : public content::WebContentsObserver,
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void OnVisibilityChanged(content::Visibility visibility) override;
+  void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
 
   // Will be unset until the first navigation commits. Will be set to the total
   // time the contents was visible at commit time.
@@ -50,10 +53,16 @@ class PopupTracker : public content::WebContentsObserver,
 
   ScopedVisibilityTracker visibility_tracker_;
 
+  // The number of user interactions occuring in this popup tab.
+  int num_interactions_ = 0;
+
   // The id of the web contents that created the popup at the time of creation.
   // SourceIds are permanent so it's okay to use at any point so long as it's
   // not invalid.
   const ukm::SourceId opener_source_id_;
+
+  bool is_trusted_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(PopupTracker);
 };
 

chrome/browser/ui/blocked_content/popup_tracker_browsertest.cc

@@ -41,6 +41,8 @@ const char kPopupGestureClose[] =
 
 const char kUkmEngagementTime[] = "EngagementTime";
 const char kUkmUserInitiatedClose[] = "UserInitiatedClose";
+const char kUkmTrusted[] = "Trusted";
+const char kUkmNumInteractions[] = "NumInteractions";
 }  // namespace
 
 using UkmEntry = ukm::builders::Popup_Closed;
@@ -66,17 +68,14 @@ class PopupTrackerBrowserTest : public InProcessBrowserTest {
     return test_ukm_recorder_->GetEntriesByName(UkmEntry::kEntryName).size();
   }
 
-  void VerifyUkm(const GURL& expected_url,
-                 UserClosedPopup expected_user_closed) {
+  const ukm::mojom::UkmEntry* ExpectAndGetEntry(const GURL& expected_url) {
     const auto& entries =
         test_ukm_recorder_->GetEntriesByName(UkmEntry::kEntryName);
-    ASSERT_EQ(1u, entries.size());
+    EXPECT_EQ(1u, entries.size());
     const auto* entry = entries[0];
     test_ukm_recorder_->ExpectEntrySourceHasUrl(entry, expected_url);
     EXPECT_TRUE(test_ukm_recorder_->EntryHasMetric(entry, kUkmEngagementTime));
-    test_ukm_recorder_->ExpectEntryMetric(
-        entry, kUkmUserInitiatedClose,
-        expected_user_closed == UserClosedPopup::kTrue ? 1u : 0u);
+    return entry;
   }
 
   std::unique_ptr<ukm::TestAutoSetUkmRecorder> test_ukm_recorder_;
@@ -126,7 +125,51 @@ IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest,
   tester.ExpectTotalCount(kPopupEngagement, 1);
   tester.ExpectTotalCount(kPopupGestureClose, 1);
 
-  VerifyUkm(first_url, UserClosedPopup::kTrue);
+  auto* entry = ExpectAndGetEntry(first_url);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmUserInitiatedClose, 1u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmTrusted, 0u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmNumInteractions, 0u);
+}
+
+IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest,
+                       WindowOpenPopup_WithInteraction) {
+  base::HistogramTester tester;
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  content::TestNavigationObserver navigation_observer(nullptr, 1);
+  navigation_observer.StartWatchingNewWebContents();
+
+  EXPECT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "window.open('/title1.html')"));
+  navigation_observer.Wait();
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+  content::WebContents* popup =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(PopupTracker::FromWebContents(popup));
+
+  // Perform some user gestures on the page.
+  content::SimulateMouseClick(popup, 0, blink::WebMouseEvent::Button::kLeft);
+  content::SimulateMouseClick(popup, 0, blink::WebMouseEvent::Button::kLeft);
+  content::SimulateMouseClick(popup, 0, blink::WebMouseEvent::Button::kLeft);
+
+  // Close the popup and check metric.
+  int active_index = browser()->tab_strip_model()->active_index();
+  content::WebContentsDestroyedWatcher destroyed_watcher(popup);
+  browser()->tab_strip_model()->CloseWebContentsAt(
+      active_index, TabStripModel::CLOSE_USER_GESTURE);
+  destroyed_watcher.Wait();
+
+  tester.ExpectTotalCount(kPopupFirstDocumentEngagement, 1);
+  tester.ExpectTotalCount(kPopupEngagement, 1);
+  tester.ExpectTotalCount(kPopupGestureClose, 1);
+
+  auto* entry = ExpectAndGetEntry(first_url);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmUserInitiatedClose, 1u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmTrusted, 0u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmNumInteractions, 3u);
 }
 
 // OpenURLFromTab goes through a different code path than traditional popups
@@ -166,7 +209,10 @@ IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, ControlClick_HasTracker) {
   tester.ExpectTotalCount(kPopupFirstDocumentEngagement, 1);
   tester.ExpectTotalCount(kPopupEngagement, 1);
 
-  VerifyUkm(url, UserClosedPopup::kFalse);
+  auto* entry = ExpectAndGetEntry(url);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmUserInitiatedClose, 0u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmTrusted, 1u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmNumInteractions, 0u);
 }
 
 IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, ShiftClick_HasTracker) {
@@ -200,7 +246,10 @@ IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, ShiftClick_HasTracker) {
   tester.ExpectTotalCount(kPopupFirstDocumentEngagement, 1);
   tester.ExpectTotalCount(kPopupEngagement, 1);
 
-  VerifyUkm(url, UserClosedPopup::kFalse);
+  auto* entry = ExpectAndGetEntry(url);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmUserInitiatedClose, 0u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmTrusted, 1u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmNumInteractions, 0u);
 }
 
 IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, WhitelistedPopup_HasTracker) {
@@ -233,7 +282,10 @@ IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, WhitelistedPopup_HasTracker) {
   tester.ExpectTotalCount(kPopupFirstDocumentEngagement, 1);
   tester.ExpectTotalCount(kPopupEngagement, 1);
 
-  VerifyUkm(url, UserClosedPopup::kFalse);
+  auto* entry = ExpectAndGetEntry(url);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmUserInitiatedClose, 0u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmTrusted, 1u);
+  test_ukm_recorder_->ExpectEntryMetric(entry, kUkmNumInteractions, 0u);
 }
 
 IN_PROC_BROWSER_TEST_F(PopupTrackerBrowserTest, NoOpener_NoTracker) {

chrome/browser/ui/browser.cc

@@ -1351,9 +1351,12 @@ WebContents* Browser::OpenURLFromTab(WebContents* source,
 
   Navigate(&nav_params);
 
-  if (is_popup && nav_params.navigated_or_inserted_contents)
-    PopupTracker::CreateForWebContents(
+  if (is_popup && nav_params.navigated_or_inserted_contents) {
+    auto* tracker = PopupTracker::CreateForWebContents(
         nav_params.navigated_or_inserted_contents, source);
+    tracker->set_is_trusted(params.triggering_event_info !=
+                            blink::WebTriggeringEventInfo::kFromUntrustedEvent);
+  }
 
   return nav_params.navigated_or_inserted_contents;
 }

tools/metrics/ukm/ukm.xml

@@ -4061,6 +4061,20 @@ be describing additional metrics about the same event.
       popup opener's URL (not the URL of the popup).
     </summary>
   </metric>
+  <metric name="NumInteractions">
+    <summary>
+      Integer value representing how many user interactions the popup had. This
+      is capped at 100. See IsUserInteractionInputType in web_contents_impl for
+      what an interaction is. Currently it is a mouse, scroll begin, touch
+      start, or raw key down event.
+    </summary>
+  </metric>
+  <metric name="Trusted">
+    <summary>
+      Boolean value representing whether the popup was opened via some trusted
+      event (e.g. context menu, ctrl-click, etc).
+    </summary>
+  </metric>
   <metric name="UserInitiatedClose">
     <summary>
       Boolean value to represent whether the popup was closed by user gesture.