Closed
Bug 1267639
Opened 9 years ago
Closed 9 years ago
Content notifications: No telemetry for clicking notifications with one URL
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox48 fixed, firefox49 fixed)
RESOLVED
FIXED
Firefox 49
People
(Reporter: sebastian, Assigned: sebastian)
References
Details
Attachments
(3 files)
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
16.17 KB,
patch
|
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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 | ||
Updated•9 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49449/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49449/
Attachment #8746545 -
Flags: review?(gkruglov)
Attachment #8746546 -
Flags: review?(gkruglov)
Assignee | ||
Comment 2•9 years ago
|
||
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/
Assignee | ||
Comment 3•9 years ago
|
||
Okay, now there's only one Intent and only one piece of code handling it.
Updated•9 years ago
|
Attachment #8746546 -
Flags: review?(gkruglov) → review+
Comment 4•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8746545 -
Flags: review?(gkruglov) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8746545 [details]
MozReview Request: Bug 1267639 - (Pre) Add onNewIntent() method to BrowserAppDelegate. r=grisha
https://reviewboard.mozilla.org/r/49449/#review46855
Assignee | ||
Comment 6•9 years ago
|
||
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
Assignee | ||
Comment 7•9 years ago
|
||
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/
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
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)
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a941ec94925
https://hg.mozilla.org/mozilla-central/rev/2541e115767f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 11•9 years ago
|
||
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?
Assignee | ||
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
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+
Updated•9 years ago
|
status-firefox48:
--- → affected
Comment 14•9 years ago
|
||
bugherder uplift |
Updated•4 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
•