Closed Bug 1305498 Opened 3 years ago Closed 3 years ago

Refactor notification code to be more concise

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

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.
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)
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)
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)
Remove AppNotificationClient, ServiceNotificationClient, and
NotificationHandler, now that they've all been replaced by the new,
consolidated NotificationClient.
Attachment #8794970 - Flags: review?(s.kaspari)
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)
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)
Attachment #8794967 - Attachment description: Remove NotificationClient task queue (v1) → 1. Remove NotificationClient task queue (v1)
Attachment #8794968 - Attachment description: Integrate NotificationHandler et al into NotificationCllient (v1) → 2. Integrate NotificationHandler et al into NotificationCllient (v1)
Attachment #8794969 - Attachment description: Set NotificationListener in GeckoApplication (v1) → 3. Set NotificationListener in GeckoApplication (v1)
Attachment #8794970 - Attachment description: Remove obsolete notification classes (v1) → 4. Remove obsolete notification classes (v1)
Attachment #8794971 - Attachment description: Use NotificationReceiver for web notification callbacks (v1) → 5. Use NotificationReceiver for web notification callbacks (v1)
Attachment #8794972 - Attachment description: Don't keep notification cookie in native code (v1) → 6. Don't keep notification cookie in native code (v1)
Attachment #8794967 - Flags: review?(s.kaspari) → review+
Attachment #8794968 - Flags: review?(s.kaspari) → review+
Attachment #8794969 - Flags: review?(s.kaspari) → review+
Attachment #8794970 - Flags: review?(s.kaspari) → review+
Attachment #8794971 - Flags: review?(s.kaspari) → review+
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+
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+
Attachment #8794968 - Attachment is obsolete: true
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)
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+
Attachment #8796579 - Attachment is obsolete: true
Updated patch that removes the NotificationClient.mNotifications association
when a NotfiicationHelper notification closes.
Attachment #8797771 - Flags: review+
Attachment #8794971 - Attachment is obsolete: true
Attachment #8797215 - Flags: review?(s.kaspari) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7810eb621144
Refactor notification code to be more concise; r=sebastian
https://hg.mozilla.org/mozilla-central/rev/7810eb621144
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.