Telemetry data from Search bar is not properly collected when searching in new tab from context menu

VERIFIED FIXED in Firefox 51

Status

()

defect
P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cirdeiliviu, Assigned: adw)

Tracking

51 Branch
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 verified, firefox52 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(2 attachments)

Reporter

Description

3 years ago
[Affected versions]:
Nightly 51 Build ID 	20160831030224
 
[Affected platforms]:
Ubuntu 15.04 x64, Windows 10 x64, Mac OS X 10.10

[Steps to reproduce]:
1. Open Firefox with a new profile.
2. Type something in Search bar.
3. Right click on a one-off search engine (e.g Yahoo) and choose "Search in new tab".
4. Open about:telemetry page, and under "Simple Measurements" inspect the UITelemetry property.

[Expected result]:
Telemetry data should show the name on the search engine used for the search. 
For example: "search-oneoff":{"yahoo.oneoff-context-searchbar":{"unknown":{"tab-background":1}}

[Actual result]:
Telemetry data does not show the name of the engine used for the search. It shows "other" indeed.
Example: {"other.oneoff-context-searchbar":{"unknown":{"tab-background":1}}

[Notes]: 
 - Telemetry for searching in the current tab by mouse or key works as expected
 - In Awesomebar the name of the engine is shown even for context menu searches
 - If I add a new search engine (e.g. Booking.com) it shows the same {"other.oneoff-context-searchbar":{"unknown":{"tab-background":1}} while in Awesomebar it is "other-Booking.com".
Priority: -- → P3
Whiteboard: [fxsearch]
Assignee

Comment 1

3 years ago
I can take a look at this, should be a simple fix.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 2

3 years ago
Looks like the urlbar one-offs regressed this since it works in a Nightly before that landed.
Blocks: 1180944
Assignee

Comment 4

3 years ago
What's happening is that the popuphidden event on the searchbar popup is firing before the "command" XBL event handler is firing on the one-offs binding.  The command handler passes along _contextEngine, which is eventually recorded in the telemetry.  But the popuphidden handler nulls out _contextEngine.  So a null engine is passed to the telemetry-recording function.

Nothing big changed about this in the refactoring except that the popuphidden handler is now added via addEventListener when a popup is set on the one-offs binding, but before the popuphidden handler was defined on the searchbar popup XBL binding.  I'm guessing that listeners added by addEventListener are called before XBL handlers?

This fix is to just move the this._contextEngine = null setter into the delayed dispatch() callback in the popuphidden listener.  That callback was already there even before the refactoring.

Comment 5

3 years ago
mozreview-review
Comment on attachment 8790502 [details]
Bug 1299458 - Telemetry data from Search bar is not properly collected when searching in new tab from context menu.

https://reviewboard.mozilla.org/r/78278/#review76926
Attachment #8790502 - Flags: review?(florian) → review+
It would be nice to have this covered by a test.
Comment hidden (mozreview-request)
Assignee

Comment 8

3 years ago
Comment on attachment 8790502 [details]
Bug 1299458 - Telemetry data from Search bar is not properly collected when searching in new tab from context menu.

Now with test.
Attachment #8790502 - Flags: review+ → review?(florian)

Comment 9

3 years ago
mozreview-review
Comment on attachment 8790502 [details]
Bug 1299458 - Telemetry data from Search bar is not properly collected when searching in new tab from context menu.

https://reviewboard.mozilla.org/r/78278/#review78080

Thanks for adding the test!
Attachment #8790502 - Flags: review?(florian) → review+

Comment 10

3 years ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a6c70abee77
Telemetry data from Search bar is not properly collected when searching in new tab from context menu. r=florian

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2a6c70abee77
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Reporter

Comment 12

3 years ago
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID 	20160920030429

This is verified on Nightly 52 (2016-09-20).
Tested on:
 * Windows 10
 * Mac OS X 10.10
 * Ubuntu 16.04
Status: RESOLVED → VERIFIED
Reporter

Comment 13

3 years ago
Is this a fix that should be uplifted to Aurora? Because Aurora is still affected.
Flags: needinfo?(adw)
Assignee

Comment 14

3 years ago
You're right, I missed this one.  Thanks Liviu!  Sorry too for the day-late replay.  I mistook this bug for another one in my bugmail.
Flags: needinfo?(adw)
Assignee

Comment 15

3 years ago
Approval Request Comment
[Feature/regressing bug #]: One-off searches in urlbar, bug 1180944
[User impact if declined]: We'll miss out on telemetry that should be recorded when the user clicks Search in New Tab from the one-off search buttons context menu in the searchbar
[Describe test coverage new/current, TreeHerder]: Automated test
[Risks and why]: Low risk, one-line change, and has automated test
[String/UUID change made/needed]: None
Attachment #8803417 - Flags: approval-mozilla-aurora?
Comment on attachment 8803417 [details] [diff] [review]
Aurora 51 patch

Fix an issue related to telemetry. Take it in 51 aurora.
Attachment #8803417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reporter

Comment 18

3 years ago
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID 	20161025004017

No problem Drew. This fix is also verified on Aurora 51 (2016-10-25) on Windows 10, Mac 10.11 and Ubuntu 16.04.
You need to log in before you can comment on or make changes to this bug.