-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Closes #18948: Update tabstray telemetry #19004
Conversation
Request for data collection review
|
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.
One question, but LGTM for a code review.
@@ -225,6 +227,11 @@ class TabsTrayFragment : AppCompatDialogFragment(), TabsTrayInteractor { | |||
) | |||
} | |||
|
|||
override fun onDestroyView() { | |||
requireComponents.analytics.metrics.track(Event.TabsTrayClosed) |
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.
I'm not sure about adding this to onDestroyView
. Would this be called even on non-user generated events? System wants to regain memory, for example.
Thinking about where else this could go, we could put it in the NavigationInteractor.onTabTrayDismissed
We can de-dupe the two callers dismissTabTray
and dismissTabTrayAndNavigateHome
, and make sure they both invoke the event.
Just a thought; haven't tested it out.
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.
Agreed. I'll update. Thanks,
) : BottomSheetBehavior.BottomSheetCallback() { | ||
|
||
override fun onStateChanged(bottomSheet: View, newState: Int) { | ||
if (newState == STATE_HIDDEN) { | ||
metrics.track(Event.TabsTrayClosed) |
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 catch!
One test is stuck. Closing and reopening to rerun them. |
Data Review
Resultdata-review+ |
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.
Approval for data-review+ only, I didn't code-review this.
Codecov Report
@@ Coverage Diff @@
## master #19004 +/- ##
============================================
+ Coverage 34.14% 34.34% +0.19%
- Complexity 1575 1576 +1
============================================
Files 535 535
Lines 21636 21653 +17
Branches 3222 3227 +5
============================================
+ Hits 7387 7436 +49
+ Misses 13352 13334 -18
+ Partials 897 883 -14
Continue to review full report at Codecov.
|
Pull Request checklist
To download an APK when reviewing a PR: