Closed Bug 1064859 Opened 5 years ago Closed 5 years ago

Telemetry probe to measure whether Fennec is default ASSIST intent handler

Categories

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

All
Android
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: Margaret, Assigned: manu.jain13, Mentored)

References

Details

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

Attachments

(1 file, 3 obsolete files)

In bug 1030935, we added telemetry to measure whether or not Fennec is the default application for handling ACTION_VIEW intents. We can similarly measure whether or not Fennec is the default for handling ACTION_ASSIST intents, to see if it's the default for the swipe-up gesture.

This probe should probably be a tri-state probe, since some users' devices won't support this intent, so there would definitely be no default for them.
Whiteboard: [lang=java] → [lang=java][shovel-ready]
I would like to patch this bug. Please assign this to me. Thanks!!
(In reply to Manu Jain from comment #1)
> I would like to patch this bug. Please assign this to me. Thanks!!

Great! Do you understand the approach we'll want to take here? We should do something very similar to what we did for bug 1030935.
Assignee: nobody → manu.jain13
Attached patch bug1064859.patch (obsolete) — Splinter Review
Patch attached. Please check it. Thanks!!
Attachment #8494801 - Flags: review?(margaret.leibovic)
Comment on attachment 8494801 [details] [diff] [review]
bug1064859.patch

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

Definitely on the right track! I just have some issues for you to address before giving an r+.

::: mobile/android/base/BrowserApp.java
@@ +1489,5 @@
>       *
>       * @return true if this package is the default browser on this device, false otherwise.
>       */
> +    private boolean isDefaultBrowser(String action) {
> +        if (action == "view") {

Instead of creating your own strings to compare here, you can just pass Intent.ACTION_VIEW and Intent.ACTION_ASSIST directly to this method, then use that action parameter in the Intent constructor, instead of creating if statements. That way you wouldn't need to duplicate the bodies of these if statements.

However, for future reference, you should use .equals for string comparisons in Java, not ==, since == tests for reference equality, not value equality.

@@ +1501,5 @@
> +        else if (action == "assist") {
> +            final Intent assistIntent = new Intent(Intent.ACTION_ASSIST, Uri.parse("http://www.mozilla.org"));
> +            final ResolveInfo info = getPackageManager().resolveActivity(assistIntent, PackageManager.MATCH_DEFAULT_ONLY);
> +            if (info == null) {
> +                return null;

I think you meant to return false here.

::: toolkit/components/telemetry/Histograms.json
@@ +6179,5 @@
>    },
> +  "BROWSER_IS_ASSIST_DEFAULT": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "The result of the default browser for gestures."

I would update this to explain that it's explicitly about the assist intent, since "gestures" is pretty generic and could confuse people.
Attachment #8494801 - Flags: review?(margaret.leibovic) → feedback+
Attached patch bug_1064859_v2.patch (obsolete) — Splinter Review
Changes made as per told. But I can't understand one thing that if I would not return null instead of false in "if(info==null)" then how would that be a tri state probe.
Attachment #8494801 - Attachment is obsolete: true
Attachment #8494885 - Flags: review?(margaret.leibovic)
(In reply to Manu Jain from comment #5)
> Created attachment 8494885 [details] [diff] [review]
> bug_1064859_v2.patch
> 
> Changes made as per told. But I can't understand one thing that if I would
> not return null instead of false in "if(info==null)" then how would that be
> a tri state probe.

Ah, sorry! I forgot about that tri-state probe idea. However, having the helper method return null does not help achieve that goal, since the probe itself is still a boolean. In fact, having the helper method return null would probably cause an exception in ternary operator.

If we're going to change this probe to be tri-state, it should be an integer, not a boolean, and we should probably make a different helper method that lets us check whether or not the device supports the ASSIST intent, since I don't think that |info == null| actually implies that the device doesn't support the intent.

However, given my experience in bug 1064857, I worry that it might be hard to actually figure out whether or not a device supports the ASSIST intent, so instead we can just record a boolean about whether or not Fennec is the default, and then use information about what versions of Android people are using to determine what set of the "false" probes are due to the fact that they couldn't even set a default for this.

So, in conclusion, I think we should proceed with your current patch :)
Comment on attachment 8494885 [details] [diff] [review]
bug_1064859_v2.patch

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

::: mobile/android/base/BrowserApp.java
@@ +1488,5 @@
>       * Use a dummy Intent to do a default browser check.
>       *
>       * @return true if this package is the default browser on this device, false otherwise.
>       */
> +    private boolean isDefaultBrowser(Intent action) {

This parameter should be a String, not an Intent.

This patch doesn't even build. Please test your patches before asking for review.
Attachment #8494885 - Flags: review?(margaret.leibovic) → review-
(In reply to :Margaret Leibovic from comment #6)

> If we're going to change this probe to be tri-state, it should be an
> integer, not a boolean

In case it's new info to a reader: telemetry supports explicit enums. On the Java side it'd be represented as an integer.


> ... and then use information about what versions of Android people are
> using to determine what set of the "false" probes are due to the fact that
> they couldn't even set a default for this.

If we have a firm association between ACTION_ASSIST and an API version, and the API version is easily accessible in telemetry, then this makes sense to me.
Attached patch bug_1064859_v3.patch (obsolete) — Splinter Review
Changes made. Sorry for not testing the previous patch.
Attachment #8494885 - Attachment is obsolete: true
Attachment #8496312 - Flags: review?(margaret.leibovic)
Status: NEW → ASSIGNED
Comment on attachment 8496312 [details] [diff] [review]
bug_1064859_v3.patch

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

Sorry for the slow review! This is looking good, there's just one change I realized we can make to improve this patch.

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

Since Intent.ACTION_ASSIST is only API level 16+ [1], you should wrap this line in an |if (Versions.feature16Plus)| statement. This has the added benefit of just not logging anything if the user's device doesn't support this intent, so we don't need to analyze those false values out of the data.

[1] https://developer.android.com/reference/android/content/Intent.html#ACTION_ASSIST
Attachment #8496312 - Flags: review?(margaret.leibovic) → feedback+
added "if" statement.
Attachment #8496312 - Attachment is obsolete: true
Attachment #8499172 - Flags: review?(margaret.leibovic)
Comment on attachment 8499172 [details] [diff] [review]
bug_1064859_v4.patch

Great, thanks! I'll land this for you.
Attachment #8499172 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/b6a704c25e79
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.