Push Notifications wiki: https://wiki.mozilla.org/WebAPI/PushAPI B2G push notifications are tracked in bug 763198.

Chris, I think the proposal Mozilla is working on is https://wiki.mozilla.org/WebAPI/SimplePush

I think this is a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=822712.

(In reply to Kamil Leszczuk from comment #2) > I think this is a duplicate of > https://bugzilla.mozilla.org/show_bug.cgi?id=822712. It's not. This bug tracks the Android work. That bug tracks the b2g work.

Not tracking for A4A

(In reply to Mark Finkle (:mfinkle) from comment #4) > Not tracking for A4A Disagree. This is part of the 2013 objectives, although a P2.

(In reply to Jason Smith [:jsmith] from comment #5) > (In reply to Mark Finkle (:mfinkle) from comment #4) > > Not tracking for A4A > > Disagree. This is part of the 2013 objectives, although a P2. It needs a use case. Implementation will continue, but it won't block A4A.

(In reply to Mark Finkle (:mfinkle) from comment #6) > (In reply to Jason Smith [:jsmith] from comment #5) > > (In reply to Mark Finkle (:mfinkle) from comment #4) > > > Not tracking for A4A > > > > Disagree. This is part of the 2013 objectives, although a P2. > > It needs a use case. Implementation will continue, but it won't block A4A. First, your wrong for treating A4A as a blocker. It's a tracking flag for a program. This is straight out of Rick's doc. Please provide firm evidence from product, otherwise, I think the data disagrees with you. The use case is 3rd party app updates. This is *better* way to implement 3rd party app updates from marketplace pushing an update to the device to notify it to get an update to lower network resources. Ask David Bialer directly - he's the one who wanted this originally straight in the 3rd party app updates bug.

Read https://bugzilla.mozilla.org/show_bug.cgi?id=813753#c4. The point of this flag was a tracking flag for a program, which means we would want to keep on this.

1. _sendRequest() caller expects promise, but channelID error returns undefined. 2. Micro-optimization of UUID string slicing. 3. "PushService:Register:KO" errback refers to nonexistent `message` var.

Is this bug about enabling Push Notifications to our apps running in our Web Runtime our for any web content?

(In reply to Mounir Lamouri (:mounir) from comment #11) > Is this bug about enabling Push Notifications to our apps running in our Web > Runtime or for any web content? Mounir, this bug's goal is to enable Push Notifications to any web content. I don't know what Web Runtime plans to do for Push.

Comment on attachment 744796 [details] [diff] [review] fix-PushService-error-handling.patch simple fix, we probably want to uplift

Part 1: Install Push and WebSocket components on Android. This patch will add some of the XPCOM components we will need for Push on Android, but without enabling Push (the "services.push.enabled" pref).

Comment on attachment 744796 [details] [diff] [review] fix-PushService-error-handling.patch Approving for better error handling.

(In reply to Alex Keybl [:akeybl] from comment #17) > Comment on attachment 744796 [details] [diff] [review] > fix-PushService-error-handling.patch > > Approving for better error handling. Uplifted error handling patch to b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/6295c81a676b

Comment on attachment 761520 [details] [diff] [review] Part 2: Import koush's android-websockets library. r= Add (a subset of) koush's fork of codebutler's android-websockets library: https://github.com/koush/android-websockets/

Part 3: Add Android JSONUtils for parsing URLs and UUIDs.

Comment on attachment 761520 [details] [diff] [review] Part 2: Import koush's android-websockets library. r= Review of attachment 761520 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/websockets/HybiParser.java @@ +333,5 @@ > + return (int) i; > + } > + > + /** > + * Copied from AOSP Arrays.java. I'm assuming this was apache licensed originally, which is fine. Just calling that out. Since we already have Arrays.java from they system, why do we need to copy the code? ::: mobile/android/base/websockets/WebSocketClient.java @@ +1,1 @@ > +package com.codebutler.android_websockets; put a comment for the origin of this code here in lue of a license block

(In reply to Brad Lassey [:blassey] from comment #24) > > + * Copied from AOSP Arrays.java. > > Since we already have Arrays.java from they system, why do we need to copy > the code? The copied code is the Arrays.copyOfRange() method, which was introduced in Android API Level 9.

* Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1779b8d2bf1 * Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/a597da48b937

https://hg.mozilla.org/mozilla-central/rev/c1779b8d2bf1 https://hg.mozilla.org/mozilla-central/rev/a597da48b937

Part 4a: Add JS front-end for Android Push API. AndroidPushService.jsm is a fork of dom/push/src/PushService.jsm that replaces the JS WebSockets, Alarm heartbeats, and UDP socket with Java messages to a Java background service (in a subsequent patch).

Comment on attachment 763069 [details] [diff] [review] part-4a-js-front-end.patch Review of attachment 763069 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good, but it has some extra code that can be removed. In addition I'd like the Java implementation up for review before I r+ this. ::: mobile/android/chrome/content/browser.js @@ +15,5 @@ > Cu.import("resource://gre/modules/AddonManager.jsm"); > Cu.import("resource://gre/modules/FileUtils.jsm"); > Cu.import("resource://gre/modules/JNI.jsm"); > Cu.import('resource://gre/modules/Payment.jsm'); > +Cu.import('resource://gre/modules/AndroidPushService.jsm'); The Firefox Desktop team prefers modules not clutter the global namespace, and instead use components to boot themselves. See bug 871372 for PushService. This may apply for android too. ::: mobile/android/installer/package-manifest.in @@ +128,5 @@ > @BINPATH@/components/dom_indexeddb.xpt > @BINPATH@/components/dom_offline.xpt > @BINPATH@/components/dom_json.xpt > @BINPATH@/components/dom_browserelement.xpt > +@BINPATH@/components/dom_messages.xpt Does system messages work on android? ::: mobile/android/modules/AndroidPushService.jsm @@ +15,5 @@ > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Preferences.jsm"); > +Cu.import("resource://gre/modules/identity/Sandbox.jsm"); As part of the bug 868322, bug 857464 nexus, sandbox & co. will be offloaded to system messages. This is no longer needed. @@ +41,5 @@ > +} > + > +this.PushService = { > + _requests: {}, > + _sandboxes: {}, Same as above. @@ +93,5 @@ > + this._notifyURL(update.wakeupURL, update.pushEndpoint, update.version); > + } > + break; > + > + case "PushService:Notification": Redundant? @@ +119,5 @@ > + throw "observe: unexpected topic: " + topic; > + } > + }, > + > + _updateWebSocketURI: function() { Can this be done in Java? If it is simpler in JS, this is perfectly alright. @@ +130,5 @@ > + debug("_notifyURL: wakeupURL=" + wakeupURL + ", pushEndpoint=" + pushEndpoint + > + ", version=" + version); > + > + let message = { pushEndpoint: pushEndpoint, version: version }; > + this._sendMessageToSandbox('push', message, wakeupURL); Will be replaced with a call to sendMessage() or sendMessageToWorker() based on the type of the page that registered (app/web page) and the parameters passed to push.register() (again, see bug 868322 and 857464). @@ +133,5 @@ > + let message = { pushEndpoint: pushEndpoint, version: version }; > + this._sendMessageToSandbox('push', message, wakeupURL); > + }, > + > + _sendMessageToSandbox: function(type, message, wakeupURL) { This function can be dropped entirely. @@ +158,5 @@ > + Services.obs.notifyObservers(null, "webapps-registry-ready", null); > + > + // Add push permissions to the wakeup URL. > + Services.perms.add(wakeupURI, 'push', Ci.nsIPermissionManager.ALLOW_ACTION, > + Ci.nsIPermissionManager.EXPIRE_SESSION); This should be removed. For webpages, the webpage calling push.register() should ask for permission using nsIContentPermissionPrompt. That will suffice. ::: mobile/android/modules/Makefile.in @@ +9,5 @@ > > include $(DEPTH)/config/autoconf.mk > > EXTRA_JS_MODULES = \ > + AndroidPushService.jsm \ A mach change landed on June 10, 2013 that moves EXTRA_JS_MODULES to moz.build.

Comment on attachment 763069 [details] [diff] [review] part-4a-js-front-end.patch Review of attachment 763069 [details] [diff] [review]: ----------------------------------------------------------------- Finkle would be a better reviewer for this ::: mobile/android/app/mobile.js @@ +612,5 @@ > pref("browser.dom.window.dump.enabled", true); > > // SimplePush > pref("services.push.enabled", false); > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); what is wss:?

(In reply to Brad Lassey [:blassey] from comment #30) > > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > > what is wss:? wss: is a WebSocket Secure connection.

Patch v2 incorporates (some of) Nikhil's feedback: > The Firefox Desktop team prefers modules not clutter the global namespace, > and instead use components to boot themselves. See bug 871372 for > PushService. This may apply for android too. I'm still investigating this.. > > +@BINPATH@/components/dom_messages.xpt > > Does system messages work on android? Not yet. I'm waiting to see how bug 868322 implements system message on Firefox Desktop. > > + case "PushService:Notification": > > Redundant? Fixed. > > + _updateWebSocketURI: function() { > > Can this be done in Java? If it is simpler in JS, this is perfectly alright. Fixed. > > + _sendMessageToSandbox: function(type, message, wakeupURL) { > > This function can be dropped entirely. Removed > This should be removed. For webpages, the webpage calling push.register() > should ask for permission using nsIContentPermissionPrompt. That will > suffice. Removed. > A mach change landed on June 10, 2013 that moves EXTRA_JS_MODULES to > moz.build. I posted Android EXTRA_JS_MODULES patch to bug 880245 and it has been r+'d.

Part 4b: Add Java back-end for Android Push API. A subsequent patch will add support for the Java service responding to battery and network connectivity intents.

Comment on attachment 764818 [details] [diff] [review] part-4a-js-front-end-v2.patch Review of attachment 764818 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/AndroidPushService.jsm @@ +121,5 @@ > + let manifestURI = Services.io.newURI(principal.origin, null, null); > + > + let messenger = Cc["@mozilla.org/system-message-internal;1"] > + .getService(Ci.nsISystemMessagesInternal); > + messenger.sendMessage('push', message, wakeupURI, manifestURI); You'll likely want to use sendMessageToEventPage or similar based on the arguments passed to the register() call once/if 868322 lands. This is fine for now, but will only work with apps.

Comment on attachment 764822 [details] [diff] [review] part-4b-java-back-end-v2.patch Review of attachment 764822 [details] [diff] [review]: ----------------------------------------------------------------- I didn't yet review the PushSocket code, but leaving review comment now. I think we should take the following actions: * Switch to using OrderedBroadcast to trigger the service from Gecko without having to add Yet Another Messaging Channel to GeckoApp. * Switch to using the existing prefs startup broadcast idiom in GeckoApp/GeckoPreferences. I'm open to unifying these if you want to write the dispatcher, or we can leave that until later. * Move all of the new code into android-services, where we can test it and share more with existing background stuff (if it isn't already). * Write JUnit3 tests in android-services to get some code coverage here. I'd be worried about landing this much stuff (including a new DB layer) with no tests. Setting f- for clarity, but please don't take offense! ::: mobile/android/base/GeckoApp.java @@ +696,5 @@ > + } else if ("Push:Ack".equals(event) || > + "Push:Register".equals(event) || > + "Push:Registrations".equals(event) || > + "Push:Unregister".equals(event)) { > + runPushIntentInService(event, message.getString("data")); Did you consider using OrderedBroadcast.jsm (Bug 870371) for the JS->Java communication here? If all you're doing is trying to trigger a service handler from JS, that's a great way to do it. @@ +1533,5 @@ > } > }); > > + PrefsHelper.getPref("services.push.enabled", new PrefsHelper.PrefHandlerBase() { > + @Override public void prefValue(String pref, boolean enabled) { This whole block of code should probably live in GeckoPreferences, alongside GeckoPreferences.broadcastAnnouncementsPref(context); GeckoPreferences.broadcastHealthReportUploadPref(context); which are called a few lines below this. That's the point where we let background services know about Gecko prefs. @@ +1538,5 @@ > + Log.d(LOGTAG, "getPref: " + pref + ": " + enabled); > + if (enabled) { > + PrefsHelper.getPref("services.push.serverURL", new PrefsHelper.PrefHandlerBase() { > + @Override public void prefValue(String pref, String serverURL) { > + Log.d(LOGTAG, "getPref: " + pref + ": " + serverURL); Two layers of logging. Let's kill 'em. ::: mobile/android/base/push/PushRegistrations.java @@ +13,5 @@ > +import android.database.sqlite.SQLiteException; > +import android.util.Log; > + > +import java.net.MalformedURLException; > +import java.net.URL; I think you probably want java.net.URI or the Android Uri class here. (For example: URL does DNS resolution in the process of computing hashCode, and random awfulness like that.) @@ +15,5 @@ > + > +import java.net.MalformedURLException; > +import java.net.URL; > + > +public final class PushRegistrations extends CachedSQLiteOpenHelper { Is a database the right course of action here? What kind of load is this storage layer under (queries for every message? frequent writes?), and what's the expected capacity? I'm concerned about having yet another open DB connection, DB fragmentation, and performance issues that might prompt us to need an in-memory cache regardless. An alternative, if you don't expect millions of registrations, would be to load and serialize to/from a single disk file, having the service simply keep its registry in memory, flushing it to disk when it changes. @@ +77,5 @@ > + SQLiteDatabase db = this.getCachedWritableDatabase(); > + onUpgrade(db, SCHEMA_VERSION, SCHEMA_VERSION); > + } > + > + public void insertChannel(String channelID, URL pageURL, URL wakeupURL, URL pushEndpoint) { You probably want defensive null checks and validity checks here. @@ +78,5 @@ > + onUpgrade(db, SCHEMA_VERSION, SCHEMA_VERSION); > + } > + > + public void insertChannel(String channelID, URL pageURL, URL wakeupURL, URL pushEndpoint) { > + Log.d(LOGTAG, "insertChannel: channelID=" + channelID); I can imagine this being classed as PII. If you were doing all this in android-services, driven by an ordered broadcast, I'd suggest using Logger.pii… @@ +81,5 @@ > + public void insertChannel(String channelID, URL pageURL, URL wakeupURL, URL pushEndpoint) { > + Log.d(LOGTAG, "insertChannel: channelID=" + channelID); > + > + ContentValues cv = new ContentValues(4); > + cv.put(COL_CHANNEL_ID, channelID.toString()); channelID is already a string. @@ +99,5 @@ > + public void deleteChannel(String channelID) { > + Log.d(LOGTAG, "deleteChannel: channelID=" + channelID); > + > + String where = COL_CHANNEL_ID + "=?"; > + String[] whereArgs = { channelID.toString() }; channelID is already a string. @@ +106,5 @@ > + int rowsDeleted = db.delete(TBL_PUSH_REGISTRATIONS, where, whereArgs); > + > + if (rowsDeleted != 0 && rowsDeleted != 1) { > + Log.e(LOGTAG, "", new SQLiteException("delete(channelID=" + channelID > + + ") returned rowsDeleted=" + rowsDeleted)); You probably want IllegalStateException here. `delete` will throw its own SQLiteException if there's a real SQL problem, so what you mean is "this should have deleted one or zero rows, but the DB somehow didn't have zero or one row to delete". @@ +134,5 @@ > + return select(COL_PAGE_URL, pageURL.toString()); > + } > + > + public Record fetchByChannel(String channelID) { > + return selectOne(COL_CHANNEL_ID, channelID.toString()); No need for toString(). @@ +168,5 @@ > + int rowsUpdated = db.update(TBL_PUSH_REGISTRATIONS, cv, where, whereArgs); > + > + if (rowsUpdated != 1) { > + Log.e(LOGTAG, "", new SQLiteException("update(channelID=" + channelID > + + ") returned rowsUpdated=" + rowsUpdated)); Again, you probably want IllegalStateException or IllegalArgumentException. @@ +213,5 @@ > + null, null, "1"); > + > + try { > + int len = (cursor.moveToFirst() ? cursor.getCount() : 0); > + Record[] records = new Record[len]; This should share code with `fetchAll`. @@ +227,5 @@ > + } > + } > + > + private Record selectOne(String columnName, String columnValue) { > + Record[] records = select(columnName, columnValue); Might want to add a LIMIT 2 here. ::: mobile/android/base/push/PushService.java @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +package org.mozilla.gecko.background.push; If it's in `background`, it should be in android-services (if it isn't already). This is probably true anyway; you don't need tight coupling with GeckoApp, so let's take advantage of testability and better tools. @@ +30,5 @@ > +public final class PushService extends SimpleService { > + private static final String LOGTAG = "GeckoPushService"; > + private static final boolean DEBUG = PushSocket.DEBUG; > + > + private static final String PREFS_BRANCH = AppConstants.ANDROID_PACKAGE_NAME + ".background.push"; I would generally recommend sharing the background prefs file with FHR and Product Announcements, and prefixing your pref names. SharedPreferences is a filesystem access, so let's do it as few times as possible. @@ +58,5 @@ > + } > + > + @Override > + public void onDestroy() { > + Log.d(LOGTAG, "onDestroy: " + this); Kill superfluous logging. @@ +76,5 @@ > + Log.d(LOGTAG, "onHandleIntent: " + type + ", data=" + data); > + } > + > + if ("Push:WebSocketURI".equals(type)) { > + updateWebSocketURI(data); What happens when this throws? Does the worker thread die, restart, catch? @@ +195,5 @@ > + > + private void onPushRegistrations(JSONObject request) { > + URL pageURL = JSONUtils.getURL("pageURL", request); > + if (DEBUG) { > + Log.d(LOGTAG, "onPushRegistrations: pageURL=" + pageURL); Don't log page URLs except for with Logger.pii(). @@ +261,5 @@ > + } > + } > + } > + > + private void connectSocket() { This should be synchronized, or you can get a race between onCreate and the first onHandleIntent. @@ +299,5 @@ > + } > + }); > + } > + > + private void disconnectSocket() { synchronized. @@ +319,5 @@ > + int i = 0; > + for (Record record : records) { > + channelIDs[i] = record.channelID; > + if (DEBUG) { > + Log.d(LOGTAG, "fetchAll: #" + i PII or delete. @@ +521,5 @@ > + Log.d(LOGTAG, "startGeckoActivity: messageType=" + messageType + ", data=" + data); > + } > + > + Intent intent = new Intent(ACTION_PUSHSERVICE_RESPONSE); > + intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME, AppConstants.ANDROID_PACKAGE_NAME + ".App"); ISTR that we have a constant for this in BackgroundConstants.

Tests pass locally; haven't tried on try yet.

nalexander: is this change what you were imagining for handling null and undefined OrderedBroadcast permissions? This new code probably needs a couple tests, too. <:)

Comment on attachment 766889 [details] [diff] [review] OrderedBroadcast-permissions.patch Yep, this is what I intended.

Moved OrderedBroadcast patches to bug 889185.

Part 3b: Add JSON methods for reading and writing .json files. rnewman asked me to replace my Push SQLite DB with a simpler JSON file. This patch adds helper methods to read and write JSON files.

Comment on attachment 770651 [details] [diff] [review] part-3b-read-write-JSON-files.patch Review of attachment 770651 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/util/JSONUtils.java @@ +77,5 @@ > + Log.e(LOGTAG, "", e); > + return null; > + } finally { > + close(fileReader); > + close(reader); I think you only need to close the fileReader

Comment on attachment 770651 [details] [diff] [review] part-3b-read-write-JSON-files.patch Review of attachment 770651 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/util/JSONUtils.java @@ +24,5 @@ > private static final String LOGTAG = "JSONUtils"; > > private JSONUtils() {} > > + public static URI getURI(String name, JSONObject json) { This should really be 'optURI', no? @@ +77,5 @@ > + Log.e(LOGTAG, "", e); > + return null; > + } finally { > + close(fileReader); > + close(reader); Other way 'round: if you close `reader`, it'll close the `fileReader` for you. But note that the way this code is written, if one of the two constructors throws, things won't be cleaned up successfully unless you include both clauses. Personally I'd split this up and have individual try..catch blocks for the individual points of failure, and eliminate `close(Closeable)` altogether. Excessive null checks in single-threaded code are a code smell IMO. But take a look at https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/background/healthreport/ProfileInformationCache.java?source=cc#L88 which might contain some inspiration for you, particularly if this code already lives in android-services! @@ +89,5 @@ > + } > + } > + > + public static void writeFile(JSONObject json, String filename) { > + String text = json.toString(); You might want to move this clause to _after_ opening the file. Saves memory if something goes wrong. @@ +100,5 @@ > + File tempFile = File.createTempFile("gecko.", ".json.tmp"); > + writer = new FileWriter(tempFile); > + writer.write(text); > + writer.close(); > + writer = null; Belt and braces, huh? :D It's not possible to use the writer after it's closed, so I'd settle for making it final... but again, I think splitting up the failing clauses into sections could simplify this a little. @@ +105,5 @@ > + > + // Move the temp file to the real output file. > + File jsonFile = new File(filename); > + if (!tempFile.renameTo(jsonFile)) { > + Log.e(LOGTAG, "", new IllegalStateException("renameTo() failed: " + filename)); This can fail for the very benign reason that the destination file already exists. You'll need to ensure that it's deleted before using `renameTo()`.

Patch part 3b v2: * Rename JSONUtils.getURI() to optURI() * Remove unused JSONUtils.get/putUUID() * Remove close(Closeable) method and reader null checks * Delete target filename to ensure atomic File.renameTo() doesn't fail due to an existing file

This is an interdiff between part-4b-java-back-end-v2.patch and part-4b-java-back-end-v3.patch. Patch v3 fixes: * Moved "services.push.enabled" pref checks from Java to AndroidPushService.jsm * Replaced GeckoApp JS->Java messages with OrderedBroadcast.jsm * Replaced java.net.URL with java.net.URI * Replaced SQLite DB of push registrations with .json file. I don't expect many users will have more than a dozen registrations. * Replaced android.util.Log with Logger * Moved logging of sensitive user data to Logger.pii() * Replaced ANDROID_PACKAGE_NAME + ".App" with GlobalConstants.BROWSER_INTENT_CLASS Caveats: * Temporarily moved PushService from org.mozilla.gecko.background.push to org.mozilla.gecko.push package, matching the directory structure before trying to merge Push with android-services. * I don't have automated unit tests yet. AFAIK, neither B2G nor Desktop have automated tests for Push yet, either.

Comment on attachment 773045 [details] [diff] [review] part-3b-read-write-JSON-files-v2.patch Review of attachment 773045 [details] [diff] [review]: ----------------------------------------------------------------- r+ with trivial rework to use Scanner. ::: mobile/android/base/util/JSONUtils.java @@ +90,5 @@ > + String lines = ""; > + String line; > + while ((line = reader.readLine()) != null) { > + lines += line; > + } You should use Scanner here. scanner = new Scanner(file, "UTF-8"); final String contents = scanner.useDelimiter("\\A").next(); Note that this avoids the need to have a BufferedReader at all, which will simplify readFile.

rnewman: btw, here is the full patch (part-4b-java-back-end-v3.patch) for that interdiff, in case you prefer to read or r+ the full patch.

Comment on attachment 773131 [details] [diff] [review] 834033-part-4b-java-back-end-v3.interdiff Review of attachment 773131 [details] [diff] [review]: ----------------------------------------------------------------- Didn't do a total end-to-end review, but here are my comments about how the JSON handling should work. Broad thrusts: don't keep data in JSON; make sure this'll work well at scale; be more circumspect about when you write (which might involve exposing an Editor primitive to callers); avoid using optFoo, because almost every call will NPE on the next line if the key is missing. ::: b/mobile/android/base/GeckoApp.java @@ +685,4 @@ > } else { > // something went wrong. > Log.e(LOGTAG, "Received Contact:Add message with no email nor phone number"); > + } Trailing whitespace. ::: b/mobile/android/base/push/PushRegistrations.java @@ +59,1 @@ > public long version; These should all be final. @@ +62,5 @@ > + this.channelID = json.optString(CHANNEL_ID, null); > + this.pageURL = JSONUtils.optURI(PAGE_URL, json); > + this.wakeupURL = JSONUtils.optURI(WAKEUP_URL, json); > + this.pushEndpoint = JSONUtils.optURI(PUSH_ENDPOINT, json); > + this.version = json.optLong(NOTIFICATION_VERSION, 0); Are these really all optional? (Just a clarifying question re the spec.) @@ +69,5 @@ > > public PushRegistrations(Context context) { > + // Look for registrations file in /data/data/org.mozilla.xxx/files/mozilla/push/ directory. > + File pushDir = new File(context.getFilesDir(), "/mozilla"); > + mJSONFile = new File(pushDir.getAbsolutePath() + "/pushRegistrations.json"); Prefer chaining to another constructor that accepts a directory, and chains to another that takes a full path. That'll help testability, and won't punish us unduly if we change this in the future. @@ +70,5 @@ > public PushRegistrations(Context context) { > + // Look for registrations file in /data/data/org.mozilla.xxx/files/mozilla/push/ directory. > + File pushDir = new File(context.getFilesDir(), "/mozilla"); > + mJSONFile = new File(pushDir.getAbsolutePath() + "/pushRegistrations.json"); > + loadFile(); Do you want to do this even if the file doesn't exist? loadFile logs an error in that instance, which seems hostile on first run. See comments in loadFile. @@ +145,2 @@ > > + saveFile(); See point about Editor. @@ +189,4 @@ > } > > + JSONArray registrations = mRegistrations.optJSONArray(PUSH_REGISTRATIONS); > + JSONObject row = registrations.optJSONObject(index.intValue()); NPE. @@ +189,5 @@ > } > > + JSONArray registrations = mRegistrations.optJSONArray(PUSH_REGISTRATIONS); > + JSONObject row = registrations.optJSONObject(index.intValue()); > + JSONUtils.putURI(PUSH_ENDPOINT, pushEndpoint, row); I'd put `row` as the first argument to putURI: matches the argument order of row.put(foo, bar). @@ +204,4 @@ > } > > + JSONArray registrations = mRegistrations.optJSONArray(PUSH_REGISTRATIONS); > + JSONObject row = registrations.optJSONObject(index.intValue()); NPE will happen on this line. This kind of error is a reason to parse and constitute your domain objects, rather than working with raw JSON directly. @@ +214,3 @@ > } > + > + saveFile(); I think you might benefit from the Editor pattern here. I'm not super familiar with the protocol and its interactions, but you're going to kill throughput in the scenario of, e.g., adding a bunch of channels or updating a ton of versions. @@ +217,3 @@ > } > > + private Integer select(String name, String value) { Firstly, the names of the arguments are a little misleading. Secondly, all of the callers of this method do something like: Integer index = select(...); if (index == null) { // Behave exactly as if we've caught new IllegalArgumentException(...); } so why not have this be private int getRegistrationIndexBy(String key, String value) { ... // On failure: throw new IllegalStateException("No such registration for " + key + " = " + value); } @@ +219,5 @@ > + private Integer select(String name, String value) { > + JSONArray registrations = mRegistrations.optJSONArray(PUSH_REGISTRATIONS); > + int len = registrations.length(); > + > + for (int i = 0; i < len; i++) { Might be worth finding out what's the tipping point between linear search and maintaining multiple hash tables. If we do a lot of reads here, or there are lots of channels, this will get expensive fast. @@ +232,3 @@ > } > > + private Record fetchRowAt(int index) { This raises the question: why is mRegistrations not an array (or set of synchronized maps) of Record instances? The only instance in which we should keep the JSON around, and construct Records on-demand, is if fetching Records is a really infrequent task. Otherwise, do this translation on load, and keep the JSON for on-disk serialization only. @@ +233,5 @@ > > + private Record fetchRowAt(int index) { > + JSONArray registrations = mRegistrations.optJSONArray(PUSH_REGISTRATIONS); > + JSONObject row = registrations.optJSONObject(index); > + return new Record(row); This will throw a NPE if the optJSONObject on line 236 behaves any differently to getJSONObject. You probably want to throw something sane (e.g., use getJSONObject, or explicitly raise ArrayIndexOutOfBoundsException). @@ +239,2 @@ > > + private void loadFile() { Can we move loadFile up under the constructor? Read the file in order of execution and all that... @@ +248,2 @@ > > + if (mRegistrations == null) { Suggest splitting this into two methods, and having callers handle exceptions in loadFile: // constructor if (!mJSONFile.exists()) { initRegistrations(); } try { loadFile(); } catch (Exception e) { Logger.error(LOG_TAG, "Unable to load push registrations; starting over.", e); initRegistrations(); } @@ +259,5 @@ > + private void saveFile() { > + try { > + JSONUtils.writeFile(mRegistrations, mJSONFile.getAbsolutePath()); > + } catch (IOException e) { > + throw new IllegalStateException(e); Suggest making this a checked exception and either using IOException or another checked Exception subclass. This is the exact scenario in which checked exceptions are good -- you want to force the caller to explicitly declare that an error is possible, even if they delegate it up the chain. There's nothing to clue in callers (e.g., callers of insertChannel()) that they might get an illegal state exception that's nothing to do with them! ::: b/mobile/android/base/push/PushService.java @@ +27,5 @@ > import java.util.TreeMap; > import java.util.UUID; > > public final class PushService extends SimpleService { > + private static final String LOG_TAG = "GeckoPushService"; N.B., if you use Logger, you ought to make sure that Fennec's main and background threads have suitable per-thread log tags set. It might already be done, but worth checking. If you see "GeckoLogger" for your stuff, then it's not! (Logger has two tiers: one per thread that's the Android log tag, then another for each logging component.) @@ +28,5 @@ > import java.util.UUID; > > public final class PushService extends SimpleService { > + private static final String LOG_TAG = "GeckoPushService"; > + private static final boolean DEBUG = true; Might be worth setting this to true if our channel is pre-release. @@ +136,1 @@ > int len = updates.length(); NPE. @@ +365,5 @@ > + if (wakeupURL == null) { > + Logger.error(LOG_TAG, "", new IllegalArgumentException("'wakeupURL' not found")); > + status = 500; > + } > + Trailing whitespace.

Comment on attachment 773677 [details] [diff] [review] part-4b-java-back-end-v3.patch Reviewed the interdiff, so clearing this flag.

> @@ +62,5 @@ > > + this.channelID = json.optString(CHANNEL_ID, null); > > + this.pageURL = JSONUtils.optURI(PAGE_URL, json); > > + this.wakeupURL = JSONUtils.optURI(WAKEUP_URL, json); > > + this.pushEndpoint = JSONUtils.optURI(PUSH_ENDPOINT, json); > > + this.version = json.optLong(NOTIFICATION_VERSION, 0); > > Are these really all optional? (Just a clarifying question re the spec.) These properties are not optional, but it is a bug if they are missing. I guess a user could manually edit the saved JSON file and remove some fields, in which case we would NPE. Do we care?

(In reply to Chris Peterson (:cpeterson) from comment #49) > > Are these really all optional? (Just a clarifying question re the spec.) > > These properties are not optional, but it is a bug if they are missing. I > guess a user could manually edit the saved JSON file and remove some fields, > in which case we would NPE. Do we care? My suggestion would be to (a) check that they're all present with .has(), throwing an appropriate exception if they're not there, and (b) use getURI/getString/getLong, because they'll throw for a type error. Of course, wherever is calling the constructor must catch those exceptions and do the right thing -- e.g., discard that record and log, blow away a file, whatever. optFoo only exists to avoid a has-check-falling-back-to-null.

Comment on attachment 773677 [details] [diff] [review] part-4b-java-back-end-v3.patch Review of attachment 773677 [details] [diff] [review]: ----------------------------------------------------------------- Everything about the protocol looks good. I'd like the timer logic to be added before it lands: 1) Timers for DOMRequest timeouts 2) The timer that sends a ping on the websocket every pingInterval, then waits requestTimeout for reply, and re-connect with back-off. ::: mobile/android/base/push/PushRegistrations.java @@ +26,5 @@ > + * { "channelID":"aaa", > + * "pageURL":"http://aaa", > + * "wakeupURL":"http://aaa", > + * "pushEndpoint":"http://aaa", > + * "version":1, Is Android Push only going to be for web pages? For web-apps you'll want to store the manifest URL. ::: mobile/android/base/push/PushService.java @@ +42,5 @@ > + public static final String EXTRA_PUSH_TYPE = GlobalConstants.BROWSER_INTENT_PACKAGE + ".extra.PUSH_TYPE"; > + > + private PushSocket mPushSocket; > + private final List<JSONObject> mQueuedRequests = new ArrayList<JSONObject>(1); > + private final Map<String, JSONObject> mActiveRequestMessages = new TreeMap<String, JSONObject>(); I'm not sure what you're using this for. I think you'd want some sort of timeout system, so that if the server doesn't respond or there is no network, within services.push.requestTimeout, the DOMRequest errors. @@ +177,5 @@ > + } > + > + String channelID = record.channelID; > + mRegistrations.deleteChannel(channelID); > + mActiveRequestMessages.put(channelID, message); See bug 867632, you can just reply back to JS here. @@ +390,5 @@ > + > + startGeckoActivity(messageType, response); > + } > + > + private void onSocketUnregister(String channelID, int status) { Can get rid of this too. @@ +447,5 @@ > + fakeAck.add(channelUpdate); > + mPushSocket.ack(fakeAck); > + > + // Send a fake Unregister so the server will drop this unknown channel. > + mPushSocket.unregister(channelID); Might be a good idea to add logging here since this behaviour isn't exactly according to protocol (though it's not against it either), so that if server admins get flummoxed, they can view device output. This might be a good idea to implement in b2g/desktop as well. Can you bring it up on IRC and see what Services people think. ::: mobile/android/base/push/PushSocket.java @@ +283,5 @@ > + * { > + * "messageType": "register", > + * "channelID": "d9b74644-4f97-46aa-b8fa-9393985cd6cd", > + * "status": 200, 409 or 500 > + * "pushEndpoint": "http://pushserver.example.org/" "pushEndpoint": "http://pushserver.example.org/d9b74644-4f97-46aa-b8fa-9393985cd6cd" ? @@ +308,5 @@ > + > + mPushSocketListener.onRegister(channelID, status, pushEndpoint); > + } > + > + private void onUnregister(JSONObject response) { You should be able to get rid of this function and related logic in onMessage() entirely since we don't care about unregister() responses from the server.

rnewman's feedback fixed: * JSONUtils: Made readFile() and writeFile() throw IOException and have callers handle errors * JSONUtils: Refactor FileReader/BufferedReader code to avoid complicated close() logic * JSONUtils: Swapped getURI() and putURI() argument order

These files were too big to successfully interdiff, so this patch contains the whole enchilada: JS/JSM front-end, Java back-end service, and System Messages for delivering callbacks to Event Pages. rnewman's feedback fixed: * PushRegistrations: replaced mRegistrations JSONObject with an array (list) of Records * PushRegistrations: Handled JSONExceptions for corrupted JSON file * PushRegistrations: Replaced select() methods with getRegistrationIndexBy() * PushRegistrations: Made getRegistrationIndexBy() throw a checked exception instead of returning null * PushRegistrations: Made Record's instance variables final * PushRegistrations: Added chaining constructors that take a directory and filename arguments * PushRegistrations: Don't log an error when attempting to read JSON file for the first time * PushRegistrations: Moved loadFile() definition under constructors * PushRegistrations: Extracted initRegistrations() method from loadFile() * PushService: Automatically set DEBUG=true for local builds nsm's feedback fixed: * PushSocket/PushSevice: Removed processing of server's "unregister" responses * PushService: ACK all channel updates, even ones we don't know about NOT IMPLEMENTED: * PushRegistration: Use Editor pattern for batch updates to JSON file * Move PushService to android-services repo * Write JUnit3 tests for PushService

Comment on attachment 784230 [details] [diff] [review] R?_834033-part-4-push-service-v4.patch Review of attachment 784230 [details] [diff] [review]: ----------------------------------------------------------------- Things look good, but I'm converting to feedback rather than r? because the timers (noted in your TODOs) and scope-by-origin is missing. Can you use android's AlarmManager for socket retry timers? That'll allow wakeups of the device. ::: mobile/android/app/mobile.js @@ +618,5 @@ > pref("browser.dom.window.dump.enabled", true); > > // SimplePush > pref("services.push.enabled", false); > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); Doesn't need to be set here since libpref already has it set. @@ +619,5 @@ > > // SimplePush > pref("services.push.enabled", false); > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > +pref("dom.sysmsg.enabled", true); This will enable system messages even for web pages. You should ask for sr from a DOM peer (mounir is a good choice for system message stuff) ::: mobile/android/base/push/PushService.java @@ +145,5 @@ > + mActiveRequestMessages.put(channelID, message); > + mPushSocket.register(channelID); > + } > + > + private void onPushUnregister(JSONObject message) { This function needs same origin checks to prevent pages from unregistering other origin's endpoints @@ +216,5 @@ > + } catch (JSONException e) { > + throw new IllegalArgumentException("'requestID' not found"); > + } > + > + Collection<Record> records = mRegistrations.fetchByPageURL(pageURL); Scope by origin. This way pages across an app can make decisions about registering for push based on prior registrations ::: mobile/android/chrome/content/browser.js @@ +17,5 @@ > Cu.import("resource://gre/modules/JNI.jsm"); > Cu.import('resource://gre/modules/Payment.jsm'); > Cu.import("resource://gre/modules/PermissionPromptHelper.jsm"); > Cu.import("resource://gre/modules/ContactService.jsm"); > +Cu.import('resource://gre/modules/AndroidPushService.jsm'); Is it standard practice in Android to import JSMs into browser.js? On desktop I had to use a component - bug 871372. I'll let rnewman decide. ::: mobile/android/modules/AndroidPushService.jsm @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + Can you add an detailed explanatory comment of the service here? Particularly since we have a 3 layer communication here from Push.js <-> AndroidPushService.jsm <-> PushService.java. @@ +85,5 @@ > + switch (data) { > + case PUSH_ENABLED_PREF: > + let enabled = Preferences.get(PUSH_ENABLED_PREF); > + debug("observe: " + data + "=" + enabled); > + // TODO: start/stop service based on new pref value The way the original PushService works is to treat "services.push.enabled" as a boot time setting and not monitor it. There is another pref called "services.push.userEnabled" which is monitored and only affects the websocket (the service stays around in memory, at least for now). When userEnabled is set to false, the websocket should not be established.

Comment on attachment 784227 [details] [diff] [review] R?_834033-part-3b-read-write-JSON-files-v3.patch Review of attachment 784227 [details] [diff] [review]: ----------------------------------------------------------------- N.B., patch r= flag is wrong. I'd like to see a test for the very basic in-out methods in these utils. Seems like a high-value thing to have test coverage for... ::: mobile/android/base/util/JSONUtils.java @@ +87,5 @@ > + String line; > + while ((line = reader.readLine()) != null) { > + lines += line; > + } > + return lines; Previous comment about using Scanner applies, and even if it didn't, you should use StringBuilder rather than iterative concatenation. Note also that you're removing newlines from the file with this method, which is probably not what you want to do. Using Scanner will fix this.

Fixed rnewman's JSONUtils feedback: * JSONUtils.java: Replaced BufferedReader with Scanner to avoid removing JSON newlines * JSONUtils.java: Added tests for JSONUtils' putURI/getURI and writeFile/readFile

btw, part-6-jsonutils-tests-rnewman.patch from comment 56 is addressing rnewman's review feedback from comment 55. Here is a green try build for part-6-jsonutils-tests-rnewman.patch: https://tbpl.mozilla.org/?tree=Try&rev=70f5475de59b

rnewman, review poke?

Sorry, Chris, got caught up in other stuff. Will put this to the top of my list!

Comment on attachment 786587 [details] [diff] [review] part-6-jsonutils-tests-rnewman.patch Review of attachment 786587 [details] [diff] [review]: ----------------------------------------------------------------- Wow, that test will get way less painful after Bug 903528. ::: mobile/android/base/tests/testJSONUtils.java.in @@ +139,5 @@ > + } > + > + // This test completes very quickly. If it completes too soon, the > + // minidumps directory may not be created before the process is > + // taken down, causing bug 722166. Sadface! ::: mobile/android/base/util/JSONUtils.java @@ +54,2 @@ > } > + } Scanner throws FileNotFound in the constructor, which you don't catch and will break control flow anyway, so: Scanner scanner = new Scanner(file, "UTF-8"); try { contents = ...; } finally { scanner.close(); } is equivalent and neater. @@ +63,2 @@ > } catch (JSONException e) { > throw new IOException("JSONException: " + filename); Add the `cause` argument.

Comment on attachment 784230 [details] [diff] [review] R?_834033-part-4-push-service-v4.patch Review of attachment 784230 [details] [diff] [review]: ----------------------------------------------------------------- sr? mounir per nsm's comment. f? mfinkle for multi-profile question. This will be an r+ when: (a) nits addressed, (b) nsm's review comments addressed, (c) questions to mfinkle and mounir addressed, (d) concurrency issues addressed -- prove to me that this is thread-safe! Thanks for being patient with this! ::: mobile/android/base/push/PushRegistrations.java @@ +40,5 @@ > + * > + * When we support web apps, we will need to save their manifest URLs, too. > + */ > + > +public final class PushRegistrations { Does this need to be final for a particular reason? @@ +51,5 @@ > + public static final String PUSH_ENDPOINT = "pushEndpoint"; > + public static final String NOTIFICATION_VERSION = "version"; > + > + private final File mJSONFile; > + private ArrayList<Record> mRegistrations; mRegistrations needs to be accessed in a thread-safe way. Currently you'll: (a) get races between inserts, removals, updates (b) get ConcurrentModificationExceptions while iterating if modifications occur. Consider whether you want locking, CopyOnWriteArrayList, or something else. You can also mandate that PushRegistrations be used only from one thread, but that seems likely to footgun: you are *really* close to being able to do so, because PushService is an IntentService, and as such has a single worker thread, but that worker thread opens PushSockets that have callbacks on the websocket background thread. Aiee! @@ +58,5 @@ > + public String channelID; > + public URI pageURL; > + public URI wakeupURL; > + public URI pushEndpoint; > + public long version; I think all of these should be final. They're not because you mutate them, but if you mutate them you need to make sure that the mutating accessors are thread-safe. Needs fixing. @@ +72,5 @@ > + Record(String channelID, URI pageURL, URI wakeupURL, URI pushEndpoint) { > + this.channelID = channelID; > + this.pageURL = pageURL; > + this.wakeupURL = wakeupURL; > + this.pushEndpoint = pushEndpoint; Explicitly set this.version. @@ +86,5 @@ > + } > + > + public PushRegistrations(Context context) { > + // Look for registrations file in /data/data/org.mozilla.xxx/files/mozilla/ directory. > + this(context, new File(context.getFilesDir(), "/mozilla")); Big open question here: this is fundamentally incompatible with multi-profile support. mfinkle, do you have an existing strategy in mind for that? @@ +216,5 @@ > + } > + > + public void insertChannel(String channelID, URI pageURL, URI wakeupURL, URI pushEndpoint) throws IOException { > + Log.d(LOGTAG, "insertChannel: channelID=" + channelID); > + deleteChannel(channelID); This seems like an inefficient way to do this: you'll serialize twice, and you're also opening the door to concurrency problems. I suggest either: * Using an associative data structure for mRegistrations, and simply using `put`, or * Directly using mRegistrations.remove() and .add(). ::: mobile/android/base/push/PushService.java @@ +345,5 @@ > + } > + > + private void onSocketDisconnect() { > + // TODO: attempt to reconnect service now or try again later? > + // TODO: Return "KO" error for each queued or active request File bugs, fix, or discard. ::: mobile/android/locales/en-US/chrome/browser.properties @@ +109,5 @@ > +push.dontAllow=Don't allow > +push.ask=Allow %S to use push notifications? > +# LOCALIZATION NOTE (push.useNotifications): Label that will be > +# used in site settings dialog. > +push.useNotifications=Use Push Notifications I think Android style is to use sentence case: Use push notifications ::: mobile/android/modules/AndroidPushService.jsm @@ +21,5 @@ > +this.EXPORTED_SYMBOLS = ["PushService"]; > + > +const kCHILD_PROCESS_MESSAGES = ["Push:Register", > + "Push:Registrations", > + "Push:Unregister" ]; s/" ]/"] @@ +29,5 @@ > + "PushService:Register:KO", > + "PushService:Registrations:OK", > + "PushService:Registrations:KO", > + "PushService:Unregister:OK", > + "PushService:Unregister:KO" ]; Same: spaces front and back.

Comment on attachment 784230 [details] [diff] [review] R?_834033-part-4-push-service-v4.patch Review of attachment 784230 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/app/mobile.js @@ +619,5 @@ > > // SimplePush > pref("services.push.enabled", false); > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > +pref("dom.sysmsg.enabled", true); Why do you need that?

(In reply to Mounir Lamouri (:mounir) from comment #62) > Comment on attachment 784230 [details] [diff] [review] > R?_834033-part-4-push-service-v4.patch > > > // SimplePush > > pref("services.push.enabled", false); > > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > > +pref("dom.sysmsg.enabled", true); > > Why do you need that? Mounir, can you please elaborate on what you mean? I needed to enable "dom.sysmsg.enabled" so I could deliver the push notifications to content with getService(Ci.nsISystemMessagesInternal).sendMessageToEventPage('push', message, wakeupURI).

(In reply to Richard Newman [:rnewman] from comment #61) > > + public PushRegistrations(Context context) { > > + // Look for registrations file in /data/data/org.mozilla.xxx/files/mozilla/ directory. > > + this(context, new File(context.getFilesDir(), "/mozilla")); > > Big open question here: this is fundamentally incompatible with > multi-profile support. mfinkle, do you have an existing strategy in mind for > that? It really depends on how the wakeups will be handled. We already store the webapps JSON file outside a profile so it can be accessed from multiple profiles, as needed. It seems like we are storing the push registrations outside of profiles too. This seems like a good thing, as long as we make some assumptions or answer some questions: 1. Are we only allowing push notifications for packaged webapps? If so, each webapp has a defined profile so we know which profile to use because we know which webapp to launch. 2. If we allow regular web pages to use push notifications, now we need to guess which profile to use to open Firefox and load the web page. We could just use the last active profile, but what if that's the guest profile? 3. We don't yet support multiple profiles yet, but guest profile might be enough to trip us up. Until we support multi-profiles, we could assume that the non-webapp profile to use is the last active (non-guest) profile. It's hairy, but not too crazy... yet. If we stick with "webapps-only" for push notifications, it's a lot more simple, but not as much fun for web devs.

Comment on attachment 784230 [details] [diff] [review] R?_834033-part-4-push-service-v4.patch f+ cause we are sticking to webapps for now, so this should work out fine, or push the hard stuff to a different part of the code.

(In reply to Mark Finkle (:mfinkle) from comment #65) > Comment on attachment 784230 [details] [diff] [review] > R?_834033-part-4-push-service-v4.patch > > f+ cause we are sticking to webapps for now, so this should work out fine, > or push the hard stuff to a different part of the code. btw, the patches' current implementation exposes push notifications to any web content and has no special handling for packaged webapps.

(In reply to Chris Peterson (:cpeterson) from comment #63) > (In reply to Mounir Lamouri (:mounir) from comment #62) > > Comment on attachment 784230 [details] [diff] [review] > > R?_834033-part-4-push-service-v4.patch > > > > > // SimplePush > > > pref("services.push.enabled", false); > > > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > > > +pref("dom.sysmsg.enabled", true); > > > > Why do you need that? > > Mounir, can you please elaborate on what you mean? I needed to enable > "dom.sysmsg.enabled" so I could deliver the push notifications to content > with getService(Ci.nsISystemMessagesInternal).sendMessageToEventPage('push', > message, wakeupURI). But this is going to expose the System Messages API to Gecko Android. That is a pretty huge side effect.

(In reply to Mounir Lamouri (:mounir) from comment #67) > But this is going to expose the System Messages API to Gecko Android. That > is a pretty huge side effect. Yes, but bug 868322 will enable the System Messages API for all desktop content, so I assumed Android could do the same thing.

Rebased Nikhil patch from desktop bug 857464 adds push prefs: modules/libpref/src/init/all.js

Rebased Nikhil patch from desktop bug 857464 extends Push WebAPI to work for both packaged web apps (with Push URL in manifest) and regular web pages (that pass Push URL at navigator.push.register() call).

Rebased Nikhil patch from desktop bug 868322 adds System Messages WebAPI.

Rebased JSONUtils helper methods for reading and writing .json files.

Rebased JS front-end and Java back-end for Push WebAPI. Even though Android Push will now use GCM instead of WebSockets, this patch has some useful bits for the navigator.push JS/Java glue.

Rebased JSONUtils Robocop tests. Unfortunately, these tests are broken because they have not kept up with the Robocop refactorings. In fact, these JSONUtils tests should probably be ported to JUnit instead of Robocop because they don't rely on any of Robocop's UI testing features. JUnit was not part of our Fennec test infrastructure when this patch was originally written.

btw, after you implement GSM, don't forget to hg rm the third-party WebSocket code in mobile/android/thirdparty/com/codebutler/android_websockets! :)

This patch allows us to start an Android service which then spins up a hidden window in Gecko. The idea is that this hidden window would then run any stuff required to make Service Workers do their thing. If Fennec is launched while this service is running, it uses the existing instance, and just opens the normal browser.xul in a new (visible, of course) window. There are still some edge cases and cruft to consider, but it seems to generally work well.

Can you please post an intent to ship email to dev-platform? Thanks!

Comment on attachment 8406982 [details] [diff] [review] Add groundwork for Service Workers on Android Review of attachment 8406982 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApplication.java @@ +76,5 @@ > super.onConfigurationChanged(config); > } > > public void onActivityPause(GeckoActivityStatus activity) { > + ws ::: mobile/android/base/GeckoThread.java @@ +95,5 @@ > + > + // FIXME: This is terrible, but necessary right now because many things > + // send events based on this state. If transitioning from service to > + // window mode, we won't have anything listening for those events yet. > + setLaunchState(GeckoThread.LaunchState.Launched); We talked over email about some ways to make this cleaner @@ +192,5 @@ > + mAction = action; > + mUri = uri; > + } > + > + public void runGecko() { I'd rather keep mArgs, mAction & mUri final and not update them. Have runGecko take aArgs, aAction & aUri as arguments. In GeckoThread.run() pass in mArgs, mAction & mUri. Then have runCommand() take the same 3 arguments and pass them through in the run() your anonymous Runnable().

Comment on attachment 8427444 [details] [diff] [review] Allow running Gecko headless from an Android service Review of attachment 8427444 [details] [diff] [review]: ----------------------------------------------------------------- mostly glossed over the js changes, assuming mfinkle will look at those closer ::: mobile/android/base/GeckoApp.java @@ -1477,5 @@ > - } > - > - // We now have tab stubs from the last session. Any future tabs should > - // be animated. > - Tabs.getInstance().notifyListeners(null, Tabs.TabEvents.RESTORED); moving this worries me a bit. Can you test performance of starting up and loading a page with the radio powered down with and without please? ::: mobile/android/base/GeckoAppShell.java @@ +367,5 @@ > } > > // Called on the UI thread after Gecko loads. > private static void geckoLoaded() { > + if (getContext() instanceof Activity) { getGeckoInterface().getActivity() != null

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #82) > Comment on attachment 8427444 [details] [diff] [review] > Allow running Gecko headless from an Android service > > Review of attachment 8427444 [details] [diff] [review]: > ----------------------------------------------------------------- > > mostly glossed over the js changes, assuming mfinkle will look at those > closer > > ::: mobile/android/base/GeckoApp.java > @@ -1477,5 @@ > > - } > > - > > - // We now have tab stubs from the last session. Any future tabs should > > - // be animated. > > - Tabs.getInstance().notifyListeners(null, Tabs.TabEvents.RESTORED); > > moving this worries me a bit. Can you test performance of starting up and > loading a page with the radio powered down with and without please? What does this have to do with the radio?

I thought loastStartupTab() was where we pre-warmed up the radio, but I don't see that code anymore.

Comment on attachment 8427446 [details] [diff] [review] Rename the robocop 'Assert' class to 'Asserter' Review of attachment 8427446 [details] [diff] [review]: ----------------------------------------------------------------- r+ w/ nits. ::: mobile/android/base/tests/SessionTest.java @@ +274,5 @@ > * Verifies a session JSON string against the given session. > * > * @param session Session to verify against > * @param sessionString JSON string to verify > * @param asserter Assert class to use during verification nit: "Asserter", or just remove the class name to prevent this from occurring in the future. ::: mobile/android/base/tests/helpers/AssertionHelper.java @@ +10,4 @@ > import org.mozilla.gecko.tests.UITestContext; > > /** > * Provides assertions in a JUnit-like API that wraps the robocop Assert interface. nit: Asserter. @@ +12,5 @@ > /** > * Provides assertions in a JUnit-like API that wraps the robocop Assert interface. > */ > public final class AssertionHelper { > // Assert.ok has a "diag" ("diagnostic") parameter that has no useful purpose. nit: Asserter. ::: mobile/android/base/tests/helpers/JavascriptMessageParser.java @@ +37,5 @@ > private final boolean endOnAssertionFailure; > > /** > * Constructs a message parser for test result messages sent from JavaScript. When seeing an > * assertion failure, the message parser can use the given {@link org.mozilla.gecko.Assert} nit: ...gecko.Asserter.

Comment on attachment 8427444 [details] [diff] [review] Allow running Gecko headless from an Android service Some overall thoughts: 1. Naming OCD: I think I like "service" instead of "headless", especially for the headless.js and headless.xul files. HeadlessService.java might be better as GeckoService.java 2. Why is BrowserApp.jsm a JSM? Why not follow the pattern we have for WebappRT.js and make a BrowserRT.js file? Making it a JS file means you can just use BrowserApp and skip the AndroidBrowserApp.tabGetter stuff too. Some detailed comments coming...

(In reply to Mark Finkle (:mfinkle) from comment #86) > Comment on attachment 8427444 [details] [diff] [review] > Allow running Gecko headless from an Android service > > Some overall thoughts: > 1. Naming OCD: I think I like "service" instead of "headless", especially > for the headless.js and headless.xul files. HeadlessService.java might be > better as GeckoService.java Yeah, it was originally service.xul/js but to me "service" sort of implies long-running, which is not necessarily a trait this will have. The most important aspect is that it doesn't show a visible window. Maybe background.xul/js is better? > 2. Why is BrowserApp.jsm a JSM? Why not follow the pattern we have for > WebappRT.js and make a BrowserRT.js file? That's fine I guess. I don't have strong feelings really, it just needs to be factored out of browser.js. > Making it a JS file means you can > just use BrowserApp and skip the AndroidBrowserApp.tabGetter stuff too. Not following here.... > > Some detailed comments coming... Ok.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #87) > Yeah, it was originally service.xul/js but to me "service" sort of implies > long-running, which is not necessarily a trait this will have. The most > important aspect is that it doesn't show a visible window. Maybe > background.xul/js is better? Concur: "service" is super overloaded (XPCOM service? Android service? web service?) Background kinda has that problem, too. Headless is pretty descriptive :D

headless: http://timelifeblog.files.wordpress.com/2012/01/01_3445355.jpg?w=403

Like I said, descriptive!

Comment on attachment 8427444 [details] [diff] [review] Allow running Gecko headless from an Android service >diff --git a/mobile/android/base/AndroidManifest.xml.in b/mobile/android/base/AndroidManifest.xml.in >+ <service >+ android:exported="true" >+ android:taskAffinity="@ANDROID_PACKAGE_NAME@.BROWSER" >+ android:name="org.mozilla.gecko.HeadlessService"> >+ </service> >+ > No need for the extra blank line >diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java >- return get(context, ""); >+ return get(context, "default"); Make sure this does not mess up Webapps > private void setDir(File dir) { >+ Log.w(LOGTAG, "Requested profile directory missing: " + dir); I thought we removed this kind of output because it was not fatal and happened a lot with background services. >diff --git a/mobile/android/base/GeckoThread.java b/mobile/android/base/GeckoThread.java > public static boolean ensureInit() { > ThreadUtils.assertOnUiThread(); > if (isCreated()) >- return false; >+ return true; Was this a bug previously? > public static void createAndStart() { >+ Log.d(LOGTAG, "starting new gecko"); Remove before checkin >+ public static void runCommandLine() { >+ ThreadUtils.sGeckoHandler.post(new Runnable() { >+ public void run() { >+ Log.d(LOGTAG, "running existing gecko"); Remove before checkin > public void handleMessage(String event, JSONObject message) { >+ Log.d(LOGTAG, "Ready!"); Remove before checkin >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+Cu.import("resource://gre/modules/BrowserApp.jsm"); > startup: function startup() { >+ AndroidBrowserApp.tabGetter = this; >+ > shutdown: function shutdown() { >+ AndroidBrowserApp.tabGetter = null; >diff --git a/mobile/android/chrome/content/headless.js b/mobile/android/chrome/content/headless.js >+Cu.import("resource://gre/modules/BrowserApp.jsm"); >+function dump(a) { >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(a); >+} Unused? And if needed, use the new AndroidLog hotness in browser.js >+sendMessageToJava({ type: "Gecko:Ready" }); You might want to put this in a: window.addEventListener("load", function() { /* HERE */ }, false); To give the window a chance to fully load. >\ No newline at end of file Add one >diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js >+function BrowserCLH() { >+ this._windows = {}; Is this really needed? > handle: function fs_handle(aCmdLine) { >+ let headlessUrl = null; A boolean should be enough >+ try { >+ headlessUrl = aCmdLine.handleFlagWithParam("headless", false); >+ } catch (e) { /* Optional */ } Why do we need to pass in the URL? It's always the same, so just pass a simple flag and hardcode the URL here instead of in HeadlessService.java That's what we do for browser.xul >+ if (this._windows['browser']) { >+ if (!pinned) { >+ this._windows['browser'].browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB, Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL); nit: Use " instead of ' >+ this._windows['browser'] = openWindow(null, "chrome://browser/content/browser.xul", "_blank", flags, args); Here too >diff --git a/mobile/android/modules/BrowserApp.jsm b/mobile/android/modules/BrowserApp.jsm I do not like the way this file is evolving. I want a bit cleaner approach. The | if (tabGetter == null) return; | checks smell bad. I would like a different approach for this file: 1. In headless.js, create an object like BrowserApp in browser.js (call it ServiceApp) and make it have the methods needed for | Services.androidBridge.browserApp = this; | to work 2. Return null in getBrowserTab() 3. Import UITelemetry.jsm and return it from getUITelemetryObserver() 4. Create a new Preferences.js file modeled on files like MasterPassword.js or FindHelper.js or FeedHandler.js scripts. This will basically put the preference handing code in it's own JS file. Which is mostly what you've done here. We have wanted this anyway. In the remaining preference related methods, just delegate to the new Preferences object to do the work. You'll need to do the same in browser.js BrowserApp. At least that's what I am thinking for now. The JSM approach used here is not feeling right. >diff --git a/toolkit/xre/nsAndroidStartup.cpp b/toolkit/xre/nsAndroidStartup.cpp > GeckoStart(void *data, const nsXREAppData *appData) >- free(targs[0]); >- nsMemory::Free(data); We no longer need to worry about this?

(In reply to Richard Newman [:rnewman] from comment #88) > (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #87) > > > Yeah, it was originally service.xul/js but to me "service" sort of implies > > long-running, which is not necessarily a trait this will have. The most > > important aspect is that it doesn't show a visible window. Maybe > > background.xul/js is better? > > Concur: "service" is super overloaded (XPCOM service? Android service? web > service?) > > Background kinda has that problem, too. Headless is pretty descriptive :D I like service.js and service.xul and GeckoService.java Service on Android does not mean "long running" and the title of this patch is "Allow running Gecko headless from an Android service" - It has "service" in the name. Headless is useless. It has no context or intent.

(In reply to Mark Finkle (:mfinkle) from comment #91) > >diff --git a/mobile/android/base/GeckoProfile.java b/mobile/android/base/GeckoProfile.java > > >- return get(context, ""); > >+ return get(context, "default"); > > Make sure this does not mess up Webapps Seems to be working fine > > > private void setDir(File dir) { > > >+ Log.w(LOGTAG, "Requested profile directory missing: " + dir); > > I thought we removed this kind of output because it was not fatal and > happened a lot with background services. > > >diff --git a/mobile/android/base/GeckoThread.java b/mobile/android/base/GeckoThread.java > > > public static boolean ensureInit() { > > ThreadUtils.assertOnUiThread(); > > if (isCreated()) > >- return false; > >+ return true; > > Was this a bug previously? It was a little weird, given that ensureFoo() is usually idempotent. My other changes require that. > > >diff --git a/mobile/android/components/BrowserCLH.js b/mobile/android/components/BrowserCLH.js > > >+function BrowserCLH() { > >+ this._windows = {}; > > Is this really needed? Mostly necessary for the browser window, but makes sense for the others too, IMO. > > > handle: function fs_handle(aCmdLine) { > > >+ let headlessUrl = null; > > A boolean should be enough > > >+ try { > >+ headlessUrl = aCmdLine.handleFlagWithParam("headless", false); > >+ } catch (e) { /* Optional */ } > > Why do we need to pass in the URL? It's always the same, so just pass a > simple flag and hardcode the URL here instead of in HeadlessService.java The idea is that the service would load arbitrary types of "headless" windows via the one Android service. That's why the url is passed in there. For instance, the plan is to have pushNotificationHandler.xul/js which has all the logic for delivering a Google Cloud Messaging message to the push stuff. I guess I'm ok with having a flag to indicate what type of thing needs to happen instead of using a different url. > > That's what we do for browser.xul > > >+ if (this._windows['browser']) { > >+ if (!pinned) { > >+ this._windows['browser'].browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWTAB, Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL); > > nit: Use " instead of ' > > >+ this._windows['browser'] = openWindow(null, "chrome://browser/content/browser.xul", "_blank", flags, args); > > Here too > > >diff --git a/mobile/android/modules/BrowserApp.jsm b/mobile/android/modules/BrowserApp.jsm > > I do not like the way this file is evolving. I want a bit cleaner approach. > The | if (tabGetter == null) return; | checks smell bad. I would like a > different approach for this file: > > 1. In headless.js, create an object like BrowserApp in browser.js (call it > ServiceApp) and make it have the methods needed for | > Services.androidBridge.browserApp = this; | to work > 2. Return null in getBrowserTab() > 3. Import UITelemetry.jsm and return it from getUITelemetryObserver() > 4. Create a new Preferences.js file modeled on files like MasterPassword.js > or FindHelper.js or FeedHandler.js scripts. This will basically put the > preference handing code in it's own JS file. Which is mostly what you've > done here. We have wanted this anyway. In the remaining preference related > methods, just delegate to the new Preferences object to do the work. You'll > need to do the same in browser.js BrowserApp. > > At least that's what I am thinking for now. The JSM approach used here is > not feeling right. I thought about that a little. Consider the following: 1) App is initially launched headless, and the Services.androidBridge.browserApp instance is the one in headless.js 2) App is launched with a VIEW intent, so a normal browser window is created. This displaces the Services.androidBridge.browserApp instance with a new one. Do we notify headless.js? Seems a little wrong to just kick it out without warning, though in practice there are probably no ill effects. 3) User exits Activity and the browser window is closed, and browserApp is set to null. We now need a way to get the BrowserApp one back in place (or create a new one, etc). I thought the BrowserApp.jsm singleton was a better solution than all of that. We could still split out the prefs part, though. > > >diff --git a/toolkit/xre/nsAndroidStartup.cpp b/toolkit/xre/nsAndroidStartup.cpp > > > GeckoStart(void *data, const nsXREAppData *appData) > > >- free(targs[0]); > >- nsMemory::Free(data); > > We no longer need to worry about this? This is actually just busted, but apparently didn't cause a problem until we started to run the command line handler more than once. The caller free the targs, and appData is stack allocated in the caller[1] http://dxr.mozilla.org/mozilla-central/source/mozglue/android/APKOpen.cpp#380

This partly landed in 2013. The landed code isn't usable (because sockets; see Bug 1064363). Let's resolve this one, do some cleanup in the above bug, and we'll file a follow-up for remaining work, so we can track release more accurately.

(Updating dependency here to standard Push API and not SimplePush)

Removing leave-open keyword from resolved bugs, per :sylvestre.