System Notifications are broken in webapps

VERIFIED FIXED in Firefox 17

Status

()

P1
normal
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

({regression})

Trunk
Firefox 17
regression
Points:
---

Firefox Tracking Flags

(firefox16 verified disabled, firefox17 verified)

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 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

6 years ago
Created attachment 650357 [details] [diff] [review]
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?
Assignee: nobody → wjohnston
Attachment #650357 - Flags: review?(blassey.bugs)
(Assignee)

Updated

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

Comment 3

6 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+]
Blocks: 766259
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

6 years ago
Created attachment 653523 [details] [diff] [review]
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.
Attachment #650357 - Attachment is obsolete: true
Attachment #653523 - Flags: review?(blassey.bugs)
Attachment #653523 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 7

6 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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
(Assignee)

Updated

6 years ago
Duplicate of this bug: 784468
WFM on latest-inbound/central. Confirmed dupe above too.
Status: RESOLVED → VERIFIED
status-firefox16: --- → affected
status-firefox17: --- → 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+

Updated

6 years ago
status-firefox16: affected → fixed
status-firefox16: fixed → verified
Please prepare a backout patch for FF16 to resolve bug 786338.
tracking-firefox16: --- → +

Updated

6 years ago
status-firefox16: verified → disabled
status-firefox16: disabled → verified disabled
(Assignee)

Comment 15

6 years ago
Backed out on 16.
tracking-firefox16: + → ---
You need to log in before you can comment on or make changes to this bug.