Closed
Bug 1305498
Opened 8 years ago
Closed 8 years ago
Refactor notification code to be more concise
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jchen, Assigned: jchen)
References
Details
Attachments
(7 files, 3 obsolete files)
5.71 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
9.40 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
13.93 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
14.99 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
8.90 KB,
patch
|
sebastian
:
review+
|
Details | Diff | Splinter Review |
21.37 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
14.79 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
Our notification code is kind of a mess and unnecessarily complex.
Assignee | ||
Comment 1•8 years ago
|
||
Not sure why we needed a task queue for NotificationClient actions. The actions all go through IPC and are non-blocking, so it's perfectly fine to perform them off of whatever thread we're on.
Attachment #8794967 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 2•8 years ago
|
||
There's no reason to have NotificationHandler, AppNotificationClient, and ServiceNotificationClient all separate from the base NotificationClient class. This patch adds the functionality of those three classes to NotificationClient. The notifications hash map is changed from a ConcurrentHashMap to a regular HashMap with synchronization because I think the use case here doesn't warrant the added performance and overhead of ConcurrentHashMap. NotificationService is changed to match the new NotificationClient. Now the only job for NotificationService is to set a notification as foreground, rather than to manage all notifications like before. NotificationHandler, AppNotificationClient, and ServiceNotificationClient will be removed in a later patch.
Attachment #8794968 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 3•8 years ago
|
||
Set NotificationListener once in GeckoApplication.onCreate, instead of spreading it out in GeckoApp, BrowserApp, and GeckoService. This is possible because there's no longer a distinction between AppNotificationClient and ServiceNotificationClient in the new, consolidated NotificationClient.
Attachment #8794969 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 4•8 years ago
|
||
Remove AppNotificationClient, ServiceNotificationClient, and NotificationHandler, now that they've all been replaced by the new, consolidated NotificationClient.
Attachment #8794970 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 5•8 years ago
|
||
Previously, web notification callbacks went to GeckoApp directly, but that presented some problems such as not being able to implement the on-close callback, because we don't want to launch GeckoApp when the notification is closed by swiping. This patch makes us use NotificationReceiver for callbacks, and let NotificationReceiver launch GeckoApp if necessary.
Attachment #8794971 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 6•8 years ago
|
||
Keep the notification cookie a single location (in the notification intent itself), and simplify the native notification handling code.
Attachment #8794972 -
Flags: review?(s.kaspari)
Assignee | ||
Updated•8 years ago
|
Attachment #8794967 -
Attachment description: Remove NotificationClient task queue (v1) → 1. Remove NotificationClient task queue (v1)
Assignee | ||
Updated•8 years ago
|
Attachment #8794968 -
Attachment description: Integrate NotificationHandler et al into NotificationCllient (v1) → 2. Integrate NotificationHandler et al into NotificationCllient (v1)
Assignee | ||
Updated•8 years ago
|
Attachment #8794969 -
Attachment description: Set NotificationListener in GeckoApplication (v1) → 3. Set NotificationListener in GeckoApplication (v1)
Assignee | ||
Updated•8 years ago
|
Attachment #8794970 -
Attachment description: Remove obsolete notification classes (v1) → 4. Remove obsolete notification classes (v1)
Assignee | ||
Updated•8 years ago
|
Attachment #8794971 -
Attachment description: Use NotificationReceiver for web notification callbacks (v1) → 5. Use NotificationReceiver for web notification callbacks (v1)
Assignee | ||
Updated•8 years ago
|
Attachment #8794972 -
Attachment description: Don't keep notification cookie in native code (v1) → 6. Don't keep notification cookie in native code (v1)
Updated•8 years ago
|
Attachment #8794967 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8794968 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8794969 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8794970 -
Flags: review?(s.kaspari) → review+
Updated•8 years ago
|
Attachment #8794971 -
Flags: review?(s.kaspari) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8794972 [details] [diff] [review] 6. Don't keep notification cookie in native code (v1) Review of attachment 8794972 [details] [diff] [review]: ----------------------------------------------------------------- Great patches!
Attachment #8794972 -
Flags: review?(s.kaspari) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Updated patch that manually calls the delete intent on a notification when canceling it, because Android doesn't call it for us.
Attachment #8796579 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8794968 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
One last patch... Currently, persistent notification callbacks go through a different code path, but it'd be more consistent and correct to let persistent notification callbacks go through NotificationReceiver as well. This takes care of some housekeeping work that was missing for persistent notifications, such as deleting the mNotifications entry when the notification is closed.
Attachment #8797215 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 10•8 years ago
|
||
Updated patch that no longer sends the delete intent when we update an existing notification, because the delete intent may end up affecting the new notification.
Attachment #8797768 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8796579 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Updated patch that removes the NotificationClient.mNotifications association when a NotfiicationHelper notification closes.
Attachment #8797771 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8794971 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8797215 -
Flags: review?(s.kaspari) → review+
Comment 12•8 years ago
|
||
Pushed by nchen@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/7810eb621144 Refactor notification code to be more concise; r=sebastian
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7810eb621144
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•3 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
•