Closed Bug 781061 Opened 8 years ago Closed 8 years ago

System Notifications are broken in webapps

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox16 --- verified disabled
firefox17 --- verified

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: regression, Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 file, 1 obsolete file)

System notification callbacks fire in the org.mozilla.fennec_<> process, which means that if an app fires a notification its callback is never called. This hurts the marketplace webapp a lot.

I think, since this requires cross-process communication, we need to fire this via. an intent. I'm working on making that work. Detecting if the app is running might mean we have to dig into the activityManger?
Attached patch Patch v1 (obsolete) — Splinter Review
This

1.) uses an intent to pass data back and forth,
2.) switches to using query parameters for our uri because... they're easier,
3.) passes the name of the class that requested the notification so that we can send the return intent back to it, and 
4.) uses the ActivityManager in an ugly hack to figure out if the app is running

What do you think?
Assignee: nobody → wjohnston
Attachment #650357 - Flags: review?(blassey.bugs)
Duplicate of this bug: 781527
Keywords: regression
Whiteboard: [blocking-webrtandroid1?]
blassey ping? I know you said you had questions about one of my patches. Can you drop them in here?
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1?] → [blocking-webrtandroid1+]
Comment on attachment 650357 [details] [diff] [review]
Patch v1

Review of attachment 650357 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/NotificationHandler.java.in
@@ +17,5 @@
> +import android.app.ActivityManager;
> +import java.util.List;
> +import android.os.Process;
> +import android.app.ActivityManager.RunningAppProcessInfo;
> +import org.mozilla.gecko.GeckoAppShell;

alphabetical order with the imports, separate packages with a blank line

@@ +63,5 @@
> +        String nameMod = appName.replace("WebApps$", "");
> +        for(RunningAppProcessInfo procInfo : procInfos) {
> +            if (procInfo.processName.contains(nameMod)) {
> +              isRunning = true;
> +              break;

let's not do this
Attachment #650357 - Flags: review?(blassey.bugs) → review-
Status: NEW → ASSIGNED
Attached patch Patch v2Splinter Review
This patch removes all the "check if this app is running" stuff and fires the intent regardless. Worst case scenario I guess we show Gecko and do nothing, but we should move callers of the alertsService to start registering for these instead.
Attachment #650357 - Attachment is obsolete: true
Attachment #653523 - Flags: review?(blassey.bugs)
Attachment #653523 - Flags: review?(blassey.bugs) → review+
Comment on attachment 653523 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Only showed up when we apps with multiple processes
User impact if declined: Notifications don't work from webapps (i.e. the marketplace)
Testing completed (on m-c, etc.): Landed today
Risk to taking this patch (and alternatives if risky): Medium risk. Changes behavior of our notifications slightly.
String or UUID changes made by this patch: None.
Attachment #653523 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1c14ca0ce21f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Duplicate of this bug: 784468
WFM on latest-inbound/central. Confirmed dupe above too.
Status: RESOLVED → VERIFIED
Comment on attachment 653523 [details] [diff] [review]
Patch v2

Approving for Aurora due to need for wider webapps testing on mobile.
Attachment #653523 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please prepare a backout patch for FF16 to resolve bug 786338.
Backed out on 16.
You need to log in before you can comment on or make changes to this bug.