From c61d59732ba6a1bd8c25829d2aa7f854236d464d Mon Sep 17 00:00:00 2001 From: Cosmin Sabou Date: Wed, 20 Sep 2023 01:23:00 +0300 Subject: [PATCH] Backed out changeset 36e95068e103 (bug 1848160) for bc failures on browser_preferences_usage.js. --- browser/base/content/browser.js | 41 ++++--- browser/components/shopping/ShoppingUtils.sys.mjs | 56 ---------- browser/components/shopping/metrics.yaml | 20 ---- .../components/shopping/tests/browser/browser.ini | 1 - .../tests/browser/browser_exposure_telemetry.js | 123 --------------------- .../tests/browser/browser_settings_telemetry.js | 5 +- toolkit/components/nimbus/FeatureManifest.yaml | 6 +- toolkit/components/telemetry/Scalars.yaml | 16 --- 8 files changed, 27 insertions(+), 241 deletions(-) delete mode 100644 browser/components/shopping/tests/browser/browser_exposure_telemetry.js diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index 3c365c4892bc..19fe29b22a86 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -10069,7 +10069,7 @@ var ShoppingSidebarManager = { } let { selectedBrowser, currentURI } = gBrowser; - this._maybeToggleSidebar(selectedBrowser, currentURI, 0); + this.onLocationChange(selectedBrowser, currentURI, 0); }, /** @@ -10079,15 +10079,6 @@ var ShoppingSidebarManager = { * those can be significant for us. */ onLocationChange(aBrowser, aLocationURI, aFlags) { - ShoppingUtils.maybeRecordExposure(aLocationURI, aFlags); - this._maybeToggleSidebar(aBrowser, aLocationURI, aFlags); - }, - - // The strange signature is because this function was formerly the - // onLocationChange function, but we needed to differentiate between - // calls triggered by actual location changes and calls triggered by - // TabSelect. We will refactor this code in bug 1845842. - _maybeToggleSidebar(aBrowser, aLocationURI, aFlags) { if (!this._enabled) { return; } @@ -10119,12 +10110,30 @@ var ShoppingSidebarManager = { this._updateBCActiveness(aBrowser); this._setShoppingButtonState(aBrowser); - if ( - sidebar && - !sidebar.hidden && - ShoppingUtils.isProductPageNavigation(aLocationURI, aFlags) - ) { - Glean.shopping.surfaceDisplayed.record(); + if (isProduct) { + let isVisible = sidebar && !sidebar.hidden; + + // Ignore same-document navigation, except in the case of Walmart + // as they use pushState to navigate between pages. + let isWalmart = aLocationURI.host.includes("walmart"); + let isNewDocument = !aFlags; + + let isSameDocument = + aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT; + let isReload = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD; + let isSessionRestore = + aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SESSION_STORE; + + if ( + isVisible && + // On initial visit to a product page, even from another domain, both a page + // load and a pushstate will be triggered by Walmart, so this will + // capture only a single displayed event. + ((!isWalmart && (isNewDocument || isReload || isSessionRestore)) || + (isWalmart && isSameDocument)) + ) { + Glean.shopping.surfaceDisplayed.record(); + } } if (isProduct) { diff --git a/browser/components/shopping/ShoppingUtils.sys.mjs b/browser/components/shopping/ShoppingUtils.sys.mjs index 6a51346f1171..c25437c56cd2 100644 --- a/browser/components/shopping/ShoppingUtils.sys.mjs +++ b/browser/components/shopping/ShoppingUtils.sys.mjs @@ -7,9 +7,7 @@ import { XPCOMUtils } from "resource://gre/modules/XPCOMUtils.sys.mjs"; const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { - isProductURL: "chrome://global/content/shopping/ShoppingProduct.mjs", NimbusFeatures: "resource://nimbus/ExperimentAPI.sys.mjs", - setTimeout: "resource://gre/modules/Timer.sys.mjs", }); XPCOMUtils.defineLazyModuleGetters(lazy, { @@ -77,60 +75,6 @@ export const ShoppingUtils = { this.initialized = false; }, - isProductPageNavigation(aLocationURI, aFlags) { - if (!lazy.isProductURL(aLocationURI)) { - return false; - } - - // Ignore same-document navigation, except in the case of Walmart - // as they use pushState to navigate between pages. - let isWalmart = aLocationURI.host.includes("walmart"); - let isNewDocument = !aFlags; - - let isSameDocument = - aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT; - let isReload = aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD; - let isSessionRestore = - aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SESSION_STORE; - - // Unfortunately, Walmart sometimes double-fires history manipulation - // events when navigating between product pages. To dedupe, cache the - // last visited Walmart URL just for a few milliseconds, so we can avoid - // double-counting such navigations. - if (isWalmart) { - if ( - this.lastWalmartURI && - aLocationURI.equalsExceptRef(this.lastWalmartURI) - ) { - return false; - } - this.lastWalmartURI = aLocationURI; - lazy.setTimeout(() => { - this.lastWalmartURI = null; - }, 100); - } - - return ( - // On initial visit to a product page, even from another domain, both a page - // load and a pushState will be triggered by Walmart, so this will - // capture only a single displayed event. - (!isWalmart && (isNewDocument || isReload || isSessionRestore)) || - (isWalmart && isSameDocument) - ); - }, - - // For users in either the nimbus control or treatment groups, increment a - // counter when they visit supported product pages. - maybeRecordExposure(aLocationURI, aFlags) { - if ( - (lazy.NimbusFeatures.shopping2023.getVariable("enabled") || - lazy.NimbusFeatures.shopping2023.getVariable("control")) && - ShoppingUtils.isProductPageNavigation(aLocationURI, aFlags) - ) { - Glean.shopping.productPageVisits.add(1); - } - }, - setOnUpdate(_pref, _prev, current) { Glean.shoppingSettings.componentOptedOut.set(current === 2); Glean.shoppingSettings.hasOnboarded.set(current > 0); diff --git a/browser/components/shopping/metrics.yaml b/browser/components/shopping/metrics.yaml index faf78610b812..c753c5bce9c5 100644 --- a/browser/components/shopping/metrics.yaml +++ b/browser/components/shopping/metrics.yaml @@ -428,26 +428,6 @@ shopping: send_in_pings: - events - product_page_visits: - type: counter - description: | - Counts number of visits to a supported retailer product page - while enrolled in either the control or treatment branches - of the shopping experiment. - bugs: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1848160 - data_reviews: - - https://bugzilla.mozilla.org/show_bug.cgi?id=1848160 - data_sensitivity: - - interaction - expires: 122 - notification_emails: - - betling@mozilla.com - - fx-desktop-shopping-eng@mozilla.com - send_in_pings: - - metrics - telemetry_mirror: SHOPPING_PRODUCT_PAGE_VISITS - surface_powered_by_fakespot_link_clicked: type: event description: | diff --git a/browser/components/shopping/tests/browser/browser.ini b/browser/components/shopping/tests/browser/browser.ini index 79ef117c39d1..8f9de1ab7f5c 100644 --- a/browser/components/shopping/tests/browser/browser.ini +++ b/browser/components/shopping/tests/browser/browser.ini @@ -15,7 +15,6 @@ prefs = browser.newtabpage.activity-stream.asrouter.userprefs.cfr.features=false [browser_adjusted_rating.js] -[browser_exposure_telemetry.js] [browser_inprogress_analysis.js] [browser_network_offline.js] [browser_not_enough_reviews.js] diff --git a/browser/components/shopping/tests/browser/browser_exposure_telemetry.js b/browser/components/shopping/tests/browser/browser_exposure_telemetry.js deleted file mode 100644 index 8722ec3af835..000000000000 --- a/browser/components/shopping/tests/browser/browser_exposure_telemetry.js +++ /dev/null @@ -1,123 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -ChromeUtils.defineESModuleGetters(this, { - ShoppingUtils: "resource:///modules/ShoppingUtils.sys.mjs", -}); - -// Tests in this file simulate exposure detection without actually loading the -// product pages. Instead, we call the `ShoppingUtils.maybeRecordExposure` -// method, passing in flags and URLs to simulate onLocationChange events. -// Bug 1853401 captures followup work to add integration tests. - -const PRODUCT_PAGE = Services.io.newURI( - "https://example.com/product/B09TJGHL5F" -); -const WALMART_PAGE = Services.io.newURI( - "https://www.walmart.com/ip/Utz-Cheese-Balls-23-Oz/15543964" -); -const WALMART_OTHER_PAGE = Services.io.newURI( - "https://www.walmart.com/ip/Utz-Gluten-Free-Cheese-Balls-23-0-OZ/10898644" -); - -async function setup(pref) { - await SpecialPowers.pushPrefEnv({ - set: [[`browser.shopping.experience2023.${pref}`, true]], - }); - await Services.fog.testFlushAllChildren(); - Services.fog.testResetFOG(); -} - -async function teardown(pref) { - await SpecialPowers.popPrefEnv(); - await Services.fog.testFlushAllChildren(); - Services.fog.testResetFOG(); - - // Clear out the normally short-lived pushState navigation cache in between - // runs, to avoid accidentally deduping when we shouldn't. - ShoppingUtils.lastWalmartURI = null; -} - -async function runTest({ aLocationURI, aFlags, expected }) { - async function _run() { - Assert.equal(undefined, Glean.shopping.productPageVisits.testGetValue()); - ShoppingUtils.maybeRecordExposure(aLocationURI, aFlags); - await Services.fog.testFlushAllChildren(); - Assert.equal(expected, Glean.shopping.productPageVisits.testGetValue()); - } - - await setup("enabled"); - await _run(); - await teardown("enabled"); - - await setup("control"); - await _run(); - await teardown("control"); -} - -add_task(async function test_shopping_exposure_new_page() { - await runTest({ - aLocationURI: PRODUCT_PAGE, - aFlags: 0, - expected: 1, - }); -}); - -add_task(async function test_shopping_exposure_reload_page() { - await runTest({ - aLocationURI: PRODUCT_PAGE, - aFlags: Ci.nsIWebProgressListener.LOCATION_CHANGE_RELOAD, - expected: 1, - }); -}); - -add_task(async function test_shopping_exposure_session_restore_page() { - await runTest({ - aLocationURI: PRODUCT_PAGE, - aFlags: Ci.nsIWebProgressListener.LOCATION_CHANGE_SESSION_STORE, - expected: 1, - }); -}); - -add_task(async function test_shopping_exposure_ignore_same_page() { - await runTest({ - aLocationURI: PRODUCT_PAGE, - aFlags: Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT, - expected: undefined, - }); -}); - -add_task(async function test_shopping_exposure_count_same_page_pushstate() { - await runTest({ - aLocationURI: WALMART_PAGE, - aFlags: Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT, - expected: 1, - }); -}); - -add_task(async function test_shopping_exposure_ignore_pushstate_repeats() { - async function _run() { - let aFlags = Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT; - Assert.equal(undefined, Glean.shopping.productPageVisits.testGetValue()); - - // Slightly different setup here: simulate deduping by setting the first - // walmart page's URL as the `ShoppingUtils.lastWalmartURI`, then fire the - // pushState for the first page, then twice for a second page. This seems - // to be roughly the observed behavior when navigating between walmart - // product pages. - ShoppingUtils.lastWalmartURI = WALMART_PAGE; - ShoppingUtils.maybeRecordExposure(WALMART_PAGE, aFlags); - ShoppingUtils.maybeRecordExposure(WALMART_OTHER_PAGE, aFlags); - ShoppingUtils.maybeRecordExposure(WALMART_OTHER_PAGE, aFlags); - await Services.fog.testFlushAllChildren(); - Assert.equal(1, Glean.shopping.productPageVisits.testGetValue()); - } - await setup("enabled"); - await _run(); - await teardown("enabled"); - await setup("control"); - await _run(); - await teardown("control"); -}); diff --git a/browser/components/shopping/tests/browser/browser_settings_telemetry.js b/browser/components/shopping/tests/browser/browser_settings_telemetry.js index 452bd74ee05a..4ccde3746fc8 100644 --- a/browser/components/shopping/tests/browser/browser_settings_telemetry.js +++ b/browser/components/shopping/tests/browser/browser_settings_telemetry.js @@ -8,10 +8,7 @@ */ add_task(async function test_shopping_settings() { await SpecialPowers.pushPrefEnv({ - set: [ - ["toolkit.telemetry.testing.overridePreRelease", true], - ["browser.shopping.experience2023.optedIn", 0], - ], + set: [["toolkit.telemetry.testing.overridePreRelease", true]], }); let opt_in_status = Services.prefs.getIntPref( diff --git a/toolkit/components/nimbus/FeatureManifest.yaml b/toolkit/components/nimbus/FeatureManifest.yaml index 1c631d892bb1..83c65fcea5ff 100644 --- a/toolkit/components/nimbus/FeatureManifest.yaml +++ b/toolkit/components/nimbus/FeatureManifest.yaml @@ -1663,13 +1663,9 @@ shopping2023: the shopping feature. variables: enabled: - description: True if the experience is enabled (experimental treatment group) + description: True if the experience is enabled type: boolean fallbackPref: browser.shopping.experience2023.enabled - control: - description: True if the experiment is enabled but experience is disabled (experimental control group) - type: boolean - fallbackPref: browser.shopping.experience2023.control adsEnabled: description: True if showing recommended products is enabled type: boolean diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml index edb17b9ea594..89a167f5d8f7 100644 --- a/toolkit/components/telemetry/Scalars.yaml +++ b/toolkit/components/telemetry/Scalars.yaml @@ -9231,22 +9231,6 @@ shopping: - 'firefox' record_in_processes: - main - product_page_visits: - bug_numbers: - - 1848160 - description: > - Counts number of visits to a supported retailer product page while - enrolled in either the control or treatment branches of the shopping - experiment. - expires: "122" - kind: uint - notification_emails: - - betling@mozilla.com - - fx-desktop-shopping-eng@mozilla.com - products: - - 'firefox' - record_in_processes: - - main power: cpu_time_bogus_values: -- 2.11.4.GIT