RUM-2503 Use internal key for View Scopes#1812
Conversation
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
Nice work! I left few questions.
| internal data class RumScopeKey( | ||
| val id: String, | ||
| val url: String, | ||
| val name: String? |
There was a problem hiding this comment.
It seems that this property is not used?
There was a problem hiding this comment.
Missed an additional commit
| is FragmentNavigator.Destination -> key.className | ||
|
|
||
| else -> key.javaClass.name | ||
| } |
There was a problem hiding this comment.
Minor: since the logic for resolveName and resolveUrl are the same, we could consolidate this by extracting the when to a third method - wdyt? Do we expect them to differ in future?
There was a problem hiding this comment.
They could be different in the future indeed
0xnm
left a comment
0xnm
left a comment
There was a problem hiding this comment.
lgtm! I've added one minor thought.
| val childScope = iterator.next() | ||
| if (event is RumRawEvent.StopView) { | ||
| if (childScope.isActive() && (childScope as? RumViewScope)?.keyRef?.get() == event.key) { | ||
| if (childScope.isActive() && (childScope as? RumViewScope)?.key?.id == event.key.id) { |
There was a problem hiding this comment.
In order to make sure that we check id property in the test and not the whole key, maybe we can modify the tests for this class (and for RumViewScope) to use different RumScopeKey for different calls (same ID, but different name/url), but it is not a blocker for me, more like something useful to add.
cecac6d to
7672da1
Compare
7672da1 to
04d4a93
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #1812 +/- ##
===========================================
+ Coverage 83.47% 83.49% +0.02%
===========================================
Files 466 467 +1
Lines 16360 16412 +52
Branches 2456 2482 +26
===========================================
+ Hits 13656 13702 +46
- Misses 2025 2032 +7
+ Partials 679 678 -1
|
5 participants
What does this PR do?
Use an Internal Key representation instead of a (weak) reference to actual customer instances.
Motivation
Prevent leaking memory with UI related instances (Activity, Fragment, …)