Closed Bug 1208268 Opened 4 years ago Closed 4 years ago

Decouple Sync notifications from Sync backend by moving code to a BroadcastReceiver

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: mcomella, Assigned: mcomella, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java])

Attachments

(1 file)

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.
Summary: Decouple Sync notifications from Sync backend → Decouple Sync notifications from Sync backend by moving code to a BroadcastReceiver
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.
Attachment #8665712 - Attachment description: MozReview Request: Bug 1208268 - WIP → MozReview Request: Bug 1208268 - Move tab received notifications to a broadcast receiver. r=nalexander
Attachment #8665712 - Flags: review?(nalexander)
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.
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
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.
re comment 12:
  * I saw no better package – it stays in sync until bug 1214332
  * Did not go with SafeIntent
https://hg.mozilla.org/mozilla-central/rev/98a6d623cea7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.