[Default Browser] Move the string choice logic to the front-end [chromium/src : main]
Rainhard Findling (Gerrit)
Rainhard Findling added 2 comments![]( "Open in Gerrit")
Thx! As discussed, my recommendation is to figure out if you can simply rely on existing metrics between experiment group and control group. If yes then that avoids a more complex setup.
Related details
- Aisulu Rakhmetullina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aisulu Rakhmetullina (Gerrit)
Aisulu Rakhmetullina voted and added 3 comments![]( "Open in Gerrit")
Votes added by Aisulu Rakhmetullina
| Commit-Queue | +1 |
Thx! As discussed, my recommendation is to figure out if you can simply rely on existing metrics between experiment group and control group. If yes then that avoids a more complex setup.
The PM feedback was that we really want to scope the metrics only to the users exposed to the feature.
The good news is that after talking to the Finch team it seems the setup is not that complex (just need to use a browser proxy to pull the feature state from the backend only at the creation of the page).
Hi Raihard, sending this your way as the complexity was resolved, PTAL!
Related details
- Rainhard Findling
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rainhard Findling (Gerrit)
Rainhard Findling voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rainhard Findling
| Code-Review | +1 |
Aisulu RakhmetullinaThx! As discussed, my recommendation is to figure out if you can simply rely on existing metrics between experiment group and control group. If yes then that avoids a more complex setup.
The PM feedback was that we really want to scope the metrics only to the users exposed to the feature.
The good news is that after talking to the Finch team it seems the setup is not that complex (just need to use a browser proxy to pull the feature state from the backend only at the creation of the page).
OK, thx!
settings_localized_strings_provider.cc LGTM
Related details
- Aisulu Rakhmetullina
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aisulu Rakhmetullina (Gerrit)
Aisulu Rakhmetullina voted and added 1 comment![]( "Open in Gerrit")
Votes added by Aisulu Rakhmetullina
| Commit-Queue | +1 |
1 comment
Hi Darryl, PTAL!
Adding chromium-ui-webui-reviews@ for the WebUI part based on the similar CL https://crrev.com/c/6898373 comments.
Related details
- Chromium UI WebUI Reviews
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
dpa...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ui/webui/config.gwsq)
Related details
- Darryl James
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 3 comments![]( "Open in Gerrit")
Overall this looks good to me - I did recommend a slightly different approach which I thought would be a little bit cleaner but will ultimately defer to @dpapad here 👍; Thanks!
export interface DefaultBrowserInfo {Would it be possible to add the new boolean / paremeter into this object instead? That way we can maintain a single call through mojo? Or at the very least 1 less call through mojo (there may be other calls that I am not aware of).
If the `userValueDefaultBrowserStringsEnabled` is moved into the `DefaultBrowserInfo` object a new line can be added here like this:
`dict.Set("userValueDefaultBrowserStringsEnabled", base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);`
I think this is a little bit cleaner but will defer to @dpapad 👍
Related details
- Aisulu Rakhmetullina
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 7 comments![]( "Open in Gerrit")
Bug: 446119669, 459593729Use proper references for internal bugs.
```suggestion
Bug: b:446119669, 459593729
```
<template is="dom-if" if="[[userValueDefaultBrowserStringsEnabled_]]">
<span>$i18n{defaultBrowserDefaultThankYou}</span>
</template>
<template is="dom-if"
if="[[!userValueDefaultBrowserStringsEnabled_]]">
<span>$i18n{defaultBrowserDefault}</span>
</template>Per our Polymer styleguide, should not be using dom-if to show/hide plain DOM nodes, see [1].
[1] https://chromium.googlesource.com/chromium/src/+/main/styleguide/web/web.md#Polymer
this.set('userValueDefaultBrowserStringsEnabled_', isEnabled);Please don't use `this.set()` for no good reasons. This bypasses all TS type checking.
canPin: boolean, userValueDefaultBrowserStringsEnabled: boolean): string {Per Polymer styleguide should be grabbing these from `this` not from the parameter list.
[] https://chromium.googlesource.com/chromium/src/+/main/styleguide/web/web.md#Polymer
if (userValueDefaultBrowserStringsEnabled) {
return loadTimeData.getString('defaultBrowserMakeDefaultUserValue');
}
return loadTimeData.getString('defaultBrowserMakeDefault');Nit: Use ternary operator.
```suggestion
return loadTimeData.getString(userValueDefaultBrowserStringsEnabled ?
'defaultBrowserMakeDefaultUserValue' : 'defaultBrowserMakeDefault';
```
&DefaultBrowserHandler::RequestUserValueStringsFeatureState,FWIW, all C++ callbacks that handle messages coming from the frontend are per convetion named with the `Handle` prefix. The rest of the code here already violates this convention, but see [1] for examples.
```suggestion
&DefaultBrowserHandler::HandleRequestUserValueStringsFeatureState,
```
bool is_enabled =
base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);Why are we sending this back asynchronously instead of putting it in `loadTimeData` ? This seems unnecessary. Putting it in loadTimeData would simplify the frontend as well.
Related details
- Aisulu Rakhmetullina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aisulu Rakhmetullina (Gerrit)
Aisulu Rakhmetullina added 3 comments![]( "Open in Gerrit")
Thank you both! Added 2 questions before I move on to address these (as it might influence the other comments).
bool is_enabled =
base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);Why are we sending this back asynchronously instead of putting it in `loadTimeData` ? This seems unnecessary. Putting it in loadTimeData would simplify the frontend as well.
True, initially I did use `loadTimeData` (PS#1), but while discussing `starts_active` in Finch chat I was told [1] that using it will activate every user for the experiment (which we don't want). This is because putting the flag in `loadTimeData` during the start-up is already triggering Finch to mark the user as active. Thus the async call.
PLMK if you think this is not the case / there is a different way to achieve scoping only to the users exposed to the feature (i.e. page).
[1] https://chat.google.com/room/AAAAIiFUug0/zTZN5UTgyc0/EQV66I8_X5E?cls=10
If the `userValueDefaultBrowserStringsEnabled` is moved into the `DefaultBrowserInfo` object a new line can be added here like this:
`dict.Set("userValueDefaultBrowserStringsEnabled", base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);`I think this is a little bit cleaner but will defer to @dpapad 👍
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
bool is_enabled =
base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);Aisulu RakhmetullinaWhy are we sending this back asynchronously instead of putting it in `loadTimeData` ? This seems unnecessary. Putting it in loadTimeData would simplify the frontend as well.
True, initially I did use `loadTimeData` (PS#1), but while discussing `starts_active` in Finch chat I was told [1] that using it will activate every user for the experiment (which we don't want). This is because putting the flag in `loadTimeData` during the start-up is already triggering Finch to mark the user as active. Thus the async call.
PLMK if you think this is not the case / there is a different way to achieve scoping only to the users exposed to the feature (i.e. page).
[1] https://chat.google.com/room/AAAAIiFUug0/zTZN5UTgyc0/EQV66I8_X5E?cls=10
If that's the motivation, please add this context into the code as a comment. Otherwise this stands out from the rest of the code, which habitually adds Finch flags into loadTimeData all over the place (it has never been considered a problem AFAIK). This is even recommended as the way to expose feature flags to the frontend at https://chromium.googlesource.com/chromium/src/+/main/docs/webui/webui_explainer.md#WebUIDataSource_AddBoolean.
Also, just to make sure I understand, the current approach of "lazily instantiating the finch flag value" relies silently on the fact that Settings lazily renders pages as they are visited (in this case at [1])?
Related details
- Aisulu Rakhmetullina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
Aisulu RakhmetullinaIf the `userValueDefaultBrowserStringsEnabled` is moved into the `DefaultBrowserInfo` object a new line can be added here like this:
`dict.Set("userValueDefaultBrowserStringsEnabled", base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);`I think this is a little bit cleaner but will defer to @dpapad 👍
@dpa...@chromium.org any opinion on this?
I don't have a strong opinion, and ultimately the alternative strings will be deleted once the feature launches IIUC, at which point they can be simply used by the frontend usinsg `i18n{...}` placeholders which does not require any message passing).
So feel free whichever approach you think is easier to cleanup later.
Aisulu Rakhmetullina (Gerrit)
Aisulu Rakhmetullina added 9 comments![]( "Open in Gerrit")
Use proper references for internal bugs.
```suggestion
Bug: b:446119669, 459593729
```
Done
Would it be possible to add the new boolean / paremeter into this object instead? That way we can maintain a single call through mojo? Or at the very least 1 less call through mojo (there may be other calls that I am not aware of).
Replied in another related comment.
<template is="dom-if" if="[[userValueDefaultBrowserStringsEnabled_]]">
<span>$i18n{defaultBrowserDefaultThankYou}</span>
</template>
<template is="dom-if"
if="[[!userValueDefaultBrowserStringsEnabled_]]">
<span>$i18n{defaultBrowserDefault}</span>
</template>Per our Polymer styleguide, should not be using dom-if to show/hide plain DOM nodes, see [1].
[1] https://chromium.googlesource.com/chromium/src/+/main/styleguide/web/web.md#Polymer
Done
this.set('userValueDefaultBrowserStringsEnabled_', isEnabled);Please don't use `this.set()` for no good reasons. This bypasses all TS type checking.
Done
canPin: boolean, userValueDefaultBrowserStringsEnabled: boolean): string {Per Polymer styleguide should be grabbing these from `this` not from the parameter list.
[] https://chromium.googlesource.com/chromium/src/+/main/styleguide/web/web.md#Polymer
Done
if (userValueDefaultBrowserStringsEnabled) {
return loadTimeData.getString('defaultBrowserMakeDefaultUserValue');
}
return loadTimeData.getString('defaultBrowserMakeDefault');Nit: Use ternary operator.
```suggestion
return loadTimeData.getString(userValueDefaultBrowserStringsEnabled ?
'defaultBrowserMakeDefaultUserValue' : 'defaultBrowserMakeDefault';
```
Done
&DefaultBrowserHandler::RequestUserValueStringsFeatureState,FWIW, all C++ callbacks that handle messages coming from the frontend are per convetion named with the `Handle` prefix. The rest of the code here already violates this convention, but see [1] for examples.
```suggestion
&DefaultBrowserHandler::HandleRequestUserValueStringsFeatureState,
```
Done. I initially preferred consistency, but glad to change to the conventional naming.
bool is_enabled =
base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);Aisulu RakhmetullinaWhy are we sending this back asynchronously instead of putting it in `loadTimeData` ? This seems unnecessary. Putting it in loadTimeData would simplify the frontend as well.
Demetrios PapadopoulosTrue, initially I did use `loadTimeData` (PS#1), but while discussing `starts_active` in Finch chat I was told [1] that using it will activate every user for the experiment (which we don't want). This is because putting the flag in `loadTimeData` during the start-up is already triggering Finch to mark the user as active. Thus the async call.
PLMK if you think this is not the case / there is a different way to achieve scoping only to the users exposed to the feature (i.e. page).
[1] https://chat.google.com/room/AAAAIiFUug0/zTZN5UTgyc0/EQV66I8_X5E?cls=10
If that's the motivation, please add this context into the code as a comment. Otherwise this stands out from the rest of the code, which habitually adds Finch flags into loadTimeData all over the place (it has never been considered a problem AFAIK). This is even recommended as the way to expose feature flags to the frontend at https://chromium.googlesource.com/chromium/src/+/main/docs/webui/webui_explainer.md#WebUIDataSource_AddBoolean.
Also, just to make sure I understand, the current approach of "lazily instantiating the finch flag value" relies silently on the fact that Settings lazily renders pages as they are visited (in this case at [1])?
That's right, I am basing the design on the fact that the page is rendered only on navigation.
Added a comment about "why async call" to default_browser_page.ts and updated the CL description.
Thank you!
------
P.S. My understanding of `starts_active` and how my implementation affects it was also confirmed during a Finch config review [1].
Aisulu RakhmetullinaIf the `userValueDefaultBrowserStringsEnabled` is moved into the `DefaultBrowserInfo` object a new line can be added here like this:
`dict.Set("userValueDefaultBrowserStringsEnabled", base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);`I think this is a little bit cleaner but will defer to @dpapad 👍
Demetrios Papadopoulos@dpa...@chromium.org any opinion on this?
I don't have a strong opinion, and ultimately the alternative strings will be deleted once the feature launches IIUC, at which point they can be simply used by the frontend usinsg `i18n{...}` placeholders which does not require any message passing).
So feel free whichever approach you think is easier to cleanup later.
Thanks for the suggestion, Darryl! As @dpapad mentioned, I'll stick with the current approach of a separate handler, as I believe it will be more straightforward to clean up after the feature is launched.
Related details
- Darryl James
- Demetrios Papadopoulos
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 2 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
2 comments
// We fetch the feature flag state asynchronously on page navigation,Nit: Avoid "we" in comments.
// lazily rendered, and this component's connectedCallback() is only called
// upon navigation to this subpage.FWIW, this also happens if any search is issued in the settings searchbox (everything is force-rendered in the background, whether there are search hits or not).
Related details
- Aisulu Rakhmetullina
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aisulu Rakhmetullina (Gerrit)
Aisulu Rakhmetullina added 4 comments![]( "Open in Gerrit")
Thank you Demetrios for a thorough review and your suggestions!
// We fetch the feature flag state asynchronously on page navigation,Nit: Avoid "we" in comments.
Done
// lazily rendered, and this component's connectedCallback() is only called
// upon navigation to this subpage.FWIW, this also happens if any search is issued in the settings searchbox (everything is force-rendered in the background, whether there are search hits or not).
Thanks for the info! Updated the comment for completeness and added a TODO for a future improvement (e.g. using `currentRouteChanged()`).
bool is_enabled =
base::FeatureList::IsEnabled(features::kUserValueDefaultBrowserStrings);Aisulu RakhmetullinaWhy are we sending this back asynchronously instead of putting it in `loadTimeData` ? This seems unnecessary. Putting it in loadTimeData would simplify the frontend as well.
Demetrios PapadopoulosTrue, initially I did use `loadTimeData` (PS#1), but while discussing `starts_active` in Finch chat I was told [1] that using it will activate every user for the experiment (which we don't want). This is because putting the flag in `loadTimeData` during the start-up is already triggering Finch to mark the user as active. Thus the async call.
PLMK if you think this is not the case / there is a different way to achieve scoping only to the users exposed to the feature (i.e. page).
[1] https://chat.google.com/room/AAAAIiFUug0/zTZN5UTgyc0/EQV66I8_X5E?cls=10
Aisulu RakhmetullinaIf that's the motivation, please add this context into the code as a comment. Otherwise this stands out from the rest of the code, which habitually adds Finch flags into loadTimeData all over the place (it has never been considered a problem AFAIK). This is even recommended as the way to expose feature flags to the frontend at https://chromium.googlesource.com/chromium/src/+/main/docs/webui/webui_explainer.md#WebUIDataSource_AddBoolean.
Also, just to make sure I understand, the current approach of "lazily instantiating the finch flag value" relies silently on the fact that Settings lazily renders pages as they are visited (in this case at [1])?
That's right, I am basing the design on the fact that the page is rendered only on navigation.
Added a comment about "why async call" to default_browser_page.ts and updated the CL description.
Thank you!
------
P.S. My understanding of `starts_active` and how my implementation affects it was also confirmed during a Finch config review [1].
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
21 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resources/settings/default_browser_page/default_browser_page.ts
Insertions: 5, Deletions: 2.
@@ -71,13 +71,16 @@
this.browserProxy_.requestDefaultBrowserState().then(
this.updateDefaultBrowserState_.bind(this));
- // We fetch the feature flag state asynchronously on page navigation,
+ // The feature flag state is fetched asynchronously on page navigation,
// instead of using loadTimeData, to support a Finch experiment with
// `starts_active = false`. This ensures that the user is activated in the
// experiment (e.g. the feature flag is checked) only when visiting the
// page. This approach relies on the fact that the settings subpages are
// lazily rendered, and this component's connectedCallback() is only called
- // upon navigation to this subpage.
+ // upon rendering of this page (happens on navigation to this subpage or
+ // on search issue in settings searchbox.
+ // TODO(crbug.com/459593729): Refactor to be called only at navigation, e.g.
+ // by using currentRouteChanged() from RouteObserverMixin.
this.browserProxy_.requestUserValueStringsFeatureState().then(
(isEnabled) => {
this.userValueDefaultBrowserStringsEnabled_ = isEnabled;
```
Change information
[Default Browser] Move the string choice logic to the front-end
The experiment in https://b.corp.google.com/issues/446119669 swaps 2
existing strings from chrome://settings/defaultBrowser to more user-
centric ones. It is already implemented in https://crrev.com/c/7131700.
However, we need to update the implementation to accommodate the
correct metrics tracking by a future Finch experiment. Namely, we want
to track Chrome default rate but only for users who have seen new
strings (i.e. visited the settings page).
To scope metrics to only users who visited the page, we want to rely on
`starts_active` = false. This requires the code that is triggered upon
navigation to contain the flag-dependent code.
That is why this CL moves the logic (the choice between an existing
string and new strings based on the feature flag) from back-end to
front-end in order to support `starts_active`.
To ensure that the feature flag-dependent code is triggered only on
navigation, an asynchronous call to fetch the feature flag's state is
used. This approach relies on the fact that the Settings UI lazily
renders its subpages.
Manually tested: https://crbug.com/459593729#comment5
- M chrome/browser/resources/settings/default_browser_page/default_browser_browser_proxy.ts
- M chrome/browser/resources/settings/default_browser_page/default_browser_page.html
- M chrome/browser/resources/settings/default_browser_page/default_browser_page.ts
- M chrome/browser/ui/webui/settings/settings_default_browser_handler.cc
- M chrome/browser/ui/webui/settings/settings_default_browser_handler.h
- M chrome/browser/ui/webui/settings/settings_default_browser_handler_unittest.cc
- M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
- M chrome/test/data/webui/settings/default_browser_test.ts
Code-Review: +1 by Rainhard Findling, +1 by Demetrios Papadopoulos
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |