Closed Bug 1030935 Opened 6 years ago Closed 5 years ago

Measure default application setting in telemetry

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34
Tracking Status
firefox32 --- affected
firefox33 --- fixed
firefox34 --- fixed
fennec 33+ ---

People

(Reporter: rnewman, Assigned: mfinkle)

References

Details

Attachments

(1 file, 2 obsolete files)

In principle, we can ask Android which activity will handle an intent:

http://developer.android.com/reference/android/content/pm/PackageManager.html#resolveActivity%28android.content.Intent,%20int%29

As such, we can find out which app is the default for, say, http URIs, or HTML files. If it's not Firefox, something might be wrong.

We should be recording this in telemetry.
We'd like to use this data to figure out if many of our installs have a different browser set as the default. If so, and since it's not trivial to unset defaults, we might add some UI to make it easier for people to unset the current default and set a new one.
tracking-fennec: --- → ?
Are we going to report specific apps that are the default or is this going to be binary default/not default. The reporting of what apps are installed is invasive to the user's privacy.
(In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> Are we going to report specific apps that are the default or is this going
> to be binary default/not default. The reporting of what apps are installed
> is invasive to the user's privacy.

All we need is a binary bucket: 0 or 1
Assignee: nobody → liuche
tracking-fennec: ? → 33+
I would say this is a "simple measures" type of telemetry, because we're storing a key-value pair of "default: <default-browser>", which UITelemetry actually already supports. However, the current version only allows for registering JS functions for taking simple measures.

The approach I'm thinking about implementing is having Java (on Telemetry:Gather or something) register a "simple measures" function that returns a static value. Updating the value on ever Telemetry:Gather would need to unregister the previous function, and register a new one. (See patch.)

Alternatively, we could register something in browser.js that queries Java about whether FF is the default, but the response would be async, and it's possible that Java won't even be running. The simple measures code can't handle async anyways.

Another alternative is have Java periodically write to a Gecko pref on Telemetry:Gather, and have the isdefault simple measures function query that...This seems the least gross, but also distributes the logic into separate pieces, which I don't like.

Maybe I'm coming up with much too complicated ways to try to use simple measures? We could alternatively just use addHistogram, but I don't think that's the necessarily the right tool.

Thoughts, mfinkle, rnewman?
Attachment #8454838 - Flags: feedback?(mark.finkle)
Flags: needinfo?(rnewman)
A few things to keep in mind:
1. The "gather-telemetry" notification, which ends up sending the "Telemetry:Gather" message, is sent before Telemetry actually collects data for a ping. It's just a "heads up" that a collection ping is coming soon. So async is fine.
2. A simple measures function is called *during* a collection ping. So you could set some data in the "gather" phase and then use that data in the "ping" phase.
(In reply to Chenxia Liu [:liuche] from comment #4)

> I would say this is a "simple measures" type of telemetry, because we're
> storing a key-value pair of "default: <default-browser>", which UITelemetry
> actually already supports. However, the current version only allows for
> registering JS functions for taking simple measures.

Lest anyone else be as initially confused as I was, TelemetryPing actually delegates to UITelemetry for capturing an extensible set of simple measures.

http://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.jsm#276

+1 for simple measure, though. (On the down side, anything but a histogram is poorly documented, as you've no doubt found.)


> Alternatively, we could register something in browser.js that queries Java
> about whether FF is the default, but the response would be async, and it's
> possible that Java won't even be running. The simple measures code can't
> handle async anyways.

Java will always be running. The question is whether the class you've registered as the listener for the message (assuming message passing) is running, which is a different matter.

But I think we can sidestep that altogether. I'd suggest the approach taken in Bug 1021395: directly call into Java via JNI during measurement collection. This will be synchronous and simple.

Leave the same function registered all the time. For sanity, measure to make sure this isn't a painful expense, just as we did in Bug 1021395.
Flags: needinfo?(rnewman)
Status: NEW → ASSIGNED
Comment on attachment 8454838 [details] [diff] [review]
Approach 1: register static simple measures function

Let's see if we can do something without needing to tweak UITelemetry.jsm
Attachment #8454838 - Flags: feedback?(mark.finkle) → feedback-
Initially started off trying to make a new JNI method for booleans, but decided to just switch to using ints for the time being.

Running into some null intent resolvers.
Attachment #8454838 - Attachment is obsolete: true
Comment on attachment 8459059 [details] [diff] [review]
WIP: Compare intent resolver default to Fennec

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

>+    private boolean isDefaultResolveActivity() {
>+        final Intent viewIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("www.mozilla.org"));
>+        ResolveInfo info = getPackageManager().resolveActivity(viewIntent, PackageManager.MATCH_DEFAULT_ONLY);
>+        String activityName = info.activityInfo.name;
>+        Log.e(LOGTAG, "AOEU name = " + activityName);
>+        Log.e(LOGTAG, "AOEU package = " + info.activityInfo.packageName);
>+        if (TextUtils.equals(activityName, AppConstants.MOZ_APP_NAME)) {
>+            return true;
>+        }
>+        return false;
>+    }

We might want to use package name instaed of activity name, but in general this looks OK

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

>+    Telemetry.init();

> var Telemetry = {
>+  init: function Telemetry_init() {
>+    // Set up simple measures to measure.
>+    UITelemetry.addSimpleMeasureFunction("app-default", function() {
>+      let jni = new JNI();
>+      let cls = jni.findClass("org/mozilla/gecko/BrowserApp");
>+      let method = jni.getStaticMethodID(cls, "isDefault", "()I");
>+      let isdefault = jni.callStaticIntMethod(cls, method);
>+      jni.close();
>+      return isdefault;
>+    });
>+  },

Let's skip all of this browser.js part and instead handle this like we do these other telemetry probes:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1327

We even already have a Telemetry bucket we can use: BROWSER_IS_USER_DEFAULT
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#6161

So the new code in BrowserApp should look like:

Telemetry.HistogramAdd("BROWSER_IS_USER_DEFAULT", (isDefaultResolveActivity() ? 1 : 0));

since HistogramAdd takes a string and an int

That would keep all the code in BrowserApp.java
Building on Chenxia's WIP. This patch makes the overall changes I describe. We re-use the existing "BROWSER_IS_USER_DEFAULT" telemetry bucket used by desktop.

I verified the following:
1. No default browser: Android will prompt people for which browser to use. The resolveActivity call can return a null info or an info with a packageName of "android".
2. A default browser that is not Firefox. I set the default browser for loading HTTP URLs to Chrome. The resolveActivity call returns an info with a packageName of "com.google.chrome"
3. A default browser that is Firefox. The resolveActivity call returns an info with a packageName equal to getPackageName(), which in my test build was "org.mozilla.fennec_mfinkle"

I'd like to see about getting this uplifted to Beta if we can.
Assignee: liuche → mark.finkle
Attachment #8459059 - Attachment is obsolete: true
Attachment #8476427 - Flags: review?(nalexander)
Comment on attachment 8476427 [details] [diff] [review]
telemetry-default v0.2

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

Looks good.  Minor nits; ignore as you see fit.

::: mobile/android/base/BrowserApp.java
@@ +1336,5 @@
>              Telemetry.HistogramAdd("FENNEC_FAVICONS_COUNT",
>                      BrowserDB.getCount(getContentResolver(), "favicons"));
>              Telemetry.HistogramAdd("FENNEC_THUMBNAILS_COUNT",
>                      BrowserDB.getCount(getContentResolver(), "thumbnails"));
> +            Telemetry.HistogramAdd("BROWSER_IS_USER_DEFAULT", (isDefaultBrowser() ? 1 : 0));

Looks like this name is given to us.  Not my favourite, but whatever.

@@ +1348,5 @@
>              super.handleMessage(event, message, callback);
>          }
>      }
>  
> +    private boolean isDefaultBrowser() {

Short docstring, please: "Return true if this package is the default browser on this device."

@@ +1350,5 @@
>      }
>  
> +    private boolean isDefaultBrowser() {
> +        final Intent viewIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://www.mozilla.org"));
> +        ResolveInfo info = getPackageManager().resolveActivity(viewIntent, PackageManager.MATCH_DEFAULT_ONLY);

nit: I'm a final-ista.  Here and below.

@@ +1357,5 @@
> +            return false;
> +        }
> +
> +        String packageName = info.activityInfo.packageName;
> +        if (TextUtils.equals(packageName, getPackageName())) {

Prefer not to branch and return booleans.  If you want to be descriptive:

final boolean isDefault = TextUtils.equals...
return isDefault;
Attachment #8476427 - Flags: review?(nalexander) → review+
https://hg.mozilla.org/mozilla-central/rev/098524c6392d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8476427 [details] [diff] [review]
telemetry-default v0.2

Approval Request Comment
[Feature/regressing bug #]: Just getting some useful telemetry
[User impact if declined]: We want to learn how many people with Firefox installed are stuck on a different default browser. We can try suggesting people reset the default, which is not simple on Android.
[Describe test coverage new/current, TBPL]: I see Nightly telemetry showing up on the dashboard
[Risks and why]: Low risk. Simple isolated patch.
[String/UUID change made/needed]: None
Attachment #8476427 - Flags: approval-mozilla-beta?
Attachment #8476427 - Flags: approval-mozilla-aurora?
Attachment #8476427 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8476427 [details] [diff] [review]
telemetry-default v0.2

Although this is a Telemetry probe and is fairly simple, we're really late in beta with beta10 set to go to build today and the RC ready to spin tomorrow. I'm denying this approval request for a non critical change on beta.
Attachment #8476427 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Blocks: 1064857
You need to log in before you can comment on or make changes to this bug.