781061 - System Notifications are broken in webapps

Status: VERIFIED FIXED

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

Description

[Wesley Johnston (:wesj)]

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?

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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?

Comment 3

[Wesley Johnston (:wesj)]

blassey ping? I know you said you had questions about one of my patches. Can you drop them in here?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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.

Comment 6

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1c14ca0ce21f

Comment 7

[Wesley Johnston (:wesj)]

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.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1c14ca0ce21f

Comment 10

[Aaron Train [:aaronmt]]

WFM on latest-inbound/central. Confirmed dupe above too.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 653523 [details] [diff] [review]
Patch v2

Approving for Aurora due to need for wider webapps testing on mobile.

Comment 12

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9fc11770135a

Comment 13

[Alex Keybl [:akeybl]]

Please prepare a backout patch for FF16 to resolve bug 786338.

Comment 14

[Wesley Johnston (:wesj)]

backed out of 16:
https://hg.mozilla.org/releases/mozilla-beta/rev/f5b47209610c

Comment 15

[Wesley Johnston (:wesj)]

Backed out on 16.