Closed Bug 569402 Opened 9 years ago Closed 9 years ago

Show notifications in the Status Bar on Android

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: alexp, Assigned: alexp)

References

Details

Attachments

(4 files, 23 obsolete files)

22.73 KB, image/png
Details
62.15 KB, image/png
Details
6.23 KB, patch
blassey
: review+
Details | Diff | Splinter Review
36.26 KB, patch
alexp
: review+
Details | Diff | Splinter Review
Fennec should use the system status bar to show notifications about download progress/complete, add-on install progress/complete, and similar things.
Assignee: nobody → alexp
Status: NEW → ASSIGNED
From IRC discussion:
=============================================================================
[27 May 10 11:23] * madhava * things I'd like to see in the android system bar:  download progress/complete msg; add-on install progress (or at least activity) / complete msg -- if multiple, maybe rolled up into one item
[27 May 10 11:23] * madhava * tapping on the latter would take you to the add-ons manager
[27 May 10 11:23] * blassey * madhava++
[27 May 10 11:23] * madhava * so you can restart fennec
[27 May 10 11:24] * madhava * tapping on a download complete entry would ... hm
[27 May 10 11:24] * madhava * not sure
[27 May 10 11:24] * mwu * the fun part about android notifications is that progress bars are supported
[27 May 10 11:24] * madhava * yeah - I like it a lot
[27 May 10 11:25] * mfinkle * tapping on a notification is already handled by the nsIAlertsService API
[27 May 10 11:25] * blassey * I think he was asking what you thought the notification service needed that's android specific
[27 May 10 11:25] * mfinkle * handling progress is not
[27 May 10 11:25] * blassey * progress is a good one
[27 May 10 11:25] * mfinkle * but even maemo notifications allow for progress, so that should be something we add to the platform
[27 May 10 11:25] * mwu * we need to be able to overwrite previous notifications too
[27 May 10 11:25] * mwu * overwrite/remove
[27 May 10 11:25] * mfinkle * mwu: optionally?
[27 May 10 11:26] * mfinkle * or always
[27 May 10 11:26] * blassey * mfinkle: an android notification service could implement progress listenber
[27 May 10 11:26] * mwu * optionally
[27 May 10 11:26] * mfinkle * mwu: that would be something new too
[27 May 10 11:26] * madhava * wouldn't be surprised to see maemo centralize download progress, etc.
[27 May 10 11:27] * mwu * other things may come up when looking at the android notification api
[27 May 10 11:27] * mfinkle * blassey: might be overkill
[27 May 10 11:27] * mfinkle * and backwards - it would need to register the listener with someone
[27 May 10 11:28] * mfinkle * but it won't be hard to make progress work
[27 May 10 11:28] * mwu * there's also persistent notifications, but I can't think of what we'd need that for
[27 May 10 11:29] * blassey * "you're currently sharing your location"
[27 May 10 11:29] * blassey * maybe
[27 May 10 11:29] * blassey * "there's a security update pending, go upgrade"
[27 May 10 11:29] * blassey * maybe
[27 May 10 11:29] * blassey * these are madhava questions
[27 May 10 11:29] * madhava * wait - that's me
[27 May 10 11:30] * madhava * yeah - I think we want to err on the side of being quiet and transparent
[27 May 10 11:31] * madhava * mostly, I think notifications are worth it when they're about something user-initiated
[27 May 10 11:31] * madhava * i.e. - this thing that you care enough to have initiated is now complete
[27 May 10 11:31] * blassey * the persistent notifications might make sense for security/privacy problems
[27 May 10 11:31] * madhava * vs. , for example, auto updating of add-ons
[27 May 10 11:31] * madhava * wouldn't want to be chirpy about that
[27 May 10 11:32] * blassey * no
[27 May 10 11:32] * madhava * blassey - yeah - security is an interesting point
[27 May 10 11:32] * mwu * I don't think persistent notifications exist for the purpose of being more annoying
[27 May 10 11:33] * madhava * what's an example of one, just so we're talking about the same thing?
[27 May 10 11:33] * mwu * when you plugin your device
[27 May 10 11:33] * madhava * like a download sticking around after it's done
[27 May 10 11:33] * mwu * you get two
[27 May 10 11:33] * madhava * oh
[27 May 10 11:33] * mwu * one for usb debugging, another for mounting
[27 May 10 11:33] * madhava * oh, hm
[27 May 10 11:34] * blassey * downloads aren't persistant
[27 May 10 11:34] * blassey * once you deal with them they go away
[27 May 10 11:34] * madhava * oh, I see
[27 May 10 11:34] * blassey * or you can click "clear" and they all go away
[27 May 10 11:34] * madhava * persistant that way
[27 May 10 11:34] * madhava * not "remain until they're dealt with"
[27 May 10 11:34] * madhava * which is what I was thinking
[27 May 10 11:46] * janD * madhava: what about integrating the persistent notifications as icons in the adressbar?
[27 May 10 11:47] * madhava * janD - we've been trying to keep as much as possible out of the url bar, but yes - it's the same idea
[27 May 10 11:47] * madhava * on android, they've got a place dedicated to this kind of thing
=============================================================================
Depends on: 570514
Draft implementation of the nsIAlertsService using Android status bar notifications.
- New Android-specific nsAlertsService class;
- New functions to issue and handle native Android notifications;
- Modified build scripts to generate resource class to get proper icon IDs.

There are still things to do:
- Make intents from notifications work properly;
- Send an event to Gecko to open add-ons manager.
Attached patch Notifications (obsolete) — Splinter Review
- Cleaned up the code
- Fixed a problem with intents data
- Used the new icons from a proper location
Attachment #452385 - Attachment is obsolete: true
Attached patch Notifications (mobile) (obsolete) — Splinter Review
- Disable AlertsService.js for Android (instead use a new implementation in the platform code)
- Provide additional data to showAlertNotification()
- Notification icons for Android
Attachment #453422 - Flags: review?(mark.finkle)
Comment on attachment 453420 [details] [diff] [review]
Notifications


>+#filter substitution
>+package org.mozilla.@MOZ_APP_NAME@;

can this be org.mozilla.gecko? It would be nice to avoid the preprocessed file.
(In reply to comment #5)
> can this be org.mozilla.gecko? It would be nice to avoid the preprocessed file.

I don't like this preprocessing either, but it didn't work another way, when I tried. Unfortunately it seems the activity has to be in the main application package, otherwise it's not visible for the intents.

Another option would be to add this notification handling to GeckoApp, but I'd prefer not to over-complicate that one, especially for the case of completed downloads, when Fennec itself doesn't need to be launched.
Attached patch Notifications v2 (obsolete) — Splinter Review
Use hashCode() to generate unique notification ID based on the data
Attachment #453420 - Attachment is obsolete: true
Attachment #453499 - Flags: review?(mwu)
Attached patch Notifications v2 (mobile) (obsolete) — Splinter Review
Provide target file name for the start download notification.
Attachment #453422 - Attachment is obsolete: true
Attachment #453500 - Flags: review?(mark.finkle)
Attachment #453422 - Flags: review?(mark.finkle)
Comment on attachment 453500 [details] [diff] [review]
Notifications v2 (mobile)


>diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js
>       if (aTopic == "dl-start") {
>+        var cookie = "";
>+#ifdef ANDROID
>+        cookie = download.target.spec.replace("file:", "download:");
>+#endif

Use | let cookie = ""; |

>+        var cookie = "";
>+#ifdef ANDROID
>+        cookie = download.target.spec;
>+#endif

Same

>diff --git a/chrome/content/extensions.js b/chrome/content/extensions.js

>+    var cookie = "";
>+#ifdef ANDROID
>+    cookie = "addon:addon";
>+#endif

Same
Attachment #453500 - Flags: review?(mark.finkle) → review+
Attached patch Notifications v3 (mobile) (obsolete) — Splinter Review
Fixed the vars [r=mfinkle]
Attachment #453500 - Attachment is obsolete: true
Attachment #453627 - Flags: review+
Comment on attachment 453499 [details] [diff] [review]
Notifications v2

>@@ -275,9 +277,51 @@ class GeckoAppShell
>         intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
>         try {
>             GeckoApp.surfaceView.getContext().startActivity(intent);
>             return true;
>         } catch(ActivityNotFoundException e) {
>             return false;
>         }
>     }
>+
>+    static void showAlertNotification(String alertTitle, String alertText, String alertData) {
>+        Log.i("GeckoAppJava", "GeckoAppShell.showAlertNotification\n" +
>+              "title = '" + alertTitle + "', text = '" + alertText + "', data='" + alertData + "'");
>+
>+        GeckoApp context = GeckoApp.mAppContext;
>+
>+        int notificationID = GeckoApp.SIMPLE_NOTIFICATION_ID;
>+        int icon = R.drawable.icon; // TODO: May have a generic icon for other notifications?
>+        if (alertData != null && alertData.length() >= 5)
>+        {
{ goes on the same line as the if.

>+            String type = alertData.substring(0, 5);
>+            if (type.equals("file:") || type.equals("downl")) {
>+                icon = R.drawable.alertdownloads;
>+                int colon = alertData.indexOf(":");
>+                if (colon > 0)
>+                    notificationID = alertData.substring(colon).hashCode();
>+            } else {
>+                if (type.equals("addon"))
>+                    icon = R.drawable.alertaddons;
>+                notificationID = alertData.hashCode();
>+            }
>+        }
Why special case file: and downl hashing?

>+RES_DRAWABLE = \
>+  res/drawable/alertaddons.png \
>+  res/drawable/alertdownloads.png \
>+  $(NULL)
>+
>+RES_DRAWABLE_HDPI = \
>+  res/drawable-hdpi/alertaddons.png \
>+  res/drawable-hdpi/alertdownloads.png \
>+  $(NULL)
>+
Hmm.. this strikes me as wrong, but I don't know enough to do this better and this file is full of wrong things anyway.

>diff --git a/embedding/android/NotificationHandler.java.in b/embedding/android/NotificationHandler.java.in
>new file mode 100644
>--- /dev/null
>+++ b/embedding/android/NotificationHandler.java.in
>@@ -0,0 +1,127 @@
>+/* -*- Mode: Java; tab-width: 20; indent-tabs-mode: nil; -*-
>+ * ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Mozilla Android code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Alex Pakhotin <alexp@mozilla.com>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#filter substitution
>+package org.mozilla.@MOZ_APP_NAME@;
>+
>+import android.app.Activity;
>+import android.app.NotificationManager;
>+import android.content.Intent;
>+import android.content.ActivityNotFoundException;
>+import android.os.Bundle;
>+import android.util.Log;
>+import android.net.Uri;
>+
>+import org.mozilla.gecko.GeckoApp;
>+
>+public class NotificationHandler
>+    extends Activity
>+{
>+    @Override
>+    protected void onCreate(Bundle savedInstanceState) {
>+        super.onCreate(savedInstanceState);
>+        Log.i("GeckoAppJava", "NotificationHandler.onCreate");
>+
>+        Intent intent = getIntent();
>+        if (intent != null)
>+            handleIntent(intent);
>+
>+        finish();
>+    }
>+
>+    protected void handleIntent(Intent intent) {
>+        Log.i("GeckoAppJava", "NotificationHandler.handleIntent");
>+        final String action = intent.getAction();
>+        final String data = intent.getDataString();
>+        Log.i("GeckoAppJava", "action = '" + action + "', data = '" + data + "'");
>+
>+        NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
>+        int notificationID = GeckoApp.SIMPLE_NOTIFICATION_ID;
>+
Using SIMPLE_NOTIFICATION_ID here shouldn't work anymore, should it?

>+    static String getMimeTypeFromName(String aFileName) {
>+        final String defautMime = "application/octet-stream";
defaultMime

Also, what's with all the final in this function?

>diff --git a/toolkit/components/alerts/src/android/Makefile.in b/toolkit/components/alerts/src/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/Makefile.in
You'll probably also want a toolkit peer to review this stuff.

>diff --git a/toolkit/components/alerts/src/android/nsAlertsService.cpp b/toolkit/components/alerts/src/android/nsAlertsService.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/nsAlertsService.cpp
>+NS_IMETHODIMP nsAlertsService::ShowAlertNotification(const nsAString & aImageUrl, const nsAString & aAlertTitle, 
>+                                                     const nsAString & aAlertText, PRBool aAlertTextClickable,
>+                                                     const nsAString & aAlertCookie,
>+                                                     nsIObserver * aAlertListener,
>+                                                     const nsAString & aAlertName)
>+{
>+  nsAutoString title(aAlertTitle), text(aAlertText), data(aAlertCookie);
>+  mozilla::AndroidBridge::Bridge()->ShowAlertNotification(title.get(), title.Length(),
>+                                                          text.get(), text.Length(),
>+                                                          data.get(), data.Length());
>+  return NS_OK;
>+}
Probably better off passing the nsAString and then PromiseFlatString in the AndroidBridge so you can get() the string.

>diff --git a/toolkit/components/alerts/src/android/nsAlertsService.h b/toolkit/components/alerts/src/android/nsAlertsService.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/nsAlertsService.h
>+class nsAlertsService : public nsIAlertsService
>+{
>+public:
>+  NS_DECL_NSIALERTSSERVICE
>+  NS_DECL_ISUPPORTS
>+
>+  nsAlertsService();
>+  virtual ~nsAlertsService();
>+  nsresult Init();
>+
>+protected:
No need for protected: if there's nothing there.

Looks pretty close. I want to review this one more time before r+ing.
Attachment #453499 - Flags: review?(mwu)
(In reply to comment #11)
> >+            String type = alertData.substring(0, 5);
> >+            if (type.equals("file:") || type.equals("downl")) {
> >+                icon = R.drawable.alertdownloads;
> >+                int colon = alertData.indexOf(":");
> >+                if (colon > 0)
> >+                    notificationID = alertData.substring(colon).hashCode();
> >+            } else {
> >+                if (type.equals("addon"))
> >+                    icon = R.drawable.alertaddons;
> >+                notificationID = alertData.hashCode();
> >+            }
> >+        }
> Why special case file: and downl hashing?

"download:///filename" is the notification of download start, and "file:///filename" is the finished download.
We hash only the "filename" portion so that the latter "file:" notification will get the same ID and replace the former "download:" one for that file.
For other types it doesn't really matter at the moment, so we hash the whole string.

> >+        NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
> >+        int notificationID = GeckoApp.SIMPLE_NOTIFICATION_ID;
> >+
> Using SIMPLE_NOTIFICATION_ID here shouldn't work anymore, should it?

SIMPLE_NOTIFICATION_ID is supposed to be used in those cases when the cookie (alertData) is not provided in a call to ShowAlertNotification.
Currently we don't have such cases, when a notification doesn't require any apecial action, but could add those if needed, and they will be already supported.

> >+        final String defautMime = "application/octet-stream";
> defaultMime
> 
> Also, what's with all the final in this function?

Well, "final" is not required here - it's like "const" just helps to find the unintentional modifications of the variables in compile time.
But probably it might be better to remove them here to be consistent with the rest of our code.

> >+++ b/toolkit/components/alerts/src/android/Makefile.in
> You'll probably also want a toolkit peer to review this stuff.

Yes, someone else has to look at this, as some parts there are not very obvious.
Attached patch Notifications v3 (obsolete) — Splinter Review
Fixed as per the review.
Attachment #453499 - Attachment is obsolete: true
Attachment #453840 - Flags: review?(mwu)
(In reply to comment #12)
> (In reply to comment #11)
> > >+            String type = alertData.substring(0, 5);
> > >+            if (type.equals("file:") || type.equals("downl")) {
> > >+                icon = R.drawable.alertdownloads;
> > >+                int colon = alertData.indexOf(":");
> > >+                if (colon > 0)
> > >+                    notificationID = alertData.substring(colon).hashCode();
> > >+            } else {
> > >+                if (type.equals("addon"))
> > >+                    icon = R.drawable.alertaddons;
> > >+                notificationID = alertData.hashCode();
> > >+            }
> > >+        }
> > Why special case file: and downl hashing?
> 
> "download:///filename" is the notification of download start, and
> "file:///filename" is the finished download.
> We hash only the "filename" portion so that the latter "file:" notification
> will get the same ID and replace the former "download:" one for that file.
> For other types it doesn't really matter at the moment, so we hash the whole
> string.
> 
Why not download: for both?

> > >+        NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
> > >+        int notificationID = GeckoApp.SIMPLE_NOTIFICATION_ID;
> > >+
> > Using SIMPLE_NOTIFICATION_ID here shouldn't work anymore, should it?
> 
> SIMPLE_NOTIFICATION_ID is supposed to be used in those cases when the cookie
> (alertData) is not provided in a call to ShowAlertNotification.
> Currently we don't have such cases, when a notification doesn't require any
> apecial action, but could add those if needed, and they will be already
> supported.
> 
What happens when alertData isn't provided? Do we crash or is an empty string provided?
(In reply to comment #14)
> Why not download: for both?

We have to distinguish between start download and the finished one.
Clicking on a notification of the first type should open download manager (still a TODO), while on the second - opens the file itself with ACTION_VIEW intent.

nsIAlertsService does not have a standard way to provide a type of the notification at the moment, so I implemented it through the cookie URI.

> What happens when alertData isn't provided? Do we crash or is an empty string
> provided?

An empty string.
With current implementation the Fennec icon (and provided title/text) will be used for the notification in this case, and nothing will happen when user clicks on that notification - it will just be cleared.
(In reply to comment #15)
> (In reply to comment #14)
> > Why not download: for both?
> 
> We have to distinguish between start download and the finished one.
> Clicking on a notification of the first type should open download manager
> (still a TODO), while on the second - opens the file itself with ACTION_VIEW
> intent.
> 
I think that should be up to the frontend to decide.
Attachment #453840 - Flags: review?(doug.turner)
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Why not download: for both?
> > 
> > We have to distinguish between start download and the finished one.
> > Clicking on a notification of the first type should open download manager
> > (still a TODO), while on the second - opens the file itself with ACTION_VIEW
> > intent.
> > 
> I think that should be up to the frontend to decide.

Yeah, I kinda agree with this. The front-end can open the file, if it's already downloaded.

I missed the fact that your were using 2 different cookies for download. I'd like it better too, if they were both "download"
Comment on attachment 453627 [details] [diff] [review]
Notifications v3 (mobile)

>+        let cookie = "";
>+#ifdef ANDROID
>+        cookie = download.target.spec.replace("file:", "download:");
>+#endif

Will setting the cookie have any effect on non-Android platforms?  If not, can we get rid of the #ifdefs here?
Comment on attachment 453840 [details] [diff] [review]
Notifications v3

>+
>+        <activity android:name="NotificationHandler"
>+                  android:label="@MOZ_APP_DISPLAYNAME@ Notification"
>+                  android:theme="@android:style/Theme.NoTitleBar">

does or how will "Notification" be localized?

>     }
>+
>+    static void showAlertNotification(String alertTitle, String alertText, String alertData) {
>+        Log.i("GeckoAppJava", "GeckoAppShell.showAlertNotification\n" +
>+              "title = '" + alertTitle + "', text = '" + alertText + "', data='" + alertData + "'");

Wrap for the sake of human eyes.

>+
>+        GeckoApp context = GeckoApp.mAppContext;

Probably not very useful to have a local copy.  it is only used 3-4 times in a non critical patch.

>+        int icon = R.drawable.icon; // TODO: May have a generic icon for other notifications?

Follow up bug?


>+        if (alertData != null && alertData.length() >= 5) {
>+            String type = alertData.substring(0, 5);
>+            if (type.equals("file:") || type.equals("downl")) {
>+                icon = R.drawable.alertdownloads;
>+
>+                int colon = alertData.indexOf(":");
>+                if (colon > 0)
>+                    notificationID = alertData.substring(colon).hashCode();
>+            } else {
>+                if (type.equals("addon"))
>+                    icon = R.drawable.alertaddons;
>+
>+                notificationID = alertData.hashCode();
>+            }
>+        }

I do not think I understand this... it looks not very extensible.  If today we added a new notification, we'd have to muck with this code, right?

>+
>+        long when = System.currentTimeMillis();

Why a local?  Just use it below directly.

>+        Notification notification = new Notification(icon, alertTitle, when);
>+
>+        Uri dataUri = Uri.parse(alertData);
>+        notificationIntent.setData(dataUri);

again, no need for the local, right?

>+
>+        PendingIntent contentIntent = PendingIntent.getActivity(context, 0, notificationIntent, 0);
>+        notification.setLatestEventInfo(context, alertTitle, alertText, contentIntent);
>+
>+        NotificationManager notificationManager = (NotificationManager)
>+            context.getSystemService(Context.NOTIFICATION_SERVICE);
>+        Log.i("GeckoAppJava", "Create notification ID " + notificationID);

move this log to below the notify

>+        notificationManager.notify(notificationID, notification);
>+    }


>+        if (data == null || data.length() < 5) {
>+            // Nothing special to handle, just cancel the notification
>+            notificationManager.cancel(notificationID);
>+            return;
>+        }

using special hard coded lengths?

>+
>+        String type = data.substring(0, 5);
>+        if (type.equals("file:") || type.equals("downl")) {
>+            int colon = data.indexOf(":");
>+            if (colon > 0)
>+                notificationID = data.substring(colon).hashCode();
>+        } else {
>+            notificationID = data.hashCode();
>+        }

again.


>+            if (type.equals("file:")) {
>+                Uri path = Uri.parse(data);
>+                String mimetype = getMimeTypeFromName(data);
>+                Intent intentView = new Intent(Intent.ACTION_VIEW);
>+                intentView.setDataAndType(path, mimetype);
>+                intentView.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP);
>+
>+                startActivity(intentView);
>+            } else {
>+                intent.setClassName(this, "org.mozilla.@MOZ_APP_NAME@.App");
>+                startActivity(intent);
>+                //TODO: Open add-ons manager for "addon", download manager for "download"

follow up bug?

>+            }
>+        } catch (ActivityNotFoundException e) {
>+            Log.e("GeckoAppJava", "NotificationHandler Exception: " + e.getMessage());
>+        }
>+    }

I am sure I am missing something, but i find this not very extensible.

>+        return android.webkit.MimeTypeMap.getSingleton().getMimeTypeFromExtension(ext);

Does this mean we import webkit?  rly?


>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2000
>+# the Initial Developer. All Rights Reserved.
>+#

2010!

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2002
>+ * the Initial Developer. All Rights Reserved.

Same.



>+
>+NS_IMETHODIMP nsAlertsService::ShowAlertNotification(const nsAString & aImageUrl, const nsAString & aAlertTitle,
>+                                                     const nsAString & aAlertText, PRBool aAlertTextClickable,
>+                                                     const nsAString & aAlertCookie,
>+                                                     nsIObserver * aAlertListener,
>+                                                     const nsAString & aAlertName)
>+{
>+  mozilla::AndroidBridge::Bridge()->ShowAlertNotification(aAlertTitle, aAlertText, aAlertCookie);
>+  return NS_OK;

You must follow the interface requirement.  how is the listener ever called back?

>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2002
>+ * the Initial Developer. All Rights Reserved.

2010


r-

I would like to review followups.
Attachment #453840 - Flags: review?(mwu)
Attachment #453840 - Flags: review?(doug.turner)
Attachment #453840 - Flags: review-
also...

please provide screenshots.  it looks like there is some flexibility and I want madhava to have a look.
(In reply to comment #19)
> >+        <activity android:name="NotificationHandler"
> >+                  android:label="@MOZ_APP_DISPLAYNAME@ Notification"
> >+                  android:theme="@android:style/Theme.NoTitleBar">
> 
> does or how will "Notification" be localized?

With "NoTitleBar" style it is not supposed to be displayed anywhere.

> >+        if (alertData != null && alertData.length() >= 5) {
> >+            String type = alertData.substring(0, 5);
> >+            if (type.equals("file:") || type.equals("downl")) {
> >+                icon = R.drawable.alertdownloads;
> >+
> >+                int colon = alertData.indexOf(":");
> >+                if (colon > 0)
> >+                    notificationID = alertData.substring(colon).hashCode();
> >+            } else {
> >+                if (type.equals("addon"))
> >+                    icon = R.drawable.alertaddons;
> >+
> >+                notificationID = alertData.hashCode();
> >+            }
> >+        }
> 
> I do not think I understand this... it looks not very extensible.  If today we
> added a new notification, we'd have to muck with this code, right?

If we want to show a different type of notification, we'll have to change this code anyway at least to add another icon.
Android notifications seem to support only icons from the application resources.

Right now this code is also a bit hacky to distinguish between download start and finish, as described in the previous comments.
This should be improved with implementation of the listener support.

> >+        long when = System.currentTimeMillis();
> 
> Why a local?  Just use it below directly.

Just for better readability.

> >+        Uri dataUri = Uri.parse(alertData);
> >+        notificationIntent.setData(dataUri);
> 
> again, no need for the local, right?

Same.

> >+        if (data == null || data.length() < 5) {
> 
> using special hard coded lengths?

An attempt to optimize the check for the scheme. Though using Uri.getScheme() instead would be definitely more clear, the speed is not critical here.

> >+        return android.webkit.MimeTypeMap.getSingleton().getMimeTypeFromExtension(ext);
> 
> Does this mean we import webkit?  rly?

Only use this map. It's also used in another GeckoAppShell method.

> >+NS_IMETHODIMP nsAlertsService::ShowAlertNotification(const nsAString & aImageUrl, const nsAString & aAlertTitle,
> >+                                                     const nsAString & aAlertText, PRBool aAlertTextClickable,
> >+                                                     const nsAString & aAlertCookie,
> >+                                                     nsIObserver * aAlertListener,
> >+                                                     const nsAString & aAlertName)
> >+{
> >+  mozilla::AndroidBridge::Bridge()->ShowAlertNotification(aAlertTitle, aAlertText, aAlertCookie);
> >+  return NS_OK;
> 
> You must follow the interface requirement.  how is the listener ever called
> back?

In the current implementation it was not supposed to be called, and apparently this is a major flaw.
That's why the patch needs to be reworked to support this.
> Android notifications seem to support only icons from the application
resources.

Are you sure about that?  I can't download an icon from a remote site and use it in the notification?
(In reply to comment #22)
> > Android notifications seem to support only icons from the application
> resources.
> 
> Are you sure about that?  I can't download an icon from a remote site and use
> it in the notification?

http://developer.android.com/reference/android/app/Notification.html#Notification%28int,%20java.lang.CharSequence,%20long%29
"icon 	The resource id of the icon to put in the status bar."

Though, as Matt already mentioned in #mobile, it's possible to customize the expanded view - the detailed info shown when user opens the Notifications window:
http://developer.android.com/guide/topics/ui/notifiers/notifications.html#CustomExpandedView
Attached patch Notifications v4 (WIP) (obsolete) — Splinter Review
Fixed review comments.
Observer call implementation is still to do.
Attachment #453840 - Attachment is obsolete: true
Attachment #454158 - Attachment description: Notifications v4 (WIP → Notifications v4 (WIP)
tracking-fennec: --- → 2.0+
Attached patch Notifications (mobile) v4 (obsolete) — Splinter Review
Minor change: use the "file:" scheme both for download start and finish.
Attachment #453627 - Attachment is obsolete: true
Attachment #455797 - Flags: review+
Attached patch Notifications v5 (obsolete) — Splinter Review
- Implemented mechanism to call observers from Java layer;
- Call the alert listener observer from the notification handler.
Attachment #454158 - Attachment is obsolete: true
Attachment #455798 - Flags: review?(mwu)
Attachment #455798 - Flags: review?(doug.turner)
Comment on attachment 455798 [details] [diff] [review]
Notifications v5


>@@ -393,9 +396,13 @@ abstract public class GeckoApp
>             String amCmd = "/system/bin/am broadcast -a " + action + getEnvString() + " -n org.mozilla." + getAppName() + "/org.mozilla." + getAppName() + ".Restarter";
>             Log.i("GeckoAppJava", amCmd);
>             Runtime.getRuntime().exec(amCmd);
>         } catch (Exception e) {
>             Log.i("GeckoAppJava", e.toString());
>         }
>         System.exit(0);
>     }
>+
>+    public void handleNotification(Intent intent) {
>+        GeckoAppShell.handleNotification(intent);
>+    }
> }
Who calls this?

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java
>--- a/embedding/android/GeckoAppShell.java
>+++ b/embedding/android/GeckoAppShell.java
>@@ -47,22 +47,24 @@ import android.app.*;
> import android.text.*;
> import android.view.*;
> import android.view.inputmethod.*;
> import android.content.*;
> import android.graphics.*;
> import android.widget.*;
> import android.hardware.*;
> import android.location.*;
>+import android.net.Uri;
> 
> import android.util.*;
> import android.content.DialogInterface; 
> import android.content.pm.PackageManager;
> import android.content.pm.ResolveInfo;
> 
>+import org.mozilla.gecko.R;
> 
Is this necessary? We should be in the right package.

>@@ -80,16 +82,18 @@ class GeckoAppShell
>     public static native void nativeInit();
>     public static native void nativeRun(String args);
> 
>     // helper methods
>     public static native void setInitialSize(int width, int height);
>     public static native void setSurfaceView(GeckoSurfaceView sv);
>     public static native void putenv(String map);
>     public static native void onResume();
>+    public static native void callObserver(String observerKey, String topic, String data);
>+    public static native void removeObserver(String observerKey);
> 
As far as I can tell, the observer is always removed after being called. A separate removeObserver doesn't seem necessary.

>+        if (alertCookie != null)
>+            notificationID = alertCookie.hashCode();
I thought alertCookie is never null?

>@@ -105,34 +116,45 @@ include $(topsrcdir)/config/rules.mk
> 
> # Override the Java settings with some specific android settings
> include $(topsrcdir)/config/android-common.mk
> 
> tools:: $(MOZ_APP_NAME).apk
> 
> # Note that we're going to set up a dependency directly between embed_android.dex and the java files
> # Instead of on the .class files, since more than one .class file might be produced per .java file
>-classes.dex: $(JAVAFILES) App.java Restarter.java
>+classes.dex: $(JAVAFILES) App.java Restarter.java NotificationHandler.java R.java
> 	$(NSINSTALL) -D classes
>-	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES)) App.java Restarter.java
>+	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES)) App.java Restarter.java NotificationHandler.java R.java
Empty space at the end of line.

>+#filter substitution
>+package org.mozilla.@MOZ_APP_NAME@;
>+
>+import android.app.Activity;
>+import android.app.NotificationManager;
>+import android.content.Intent;
>+import android.content.ActivityNotFoundException;
>+import android.os.Bundle;
>+import android.util.Log;
>+import android.net.Uri;
>+
>+import org.mozilla.gecko.GeckoApp;
>+
I don't like being so specific with imports.. especially with org.mozilla.gecko.

>+        if (GeckoApp.mAppContext != null) {
>+            // This should call the observer, if any
>+            GeckoApp.mAppContext.handleNotification(notificationIntent);
>+        } else {
>+            if (scheme.equals("file")) {
>+                // Application was closed, reopen it to view the file
>+                appIntent.setAction(Intent.ACTION_VIEW);
>+                appIntent.setData(cookieUri);
>+            }
>+        }
Can we just do this all the time instead of trying to shortcut the intent to handleNotification?

>diff --git a/toolkit/components/alerts/src/android/nsAlertsService.cpp b/toolkit/components/alerts/src/android/nsAlertsService.cpp
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/nsAlertsService.cpp
There's a new world order. The static comp registration stuff breaks this, but you've probably noticed by now.

>@@ -263,16 +266,38 @@ AndroidBridge::GetMimeTypeFromExtension(
> 
> void
> AndroidBridge::MoveTaskToBack()
> {
>     mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jMoveTaskToBack);
> }
> 
> void
>+AndroidBridge::ShowAlertNotification(const nsAString & aAlertTitle,
>+                                     const nsAString & aAlertText,
>+                                     const nsAString & aAlertCookie,
>+                                     nsIObserver * aAlertListener)
>+{
>+    ALOG("ShowAlertNotification");
>+
>+    if (nsAppShell::gAppShell)
>+        nsAppShell::gAppShell->AddObserver(aAlertCookie, aAlertListener);
>+
>+    const nsPromiseFlatString &alertTitle = PromiseFlatString(aAlertTitle);
>+    const nsPromiseFlatString &alertText = PromiseFlatString(aAlertText);
>+    const nsPromiseFlatString &alertCookie = PromiseFlatString(aAlertCookie);
>+
The microoptimization here with const is silly.

>+    jvalue args[3];
>+    args[0].l = mJNIEnv->NewString(alertTitle.get(), alertTitle.Length());
>+    args[1].l = mJNIEnv->NewString(alertText.get(), alertText.Length());
>+    args[2].l = mJNIEnv->NewString(alertCookie.get(), alertCookie.Length());
AutoLocalJNIFrame required here due to object alloc by NewString.

>@@ -107,16 +116,21 @@ public:
>     void GetHandlersForMimeType(const char *aMimeType, nsStringArray* aStringArray);
> 
>     PRBool OpenUriExternal(nsCString& aUriSpec, nsCString& aMimeType);
> 
>     void GetMimeTypeFromExtension(const nsCString& aFileExt, nsCString& aMimeType);
> 
>     void MoveTaskToBack();
> 
>+    void ShowAlertNotification(const nsAString & aAlertTitle,
>+                               const nsAString & aAlertText,
>+                               const nsAString & aAlertData,
>+                               nsIObserver * aAlertListener);
>+
Follow the style elsewhere in this file. & goes to the left, * goes to the right.

>+
>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_callObserver(JNIEnv *jenv, jclass, jstring jObserverKey, jstring jTopic, jstring jData)
>+{
>+    if (!nsAppShell::gAppShell)
>+        return;
>+
>+    ALOG("JNI: callObserver");
>+
>+    const jchar *observerKey = jenv->GetStringChars(jObserverKey, NULL);
>+    nsString sObserverKey(observerKey);
>+    sObserverKey.SetLength(jenv->GetStringLength(jObserverKey));
>+    jenv->ReleaseStringChars(jObserverKey, observerKey);
>+
>+    const char *topic = jenv->GetStringUTFChars(jTopic, NULL);
>+    nsCString sTopic(topic);
>+    sTopic.SetLength(jenv->GetStringLength(jTopic));
>+    jenv->ReleaseStringUTFChars(jTopic, topic);
>+
>+    const jchar *data = jenv->GetStringChars(jData, NULL);
>+    nsString sData(data);
>+    sData.SetLength(jenv->GetStringLength(jData));
>+    jenv->ReleaseStringChars(jData, data);
>+
There's a nsJNIString for this now.

>+void
>+nsAppShell::CallObserver(const nsString &aObserverKey, const nsCString &aTopic, const nsString &aData)
Use the abstract string classes.

>+{
>+    nsCOMPtr<nsIObserver> observer;
>+    mObserversHash.Get(aObserverKey, getter_AddRefs(observer));
>+
>+    if (!observer) {
>+        ALOG("nsAppShell::CallObserver: Observer was not found!");
>+        return;
>+    }
>+    
>+    if (NS_IsMainThread()) {
>+        ALOG("nsAppShell::CallObserver: observer = 0x%08x, topic = '%s')",
>+             (unsigned int)(nsIObserver*)observer, mTopic.get());
>+        observer->Observe(nsnull, aTopic.get(), aData.get());
>+    } else {
>+        ALOG("nsAppShell::CallObserver: Not running on main thread! Using NS_DispatchToMainThread");
>+        nsCOMPtr<nsIRunnable> observerCaller = new ObserverCaller(observer, aTopic.get(), aData.get());
>+        nsresult rv = NS_DispatchToMainThread(observerCaller);
>+        ALOG("NS_DispatchToMainThread result: %d", rv);
>+    }
This will never be called in the main thread unless the java thread turns into the main thread somehow.
> >+        if (alertCookie != null)
> >+            notificationID = alertCookie.hashCode();
>I thought alertCookie is never null?

It can be null in general
(In reply to comment #31)
> > >+        if (alertCookie != null)
> > >+            notificationID = alertCookie.hashCode();
> >I thought alertCookie is never null?
> 
> It can be null in general
Not in this case. The call from the gecko side (in AndroidBridge) always sends a string. If there's no string, it will most likely crash before reaching the Java wrapper code here.
(In reply to comment #30)

>>+    public void handleNotification(Intent intent) {
>>+        GeckoAppShell.handleNotification(intent);
>>+    }
>> }
>Who calls this?

It is called from NotificationHandler, which is in a different package (org.mozilla.Fennec).

>>+        if (alertCookie != null)
>>+            notificationID = alertCookie.hashCode();
>I thought alertCookie is never null?

In this case yes, and even if we want to check for null, it must be done earlier.

>>+import org.mozilla.gecko.GeckoApp;
>>+
>I don't like being so specific with imports.. especially with
>org.mozilla.gecko.

Unfortunately it is required here, as NotificationHandler is in the Fennec package.
I'll try something else though.

>>+        if (GeckoApp.mAppContext != null) {
>>+            // This should call the observer, if any
>>+            GeckoApp.mAppContext.handleNotification(notificationIntent);
>>+        } else {
>>+            if (scheme.equals("file")) {
>>+                // Application was closed, reopen it to view the file
>>+                appIntent.setAction(Intent.ACTION_VIEW);
>>+                appIntent.setData(cookieUri);
>>+            }
>>+        }
>Can we just do this all the time instead of trying to shortcut the intent to
>handleNotification?

There are some issues with re-starting the main activity when it's already running, that's why this separate activity is used and it calls the handleNotification directly.

>>+    const nsPromiseFlatString &alertCookie = PromiseFlatString(aAlertCookie);
>>+
>The microoptimization here with const is silly.

It's just a standard pattern for using PromiseFlatString, and also a good coding practice to use const to make sure the object is not changed accidentally.

>>+void
>>+nsAppShell::CallObserver(const nsString &aObserverKey, const nsCString &aTopic, const nsString &aData)
>Use the abstract string classes.

I was torn between nsAString and nsString here - didn't want to wrap the arguments into nsPromiseFlatString to get the content when it is known it's a nsString passed here. But you're right - it must be AString.

>>+    if (NS_IsMainThread()) {
>>+        ALOG("nsAppShell::CallObserver: observer = 0x%08x, topic = '%s')",
>>+             (unsigned int)(nsIObserver*)observer, mTopic.get());
>>+        observer->Observe(nsnull, aTopic.get(), aData.get());
>>+    } else {
>>+        ALOG("nsAppShell::CallObserver: Not running on main thread! Using NS_DispatchToMainThread");
>>+        nsCOMPtr<nsIRunnable> observerCaller = new ObserverCaller(observer, aTopic.get(), aData.get());
>>+        nsresult rv = NS_DispatchToMainThread(observerCaller);
>>+        ALOG("NS_DispatchToMainThread result: %d", rv);
>>+    }
>This will never be called in the main thread unless the java thread turns into
>the main thread somehow.

I think it's better to play safe here. This way it also makes a bit more clear what's supposed to be done by this piece of code. Extra logging might not be necessary though.
Comment on attachment 455798 [details] [diff] [review]
Notifications v5

>+    public static final int SIMPLE_NOTIFICATION_ID = 1;

why 1?




>+        if (scheme != null) {
>+            scheme = scheme.toLowerCase();
>+            if (scheme.equals("file")) {
>+                icon = R.drawable.alertdownloads;
>+            } else {
>+                if (scheme.equals("addon"))
>+                    icon = R.drawable.alertaddons;
>+            }

|else if| without the extra curly braces.



>+
>+        if (GeckoApp.mAppContext != null) {
>+            // This should call the observer, if any
>+            GeckoApp.mAppContext.handleNotification(notificationIntent);
>+        } else {
>+            if (scheme.equals("file")) {
>+                // Application was closed, reopen it to view the file
>+                appIntent.setAction(Intent.ACTION_VIEW);
>+                appIntent.setData(cookieUri);

same thing here.  else if


>+
>+        // Start or bring to front the main activity
>+        try {
>+            startActivity(appIntent);
>+        } catch (ActivityNotFoundException e) {
>+            Log.e("GeckoAppJava", "NotificationHandler Exception: " + e.getMessage());
>+        }

Do we only really care about this exception?  what about others?


> void
>+AndroidBridge::ShowAlertNotification(const nsAString & aAlertTitle,
>+                                     const nsAString & aAlertText,
>+                                     const nsAString & aAlertCookie,
>+                                     nsIObserver * aAlertListener)
>+{
>+    ALOG("ShowAlertNotification");
>+
>+    if (nsAppShell::gAppShell)
>+        nsAppShell::gAppShell->AddObserver(aAlertCookie, aAlertListener);
>+

Why would gAppShell be null?


>+nsresult
>+nsAppShell::AddObserver(const nsAString &aObserverKey, nsIObserver *aObserver)
>+{
>+    nsresult result = NS_OK;
>+    if (aObserver)
>+        result = mObserversHash.Put(aObserverKey, aObserver) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

assert if the aObserver is null.  it is a programmatic error, right?


>+class ObserverCaller : public nsRunnable {
>+public:
>+    ObserverCaller(nsIObserver *aObserver, const char *aTopic, const PRUnichar *aData) :
>+        mObserver(aObserver), mTopic(aTopic), mData(aData) {}

Same here.

>+
>+    NS_IMETHOD Run() {
>+        ALOG("ObserverCaller::Run: observer = 0x%08x, topic = '%s')",
>+             (unsigned int)(nsIObserver*)mObserver, mTopic.get());
>+        if (mObserver)
>+            mObserver->Observe(nsnull, mTopic.get(), mData.get());

and drop the check for mObserver.
Comment on attachment 455798 [details] [diff] [review]
Notifications v5

Also, where does "alertfinished" get fired from:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl#68
Attachment #455798 - Flags: review?(mwu)
Attachment #455798 - Flags: review?(doug.turner)
Attachment #455798 - Flags: review-
(In reply to comment #34)
> >+    public static final int SIMPLE_NOTIFICATION_ID = 1;
> 
> why 1?

Why not?
It could be any number, which is unique inside the app.

> >+        // Start or bring to front the main activity
> >+        try {
> >+            startActivity(appIntent);
> >+        } catch (ActivityNotFoundException e) {
> >+            Log.e("GeckoAppJava", "NotificationHandler Exception: " + e.getMessage());
> >+        }
> 
> Do we only really care about this exception?  what about others?

startActivity() throws only ActivityNotFoundException.

> > void
> >+AndroidBridge::ShowAlertNotification(const nsAString & aAlertTitle,
> >+                                     const nsAString & aAlertText,
> >+                                     const nsAString & aAlertCookie,
> >+                                     nsIObserver * aAlertListener)
> >+{
> >+    ALOG("ShowAlertNotification");
> >+
> >+    if (nsAppShell::gAppShell)
> >+        nsAppShell::gAppShell->AddObserver(aAlertCookie, aAlertListener);
> >+
> 
> Why would gAppShell be null?

It's nsnull by default, so checking it here just to be safe (this is done in some other functions too). Though if it was null it would probably crash earlier anyway.

Why don't we use methods to return a singleton pointer, which would make sure the object is created and initialized?

> >+nsresult
> >+nsAppShell::AddObserver(const nsAString &aObserverKey, nsIObserver *aObserver)
> >+{
> >+    nsresult result = NS_OK;
> >+    if (aObserver)
> >+        result = mObserversHash.Put(aObserverKey, aObserver) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
> 
> assert if the aObserver is null.  it is a programmatic error, right?

Right. It actually has to be checked before calling AddObserver().

(In reply to comment #35)

> Also, where does "alertfinished" get fired from:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl#68

It was in my first draft, but I removed it as it's not used for Fennec. I guess I'll put it back anyway to make implementation correspond to the interface description.
Alex, thanks for the patch..

use of a number without a description is a so called magic number.  I hate them.  If it's suppose to be unique in the app, say that... Add a comment and everyone is better off.

> Why don't we use methods to return a singleton pointer, which would make sure
the object is created and initialized?

not sure, maybe a good idea in a follow up patch?

> It was in my first draft, but I removed it as it's not used for Fennec. I guess
I'll put it back anyway to make implementation correspond to the interface
description.

Yeah, good idea to code against the interface docs, and not worry about fennec... but that idea usually doesn't work all of the time! :p
(In reply to comment #37)
> use of a number without a description is a so called magic number.  I hate
> them.  If it's suppose to be unique in the app, say that... Add a comment and
> everyone is better off.

Yes, "magic numbers" are bad. I'll add a comment to clarify.

> > Why don't we use methods to return a singleton pointer, which would make sure
> > the object is created and initialized?
>
> not sure, maybe a good idea in a follow up patch?

I mean - what is Mozilla standard coding practice working with singletons?
Attached patch Notifications (mobile) v5 (obsolete) — Splinter Review
Updated to the latest code.
Attachment #455797 - Attachment is obsolete: true
Attachment #457261 - Flags: review+
Attached patch Notifications v6 (obsolete) — Splinter Review
- Fixed review comments;
- Removed unneeded imports in Java;
- Changed nsAlertsService to support the new way of component registration;
- Fixed nsJNIString.
Attachment #455798 - Attachment is obsolete: true
Attachment #457262 - Flags: review?(blassey.bugs)
(In reply to comment #39)
> Created attachment 457261 [details] [diff] [review]
> Notifications (mobile) v5
> 
> Updated to the latest code.

Alex, if you're carrying a review you should note it in the comment
Comment on attachment 457262 [details] [diff] [review]
Notifications v6

>+import org.mozilla.gecko.R;
please add a comment describing what R is

>+        Uri cookieUri = Uri.parse(alertCookie);
>+        String scheme = cookieUri.getScheme();
>+
>+        if (scheme != null) {
>+            scheme = scheme.toLowerCase();
>+            if (scheme.equals("file"))
>+                icon = R.drawable.alertdownloads;
>+            else if (scheme.equals("addon"))
>+                icon = R.drawable.alertaddons;
>+        }
I assume that this cookie is coming directly through the alerts service. If so, this is a bit broken in that the cookie can be anything that the caller wants. A file uri is more than reasonable, so we may be treating non-downloads as downloads.
>-classes.dex: $(JAVAFILES) App.java Restarter.java
>+classes.dex: $(JAVAFILES) App.java Restarter.java NotificationHandler.java R.java
> 	$(NSINSTALL) -D classes
>-	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES)) App.java Restarter.java
>+	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES)) App.java Restarter.java NotificationHandler.java R.java
> 	$(DX) --dex --output=$@ classes
> 
>-AndroidManifest.xml App.java Restarter.java : % : %.in
>+AndroidManifest.xml App.java Restarter.java NotificationHandler.java : % : %.in

Please put "App.java Restarter.java NotificationHandler.java R.java" into a variable, the list has gotten long enough to be error prone. 

Or perhaps just putting the preprocessed files in a macro would be good enough since R.java is produced differently.

>+        } else if (scheme.equals("file")) {
>+            // Application was closed, reopen it to view the file
>+            appIntent.setAction(Intent.ACTION_VIEW);
>+            appIntent.setData(cookieUri);
>+        }

same potential issue with the reliance on the file protocol

>diff --git a/toolkit/components/alerts/src/android/Makefile.in b/toolkit/components/alerts/src/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/Makefile.in
>@@ -0,0 +1,58 @@
>+#! gmake
>+# 
drop this


>+static const mozilla::Module::CIDEntry kAlertsCIDs[] = {
>+  { &kNS_ALERTSSERVICE_CID, false, NULL, nsAlertsServiceConstructor },
>+  { NULL }
>+};
>+
>+static const mozilla::Module::ContractIDEntry kAlertsContracts[] = {
>+  { NS_ALERTSERVICE_CONTRACTID, &kNS_ALERTSSERVICE_CID },
>+  { NULL }
>+};
>+
>+static const mozilla::Module kAlertsModule = {
>+  mozilla::Module::kVersion,
>+  kAlertsCIDs,
>+  kAlertsContracts
>+};
I think there are macros to do some of this for you. If so, use them please
>diff --git a/toolkit/components/alerts/src/android/nsAlertsService.h b/toolkit/components/alerts/src/android/nsAlertsService.h
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/nsAlertsService.h
Init() is an empty function and not called anywhere that I can see. Please drop it.

With the exception of Init() this looks to be the same as toolkit/components/alerts/src/nsAlertsService.h.  Rather than duplicate the header, let's just use the old one.  You can also just have nsAlertsServiceAndroid.cpp in toolkit/components/alerts/src/ and ifdef the CPPSRCS in toolkit/components/alerts/src/Makefile.in, which would eliminate the new subdir and the need for ALERTS_SERVICE_ANDROID and any changes to toolkit/components/build/Makefile.in and toolkit/components/alerts/Makefile.in

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h
>--- a/widget/src/android/AndroidBridge.h
>+++ b/widget/src/android/AndroidBridge.h
>@@ -38,25 +38,36 @@
> #ifndef AndroidBridge_h__
> #define AndroidBridge_h__
> 
> #include <jni.h>
> #include <android/log.h>
> 
> #include "nsCOMPtr.h"
> #include "nsIRunnable.h"
>+#include "nsIObserver.h"
> 
> #include "AndroidJavaWrappers.h"
> 
> #include "nsVoidArray.h"
> 
> // Some debug #defines
> // #define ANDROID_DEBUG_EVENTS
> // #define ANDROID_DEBUG_WIDGET
> 
>+//#define FORCE_ALOG 1
remove this

>+#ifndef ALOG
>+#if defined(DEBUG) || defined(FORCE_ALOG)
>+#define ALOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gecko" , ## args)
>+#else
>+#define ALOG(args...)
>+#endif
>+#endif
>+

And too this please (move it back to AndroidBridge.cpp). For the most part logging macros should be defined in source files or explicit logging headers.

r- mostly for my concerns with using the file:// protocol to distinguish downloads. The rest should be easily fixable nits.
Attachment #457262 - Flags: review?(blassey.bugs) → review-
(In reply to comment #42)

> >+import org.mozilla.gecko.R;
> please add a comment describing what R is

This import is actually not needed anymore as R is now in the same package.
I had already removed it, just somehow missed it in this patch.

> I assume that this cookie is coming directly through the alerts service. If so,
> this is a bit broken in that the cookie can be anything that the caller wants.
> A file uri is more than reasonable, so we may be treating non-downloads as
> downloads.

The simplest improvement here would be to switch back to the "download:", as it was originally, though it won't address the main concern about using the cookie to distinguish between the alert types.

> >+//#define FORCE_ALOG 1
> remove this
> 
> >+#ifndef ALOG
> >+#if defined(DEBUG) || defined(FORCE_ALOG)
> >+#define ALOG(args...)  __android_log_print(ANDROID_LOG_INFO, "Gecko" , ## args)
> >+#else
> >+#define ALOG(args...)
> >+#endif
> >+#endif
> >+
> 
> And too this please (move it back to AndroidBridge.cpp). For the most part
> logging macros should be defined in source files or explicit logging headers.

It was in AndroidJavaWrappers.h, I just moved it to another header to make it available in AndroidJNI.cpp as well (where __android_log_print was used directly).
Attached patch Notifications v7 (obsolete) — Splinter Review
How about this version?

- Propagated the imageUrl parameter of nsIAlertsService::ShowAlertNotification() method to Java to specify "downloads" or "addons" icon;
- Made the cookie parameter blind as per the interface definition (just to distinguish between different notifications);
- Got rid of android/nsAlertsService.cpp, put the #ifdef'd call to AndroidBridge::ShowAlertNotification() into the generic nsAlertsService.cpp.

I would like to reuse the existing interface while it's still possible until we'll really need new functionality.
Attachment #457262 - Attachment is obsolete: true
Attachment #457755 - Flags: review?(blassey.bugs)
Attached patch Notifications (mobile) v6 (obsolete) — Splinter Review
- Provide the imageUrl parameter to specify the icon;
- Use "download:" scheme instead of "file:" in the cookie to reduce possibility of a clash with other file-related notifications.
Attachment #457261 - Attachment is obsolete: true
Attachment #457756 - Flags: review?(blassey.bugs)
Comment on attachment 457755 [details] [diff] [review]
Notifications v7

As discussed on irc, let's get a patch that just does notifications with a default icon and doesn't do any hackery based on the cookie and handle specifying an icon in a follow up bug.
Attachment #457755 - Flags: review?(blassey.bugs) → review-
Attachment #457756 - Flags: review?(blassey.bugs) → review-
Attachment #457755 - Flags: review- → review?(blassey.bugs)
Attachment #457756 - Flags: review- → review?(blassey.bugs)
Comment on attachment 457755 [details] [diff] [review]
Notifications v7

I think it might be better to pass the cookie as an extra rather than as data, as extras are a little more general purpose and not used by the system to select and intent. Just use "cookie" as your extra's key.

+    const nsPromiseFlatString& imageUrl = PromiseFlatString(aImageUrl);
+    const nsPromiseFlatString& alertTitle = PromiseFlatString(aAlertTitle);
+    const nsPromiseFlatString& alertText = PromiseFlatString(aAlertText);
+    const nsPromiseFlatString& alertCookie = PromiseFlatString(aAlertCookie);

Why are you using nsPromiseFlatString here? I don't think its necessary, so please drop or add a comment as to why they're needed.

+#ifdef ANDROID
+
+#include "AndroidBridge.h"
+
+#else

nit, unnecessary new lines

r+ with those fixed.
Attachment #457755 - Flags: review?(blassey.bugs) → review+
Comment on attachment 457756 [details] [diff] [review]
Notifications (mobile) v6

>diff --git a/app/android/drawable-hdpi/alertaddons.png b/app/android/drawable-hdpi/alertaddons.png
>new file mode 100644

>diff --git a/app/android/drawable-hdpi/alertdownloads.png b/app/android/drawable-hdpi/alertdownloads.png
>new file mode 100644

>diff --git a/app/android/drawable/alertaddons.png b/app/android/drawable/alertaddons.png
>new file mode 100644

>diff --git a/app/android/drawable/alertdownloads.png b/app/android/drawable/alertdownloads.png
>new file mode 100644
madhava should ui-review these


>+#ifdef ANDROID
>+const URI_GENERIC_ICON_DOWNLOAD = "res://drawable/alertdownloads";
>+#else
> const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
>+#endif

perhaps we should have a protocol that indicates an android drawable resource rather than munge it into the existing resource protocol. In the very least the platform code should be checking for the whole path.

>+#ifdef ANDROID
>+        cookie = download.target.spec.replace("file:", "download:");
>+#endif

>+#ifdef ANDROID
>+        cookie = download.target.spec.replace("file:", "download:");
>+#endif

why are you changing the protocol of this? I'd prefer you didn't.



>+#ifdef ANDROID
>+const URI_GENERIC_ICON_XPINSTALL = "res://drawable/alertaddons";
>+#else
> const URI_GENERIC_ICON_XPINSTALL = "chrome://browser/skin/images/alert-addons-30.png";
>+#endif

same comment about a drawable protocol as above


>+#ifdef ANDROID
>+    cookie = "addons";
>+#endif

why? this doesn't seem to be used in the platform patch
Attachment #457756 - Flags: review?(blassey.bugs) → review-
Comment on attachment 457756 [details] [diff] [review]
Notifications (mobile) v6

Drive-by:

I assume the images are placeholders for now. Final ones should come from Martell.

I still don't like passing the file in the cookie. In the future, I would like avoid passing the file at all. The user can click on the alert and Fennec can handle opening the file using an "intent" - this does not need to be done in the Android alert service itself.

Is the cookie for addons needed anymore? If not, remove it.
(In reply to comment #47)
> 
> I think it might be better to pass the cookie as an extra rather than as data,
> as extras are a little more general purpose and not used by the system to
> select and intent. Just use "cookie" as your extra's key.

I agree - an extra could be better here, and my first attempt was actually to use an extra, but I experienced some weird issues with that. Seems like they are stored and passed with an intent some special way, and for some reason the subsequent notifications were somehow getting the same extras as the first one. Using data didn't have this problem, so I switched to it. I'll probably make another approach to this, because the behavior I faced with extras was definitely wrong.

> +    const nsPromiseFlatString& alertCookie = PromiseFlatString(aAlertCookie);
> 
> Why are you using nsPromiseFlatString here? I don't think its necessary, so
> please drop or add a comment as to why they're needed.

What is a better way to get a char buffer from nsAString?


(In reply to comment #48)
> >diff --git a/app/android/drawable/alertdownloads.png b/app/android/drawable/alertdownloads.png
> >new file mode 100644
> madhava should ui-review these

Madhava approved these as a draft version. The final icons will be submitted later to the bug 570514.

> >+#ifdef ANDROID
> >+const URI_GENERIC_ICON_DOWNLOAD = "res://drawable/alertdownloads";
> >+#else
> > const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
> >+#endif
> 
> perhaps we should have a protocol that indicates an android drawable resource
> rather than munge it into the existing resource protocol. In the very least the
> platform code should be checking for the whole path.

Well, we don't have a standard for this right now, and currently Java cares only about "download" and "addon" icon types, so JS code should just have these words in the image URL. As there is no way to convert passed strings to Java constant names I think it's more robust just to look for these words rather than compare to the whole string. I used "res://drawable/alertdownloads" only to make it look similar to the constants used in Java code and give an idea, which actual images will be used.

> >+#ifdef ANDROID
> >+        cookie = download.target.spec.replace("file:", "download:");
> >+#endif
>
> why are you changing the protocol of this? I'd prefer you didn't.

The cookie string is used to generate a notification ID, which is the string hashCode. As I mentioned in the patch comment, I changed it to "download" to reduce possibility of a clash with other file-related notifications.

Suppose an addon shows an additional notification about the same downloaded file, and provides its name in the cookie (which is most obvious). This file path will be hashed, and the generated ID would be the same as the download notification ID (if it also used the same "file:" path), so the addon's notification would replace the download one, which we probably don't want.

> >+#ifdef ANDROID
> >+    cookie = "addons";
> >+#endif
> 
> why? this doesn't seem to be used in the platform patch

It is also used to generate an addon-manager-specific notification ID. Otherwise it will clash with other notifications, which provide an empty string in the cookie.


(In reply to comment #49)
> I assume the images are placeholders for now. Final ones should come from
> Martell.

Exactly!

> I still don't like passing the file in the cookie. In the future, I would like
> avoid passing the file at all. The user can click on the alert and Fennec can
> handle opening the file using an "intent" - this does not need to be done in
> the Android alert service itself.

This latest implementation of the alert service does not do anything with the file - it only calls the observer, which opens the download manager. The file name is still important though - it is used to generate a unique per-file notification ID, so that different downloaded files are shown in different notifications.

> Is the cookie for addons needed anymore? If not, remove it.

Same here - the "addons" cookie string is hashed and used as a unique notification ID for the addon-manager notifications. Right now it's only one for all addons though.
> > >+#ifdef ANDROID
> > >+        cookie = download.target.spec.replace("file:", "download:");
> > >+#endif
> >
> > why are you changing the protocol of this? I'd prefer you didn't.
> 
> The cookie string is used to generate a notification ID, which is the string
> hashCode. As I mentioned in the patch comment, I changed it to "download" to
> reduce possibility of a clash with other file-related notifications.

>> Is the cookie for addons needed anymore? If not, remove it.
> 
> Same here - the "addons" cookie string is hashed and used as a unique
> notification ID for the addon-manager notifications. Right now it's only one
> for all addons though.

we should probably hash on more than just the cookie then, since the cookie isn't required and is likely to be an empty string for a lot of notifications.

> What is a better way to get a char buffer from nsAString?

mwu and I debated this for a while, I think nsDependentString is the way to go, although nsPromiseFlatString will probably work as well. Might be worth asking bsmedberg.

> > >+#ifdef ANDROID
> > >+const URI_GENERIC_ICON_DOWNLOAD = "res://drawable/alertdownloads";
> > >+#else
> > > const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
> > >+#endif
> > 
> > perhaps we should have a protocol that indicates an android drawable resource
> > rather than munge it into the existing resource protocol. In the very least the
> > platform code should be checking for the whole path.
> 
> Well, we don't have a standard for this right now, and currently Java cares
> only about "download" and "addon" icon types, so JS code should just have these
> words in the image URL. As there is no way to convert passed strings to Java
> constant names I think it's more robust just to look for these words rather
> than compare to the whole string. I used "res://drawable/alertdownloads" only
> to make it look similar to the constants used in Java code and give an idea,
> which actual images will be used.

the problem with this is that any resource url with download in it would trigger the download behavior.  drawable://download and drawable://addon would avoid that possibility.
(In reply to comment #51)
> > The cookie string is used to generate a notification ID, which is the string
> > hashCode. As I mentioned in the patch comment, I changed it to "download" to
> > reduce possibility of a clash with other file-related notifications.
> 
> we should probably hash on more than just the cookie then, since the cookie
> isn't required and is likely to be an empty string for a lot of notifications.

It is not obvious, what exactly such ID depends on, for example, title and text should not be used, as in case of downloads they are different for download start and finish, but the latter should override the prior one, i.e. have the same ID. It probably makes sense to use imageUrl+cookie, but again, for example download start may have a different icon than the finish.

What we really need here is a notification ID, but I don't think it makes sense for the systems other than Android, where it is actually a very OS-specific.

I guess I should add a comment somewhere regarding the role of the cookie on Android.

> > What is a better way to get a char buffer from nsAString?
> 
> mwu and I debated this for a while, I think nsDependentString is the way to go,
> although nsPromiseFlatString will probably work as well. Might be worth asking
> bsmedberg.

Will do.
Meanwhile it sounds like nsPromiseFlatString is not that bad.

> > Well, we don't have a standard for this right now, and currently Java cares
> > only about "download" and "addon" icon types, so JS code should just have these
> > words in the image URL. As there is no way to convert passed strings to Java
> > constant names I think it's more robust just to look for these words rather
> > than compare to the whole string. I used "res://drawable/alertdownloads" only
> > to make it look similar to the constants used in Java code and give an idea,
> > which actual images will be used.
> 
> the problem with this is that any resource url with download in it would
> trigger the download behavior.

_If_ the "res:" scheme will be used for something else, then yes.

Anyway, this was open for discussion, and seems like we've come to an agreement.

> drawable://download and drawable://addon would avoid that possibility.

Works for me. Originally I wanted to use "res://download", but this looks better.
Attached patch Notifications v8 (obsolete) — Splinter Review
Fixed the nits except the one regarding using an extra. Still facing the same weird behavior when the extra from the first notification somehow gets propagated to the subsequent notification intents. Switched back to using data, which works properly.

r=blassey
Attachment #457755 - Attachment is obsolete: true
Attachment #458570 - Flags: review+
Attached patch Notifications (mobile) v7 (obsolete) — Splinter Review
Using the "drawable:" scheme for the Android notification image URLs.
Attachment #457756 - Attachment is obsolete: true
Attachment #458572 - Flags: review?(blassey.bugs)
(In reply to comment #53)
> Created attachment 458570 [details] [diff] [review]
> Notifications v8
> 
> Fixed the nits except the one regarding using an extra. Still facing the same
> weird behavior when the extra from the first notification somehow gets
> propagated to the subsequent notification intents. Switched back to using data,
> which works properly.
> 
> r=blassey

Can you post a patch with the code you're using to set extras. This behavior doesn't sound right and I'd like to understand it.
You're still hashing on the cookie, which I still don't think is a good idea. There is no reason that this id needs to be a hash of anything.  You could simply use the order of the notifications as the id and add them as an extra to your notification intent.
(In reply to comment #56)
> You're still hashing on the cookie, which I still don't think is a good idea.
> There is no reason that this id needs to be a hash of anything.  You could
> simply use the order of the notifications as the id and add them as an extra to
> your notification intent.

Using the cookie is effective for updating notifications so that we don't leave a bunch of notifications sitting around for the same thing. A counter is good for notifications without a real cookie.
(In reply to comment #55)
> Can you post a patch with the code you're using to set extras. This behavior
> doesn't sound right and I'd like to understand it.

Attached the patch, which goes on top of the "Notifications v8".
I should be missing something obvious, but I've double checked - when using an extra instead of the data, and have two notifications, the second notification gets the same extra as the first one.
This could be related to PendingIntent.getActivity(), but using data doesn't cause this behavior.
(In reply to comment #57)
> (In reply to comment #56)
> > You're still hashing on the cookie, which I still don't think is a good idea.
> > There is no reason that this id needs to be a hash of anything.  You could
> > simply use the order of the notifications as the id and add them as an extra to
> > your notification intent.
> 
> Using the cookie is effective for updating notifications so that we don't leave
> a bunch of notifications sitting around for the same thing.

That's right. We provide the same cookie (file name) for download start and finish, which makes the same notification ID, so when download ends, the first notification for this file gets updated.

A similar behavior is for the addons - if several addons are installed, there is only one notification, as we provide the same cookie for them.

> A counter is good for notifications without a real cookie.

Yes, this might make sense.
Attachment #458570 - Flags: review?(doug.turner)
Attachment #458572 - Flags: review?(blassey.bugs) → review?(doug.turner)
Comment on attachment 458570 [details] [diff] [review]
Notifications v8


>+        if ("drawable".equals(scheme)) {
>+            String resource = imageUri.getSchemeSpecificPart().toLowerCase();

.getPath() instead.

>+            if ("//downloads".equals(resource))
>+                icon = R.drawable.alertdownloads;
>+            else if ("//addons".equals(resource))
>+                icon = R.drawable.alertaddons;
>+        }
>+

>+        int notificationID = alertCookie.hashCode();

this really needs to be the name of the notification.  See:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl#59



>+    public static void handleNotification(String alertCookie) {
>+        Log.i("GeckoAppJava", "GeckoAppShell.handleNotification: Calling the observer; " +
>+            "topic = 'alertclickcallback', alertCookie = '" + alertCookie + "'");
>+
>+        callObserver(alertCookie, "alertclickcallback", alertCookie);
>+        callObserver(alertCookie, "alertfinished", alertCookie);
>+        removeObserver(alertCookie);

I don't understand this.  is handleNotification called only when the user clicks on the notification?


I am also very confused about the use of the word cookie throughout this patch.  Cookie, in the sense of the alert interface, is exclusively used to describe the bit of context that callers can give which will be returned to them in the callbacks.  It probably should have been called context.

I think you are using the term to describe something that will allow you to only have one notification type at a time.  In the alert interface, this term is known as name.  Take a look at that interface, and feel free to update it as after you land this patch (after it being reviewed), both Growl and Android will support it.
Attachment #458570 - Flags: review?(doug.turner) → review-
Comment on attachment 458572 [details] [diff] [review]
Notifications (mobile) v7

+        let cookie = "";
+#ifdef ANDROID
+        cookie = download.target.spec.replace("file:", "download:");
+#endif

Wrong use of cookie here.  You want the last parameter.  In fact, you can use it there without any of the #ifdefs.  Please read:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl#59

Also split out the binary diffs.  no one is going to review those like that.  Attach PNGs, and mark them ui-review.  madhava will be able to work with them like that.
Attachment #458572 - Flags: review?(doug.turner) → review-
(In reply to comment #61)
> >+        if ("drawable".equals(scheme)) {
> >+            String resource = imageUri.getSchemeSpecificPart().toLowerCase();
> 
> .getPath() instead.

getPath() for our URIs (like "drawable://downloads") will return an empty string.
getSchemeSpecificPart() returns the part after ":" - exactly what we need here.

> >+        int notificationID = alertCookie.hashCode();
> 
> this really needs to be the name of the notification.

This totally makes sense to me.

> I don't understand this.  is handleNotification called only when the user
> clicks on the notification?

Yes. handleNotification is called when the user opens the notifications panel and clicks on our notification. The notification hides from the status bar, and the observer gets called.

> I am also very confused about the use of the word cookie throughout this patch.

The original idea of using the cookie was different, now after all the changes, we need something to serve only as an ID, and I absolutely agree the name parameter is the one to use for this.
Attached patch Notifications v9 (obsolete) — Splinter Review
- Using name parameter instead of cookie to identify alerts;
- Updated to the latest code.
Attachment #458570 - Attachment is obsolete: true
Attachment #460454 - Flags: review?(doug.turner)
Attached patch Notifications (mobile) v8 (obsolete) — Splinter Review
- Using name parameter instead of cookie to identify alerts;
- Updated to the latest code;
- Moved binary icons to a separate patch.
Attachment #458572 - Attachment is obsolete: true
Attachment #460455 - Flags: review?(doug.turner)
Attached patch Draft version of the icons (obsolete) — Splinter Review
The draft version of the icons (binaries are attached to the bug 570514) required by the notification-handling code.

Preliminarily approved by Madhava:

(2010-06-21 15:52:05) alexp: I've attached draft version - could you have a look?
(2010-06-21 15:55:08) madhava: alexp - that's on the right track, iconography-wise
(2010-06-21 15:55:13) madhava: we'll do some in an android style

The final version of the icons will be submitted to the bug 570514.
Comment on attachment 460454 [details] [diff] [review]
Notifications v9

a couple things:

1) cookies are the things that get passed back to observers.  I think the patch still confuses the term "name" and the term "cookie"

2) I am still confused about callObserver.  When the notification is closed, alertfinished needs to be sent.  When the notification is clicked on, the alertclickcallback one needs to be pushed.  I do not see that here.

3)

how does this work?  is alertclickcallback saved somewhere?

+    public static void handleNotification(String alertName, String alertCookie) {
+        Log.i("GeckoAppJava", "GeckoAppShell.handleNotification: Calling the observer:\n" +
+              "- name = '" + alertName + "'\n" +
+              "- topic = 'alertclickcallback'\n" +
+              "- cookie = '" + alertCookie + "'");


4)

+    * @param name           The name of the notification. This is currently
+    *                       only used on OS X with Growl and Android.
+    *                       On OS X with Growl, users can disable notifications
+    *                       with a given name. On Android the name is hashed
+    *                       and used as a notification ID.

change this to:

The name of the notification.  the name should be unique ID for the notification. It can be used for duplicate detection.  May be null.
Attachment #460454 - Flags: review?(doug.turner) → review-
Comment on attachment 460455 [details] [diff] [review]
Notifications (mobile) v8



>+const URI_GENERIC_ICON_DOWNLOAD = "drawable://downloads";
>+#else
> const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
>+#endif


Can this be a more descriptive string?

would drawable://alert-downloads-30.png work?
(In reply to comment #67)
> 1) cookies are the things that get passed back to observers.  I think the patch
> still confuses the term "name" and the term "cookie"

No. The latest version does exactly this. The cookie is used only to be passed to the observer. The notifications and corresponding observers are identified using the name parameter.

> 2) I am still confused about callObserver.  When the notification is closed,
> alertfinished needs to be sent.  When the notification is clicked on, the
> alertclickcallback one needs to be pushed.  I do not see that here.

GeckoAppShell.handleNotification() does exactly this.
Both "alertclickcallback" and "alertfinished" are sent when the user clicks on the notification, because it's supposed to hide when clicked. That's how Android notifications work.

> 3)
> 
> how does this work?  is alertclickcallback saved somewhere?
> 
> +    public static void handleNotification(String alertName, String
> alertCookie) {
> +        Log.i("GeckoAppJava", "GeckoAppShell.handleNotification: Calling the
> observer:\n" +
> +              "- name = '" + alertName + "'\n" +
> +              "- topic = 'alertclickcallback'\n" +
> +              "- cookie = '" + alertCookie + "'");

The next two lines after this writing to the log call the observer:
368  	callObserver(alertName, "alertclickcallback", alertCookie);
369 	callObserver(alertName, "alertfinished", alertCookie);

(In reply to comment #68)
> Comment on attachment 460455 [details] [diff] [review]
> Notifications (mobile) v8
> 
> >+const URI_GENERIC_ICON_DOWNLOAD = "drawable://downloads";
> >+#else
> > const URI_GENERIC_ICON_DOWNLOAD = "chrome://browser/skin/images/alert-downloads-30.png";
> >+#endif
> 
> 
> Can this be a more descriptive string?
> 
> would drawable://alert-downloads-30.png work?

Anything would work. This string is not used "as-is", it's just checked to find out what exact R.drawable.xxx constant to use in Java code.
Originally I used "res://drawable/alertdownloads", which resembles the actual constant name used in Java. Brad suggested to shorten it.
We could use "drawable://alertdownloads", which would correspond to R.drawable.alertdownloads constant.

Note that the icon resource file names, which the Java constant names are automatically generated from, should not have dashes, and better not specify the pixel size, because that size will be different depending on the device screen DPI. That's why the icon files are named "alertdownloads.png" and "alertaddons.png".
tracking-fennec: 2.0+ → 2.0b1+
Attachment #460455 - Flags: review?(doug.turner) → review?(blassey.bugs)
Attachment #460454 - Flags: review- → review?(blassey.bugs)
Comment on attachment 460454 [details] [diff] [review]
Notifications v9


>+
>+        if ("drawable".equals(scheme)) {
>+            String resource = imageUri.getSchemeSpecificPart().toLowerCase();

why toLowerCase() ?

>+            if ("//downloads".equals(resource))
>+                icon = R.drawable.alertdownloads;
>+            else if ("//addons".equals(resource))
>+                icon = R.drawable.alertaddons;
>+        }

let's match the uri up with the the resource name (so drawable://alertdownloads)

>+
>+        int notificationID = alertName.hashCode();

still question if the a hash on the name is the best thing to get an id from. But I guess we can go with it for now and make changes if it proves problematic
>+    public static void handleNotification(String alertName, String alertCookie) {
>+        Log.i("GeckoAppJava", "GeckoAppShell.handleNotification: Calling the observer:\n" +
>+              "- name = '" + alertName + "'\n" +
>+              "- topic = 'alertclickcallback'\n" +
>+              "- cookie = '" + alertCookie + "'");
>+
>+        callObserver(alertName, "alertclickcallback", alertCookie);
>+        callObserver(alertName, "alertfinished", alertCookie);

https://developer.mozilla.org/en/nsIAlertsService makes this sound like an either/or. We'll have to figure out if this was clicked on or was cleared and send the appropriate

>+    protected void handleIntent(Intent notificationIntent) {
>+        String alertName = "";
>+        String alertCookie = "";
>+        Uri data = notificationIntent.getData();
>+        if (data != null) {
>+            alertName = data.getSchemeSpecificPart();
>+            alertCookie = data.getFragment();
>+            if (alertCookie == null)
>+                alertCookie = "";
>+        }
>+
>+        Log.i("GeckoAppJava", "NotificationHandler.handleIntent\n" +
>+              "- alertName = '" + alertName + "'\n" +
>+              "- alertCookie = '" + alertCookie + "'");
>+
>+        int notificationID = alertName.hashCode();
>+
>+        Log.i("GeckoAppJava", "Handle notification ID " + notificationID);
>+
>+        NotificationManager notificationManager = (NotificationManager) getSystemService(NOTIFICATION_SERVICE);
>+        notificationManager.cancel(notificationID);
>+
>+        if (App.mAppContext != null) {
>+            // This should call the observer, if any
>+            App.mAppContext.handleNotification(alertName, alertCookie);
>+        }
>+
>+        // Start or bring to front the main activity
>+        Intent appIntent = new Intent(Intent.ACTION_MAIN);
>+        appIntent.setClassName(this, "org.mozilla.@MOZ_APP_NAME@.App");
>+        try {
>+            startActivity(appIntent);
>+        } catch (ActivityNotFoundException e) {
>+            Log.e("GeckoAppJava", "NotificationHandler Exception: " + e.getMessage());
>+        }
>+    }
>+}

presumably this is where you need to figure out if the notification was clicked or cleared.


>diff --git a/toolkit/components/alerts/src/android/Makefile.in b/toolkit/components/alerts/src/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/alerts/src/android/Makefile.in

this seems to not be used anymore



r- because the alertclickcallback/alertfinished thing. Otherwise this it looks good, good work.
Attachment #460454 - Flags: review?(blassey.bugs) → review-
Comment on attachment 460455 [details] [diff] [review]
Notifications (mobile) v8


>+#ifdef ANDROID
>+const URI_GENERIC_ICON_DOWNLOAD = "drawable://downloads";
>+#else

as mentioned in the other review, make this match the drawable name (alertdownloads)

>@@ -417,28 +421,30 @@ var DownloadsView = {
>         return;
> 
>       let download = aSubject.QueryInterface(Ci.nsIDownload);
>       let strings = Elements.browserBundle;
>       var notifier = Cc["@mozilla.org/alerts-service;1"].getService(Ci.nsIAlertsService);
> 
>       if (aTopic == "dl-start") {
>         notifier.showAlertNotification(URI_GENERIC_ICON_DOWNLOAD, strings.getString("alertDownloads"),
>-                                       strings.getFormattedString("alertDownloadsStart", [download.displayName]), false, "", null);
>+                                       strings.getFormattedString("alertDownloadsStart", [download.displayName]), false, "", null,
>+                                       download.target.spec.replace("file:", "download:"));

why are you changing the protocol? As I understood it, before this was so you could identify the alert as a download and change the icon, but that seems unnecessary now.


>         notifier.showAlertNotification(URI_GENERIC_ICON_DOWNLOAD, strings.getString("alertDownloads"),
>-                                       strings.getFormattedString("alertDownloadsDone", [download.displayName]), true, "", observer);
>+                                       strings.getFormattedString("alertDownloadsDone", [download.displayName]), true, "", observer,
>+                                       download.target.spec.replace("file:", "download:"));

same

>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> const PREFIX_ITEM_URI = "urn:mozilla:item:";
> const PREFIX_NS_EM = "http://www.mozilla.org/2004/em-rdf#";
> 
> const PREF_GETADDONS_MAXRESULTS = "extensions.getAddons.maxResults";
> 
>+#ifdef ANDROID
>+const URI_GENERIC_ICON_XPINSTALL = "drawable://addons";
>+#else

same as above, alertaddons


just clearing the review for now since this relies on the platform patch
Attachment #460455 - Flags: review?(blassey.bugs)
(In reply to comment #70)
> >+            String resource = imageUri.getSchemeSpecificPart().toLowerCase();
> 
> why toLowerCase() ?

URI is case-insensitive, so in theory it could use capital letters, but I guess this call is not required here, as we expect a specific string to be hard-coded.

> >+        callObserver(alertName, "alertclickcallback", alertCookie);
> >+        callObserver(alertName, "alertfinished", alertCookie);
> 
> https://developer.mozilla.org/en/nsIAlertsService makes this sound like an
> either/or. We'll have to figure out if this was clicked on or was cleared and
> send the appropriate

Actually both events happen here - the user clicks on the notification, and it gets canceled, and disappears like the other Android notifications.
If we should follow the interface http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl, it looks like the observer should be called with both topics, as Doug mentioned in his review comment above.


(In reply to comment #71)
> >+                       download.target.spec.replace("file:", "download:"));
> 
> why are you changing the protocol? As I understood it, before this was so you
> could identify the alert as a download and change the icon, but that seems
> unnecessary now.

This is to reduce a probability of a clash with other file-related notifications, which could use a standard "file://" URI as a name here. Changing it to "download://" guarantees the uniqueness of the notification name, as it makes it specifically related to downloads only. And now this is just a name anyway, it is not treated as an URI.
Attached patch Notifications v10 (obsolete) — Splinter Review
Fixed review comments.
Attachment #460454 - Attachment is obsolete: true
Attachment #460456 - Attachment is obsolete: true
Attachment #466058 - Flags: review?(blassey.bugs)
Attached patch Notifications (mobile) v9 (obsolete) — Splinter Review
Fixed review comments.
Attachment #460455 - Attachment is obsolete: true
Attachment #466059 - Flags: review?(blassey.bugs)
Please note that the icons from the bug 570514 have to be submitted to mobile-browser before this patch to mozilla-central.
(In reply to comment #72)
> (In reply to comment #70)
> > >+        callObserver(alertName, "alertclickcallback", alertCookie);
> > >+        callObserver(alertName, "alertfinished", alertCookie);
> > 
> > https://developer.mozilla.org/en/nsIAlertsService makes this sound like an
> > either/or. We'll have to figure out if this was clicked on or was cleared and
> > send the appropriate
> 
> Actually both events happen here - the user clicks on the notification, and it
> gets canceled, and disappears like the other Android notifications.
> If we should follow the interface
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/public/nsIAlertsService.idl,
> it looks like the observer should be called with both topics, as Doug mentioned
> in his review comment above.

If the user clicks the clear button we should get the finished message but not the clicked message.  If the user clicks the notification, we should get both messages (assuming the notification is dismissed when we click it, which may not always be true).
Comment on attachment 466058 [details] [diff] [review]
Notifications v10

clearing the review until the callback notification is addressed
Attachment #466058 - Flags: review?(blassey.bugs)
Attachment #466059 - Flags: review?(blassey.bugs)
Updated to the latest code.
Attachment #466059 - Attachment is obsolete: true
Attachment #469504 - Flags: review?(blassey.bugs)
Attached patch Notifications v11 (obsolete) — Splinter Review
Added handling for the "Clear All Notifications" button.
Attachment #458878 - Attachment is obsolete: true
Attachment #466058 - Attachment is obsolete: true
Attachment #469505 - Flags: review?(blassey.bugs)
Attachment #469505 - Flags: review?(blassey.bugs) → review+
Attachment #469504 - Flags: review?(blassey.bugs) → review+
applying attachment.cgi?id=469505
patching file embedding/android/GeckoApp.java
Hunk #1 FAILED at 50
1 out of 2 hunks FAILED -- saving rejects to file embedding/android/GeckoApp.java.rej
patching file embedding/android/GeckoAppShell.java
Hunk #2 FAILED at 419
1 out of 2 hunks FAILED -- saving rejects to file embedding/android/GeckoAppShell.java.rej
patching file embedding/android/Makefile.in
Hunk #1 FAILED at 45
1 out of 2 hunks FAILED -- saving rejects to file embedding/android/Makefile.in.rej
patching file widget/src/android/AndroidBridge.cpp
Hunk #2 FAILED at 103
Hunk #3 FAILED at 356
2 out of 3 hunks FAILED -- saving rejects to file widget/src/android/AndroidBridge.cpp.rej
patching file widget/src/android/AndroidBridge.h
Hunk #2 FAILED at 121
Hunk #3 FAILED at 176
2 out of 3 hunks FAILED -- saving rejects to file widget/src/android/AndroidBridge.h.rej
patch failed, unable to continue (try -v)
hpatch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=469505
Patch has been updated to the latest code.
r=blassey
Attachment #469505 - Attachment is obsolete: true
Attachment #470076 - Flags: review+
Keywords: checkin-needed
verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100831 Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.