Change record time for Popup_Page UKM to pop-up open time.

Current Popup_Page UKM is recorded when the web contents is destroyed,
this can lead to recording pop-up settings for source ids different
than the pop-up's opener source id.

This changelist also adds browsertests for logging of the UKM and
makes the ukm for Popup_Page singular as it should be.

BUG=1044601

Change-Id: I241a520b61bda691ce61ab4fdbac875f3397ac08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087975
Commit-Queue: Justin Miron <justinmiron@google.com>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751359}

chrome/browser/ui/blocked_content/popup_opener_tab_helper.cc

@@ -12,6 +12,7 @@
 #include "base/time/tick_clock.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/blocked_content/popup_tracker.h"
 #include "chrome/browser/ui/blocked_content/tab_under_navigation_throttle.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
@@ -51,7 +52,8 @@ PopupOpenerTabHelper::~PopupOpenerTabHelper() {
 
 void PopupOpenerTabHelper::OnOpenedPopup(PopupTracker* popup_tracker) {
   has_opened_popup_since_last_user_gesture_ = true;
-  has_opened_popup_ = true;
+  MaybeLogPagePopupContentSettings();
+
   last_popup_open_time_ = tick_clock_->NowTicks();
 }
 
@@ -76,28 +78,6 @@ PopupOpenerTabHelper::PopupOpenerTabHelper(content::WebContents* web_contents,
       web_contents->GetVisibility() != content::Visibility::HIDDEN);
 }
 
-void PopupOpenerTabHelper::WebContentsDestroyed() {
-  // If the user has opened a popup, record the page popup settings ukm.
-  if (has_opened_popup_) {
-    const GURL& url = web_contents()->GetLastCommittedURL();
-    if (!url.is_valid()) {
-      return;
-    }
-
-    const ukm::SourceId source_id =
-        ukm::GetSourceIdForWebContentsDocument(web_contents());
-    Profile* profile =
-        Profile::FromBrowserContext(web_contents()->GetBrowserContext());
-    bool user_allows_popups =
-        HostContentSettingsMapFactory::GetForProfile(profile)
-            ->GetContentSetting(url, url, ContentSettingsType::POPUPS,
-                                std::string()) == CONTENT_SETTING_ALLOW;
-    ukm::builders::Popup_Page(source_id)
-        .SetAllowed(user_allows_popups)
-        .Record(ukm::UkmRecorder::Get());
-  }
-}
-
 void PopupOpenerTabHelper::OnVisibilityChanged(content::Visibility visibility) {
   // TODO(csharrison): Consider handling OCCLUDED tabs the same way as HIDDEN
   // tabs.
@@ -119,4 +99,29 @@ void PopupOpenerTabHelper::DidStartNavigation(
     has_opened_popup_since_last_user_gesture_ = false;
 }
 
+void PopupOpenerTabHelper::MaybeLogPagePopupContentSettings() {
+  // If the user has opened a popup, record the page popup settings ukm.
+  const GURL& url = web_contents()->GetLastCommittedURL();
+  if (!url.is_valid())
+    return;
+
+  const ukm::SourceId source_id =
+      ukm::GetSourceIdForWebContentsDocument(web_contents());
+
+  // Do not record duplicate Popup.Page events for popups opened in succession
+  // from the same opener.
+  if (source_id != last_opener_source_id_) {
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+    bool user_allows_popups =
+        HostContentSettingsMapFactory::GetForProfile(profile)
+            ->GetContentSetting(url, url, ContentSettingsType::POPUPS,
+                                std::string()) == CONTENT_SETTING_ALLOW;
+    ukm::builders::Popup_Page(source_id)
+        .SetAllowed(user_allows_popups)
+        .Record(ukm::UkmRecorder::Get());
+    last_opener_source_id_ = source_id;
+  }
+}
+
 WEB_CONTENTS_USER_DATA_KEY_IMPL(PopupOpenerTabHelper)

chrome/browser/ui/blocked_content/popup_opener_tab_helper.h

@@ -59,12 +59,16 @@ class PopupOpenerTabHelper
                        const base::TickClock* tick_clock);
 
   // content::WebContentsObserver:
-  void WebContentsDestroyed() override;
   void OnVisibilityChanged(content::Visibility visibility) override;
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidGetUserInteraction(const blink::WebInputEvent::Type type) override;
 
+  // Logs user popup content settings if the last committed URL is valid and
+  // we have not recorded the settings for the opener id of the helper's
+  // web contents at the time the function is called.
+  void MaybeLogPagePopupContentSettings();
+
   // Visible time for this tab until a tab-under is detected. At which point it
   // gets the visible time from the |visibility_tracker_|. Will be unset until a
   // tab-under is detected.
@@ -81,8 +85,8 @@ class PopupOpenerTabHelper
 
   bool has_opened_popup_since_last_user_gesture_ = false;
 
-  // Whether this WebContents has opened a popup.
-  bool has_opened_popup_ = false;
+  // The last source id used for logging Popup_Page.
+  ukm::SourceId last_opener_source_id_ = ukm::kInvalidSourceId;
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 

