Closed
Bug 1064859
Opened 10 years ago
Closed 10 years ago
Telemetry probe to measure whether Fennec is default ASSIST intent handler
Categories
(Firefox for Android Graveyard :: Search Activity, defect)
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)
3.45 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [lang=java] → [lang=java][shovel-ready]
I would like to patch this bug. Please assign this to me. Thanks!!
Reporter | ||
Comment 2•10 years ago
|
||
(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
Patch attached. Please check it. Thanks!!
Attachment #8494801 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 4•10 years ago
|
||
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+
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)
Reporter | ||
Comment 6•10 years ago
|
||
(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 :)
Reporter | ||
Comment 7•10 years ago
|
||
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-
Comment 8•10 years ago
|
||
(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.
Changes made. Sorry for not testing the previous patch.
Attachment #8494885 -
Attachment is obsolete: true
Attachment #8496312 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
added "if" statement.
Attachment #8496312 -
Attachment is obsolete: true
Attachment #8499172 -
Flags: review?(margaret.leibovic)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Reporter | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b6a704c25e79
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b6a704c25e79
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Updated•6 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•