Closed Bug 1057613 Opened 6 years ago Closed 6 years ago

Telemetry for search widget buttons

Categories

(Firefox for Android Graveyard :: Search Activity, defect, P1)

All
Android
defect

Tracking

(firefox35 verified, fennec35+)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox35 --- verified
fennec 35+ ---

People

(Reporter: Margaret, Assigned: treemantris, Mentored)

References

Details

(Whiteboard: shovel-ready [lang=java])

Attachments

(1 file)

We should add telemetry probes for users hitting the various search widget buttons.
Duplicate of this bug: 1058315
Blocks: search
Priority: -- → P1
Whiteboard: shovel-ready
tracking-fennec: ? → 35+
Mentor: margaret.leibovic
Whiteboard: shovel-ready → shovel-ready [lang=java]
Hi Margaret, I'd like to take on this bug if possible. Could you point me in the right direction as to which classes I'll need to be looking at and changing?
Hi Tristan! You should look at the SearchWidget class:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/search/java/org/mozilla/search/SearchWidget.java

However, our workflow for this search activity part of the tree is a bit different than just writing a patch directly in mozilla-central, since we also have a fennec-search github repo that allows us to work on the search activity independently of the rest of fennec.

So, you should clone this repo, and make a pull request with your changes there:
https://github.com/mozilla/fennec-search

Then I can help you generate a patch for us to land on mozilla-central as well.

For more details, see this mailing list post:
https://mail.mozilla.org/pipermail/mobile-firefox-dev/2014-August/000844.html

For an example of adding telemetry probes, you can look at bug 1042956. In general we try to use existing Event/Method names as much as possible. However, we may want to add a new WIDGET method to indicate that something came from the widget. Perhaps all 3 of these buttons should be Event LAUNCH with Method WIDGET, and then we can use the extras field to specify which button was hit.

mfinkle, what do you think?
Assignee: nobody → treemantris
Flags: needinfo?(mark.finkle)
Hi Margaret, thanks for your help! I've made some changes and submitted a pull request on GitHub https://github.com/mozilla/fennec-search/pull/78
(In reply to Tristan Pollitt from comment #4)
> Hi Margaret, thanks for your help! I've made some changes and submitted a
> pull request on GitHub https://github.com/mozilla/fennec-search/pull/78

Thanks for the pull request! I left some comments there, but it looks good.

In order to generate a patch for mozilla-central, from your fennec-search repo, you should run `grunt export --tree <path to your m-c tree>`, and that will apply the changes to mozilla-central. Then you can follow the regular steps for generating a mercurial patch from that:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

You can attach that patch to this bug and flag me for review. Then I'll review it and help you land it.

Sorry for the extra steps, but hopefully it shouldn't be too hard!
Margaret and I chatted on IRC. Looks good to me.
Flags: needinfo?(mark.finkle)
I've made the changes you mentioned in the pull request, and executed 'grunt export --tree...' to bring those accross to my mozilla-central repo. I then ran a 'hg diff', and it's only showing the changes I've made to SearchWidget.java, but not the change I made to TelemetryContract.java. Any idea why?
(In reply to Tristan Pollitt from comment #7)
> I've made the changes you mentioned in the pull request, and executed 'grunt
> export --tree...' to bring those accross to my mozilla-central repo. I then
> ran a 'hg diff', and it's only showing the changes I've made to
> SearchWidget.java, but not the change I made to TelemetryContract.java. Any
> idea why?

Oh, that's because we just have a copy of TelemetryContract.java in the fennec-search repo. The real version lives in mozilla-central, but we keep of copy of that class (and some other stubbed out classes) in fennec-search so that it will still build. That's confusing, I'm sorry!

So you'll just have to make the change manually to the TelemetryContract.java file in mozilla-central, then generate your patch.
Thanks for clarifying, makes sense now. Here's the patch.
Attachment #8483756 - Flags: review+
Attachment #8483756 - Flags: review+ → review?(margaret.leibovic)
Comment on attachment 8483756 [details] [diff] [review]
Added telemetry probes for search widget buttons

Review of attachment 8483756 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks! I can land this patch for you. You should also update your pull request, and I'll merge that.
Attachment #8483756 - Flags: review?(margaret.leibovic) → review+
Thanks Margaret! I've updated the pull request.
https://hg.mozilla.org/integration/fx-team/rev/b33d49538c4c
https://github.com/mozilla/fennec-search/commit/746d7f730504c9b43c5ba1596ebc3fce6a63b3f9

Thanks for the patch! Let me know if you need help finding more bugs to work on. If you're interested in more search activity work, we have some "shovel-ready" bugs listed here:
https://wiki.mozilla.org/Mobile/Projects/Search_activity_v1
https://hg.mozilla.org/mozilla-central/rev/b33d49538c4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Verified as fixed in build 35.0a1 (2014-09-08);
Device Google Nexus 7 (Android 4.4.4);
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.