Closed Bug 793056 Opened 7 years ago Closed 7 years ago

Implement Android campaign product announcements opt-out UX

Categories

(Firefox for Android :: General, defect, P1)

18 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Whiteboard: [snippets])

Attachments

(3 files, 3 obsolete files)

No description provided.
No longer depends on: 793053
Keywords: uiwanted
* 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).
Whiteboard: [snippets]
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.
Summary: Implement Android Snippets opt-in and opt-out UX → Implement Android Snippets opt-out UX
Version: Firefox 17 → Firefox 18
Pretty much done.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attached image Screenshot.
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.
Attachment #665270 - Attachment is obsolete: true
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.
Attachment #666156 - Attachment is obsolete: true
Attachment #667638 - Flags: review?(bnicholson)
Adds string (approved by ibarlow), checkbox.
Attachment #667641 - Flags: review?(bnicholson)
Attached patch Part 2: broadcast intent. v1 (obsolete) — Splinter Review
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.
Attachment #667643 - Flags: review?(snorp)
Reviewer note: the UI for this (parts 0 and 1) need to get in before the merge on Oct 8. Urgency appreciated!
Attachment #667638 - Flags: review?(bnicholson) → review+
Attachment #667641 - Flags: review?(bnicholson) → review+
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).
Summary: Implement Android Snippets opt-out UX → Implement Android campaign product announcements opt-out UX
Whiteboard: [snippets] → [snippets][leave open after merge]
Blocks: 789296
Blocks: 799834
Split out part 2 into Bug 799834. snorp, I'll give you an updated patch this week. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Whiteboard: [snippets][leave open after merge] → [snippets]
Target Milestone: --- → Firefox 18
Attachment #667643 - Attachment is obsolete: true
Attachment #667643 - Flags: review?(snorp)
You need to log in before you can comment on or make changes to this bug.