Closed Bug 781061 Opened 12 years ago Closed 12 years ago

System Notifications are broken in webapps


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



(firefox16 verified disabled, firefox17 verified)

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


(Reporter: wesj, Assigned: wesj)



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


(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

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)
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/
@@ +17,5 @@
> +import;
> +import java.util.List;
> +import android.os.Process;
> +import;
> +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-
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?
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
WFM on latest-inbound/central. Confirmed dupe above too.
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.