chrome/browser/ui/blocked_content/popup_opener_tab_helper_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/blocked_content/popup_opener_tab_helper.h"
+
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/ukm/content/source_url_recorder.h"
+#include "components/ukm/test_ukm_recorder.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "services/metrics/public/cpp/ukm_builders.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const char kUkmAllowed[] = "Allowed";
+}  // namespace
+
+class PopupOpenerTabHelperBrowserTest : public InProcessBrowserTest {
+ public:
+  PopupOpenerTabHelperBrowserTest() = default;
+  ~PopupOpenerTabHelperBrowserTest() override = default;
+
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
+  void PreRunTestOnMainThread() override {
+    InProcessBrowserTest::PreRunTestOnMainThread();
+    test_ukm_recorder_ = std::make_unique<ukm::TestAutoSetUkmRecorder>();
+  }
+
+  content::WebContents* GetActiveWebContents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
+  // Opens and waits for a pop-up to finish navigation before closing the
+  // pop-up. Returns the opener's source id.
+  void OpenAndClosePopup() {
+    content::TestNavigationObserver navigation_observer(nullptr, 1);
+    navigation_observer.StartWatchingNewWebContents();
+
+    EXPECT_TRUE(
+        content::ExecJs(GetActiveWebContents(), "window.open('/title1.html')"));
+    navigation_observer.Wait();
+
+    // Close the popup.
+    content::WebContents* popup = GetActiveWebContents();
+    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();
+  }
+
+ protected:
+  std::unique_ptr<ukm::TestAutoSetUkmRecorder> test_ukm_recorder_;
+};
+
+IN_PROC_BROWSER_TEST_F(PopupOpenerTabHelperBrowserTest,
+                       UserDefaultPopupBlockerSetting_MetricLoggedOnPopup) {
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  // Open and close two pop-ups, the opener id does not change.
+  const ukm::SourceId opener_source_id =
+      ukm::GetSourceIdForWebContentsDocument(GetActiveWebContents());
+  OpenAndClosePopup();
+  OpenAndClosePopup();
+
+  const auto& entries = test_ukm_recorder_->GetEntriesByName(
+      ukm::builders::Popup_Page::kEntryName);
+
+  // Only a single Popup_Page should be logged per source id.
+  EXPECT_EQ(entries.size(), 1u);
+
+  // The source id should match the opener's source id at the time of opening
+  // the pop-up.
+  EXPECT_EQ(entries[0]->source_id, opener_source_id);
+
+  // Profile content settings default to pop-ups not explicitly allowed.
+  test_ukm_recorder_->ExpectEntryMetric(entries[0], kUkmAllowed, 0u);
+}
+
+IN_PROC_BROWSER_TEST_F(PopupOpenerTabHelperBrowserTest,
+                       UserAllowsAllPopups_MetricLoggedOnPopup) {
+  const GURL first_url = embedded_test_server()->GetURL("/title1.html");
+  ui_test_utils::NavigateToURL(browser(), first_url);
+
+  HostContentSettingsMap* host_content_settings_map =
+      HostContentSettingsMapFactory::GetForProfile(browser()->profile());
+  host_content_settings_map->SetContentSettingDefaultScope(
+      first_url, first_url, ContentSettingsType::POPUPS, std::string(),
+      CONTENT_SETTING_ALLOW);
+
+  // Open and close two pop-ups, the opener id does not change.
+  const ukm::SourceId opener_source_id =
+      ukm::GetSourceIdForWebContentsDocument(GetActiveWebContents());
+  OpenAndClosePopup();
+  OpenAndClosePopup();
+
+  const auto& entries = test_ukm_recorder_->GetEntriesByName(
+      ukm::builders::Popup_Page::kEntryName);
+
+  // Only a single Popup_Page should be logged per source id.
+  EXPECT_EQ(entries.size(), 1u);
+
+  // The source id should match the opener's source id at the time of opening
+  // the pop-up.
+  EXPECT_EQ(entries[0]->source_id, opener_source_id);
+
+  // Profile content settings default to pop-ups not explicitly allowed.
+  test_ukm_recorder_->ExpectEntryMetric(entries[0], kUkmAllowed, 1u);
+}

chrome/test/BUILD.gn

@@ -1158,6 +1158,7 @@ if (!is_android) {
       "../browser/site_isolation/chrome_site_per_process_test.h",
       "../browser/site_isolation/site_details_browsertest.cc",
       "../browser/ssl/known_interception_disclosure_ui_browsertest.cc",
+      "../browser/ui/blocked_content/popup_opener_tab_helper_browsertest.cc",
       "../browser/ui/blocked_content/popup_tracker_browsertest.cc",
       "../browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_browsertest.cc",
       "../browser/ui/blocked_content/tab_under_blocker_browsertest.cc",

tools/metrics/ukm/ukm.xml

@@ -8511,7 +8511,7 @@ be describing additional metrics about the same event.
   </metric>
 </event>
 
-<event name="Popup.Page">
+<event name="Popup.Page" singular="True">
   <owner>justinmiron@google.com</owner>
   <summary>
     Page level pop-up statistics.