Closed
Bug 1299458
Opened 8 years ago
Closed 8 years ago
Telemetry data from Search bar is not properly collected when searching in new tab from context menu
Categories
(Firefox :: Search, defect, P3)
Tracking
()
VERIFIED
FIXED
Firefox 52
People
(Reporter: cirdeiliviu, Assigned: adw)
References
Details
(Whiteboard: [fxsearch])
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
florian
:
review+
|
Details |
(deleted),
patch
|
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
[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".
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: [fxsearch]
Assignee | ||
Comment 1•8 years ago
|
||
I can take a look at this, should be a simple fix.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Looks like the urlbar one-offs regressed this since it works in a Nightly before that landed.
Blocks: 1180944
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 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•8 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+
Comment 6•8 years ago
|
||
It would be nice to have this covered by a test.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 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•8 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•8 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Reporter | ||
Comment 12•8 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•8 years ago
|
||
Is this a fix that should be uplifted to Aurora? Because Aurora is still affected.
Flags: needinfo?(adw)
Assignee | ||
Comment 14•8 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•8 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 16•8 years ago
|
||
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+
Comment 17•8 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 18•8 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.
Description
•