Additional "default browser" prompts: metrics #5543

LukasPaczos · 2025-01-27T11:38:49Z

Task/Issue URL: https://app.asana.com/0/1201621853593513/1208697958169120/f

Marking as blocked until all pixels are confirmed with the DS team. This is generally ready for a first round of reviews but some additional changes may be still introduced because of that.

Steps to test this PR

You can follow instructions from #5476 to go through the experiment.

Test for below pixels:

Action	Pixel
Enrolled	`experiment_enroll_defaultBrowserAdditionalPrompts202501_variant_2 with params: {enrollmentDate=2025-01-27}`
Reached Stage 1	`experiment_metrics_defaultBrowserAdditionalPrompts202501_variant_2 with params: {metric=stageImpression, value=stage_1, enrollmentDate=2025-01-27, conversionWindowDays=1-X}` (`X` is 20, 40, 60)
Reached Stage 2	`experiment_metrics_defaultBrowserAdditionalPrompts202501_variant_2 with params: {metric=stageImpression, value=stage_2, enrollmentDate=2025-01-27, conversionWindowDays=1-X}` (`X` is 20, 40, 60)
Message dialog dismissed or "not now" clicked	`m_set-as-default_prompt_dismissed with params: {expVar=variant_2, expStage=stage_1}`
Message dialog confirmation clicked	`m_set-as-default_prompt_click with params: {expVar=variant_2, expStage=stage_1}`
Menu item clicked	`m_set-as-default_in-menu_click with params: {expVar=variant_2, expStage=stage_2}`
Browser set as default (from any source)	`experiment_metrics_defaultBrowserAdditionalPrompts202501_variant_2 with params: {metric=defaultSet, value=stage_2, enrollmentDate=2025-01-27, conversionWindowDays=1-X}` (`X` is 20, 40, 60)
Browser set as default when preceding action is dialog or menu. This is sent in addition to the above metric.	`experiment_metrics_defaultBrowserAdditionalPrompts202501_variant_2 with params: {metric=defaultSetViaCta, value=menu, enrollmentDate=2025-01-27, conversionWindowDays=1-X}` (`X` is 20, 40, 60)

...com/duckduckgo/app/browser/defaultbrowsing/prompts/DefaultBrowserPromptsExperimentMetrics.kt

LukasPaczos · 2025-02-03T13:16:52Z

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

