Migrate some SessionManager usage to BrowserStore #10789
Conversation
app/src/main/java/org/mozilla/fenix/session/NotificationSessionObserver.kt
Outdated
Show resolved
Hide resolved
b9b013f
to
7038a8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for starting this. Left two minor comments below and there's a lint error to fix :).
app/src/main/java/org/mozilla/fenix/settings/deletebrowsingdata/DeleteBrowsingDataFragment.kt
Outdated
Show resolved
Hide resolved
7038a8f
to
a8c8a84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
a5e424d
to
6714d4b
Compare
Codecov Report
@@ Coverage Diff @@
## master #10789 +/- ##
============================================
+ Coverage 20.45% 20.46% +0.01%
+ Complexity 681 680 -1
============================================
Files 372 372
Lines 15434 15434
Branches 2080 2080
============================================
+ Hits 3157 3159 +2
+ Misses 11981 11979 -2
Partials 296 296
Continue to review full report at Codecov.
|
6714d4b
to
0ffff8a
Compare
notificationService.start(context) | ||
} | ||
fun stop() { | ||
scope?.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine since we want the notification to stick around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a few store observers / scopes tied to the whole application lifecycle e.g. MigrationTelemetryListener
, MigrationPushRenewer
. Those never get stopped and don't need to. I guess we could've not assigned the scope and removed the stop
method, but yes, it should not be a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekager @NotWoods Actually, we do have a problem here after all! The NotificationSessionObserver
is leaking the HomeActivity
as it's initialized in HomeActivity.onCreate
and uses it as a context. When the HomeActivitiy
is re-created, we never cancel the old scope and therefore leak the old HomeActivity
. You can reproduce this by enabling "Don't keep activities"...then leak canary gets pretty mad at us :).
@NotWoods I saw you were working on an uplift of this class so we can fix it there? Let's chat in our PR...
Pull Request checklist
After merge
To download an APK when reviewing a PR: