Implement Android campaign product announcements opt-out UX

RESOLVED FIXED in Firefox 18

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

18 Branch
Firefox 18
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snippets])

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
No longer depends on: 793053

Updated

6 years ago
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).

Updated

6 years ago
Whiteboard: [snippets]
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: Implement Android Snippets opt-in and opt-out UX → Implement Android Snippets opt-out UX
Version: Firefox 17 → Firefox 18
(Assignee)

Comment 3

6 years ago
Created attachment 665270 [details] [diff] [review]
Settings UX, strings, broadcast intent integration.

Pretty much done.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
Created attachment 665275 [details]
Screenshot.
(Assignee)

Comment 5

6 years ago
Remaining work:

* Final string
* Why does the checkbox state persist across restarts, yet SharedPreferences reports the wrong value?
* Cleanup etc.
(Assignee)

Comment 6

6 years ago
Created attachment 666156 [details] [diff] [review]
Settings UX, strings, broadcast intent integration. v2

Now persists. String is final per ibarlow.
Attachment #665270 - Attachment is obsolete: true

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
Created attachment 667638 [details] [diff] [review]
Part 0: cleanup. v1

Magic constants etc.
Attachment #666156 - Attachment is obsolete: true
Attachment #667638 - Flags: review?(bnicholson)
(Assignee)

Comment 9

6 years ago
Created attachment 667641 [details] [diff] [review]
Part 1: settings UI. v1

Adds string (approved by ibarlow), checkbox.
Attachment #667641 - Flags: review?(bnicholson)
(Assignee)

Comment 10

6 years ago
Created attachment 667643 [details] [diff] [review]
Part 2: broadcast intent. v1

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)
(Assignee)

Comment 11

6 years ago
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?
(Assignee)

Comment 13

6 years ago
> 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.)
(Assignee)

Comment 14

6 years ago
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";
(Assignee)

Comment 15

5 years ago
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]
(Assignee)

Updated

5 years ago
Blocks: 789296
(Assignee)

Updated

5 years ago
Blocks: 799834
(Assignee)

Comment 18

5 years ago
Split out part 2 into Bug 799834. snorp, I'll give you an updated patch this week. Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Whiteboard: [snippets][leave open after merge] → [snippets]
Target Milestone: --- → Firefox 18
(Assignee)

Updated

5 years ago
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.