Closed Bug 1007130 Opened 6 years ago Closed 6 years ago

Add UI telemetry for 'Adding a new search engine' button

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: aaronmt, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Based off of reaction from https://twitter.com/frabcus/status/463470668781481984 perhaps adding a new search engine is not so discoverable.
Attached patch Patch: telemetry (obsolete) — Splinter Review
I couldn't decide between "add" or "new" for the action and settled on "new" because it seemed more generic. Feel free to suggest otherwise, or give a different action altogether.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #8444848 - Flags: review?(mark.finkle)
Comment on attachment 8444848 [details] [diff] [review]
Patch: telemetry

Looks like "add_search_engine" is already being sent as an "action.1" from the Browser main menu. We landed a patch for Web content contextmenus too, but I missed a bunch of text selection actions!!

Could we just do the Web contextmenu part as an "action.1" with "add_search_engine" as the Extra? It would blend well with the data we are already sending and give us something to start using for other "actionbar" text selection items.

If we wanted to treat "search" as something different than "action", which we might want someday but I don't think we need it yet, then I would recommend a "search.1" event. But again, I don't think we need this yet.

If so, then:

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>         if (itemId == R.id.add_search_engine) {
>+            // This can be selected from either the browser Menu or the Contextmenu, depending on the size and version (v11+) of the phone.
>             Tab tab = Tabs.getInstance().getSelectedTab();
>             if (tab != null && tab.hasOpenSearch()) {
>                 JSONObject args = new JSONObject();
>                 try {
>                     args.put("tabId", tab.getId());
>                 } catch (JSONException e) {
>                     Log.e(LOGTAG, "error building json arguments");
>                     return true;
>                 }
>                 GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Add", args.toString()));
>+                Telemetry.sendUIEvent(TelemetryContract.Event.NEW, TelemetryContract.Method.MENU, "search-engine");

Just revert this part. It's already being sent.

>diff --git a/mobile/android/base/TelemetryContract.java b/mobile/android/base/TelemetryContract.java

>+        // Add a new entry.
>+        NEW("new.1"),
>+

Also revert

>diff --git a/mobile/android/base/docs/index.rst b/mobile/android/base/docs/index.rst

>+``new.1``
>+  Add a new entry.
>+

Same

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>     SelectionHandler.addAction({
>       id: "search_add_action",
>       label: Strings.browser.GetStringFromName("contextmenu.addSearchEngine"),
>       icon: "drawable://ab_add_search_engine",
>       selector: filter,
>       action: function(aElement) {
>+        UITelemetry.addEvent("new.1", "actionbar", null, "search-engine");

Change to:
          UITelemetry.addEvent("action.1", "actionbar", null, "add_search_engine");

r-, just to see if you agree with the changes and we get a new patch.
Attachment #8444848 - Flags: review?(mark.finkle) → review-
> Looks like "add_search_engine" is already being sent as an "action.1" from
> the Browser main menu.

Oops, missed that.

> Could we just do the Web contextmenu part as an "action.1" with
> "add_search_engine" as the Extra? It would blend well with the data we are
> already sending and give us something to start using for other "actionbar"
> text selection items.

This isn't a web context menu, but a BrowserToolbar context menu -- but only on pre-v11 devices (because for later devices, the "Add a Search Engine" option is in the browser menu - I added a check for v11 to send this event.

> 
> Change to:
>           UITelemetry.addEvent("action.1", "actionbar", null,
> "add_search_engine");
> 

Done.
Attachment #8444848 - Attachment is obsolete: true
Attachment #8444874 - Flags: review?(mark.finkle)
Comment on attachment 8444874 [details] [diff] [review]
Patch: Telemetry v2

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
>+            // This can be selected from either the browser Menu or the Contextmenu, depending on the size and version (v11+) of the phone.

Good comment, but my OCD wants "menu" and "contextmenu" (no caps)

>+
>+                if (Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {
>+                    Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.CONTEXT_MENU, "add_search_engine");

Can you file a bug to fix up the other items in this function? 
* pasteandgo, paste and copyurl should always send an ACTION since it is always in the contextmenu
* site_settings and subscribe should use the VERSION.SDK_INT check since they are on the Page menu for newer OS
* add_to_launcher is a lost cause I think. It appears on both the contextmenu and the Page menu. I don't know how we couldn't double count it, so just ignore it for now.
Attachment #8444874 - Flags: review?(mark.finkle) → review+
Try to uplift all the way to beta if we have time
Comment on attachment 8444874 [details] [diff] [review]
Patch: Telemetry v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): covering some missing parts of bug 1004889
User impact if declined: No telemetry in Aurora for some menu items, esp for pre-v11 devices
Testing completed (on m-c, etc.): local, baking on nightly
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none
Attachment #8444874 - Flags: approval-mozilla-beta?
Attachment #8444874 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 33
https://hg.mozilla.org/mozilla-central/rev/b7a8029273f5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8444874 - Flags: approval-mozilla-beta?
Attachment #8444874 - Flags: approval-mozilla-beta+
Attachment #8444874 - Flags: approval-mozilla-aurora?
Attachment #8444874 - Flags: approval-mozilla-aurora+
Beta31 still needs the JSONArray import that was removed by this patch. Re-added.
https://hg.mozilla.org/releases/mozilla-beta/rev/3f2a4a1c1a84
You need to log in before you can comment on or make changes to this bug.