We want to separate the back-end Sync service from any UI it can produce – this is one such instance. CommandProcessor.displayURI adds a Sync'ed tab notification. Instead, it should send a broadcast and the receiver of that broadcast should display the notification instead.

Bug 1208268 - WIP

I made a quick WIP but I stopped before registering/unregistering the LocalBroadcastReceivers and sending the Intent from CommandProcessor to the new class.

afaict, LocalBroadcastReceivers need an instance of an Activity, Service, etc. to be around to receive the broadcast so we should register a global receiver instead.

Comment on attachment 8665712 [details] MozReview Request: Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander I tested multi-locale builds and the notification is in the new language as expected.

There are two TODOs still remaining, primarily because we can't store state in a BroadcastReceiver: 1) We want to avoid doing extra work by only running a locale update when we haven't already updated the locale – I don't really know how this code works so I'm not sure this line is even necessary. 2) We need to make sure our notification IDs don't conflict. Without storing state, this could be solved by a random int. Alternatively, we can access this data from sharedprefs but I don't know the implications of doing such an action in a BroadcastReceiver since they're intended to be short-lived. Perhaps an IntentService would be more appropriate here? Any thoughts, Nick?

(In reply to Michael Comella (:mcomella) from comment #5) > 1) We want to avoid doing extra work by only running a locale update when > we haven't already updated the locale – I don't really know how this code > works so I'm not sure this line is even necessary. I tested and it is necessary. With Firefox not-in-memory, if you send a tab to that browser instance, the notification won't be in the browser locale. Since this is a BroadcastReceiver that does not maintain state, we probably need to run this locale bit everytime, unless Fennec is running.

Comment on attachment 8665712 [details] MozReview Request: Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander I tested multi-locale builds and the notification is in the new language as expected.

YOLO! I just went ahead and solved this by: 1) Updating the locale everytime. 2) Saving the notification IDs to shared preferences which is allowed in BroadcastReceivers. Let me know what you think.

Updating the locale is very cheap, so YOLO is fine.

Comment on attachment 8665712 [details] MozReview Request: Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander https://reviewboard.mozilla.org/r/20305/#review19569 Looks good! My only suggestion would be to make the receiver a little more generic and move it closer to the tab queue code. It's triggered by Sync, but not really anything to do with Sync. ::: mobile/android/base/sync/CommandProcessor.java:252 (Diff revision 3) > - // unnecessary work. > + sendTabNotificationIntent.setData(Uri.parse(uri)); I worry that `Uri.parse` will throw, but the docs say it won't (and presumably returns `null` for failure). I assume `setData(null)` is okay. ::: mobile/android/base/sync/TabReceivedBroadcastReceiver.java:1 (Diff revision 3) > +// This Source Code Form is subject to the terms of the Mozilla Public nit: I thought we used ``` /** */ ``` ::: mobile/android/base/sync/TabReceivedBroadcastReceiver.java:29 (Diff revision 3) > + private static final int MAX_NOTIFICATION_COUNT = 1000; I think we should cap this to 2 or 3 or 10 for now, and either build better UI for tabs sent recently, or rotate old tabs into the Tab Queue. I can't imagine having 1000 notifications being helpful to anybody. ::: mobile/android/base/sync/TabReceivedBroadcastReceiver.java:38 (Diff revision 3) > + final String uri = intent.getDataString(); This is not exported (yay!) but should we be using `SafeIntent` anyway? ::: mobile/android/base/sync/TabReceivedBroadcastReceiver.java:46 (Diff revision 3) > + final Intent notificationIntent = new Intent(Intent.ACTION_VIEW, intent.getData()); Can you spell out that the Intent contents will be inspected and passed through? I feel it's a good place for a future bug to arise, as the receiving Intent interpretation evolves.

(In reply to Michael Comella (:mcomella) from comment #8) > YOLO! I just went ahead and solved this by: Thumbs up for just trying a thing. Your comments were informative. > 1) Updating the locale everytime. Per rnewman, roll with it. > 2) Saving the notification IDs to shared preferences which is allowed in > BroadcastReceivers. Not sure I would have bothered -- limiting to 1000 is useless; better to really do a good thing with the complexity or save the effort entirely.

(In reply to Nick Alexander :nalexander from comment #10) > Looks good! My only suggestion would be to make the receiver a little more > generic and move it closer to the tab queue code. It's triggered by Sync, > but not really anything to do with Sync. I tried to copy the code verbatim into a new Receiver – I filed bug 1214332 to investigate this. I'll look if there's a better package for me to move this to in the mean time. > ::: mobile/android/base/sync/CommandProcessor.java:252 > I worry that `Uri.parse` will throw, but the docs say it won't I actually see it throws NPE when the uri is null – I'll check for it. > > + private static final int MAX_NOTIFICATION_COUNT = 1000; > > I think we should cap this to 2 or 3 or 10 for now, and either build better > UI for tabs sent recently I'll be building an aggregation of notifications in bug 1183659. > > + final String uri = intent.getDataString(); > > This is not exported (yay!) but should we be using `SafeIntent` anyway? I don't think this is necessary because it's to protect against storage of references to potential dead objects in Intents, which I don't think we can do within our process but I'll double-check. > Can you spell out that the Intent contents will be inspected and passed > through? I'll add to the class comment.

https://hg.mozilla.org/integration/fx-team/rev/98a6d623cea7bd34c1785ebb2ef81f85e5cec560 Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander

re comment 12: * I saw no better package – it stays in sync until bug 1214332 * Did not go with SafeIntent