Closed Bug 1267639 Opened 8 years ago Closed 8 years ago

Content notifications: No telemetry for clicking notifications with one URL

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(3 files)

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.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
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/
Okay, now there's only one Intent and only one piece of code handling it.
Attachment #8746546 - Flags: review?(gkruglov) → review+
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?
Attachment #8746545 - Flags: review?(gkruglov) → review+
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 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/
Attachment #8746545 - Attachment description: MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r?grisha → MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha
Attachment #8746546 - Attachment description: MozReview Request: Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r?grisha → MozReview Request: Bug 1267639 - Use BrowserAppDelegate implementation as single point for handling content notifications intents. r=grisha
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/
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
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.
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/4a941ec94925
https://hg.mozilla.org/mozilla-central/rev/2541e115767f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
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]: -
Flags: needinfo?(s.kaspari)
Attachment #8750341 - Flags: approval-mozilla-aurora?
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.
Attachment #8750341 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.