Closed
Bug 1267639
Opened 7 years ago
Closed 7 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•7 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 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•7 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•7 years ago
|
||
Okay, now there's only one Intent and only one piece of code handling it.
Updated•7 years ago
|
Attachment #8746546 -
Flags: review?(gkruglov) → review+
Comment 4•7 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•7 years ago
|
Attachment #8746545 -
Flags: review?(gkruglov) → review+
Comment 5•7 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•7 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•7 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•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a941ec94925 https://hg.mozilla.org/mozilla-central/rev/2541e115767f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 11•7 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•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2edadbef0e03
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+
status-firefox48:
--- → affected
Comment 14•7 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ccd48b5382d1
Updated•2 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
•