Put browser.search.showOneOffButtons value in telemetry UI

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: cww, Assigned: abhilashmhaisne, Mentored)

Tracking

35 Branch
Firefox 38
x86
All
Points:
---

Firefox Tracking Flags

(firefox36 wontfix, firefox37 fixed, firefox38 fixed)

Details

(Whiteboard: [good first bug] [lang=js])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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).
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]
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Updated

4 years ago
Flags: needinfo?(bwinton)
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)
(Assignee)

Comment 4

4 years ago
Hello Mr. Blinton. I successfully built firefox yesterday with help from irc people. :)
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!
(Assignee)

Comment 6

4 years ago
Okay, I did those changes, and now build is running. #Excitement
(Assignee)

Comment 7

4 years ago
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)
(Assignee)

Comment 8

4 years ago
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)
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

4 years ago
Yeah! the part "search for test with:" does appear. And yes, I'll change "OneOffSearchEnabled" to "oneOffSearchEnabled".
(Assignee)

Comment 11

4 years ago
Created attachment 8561025 [details] [diff] [review]
Added browser.search.showOneOffbuttons value in telemetry UI
Attachment #8561025 - Flags: review?(florian)
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+
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

4 years ago
Thank you Blake and Florian.
https://hg.mozilla.org/integration/fx-team/rev/973bc28ad21f
Whiteboard: [good first bug] [lang=js] → [good first bug] [lang=js][fixed-in-fx-team]
(Assignee)

Comment 16

4 years ago
Thank you florian. Cheers!
https://hg.mozilla.org/mozilla-central/rev/973bc28ad21f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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 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 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

4 years ago
Thank you Mr.Ledru and Mr.Queze!
status-firefox36: --- → wontfix
status-firefox37: --- → affected

Updated

3 years ago
Blocks: 1119250
You need to log in before you can comment on or make changes to this bug.