Skip to content

Commit

Permalink
Bug 1815971 - Count number of times persisted search is viewed and re…
Browse files Browse the repository at this point in the history
…verted due to Popups - r=adw

Differential Revision: https://phabricator.services.mozilla.com/D172690
  • Loading branch information
jamesteow committed Mar 22, 2023
1 parent 86932ac commit 34cbd31
Show file tree
Hide file tree
Showing 7 changed files with 507 additions and 4 deletions.
8 changes: 7 additions & 1 deletion browser/base/content/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -5263,7 +5263,13 @@ var XULBrowserWindow = {
// via simulated locationchange events such as switching between tabs, however
// if this is a document navigation then PopupNotifications will be updated
// via TabsProgressListener.onLocationChange and we do not want it called twice
gURLBar.setURI(aLocationURI, aIsSimulated, isSessionRestore);
gURLBar.setURI(
aLocationURI,
aIsSimulated,
isSessionRestore,
false,
isSameDocument
);

BookmarkingUI.onLocationChange();
// If we've actually changed document, update the toolbar visibility.
Expand Down
21 changes: 18 additions & 3 deletions browser/components/urlbar/UrlbarInput.sys.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -329,12 +329,17 @@ export class UrlbarInput {
* @param {boolean} [dontShowSearchTerms]
* True if userTypedValue should not be overidden by search terms
* and false otherwise.
* @param {boolean} [isSameDocument]
* True if the caller of setURI loaded a new document and false
* otherwise (e.g. the location change was from an anchor scroll
* or a pushState event).
*/
setURI(
uri = null,
dueToTabSwitch = false,
dueToSessionRestore = false,
dontShowSearchTerms = false
dontShowSearchTerms = false,
isSameDocument = false
) {
if (!this.window.gBrowser.userTypedValue) {
this.window.gBrowser.selectedBrowser.searchTerms = "";
Expand Down Expand Up @@ -363,6 +368,12 @@ export class UrlbarInput {
if (this.window.gBrowser.selectedBrowser.searchTerms) {
value = this.window.gBrowser.selectedBrowser.searchTerms;
valid = !dueToSessionRestore;
if (!isSameDocument) {
Services.telemetry.scalarAdd(
"urlbar.persistedsearchterms.view_count",
1
);
}
} else {
uri =
this.window.gBrowser.selectedBrowser.currentAuthPromptURI ||
Expand Down Expand Up @@ -767,10 +778,14 @@ export class UrlbarInput {
maybeHandleRevertFromPopup(anchorElement) {
if (
anchorElement?.closest("#urlbar") &&
this.window.gBrowser.selectedBrowser.searchTerms
this.window.gBrowser.selectedBrowser.searchTerms &&
!this.window.gBrowser.userTypedValue
) {
this.handleRevert(true);
// TODO: Bug 1815971, add telemetry.
Services.telemetry.scalarAdd(
"urlbar.persistedsearchterms.revert_by_popup_count",
1
);
}
}

Expand Down
23 changes: 23 additions & 0 deletions browser/components/urlbar/docs/telemetry.rst
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,29 @@ urlbar.impression.*
- ``autofill_url``
For url type autofill.

urlbar.persistedsearchterms.revert_by_popup_count
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

A uint that is incremented when search terms are persisted in the Urlbar and
the Urlbar is reverted to show a full URL due to a PopupNotification. This
can happen when a user is on a SERP and permissions are requested, e.g.
request access to location. If the popup is persistent and the user did not
dismiss it before switching tabs, the popup will reappear when they return to
the tab. Thus, when returning to the tab with the persistent popup, this
value will be incremented because it should have persisted search terms but
instead showed a full URL.

urlbar.persistedsearchterms.view_count
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

A uint that is incremented when search terms should be persisted in the
Urlbar. This will trigger when a user loads a SERP from any SAP that results
in the search terms persisting in the Urlbar, as well as switching to a tab
containing a SERP that should be persisting the search terms in the Urlbar,
regardless of whether a PopupNotification is present. Thus, for every
``revert_by_popup_count``, there should be at least one corresponding
``view_count``.

urlbar.tips
~~~~~~~~~~~

Expand Down
1 change: 1 addition & 0 deletions browser/components/urlbar/tests/browser/browser.ini
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ skip-if =
[browser_UrlbarInput_searchTerms_searchBar.js]
[browser_UrlbarInput_searchTerms_searchMode.js]
[browser_UrlbarInput_searchTerms_switch_tab.js]
[browser_UrlbarInput_searchTerms_telemetry.js]
[browser_UrlbarInput_setURI.js]
https_first_disabled = true
skip-if =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,48 @@ add_task(async function generic_popup_when_persist_is_enabled() {
await SpecialPowers.popPrefEnv();
});

// Ensure the urlbar is not being reverted when a user
// has written something.
add_task(async function generic_popup_no_revert_when_persist_is_disabled() {
let { tab } = await searchWithTab(
SEARCH_TERM,
null,
defaultTestEngine,
false
);

let userTypedValue = "chocolate cake recipe";
// Have a user typed value in the urlbar to make
// pageproxystate invalid.
await UrlbarTestUtils.promiseAutocompleteResultPopup({
window,
value: userTypedValue,
waitForFocus,
fireInputEvent: true,
});
gURLBar.blur();

await waitForPopupNotification();

// Wait a brief amount of time between when the popup is shown
// and when the event handler should fire if it's enabled.
await TestUtils.waitForTick();

Assert.equal(
gURLBar.getAttribute("pageproxystate"),
"invalid",
"Urlbar should not be reverted."
);

Assert.equal(
gURLBar.value,
userTypedValue,
"User typed value should remain in urlbar."
);

BrowserTestUtils.removeTab(tab);
});

// Ensure the urlbar is not being reverted when a prompt is shown
// and the persist feature is disabled.
add_task(async function generic_popup_no_revert_when_persist_is_disabled() {
Expand Down
Loading

0 comments on commit 34cbd31

Please sign in to comment.