-
Notifications
You must be signed in to change notification settings - Fork 1k
Add support for reportMetric message from CSS #6397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
3e32b71
to
1b00329
Compare
1b00329
to
aa0f7f8
Compare
@@ -291,6 +292,19 @@ class DuckPlayerJSHelper @Inject constructor( | |||
pixel.fire(impressionPixelName) | |||
pixel.fire(dailyPixelName, emptyMap(), emptyMap(), Daily()) | |||
} | |||
"reportMetric" -> { |
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.
IMO we are leaking by default a lot of implementation details and using DuckPlayerPageHandler
as a bypass rather than extracting messages to their own handler. Then all these messages have a meaningless trip around the tab fragment, view model, js helper, etc.
This is a perfect example of that, you could do something like this instead which will keep all the logic in impl (I would actually do an internal plugin just for duck player to not have a lot of inner classes and sort out DI but this works for now anyways).
inner class DuckPlayerReportMetricHandler(private val pixel: Pixel) : JsMessageHandler {
override fun process(jsMessage: JsMessage, secret: String, jsMessageCallback: JsMessageCallback?) {
try {
val params = jsMessage.params
val message = params.getString("message") ?: return
val kind = params.getString("kind") ?: return
pixel.fire(
DUCK_PLAYER_JS_ERROR,
mapOf("message" to message, "kind" to kind, "origin" to featureName),
)
} catch (e: Exception) {
logcat { "Error reporting metric: $e" }
}
}
override val allowedDomains: List<String> = listOf(
runBlocking { duckPlayer.getYouTubeEmbedUrl() },
)
override val featureName: String = "duckPlayerPage"
override val methods: List<String> = listOf(
"reportMetric",
)
}
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.
LGTM
Task/Issue URL: https://app.asana.com/1/137249556945/project/45878998844068/task/1210462957793120?focus=true
Description
Fire pixel when
reportMetric
message comes from CSSSteps to test this PR
Feature 1
?willThrow=true
and load againduckplayer_js-error with params: {message=Simulated Exception, kind=Error, origin=duckPlayerPage} {}
is fired