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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Based off of reaction from https://twitter.com/frabcus/status/463470668781481984 perhaps adding a new search engine is not so discoverable.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Patch: telemetry

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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Chenxia Liu [:liuche]]

Attachment: Patch: Telemetry v2

> 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.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Try to uplift all the way to beta if we have time

Comment 6

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/b7a8029273f5

Comment 7

[Chenxia Liu [:liuche]]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b7a8029273f5

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/74378edca002
https://hg.mozilla.org/releases/mozilla-beta/rev/5d3acd322743

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Beta31 still needs the JSONArray import that was removed by this patch. Re-added.
https://hg.mozilla.org/releases/mozilla-beta/rev/3f2a4a1c1a84