Closed Bug 1299458 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Search, defect, P3)

51 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 52
Tracking Status
firefox51 --- verified
firefox52 --- verified

People

(Reporter: cirdeiliviu, Assigned: adw)

References

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

[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]
I can take a look at this, should be a simple fix.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Looks like the urlbar one-offs regressed this since it works in a Nightly before that landed.
Blocks: 1180944
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 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 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 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+
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
https://hg.mozilla.org/mozilla-central/rev/2a6c70abee77
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
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
Is this a fix that should be uplifted to Aurora? Because Aurora is still affected.
Flags: needinfo?(adw)
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)
Attached patch Aurora 51 patchSplinter Review
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+
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.