@@ -361,19 +367,21 @@ class BrowserViewModel @Inject constructor(
    }

    fun onSystemDefaultBrowserDialogSuccess() {
-        defaultBrowserPromptsExperiment.onSystemDefaultBrowserDialogSuccess()
+        defaultBrowserPromptsExperiment.onSystemDefaultBrowserDialogSuccess(
+            lastSystemDefaultBrowserDialogTrigger ?: SetAsDefaultActionTrigger.UNKNOWN,


In practice, we should never fall back to UNKNOWN. I added it as a safeguard because after briefly looking at the problem I couldn't find a non-nullable way to round-trip a parameter through the "activity for result" stack.

Since we can't launch the default browser dialog or system apps and expect them to return a specific parameter back, we have to store it locally. This makes the parameter nullable, which is why we need the UNKNOWN fallback.

Why not make a case of the enum for not being handled and set it as default instead? You are checking for a null parameter, why not checking for the non-handled case?

I have a case in DefaultBrowserPromptsExperimentImpl to handle the UNKNOWN type, if that's what you mean. I can also just make the cache field default to UNKNOWN to get rid of the elvis operator, it won't change anything functionally in the end.

LukasPaczos · 2025-02-03T13:22:35Z

This is ready for review.

malmstein

Nothing blocking, I’ve got a couple questions on the implementation.

malmstein · 2025-02-04T21:05:30Z

app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt

@@ -361,19 +367,21 @@ class BrowserViewModel @Inject constructor(
    }

    fun onSystemDefaultBrowserDialogSuccess() {
-        defaultBrowserPromptsExperiment.onSystemDefaultBrowserDialogSuccess()
+        defaultBrowserPromptsExperiment.onSystemDefaultBrowserDialogSuccess(
+            lastSystemDefaultBrowserDialogTrigger ?: SetAsDefaultActionTrigger.UNKNOWN,


Why not make a case of the enum for not being handled and set it as default instead? You are checking for a null parameter, why not checking for the non-handled case?

malmstein · 2025-02-04T21:16:03Z

...va/com/duckduckgo/app/browser/defaultbrowsing/prompts/DefaultBrowserPromptsExperimentImpl.kt

    }

    override fun onMessageDialogNotNowButtonClicked() {
+        fireInteractionPixel(AppPixelName.SET_AS_DEFAULT_PROMPT_DISMISSED)


don’t we want to differentiate between dismissing a dialog and actively saying no?

The pixel is defined as "triggered when user clicks 'Not now' or uses a gesture to close the prompt". In the end, regardless of whether user collapses the dialog or clicks not now, the intention is the same.

malmstein · 2025-02-04T21:21:09Z

...va/com/duckduckgo/app/browser/defaultbrowsing/prompts/DefaultBrowserPromptsExperimentImpl.kt

    }

-    override fun onMessageDialogDismissed() {
+    override fun onMessageDialogCanceled() {
+        fireInteractionPixel(AppPixelName.SET_AS_DEFAULT_PROMPT_DISMISSED)


This is still a dismissed pixel, but it’s cancelled instead.

"Dismissed" and "canceled" mean different things in context of an Android dialog, which doesn't translate to what we mean by "dismissed" in context of the pixel. The pixel wants to know when user didn't take the action and closed the dialog either by a gesture or by a "not now" button.

In the Android impl, dialog cancellation represents one of ways to dismiss a dialog, which includes clicking away and collapsing it.
On the other hand, dialog dismissal happens at any time a dialog is removed, including when a gesture is used (so when the dialog is canceled), but also when we're programmatically dismissing a dialog after either button is clicked by the user.

In our case, clicking the "set as default" button on the dialog sends it's own "click" pixel, and we don't want to send a "dismissed" pixel at the same time when we programmatically close the dialog after the user interaction.

So each of the buttons sends its own respective pixel and dismisses the dialog, and we also listen for when dialog is canceled (no button was pressed) and send the respective pixel as well.

malmstein · 2025-02-04T21:21:48Z

...ava/com/duckduckgo/app/browser/defaultbrowsing/prompts/ui/DefaultBrowserBottomSheetDialog.kt

@@ -47,8 +47,8 @@ class DefaultBrowserBottomSheetDialog(private val context: Context) : BottomShee
            setRoundCorners(dialogInterface)
            eventListener?.onShown()
        }
-        setOnDismissListener {
-            eventListener?.onDismissed()
+        setOnCancelListener {


what’s the reasoning for this?

Explained in #5543 (comment).

malmstein · 2025-02-04T21:23:10Z

app/src/main/java/com/duckduckgo/app/pixels/AppPixelName.kt

@@ -383,4 +383,9 @@ enum class AppPixelName(override val pixelName: String) : Pixel.PixelName {
    DEDICATED_WEBVIEW_URL_EXTRACTION_FAILED("m_dedicated_webview_url_extraction_failed"),

    BLOCKLIST_TDS_FAILURE("blocklist_experiment_tds_download_failure"),
+
+    SET_AS_DEFAULT_PROMPT_IMPRESSION("m_set-as-default_prompt_impression"),


either use “-“ or “_” but don’t use a combination of both.

I'm using this to treat the new pixels like a tree:

m

set-as-default

prompt

impression

click

dismissed

in-menu

click

I see various examples of that in the codebase, but please let me know if that should cause any issues.

LukasPaczos added the BLOCKED: DO NOT MERGE label Jan 27, 2025

LukasPaczos requested review from malmstein and marcosholgado January 27, 2025 11:38

malmstein self-assigned this Jan 28, 2025

marcosholgado reviewed Jan 29, 2025

View reviewed changes

...com/duckduckgo/app/browser/defaultbrowsing/prompts/DefaultBrowserPromptsExperimentMetrics.kt Outdated Show resolved Hide resolved

LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-variant-2 branch 3 times, most recently from 1397fda to cc2c9a8 Compare January 31, 2025 12:59

Base automatically changed from feature/lukasz-p/default-browser-prompts-variant-2 to develop January 31, 2025 13:32

LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-metrics branch 6 times, most recently from f8b2c44 to ef11c74 Compare February 3, 2025 13:07

LukasPaczos commented Feb 3, 2025

View reviewed changes

LukasPaczos removed the BLOCKED: DO NOT MERGE label Feb 3, 2025

malmstein approved these changes Feb 4, 2025

View reviewed changes

Additional "default browser" prompts: metrics

4ce4e1b

LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-metrics branch from ef11c74 to 4ce4e1b Compare February 5, 2025 10:10

LukasPaczos enabled auto-merge (squash) February 5, 2025 10:12

LukasPaczos merged commit 37e34fb into develop Feb 5, 2025
5 checks passed

LukasPaczos deleted the feature/lukasz-p/default-browser-prompts-metrics branch February 5, 2025 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional "default browser" prompts: metrics #5543

Additional "default browser" prompts: metrics #5543

LukasPaczos commented Jan 27, 2025 •

edited

Loading

LukasPaczos Feb 3, 2025 •

edited

Loading

malmstein Feb 4, 2025

LukasPaczos Feb 5, 2025

LukasPaczos commented Feb 3, 2025

malmstein left a comment

malmstein Feb 4, 2025

malmstein Feb 4, 2025

LukasPaczos Feb 5, 2025

malmstein Feb 4, 2025

LukasPaczos Feb 5, 2025

malmstein Feb 4, 2025

LukasPaczos Feb 5, 2025

malmstein Feb 4, 2025

LukasPaczos Feb 5, 2025

Additional "default browser" prompts: metrics #5543

Additional "default browser" prompts: metrics #5543

Conversation

LukasPaczos commented Jan 27, 2025 • edited Loading

Steps to test this PR

LukasPaczos Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos commented Feb 3, 2025

malmstein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos commented Jan 27, 2025 •

edited

Loading

LukasPaczos Feb 3, 2025 •

edited

Loading