* We need door hangers or some such UI to allow users to opt in/out * We also need Settings implementation (which we can break off into a sep bug if needed).

Eng note: we'll either watch a pref or broadcast an intent to allow the service to enable and disable itself. This'll replace my stub "on launch" broadcast intent in Bug 793053.

Pretty much done.

Remaining work: * Final string * Why does the checkbox state persist across restarts, yet SharedPreferences reports the wrong value? * Cleanup etc.

Now persists. String is final per ibarlow.

Try run for da33131169af is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=da33131169af Results (out of 1 total builds): success: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rnewman@mozilla.com-da33131169af

Magic constants etc.

Adds string (approved by ibarlow), checkbox.

Sends a broadcast intent directly to the snippets service (Bug 793053). The intent occurs in two situations: * Asynchronously near the end of GeckoApp startup * When the checkbox is toggled. In the latter case the broadcast comes with the value (enabled); in the former case it does not. If no value is sent, the receiver will come back to get the value later. The purpose of this intent is to ensure that the state of the snippet fetching service -- running or not -- matches user preference. Note that this won't build without Bug 793053, which is still in progress, but I think this is ready to review.

Reviewer note: the UI for this (parts 0 and 1) need to get in before the merge on Oct 8. Urgency appreciated!

Comment on attachment 667643 [details] [diff] [review] Part 2: broadcast intent. v1 Review of attachment 667643 [details] [diff] [review]: ----------------------------------------------------------------- Are you sure you need to use a broadcast intent? Can't you just use Context.startService instead? Do you expect other things to listen for this?

> Are you sure you need to use a broadcast intent? Can't you just use > Context.startService instead? Do you expect other things to listen for this? This call is really a notification of a change across component boundaries, modeled using a broadcast intent, rather than an invocation of a persistent service; that a service exists at all is just an artifact of how broadcast listeners are implemented (a common pattern). This service dies as soon as it has futzed around with AlarmManager. (In other words, ACTION_SNIPPETS_PREF ~= desktop's onPrefChanged observer.) startService doesn't make any provision for the service stopping until stopService is called. Broadcast handlers do. Using startService tightly couples the snippet module lifecycle, which isn't what we want. [["Using startService() overrides the default service lifetime that is managed by bindService(Intent, ServiceConnection, int): it requires the service to remain running until stopService(Intent) is called, regardless of whether any clients are connected to it. Note that calls to startService() are not nesting: no matter how many times you call startService(), a single call to stopService(Intent) will stop it."]] If this looks dodgy to you, I'd be inclined to go the other way: decouple entirely by eliminating the direct class name access, and use an intent filter on SnippetsBroadcastReceiver with explicit exported=false. My original patches for this had another action for "Fennec has started", which we could reintroduce for one half of this. Thoughts? (Also, apparently I missed the ACTION_ off the front of the action string. Fixing locally.)

Actually, question on that last point. Which is correct style? Copying from GeckoApp.java: public static final String ACTION_ALERT_CALLBACK = "org.mozilla.gecko.ACTION_ALERT_CALLBACK"; public static final String ACTION_WEBAPP_PREFIX = "org.mozilla.gecko.WEBAPP"; public static final String ACTION_DEBUG = "org.mozilla.gecko.DEBUG";

Renaming this to something a little less contentious. I've made mechanical translations to the first two patches, which I intend to land soon (without part 2).

Part 0 and Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb72381f95c https://hg.mozilla.org/integration/mozilla-inbound/rev/dea201a3c919

https://hg.mozilla.org/mozilla-central/rev/0fb72381f95c https://hg.mozilla.org/mozilla-central/rev/dea201a3c919

Split out part 2 into Bug 799834. snorp, I'll give you an updated patch this week. Thanks!