1267639 - Content notifications: No telemetry for clicking notifications with one URL

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

I just wanted to add telemetry for the new "READ NOW" button (Bug 1259127), but then I realized that we might only send ACTION/LOAD_URL telemetry if the user clicked a notification with multiple URLs:

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1047

For single URL notifications (That should be the majority) we do not send any.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha

Review commit: https://reviewboard.mozilla.org/r/49449/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49449/

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r=grisha

This patch will create a single Intent action for all content notifications. The intent will be handled by
ContentNotificationsDelegate exclusively.

Review commit: https://reviewboard.mozilla.org/r/49451/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49451/

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, now there's only one Intent and only one piece of code handling it.

Comment 4

[:Grisha Kruglov]

Comment on attachment 8746546 [details]
MozReview Request: Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r=grisha

https://reviewboard.mozilla.org/r/49451/#review46853

::: mobile/android/base/java/org/mozilla/gecko/feeds/ContentNotificationsDelegate.java:24
(Diff revision 1)
> +/**
> + * BrowserAppDelegate implementation that takes care of handling intents from content notifications.
> + */
> +public class ContentNotificationsDelegate extends BrowserAppDelegate {
> +    // The application is opened from a content notification
> +    public static final String ACTION_CONTENT_NOTIFICATION = AppConstants.ANDROID_PACKAGE_NAME + "action.CONTENT_NOTIFICATION";

I think you're missing ".", e.g.:

AppConstants.ANDROID_PACKAGE_NAME + ".action.CONTENT_NOTIFICATION";

::: mobile/android/base/java/org/mozilla/gecko/feeds/ContentNotificationsDelegate.java:59
(Diff revision 1)
> +        }
> +
> +        Telemetry.startUISession(TelemetryContract.Session.EXPERIMENT, FeedService.getEnabledExperiment(browserApp));
> +
> +        Telemetry.sendUIEvent(TelemetryContract.Event.ACTION, TelemetryContract.Method.NOTIFICATION, "content_update");
> +        Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL, TelemetryContract.Method.INTENT, "content_update");

Do we care to distinguish between one vs. multiple url open events?

Comment 5

[:Grisha Kruglov]

Comment on attachment 8746545 [details]
MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha

https://reviewboard.mozilla.org/r/49449/#review46855

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8746545 [details]
MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49449/diff/1-2/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8746546 [details]
MozReview Request: Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r=grisha

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49451/diff/1-2/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/4a941ec949250db7dd70358d84f1f088a5c30f0e
Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/2541e115767f1b8c26ed63616b2ba5d80d8cfd64
Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r=grisha

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

NI: I'd like to uplift this to enable the experiment on Aurora (bug 1267202). However this requires a customized patch or uplift of the BrowserAppDelegate code.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a941ec94925
https://hg.mozilla.org/mozilla-central/rev/2541e115767f

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1267639-telemetry-AURORA.patch

This is a modified version of the patch for Aurora.

Approval Request Comment

[Feature/regressing bug #]: The code for content notifications is in Aurora (bug 1238087) but behind a Switchboard flag. We'd like to enable the experiment (bug 1267202). However this requires this bug to be fixed on Aurora too.

[User impact if declined]: We wouldn't enable the experiment without the patch, so this wouldn't affect the user.

[Describe test coverage new/current, TreeHerder]: Manual testing.

[Risks and why]: Low. The original patch is in Nightly for some days already. The modified version removes some of the dependencies that are not in Aurora.

[String/UUID change made/needed]: -

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2edadbef0e03

Comment 13

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8750341 [details] [diff] [review]
1267639-telemetry-AURORA.patch

Adding telemetry for an android experiment, ok to uplift to aurora, still early enough in the cycle to catch any issues we run into.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ccd48b5382d1