Closed
Bug 1130210
Opened 10 years ago
Closed 10 years ago
Put browser.search.showOneOffButtons value in telemetry UI
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 38
People
(Reporter: cww, Assigned: abhilashmhaisne, Mentored)
References
Details
(Whiteboard: [good first bug] [lang=js])
Attachments
(1 file)
(deleted),
patch
|
florian
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
It's really hard to interpret the telemetry UI data around search if the user has reverted to the old style search box. We should add this info to the telemetry UI data packet so we can filter these users out (or look at how their behavior differs from other users).
Comment 1•10 years ago
|
||
We want to do something similar to https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#496
But put the new check near the other search check at https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#569
Mentor: bwinton
Whiteboard: [good first bug] [lang=js]
Hi,
I am new to open source and would like to work on this bug as my first bug. How should I proceed?
Thanks in advance
abhilash
Comment 3•10 years ago
|
||
Hello Abhilash!
The first step is to get Firefox building. There are reasonably easy instructions at https://developer.mozilla.org/en-US/docs/Simple_Firefox_build but if you run into any problems please feel free to comment here, or email me!
Flags: needinfo?(bwinton)
Hello Mr. Blinton. I successfully built firefox yesterday with help from irc people. :)
Comment 5•10 years ago
|
||
Excellent! So the next thing to do is copy the line at https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#497 paste it down at line 569, and change browser.tabs.drawInTitlebar to browser.search.showOneOffButtons. (You'll also need to change result.titleBarEnabled to something different, like result.oneOffSearchEnabled…)
Once you're done that, re-build Firefox, and go to "about:telemetry". If you expand the "Simple Measurements" section you should see the new addition beside the word "UITelemetry".
Let me know how it goes!
Okay, I did those changes, and now build is running. #Excitement
I did the mentioned changes. However, there was no addition "besides" UITelemetry. There is an addition "activeTicks" in that same coloumn in Simple measurements section.
Flags: needinfo?(bwinton)
Okay Mr. Winton I had not enabled telemetry earlier. Now when I enabled it and run nightly, this is what I got beside UITelemetry : http://pastebin.mozilla.org/8616494
Is that what was expected?
- Abhilash
Flags: needinfo?(bwinton)
Comment 9•10 years ago
|
||
A-ha! You see the text:
"OneOffSearchEnabled":false
in the pastebin? That's what we're looking for! :D
Well, I _think_ that's what we're looking for. Can you tell me if you see the one-off search buttons at the bottom of your searchbar autocomplete? (The part that says "Search for test with:" in the picture at https://www.dropbox.com/s/t8wbbmpzucu6pjv/Screenshot%202015-02-08%2009.06.55.png?dl=0 ) If you do see those, then I would expect OneOffSearchEnabled to be true, not false, so we'll need to reverse that.
And while you're changing that bit of code, it looks like all the other keys start with a lowercase letter, so we should probably change "OneOffSearchEnabled" to "oneOffSearchEnabled".
After those two things are done, you should attach your patch, set its review request flag to ?, and ask :florian for the review. (This is probably one of the more complicated parts of writing code for Firefox, so if you can't figure it out, let me know, and I'll be happy to help. :)
Assignee: nobody → abhilashmhaisne
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•10 years ago
|
||
Yeah! the part "search for test with:" does appear. And yes, I'll change "OneOffSearchEnabled" to "oneOffSearchEnabled".
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8561025 -
Flags: review?(florian)
Comment 12•10 years ago
|
||
Comment on attachment 8561025 [details] [diff] [review]
Added browser.search.showOneOffbuttons value in telemetry UI
Review of attachment 8561025 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, thanks for the patch! :)
Attachment #8561025 -
Flags: review?(florian) → review+
Comment 13•10 years ago
|
||
Blake, given that I'm planning to start working on bug 1119250 and remove that preference soon, if we want to get useful data out of adding this value in telemetry, we probably need to uplift this patch.
Assignee | ||
Comment 14•10 years ago
|
||
Thank you Blake and Florian.
Comment 15•10 years ago
|
||
Whiteboard: [good first bug] [lang=js] → [good first bug] [lang=js][fixed-in-fx-team]
Assignee | ||
Comment 16•10 years ago
|
||
Thank you florian. Cheers!
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [good first bug] [lang=js][fixed-in-fx-team] → [good first bug] [lang=js]
Target Milestone: --- → Firefox 38
Comment 18•10 years ago
|
||
Comment on attachment 8561025 [details] [diff] [review]
Added browser.search.showOneOffbuttons value in telemetry UI
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: no user impact. Developer impact will be less data available about how many of our users turned off the new search UI. This data would be interesting to collect before we remove this preference (bug 1119250).
[Describe test coverage new/current, TreeHerder]: currently in m-c.
[Risks and why]: Low risk, one-line patch.
[String/UUID change made/needed]: none.
I don't know if it's too late or not for 36. I think we get telemetry data primarily from our beta users, so it would be useful to have at least in 37 when it reaches beta.
Attachment #8561025 -
Flags: approval-mozilla-beta?
Attachment #8561025 -
Flags: approval-mozilla-aurora?
Comment 19•10 years ago
|
||
Comment on attachment 8561025 [details] [diff] [review]
Added browser.search.showOneOffbuttons value in telemetry UI
It is a bit too late for beta indeed but ok for aurora!
Attachment #8561025 -
Flags: approval-mozilla-beta?
Attachment #8561025 -
Flags: approval-mozilla-beta-
Attachment #8561025 -
Flags: approval-mozilla-aurora?
Attachment #8561025 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 20•10 years ago
|
||
Thank you Mr.Ledru and Mr.Queze!
Updated•10 years ago
|
status-firefox36:
--- → wontfix
status-firefox37:
--- → affected
Comment 21•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•