System Notifications are broken in webapps

VERIFIED FIXED in Firefox 17

Status

()

defect
P1
normal
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({regression})

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 verified disabled, firefox17 verified)

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

7 years ago
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?
Assignee

Comment 1

7 years ago
Posted 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)
Assignee

Updated

7 years ago
Duplicate of this bug: 781527
Keywords: regression
Whiteboard: [blocking-webrtandroid1?]
Assignee

Comment 3

7 years ago
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
Assignee

Comment 5

7 years ago
Posted 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+
Assignee

Comment 7

7 years ago
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: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Assignee

Updated

7 years ago
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.
Assignee

Comment 15

7 years ago
Backed out on 16.
You need to log in before you can comment on or make changes to this bug.