Skip to content

Commit e317b45

Browse files
satweensatween
committed
RUM-8785: post-review fixes
1 parent be6f712 commit e317b45

File tree

7 files changed

+76
-38
lines changed

7 files changed

+76
-38
lines changed

‎detekt_custom.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,7 @@ datadog:
424424
- "android.view.View.getChildAt(kotlin.Int)"
425425
- "android.view.View.getTag(kotlin.Int)"
426426
- "android.view.View.hashCode()"
427+
- "android.view.View.post(java.lang.Runnable?)"
427428
- "android.view.View.setTag(kotlin.Int, kotlin.Any?)"
428429
- "android.view.ViewGroup.findViewById(kotlin.Int)"
429430
- "android.view.ViewGroup.getChildAt(kotlin.Int)"
@@ -1097,6 +1098,7 @@ datadog:
10971098
- "kotlin.collections.emptySet()"
10981099
- "kotlin.collections.listOf(android.view.Window)"
10991100
- "kotlin.collections.listOf(com.datadog.android.api.InternalLogger.Target)"
1101+
- "kotlin.collections.listOf(com.datadog.android.rum.internal.vitals.FPSVitalListener)"
11001102
- "kotlin.collections.listOf(com.datadog.android.rum.model.ActionEvent.Interface)"
11011103
- "kotlin.collections.listOf(com.datadog.android.rum.model.ErrorEvent.Interface)"
11021104
- "kotlin.collections.listOf(com.datadog.android.rum.model.LongTaskEvent.Interface)"

‎features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,6 @@ internal class RumFeature(
417417
)
418418

419419
jankStatsActivityLifecycleListener = JankStatsActivityLifecycleListener(
420-
@Suppress("UnsafeThirdPartyFunctionCall") // it's safe to call listOf here
421420
listOf(
422421
FPSVitalListener(frameRateVitalMonitor)
423422
),

‎features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/FrameMetricsData.kt

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,9 @@ internal data class FrameMetricsData(
2323
@RequiresApi(Build.VERSION_CODES.O) var vsyncTimestamp: Long = 0L,
2424
@RequiresApi(Build.VERSION_CODES.S) var gpuDuration: Long = 0L,
2525
@RequiresApi(Build.VERSION_CODES.S) var deadline: Long = 0L,
26-
var displayRefreshRate: Double = 1.0
27-
)
26+
var displayRefreshRate: Double = SIXTY_FPS
27+
) {
28+
companion object {
29+
private const val SIXTY_FPS: Double = 60.0
30+
}
31+
}

‎features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/vitals/FPSVitalListener.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ internal class FPSVitalListener(
1616
private val buildSdkVersionProvider: BuildSdkVersionProvider = BuildSdkVersionProvider.DEFAULT,
1717
private var screenRefreshRate: Double = 60.0
1818
) : FrameStateListener {
19-
private var frameDeadline = SIXTEEN_MS_NS
19+
private var frameDeadline = EXPECTED_60_FPS_FRAME_DURATION_NS
2020
private var displayRefreshRate: Double = SIXTY_FPS
2121

2222
override fun onFrame(volatileFrameData: FrameData) {
@@ -48,7 +48,7 @@ internal class FPSVitalListener(
4848
}
4949

5050
companion object {
51-
private const val SIXTEEN_MS_NS: Long = 16666666
51+
private const val EXPECTED_60_FPS_FRAME_DURATION_NS: Long = 16_666_666L
5252
private val ONE_SECOND_NS: Double = TimeUnit.SECONDS.toNanos(1).toDouble()
5353

5454
private const val MIN_FPS: Double = 1.0

‎features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/vitals/JankStatsActivityLifecycleListener.kt

Lines changed: 41 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ internal class JankStatsActivityLifecycleListener(
143143
if (activeActivities[activity.window].isNullOrEmpty()) {
144144
activeWindowsListener.remove(activity.window)
145145
activeActivities.remove(activity.window)
146-
if (buildSdkVersionProvider.version >= Build.VERSION_CODES.S) {
146+
if (buildSdkVersionProvider.version >= Build.VERSION_CODES.N) {
147147
unregisterMetricListener(activity.window)
148148
}
149149
}
@@ -199,7 +199,7 @@ internal class JankStatsActivityLifecycleListener(
199199
@SuppressLint("NewApi")
200200
@MainThread
201201
private fun trackWindowMetrics(isKnownWindow: Boolean, window: Window, activity: Activity) {
202-
if (buildSdkVersionProvider.version >= Build.VERSION_CODES.S && !isKnownWindow) {
202+
if (buildSdkVersionProvider.version >= Build.VERSION_CODES.N && !isKnownWindow) {
203203
registerMetricListener(window)
204204
} else if (display == null && buildSdkVersionProvider.version == Build.VERSION_CODES.R) {
205205
// Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have
@@ -209,29 +209,54 @@ internal class JankStatsActivityLifecycleListener(
209209
}
210210
}
211211

212-
@RequiresApi(Build.VERSION_CODES.S)
212+
@RequiresApi(Build.VERSION_CODES.N)
213213
private fun registerMetricListener(window: Window) {
214214
if (frameMetricsListener == null) {
215215
frameMetricsListener = DDFrameMetricsListener()
216216
}
217+
// todo RUM-8799: handler thread can be used instead
217218
val handler = Handler(Looper.getMainLooper())
218-
// Only hardware accelerated views can be tracked with metrics listener
219-
frameMetricsListener?.let { listener ->
220-
try {
221-
@Suppress("UnsafeThirdPartyFunctionCall") // Listener can't be null here
222-
window.addOnFrameMetricsAvailableListener(listener, handler)
223-
} catch (e: IllegalStateException) {
219+
val decorView = window.peekDecorView()
220+
221+
if (decorView == null) {
222+
internalLogger.log(
223+
InternalLogger.Level.WARN,
224+
InternalLogger.Target.MAINTAINER,
225+
{ "Unable to attach JankStatsListener to window, decorView is null" }
226+
)
227+
return
228+
}
229+
230+
// We need to postpone this operation because isHardwareAccelerated will return
231+
// false until the view is attached to the window. Note that in this case main looper should be used
232+
decorView.post {
233+
// Only hardware accelerated views can be tracked with metrics listener
234+
if (!decorView.isHardwareAccelerated) {
224235
internalLogger.log(
225-
InternalLogger.Level.ERROR,
236+
InternalLogger.Level.WARN,
226237
InternalLogger.Target.MAINTAINER,
227-
{ "Unable to attach JankStatsListener to window" },
228-
e
238+
{ "Unable to attach JankStatsListener to window, decorView is not hardware accelerated" }
229239
)
240+
return@post
241+
}
242+
243+
frameMetricsListener?.let { listener ->
244+
try {
245+
@Suppress("UnsafeThirdPartyFunctionCall") // Listener can't be null here
246+
window.addOnFrameMetricsAvailableListener(listener, handler)
247+
} catch (e: IllegalStateException) {
248+
internalLogger.log(
249+
InternalLogger.Level.ERROR,
250+
InternalLogger.Target.MAINTAINER,
251+
{ "Unable to attach JankStatsListener to window" },
252+
e
253+
)
254+
}
230255
}
231256
}
232257
}
233258

234-
@RequiresApi(Build.VERSION_CODES.S)
259+
@RequiresApi(Build.VERSION_CODES.N)
235260
private fun unregisterMetricListener(window: Window) {
236261
try {
237262
window.removeOnFrameMetricsAvailableListener(frameMetricsListener)
@@ -248,7 +273,7 @@ internal class JankStatsActivityLifecycleListener(
248273
@RequiresApi(Build.VERSION_CODES.N)
249274
inner class DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {
250275

251-
@RequiresApi(Build.VERSION_CODES.S)
276+
@RequiresApi(Build.VERSION_CODES.N)
252277
override fun onFrameMetricsAvailable(
253278
window: Window,
254279
frameMetrics: FrameMetrics,
@@ -271,7 +296,7 @@ internal class JankStatsActivityLifecycleListener(
271296
commandIssueDuration = frameMetrics.getMetric(FrameMetrics.COMMAND_ISSUE_DURATION)
272297
swapBuffersDuration = frameMetrics.getMetric(FrameMetrics.SWAP_BUFFERS_DURATION)
273298
totalDuration = frameMetrics.getMetric(FrameMetrics.TOTAL_DURATION)
274-
firstDrawFrame = frameMetrics.getMetric(FrameMetrics.FIRST_DRAW_FRAME) == 1L
299+
firstDrawFrame = frameMetrics.getMetric(FrameMetrics.FIRST_DRAW_FRAME) == TRUE
275300
}
276301
@SuppressLint("InlinedApi")
277302
if (buildSdkVersionProvider.version >= Build.VERSION_CODES.O) {
@@ -295,5 +320,6 @@ internal class JankStatsActivityLifecycleListener(
295320
internal const val JANK_STATS_TRACKING_DISABLE_ERROR =
296321
"Failed to disable JankStats tracking"
297322
private const val SIXTY_FPS: Double = 60.0
323+
private const val TRUE = 1L
298324
}
299325
}

‎features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/vitals/FPSVitalListenerTest.kt

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ internal class FPSVitalListenerTest {
5757
on { version } doReturn Build.VERSION_CODES.VANILLA_ICE_CREAM
5858
}
5959

60-
private lateinit var listenerUnderTest: FPSVitalListener
60+
private lateinit var testedListener: FPSVitalListener
6161

6262
@BeforeEach
6363
fun `set up`() {
64-
listenerUnderTest = FPSVitalListener(mockObserver, mockBuildSdkVersionProvider)
64+
testedListener = FPSVitalListener(mockObserver, mockBuildSdkVersionProvider)
6565
}
6666

6767
@Test
@@ -75,7 +75,7 @@ internal class FPSVitalListenerTest {
7575
val frameData = FrameData(timestampNs, frameDurationNs, isJank, emptyList())
7676

7777
// When
78-
listenerUnderTest.onFrame(frameData)
78+
testedListener.onFrame(frameData)
7979

8080
// Then
8181
verify(mockObserver).onNewSample(eq(expectedFrameRate, 0.0001))
@@ -89,7 +89,7 @@ internal class FPSVitalListenerTest {
8989
val frameData = FrameData(timestampNs, 0L, isJank, emptyList())
9090

9191
// When
92-
listenerUnderTest.onFrame(frameData)
92+
testedListener.onFrame(frameData)
9393

9494
// Then
9595
verify(mockObserver, never()).onNewSample(any())
@@ -110,14 +110,14 @@ internal class FPSVitalListenerTest {
110110

111111
whenever(mockBuildSdkVersionProvider.version) doReturn Build.VERSION_CODES.S
112112

113-
listenerUnderTest.onFrameMetricsData(
113+
testedListener.onFrameMetricsData(
114114
FrameMetricsData(
115115
deadline = (ONE_SECOND_NS / displayRefreshRate).toLong()
116116
)
117117
)
118118

119119
// When
120-
listenerUnderTest.onFrame(frameData)
120+
testedListener.onFrame(frameData)
121121

122122
// Then
123123
if (expectedFrameRate * refreshRateMultiplier > MIN_FPS) {
@@ -142,14 +142,14 @@ internal class FPSVitalListenerTest {
142142

143143
whenever(mockBuildSdkVersionProvider.version) doReturn Build.VERSION_CODES.R
144144

145-
listenerUnderTest.onFrameMetricsData(
145+
testedListener.onFrameMetricsData(
146146
FrameMetricsData(
147147
displayRefreshRate = displayRefreshRate
148148
)
149149
)
150150

151151
// When
152-
listenerUnderTest.onFrame(frameData)
152+
testedListener.onFrame(frameData)
153153

154154
// Then
155155
if (expectedFrameRate * refreshRateMultiplier > MIN_FPS) {

‎features/dd-sdk-android-rum/src/test/kotlin/com/datadog/android/rum/internal/vitals/JankStatsActivityLifecycleListenerTest.kt

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import fr.xgouchet.elmyr.junit5.ForgeExtension
3030
import org.assertj.core.api.Assertions.assertThat
3131
import org.junit.jupiter.api.BeforeEach
3232
import org.junit.jupiter.api.Test
33+
import org.junit.jupiter.api.assertDoesNotThrow
3334
import org.junit.jupiter.api.extension.ExtendWith
3435
import org.junit.jupiter.api.extension.Extensions
3536
import org.mockito.Mock
@@ -289,6 +290,10 @@ internal class JankStatsActivityLifecycleListenerTest {
289290
testedJankListener.onActivityStarted(mockActivity)
290291
testedJankListener.onActivityStopped(mockActivity)
291292
testedJankListener.onActivityStarted(mockActivity)
293+
argumentCaptor<Runnable> {
294+
verify(mockDecorView).post(capture())
295+
lastValue.run()
296+
}
292297

293298
// Then
294299
verify(mockWindow).addOnFrameMetricsAvailableListener(any(), any()) // should be called only once
@@ -351,10 +356,9 @@ internal class JankStatsActivityLifecycleListenerTest {
351356
whenever(mockWindow.addOnFrameMetricsAvailableListener(any(), any())) doThrow IllegalStateException()
352357

353358
// When
354-
testedJankListener.onActivityStarted(mockActivity)
355-
356-
// Then
357-
// no-crash
359+
assertDoesNotThrow {
360+
testedJankListener.onActivityStarted(mockActivity)
361+
}
358362
}
359363

360364
@Test
@@ -363,11 +367,10 @@ internal class JankStatsActivityLifecycleListenerTest {
363367
whenever(mockWindow.removeOnFrameMetricsAvailableListener(any())) doThrow IllegalArgumentException()
364368

365369
// When
366-
testedJankListener.onActivityStarted(mockActivity)
367-
testedJankListener.onActivityDestroyed(mockActivity)
368-
369-
// Then
370-
// no-crash
370+
assertDoesNotThrow {
371+
testedJankListener.onActivityStarted(mockActivity)
372+
testedJankListener.onActivityDestroyed(mockActivity)
373+
}
371374
}
372375

373376
@Test
@@ -384,7 +387,6 @@ internal class JankStatsActivityLifecycleListenerTest {
384387
// Given
385388
val dropCountSinceLastInvocation = forge.aSmallInt()
386389
val frameMetricsData = forge.getForgery<FrameMetricsData>()
387-
388390
val frameMetrics = mock<FrameMetrics> {
389391
on { getMetric(FrameMetrics.UNKNOWN_DELAY_DURATION) } doReturn frameMetricsData.unknownDelayDuration
390392
on { getMetric(FrameMetrics.INPUT_HANDLING_DURATION) } doReturn frameMetricsData.inputHandlingDuration
@@ -401,8 +403,13 @@ internal class JankStatsActivityLifecycleListenerTest {
401403
on { getMetric(FrameMetrics.GPU_DURATION) } doReturn frameMetricsData.gpuDuration
402404
on { getMetric(FrameMetrics.DEADLINE) } doReturn frameMetricsData.deadline
403405
}
406+
whenever(mockDecorView.isHardwareAccelerated) doReturn true
404407

405408
testedJankListener.onActivityStarted(mockActivity)
409+
argumentCaptor<Runnable> {
410+
verify(mockDecorView).post(capture())
411+
firstValue.run()
412+
}
406413

407414
argumentCaptor<Window.OnFrameMetricsAvailableListener> {
408415
verify(mockWindow).addOnFrameMetricsAvailableListener(capture(), any())

0 commit comments

Comments
 (0)