[Security Settings] Implement HFM Balanced Mode row and other UI [chromium/src : main]
Zack Han (Gerrit)
Zack Han voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Awad Osman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Awad Osman (Gerrit)
Awad Osman added 7 comments![]( "Open in Gerrit")
nit: screencast of general behavior would be nice since this CL makes substantial UI changes.
<div style="display: flex; align-items: center; gap: 8px;">nit: instead of inlining the styles I think it'll be better to give the div an id selector and style the selector instead.
animation: none !important;Is there a way to use these styles without `!important`?. Here and the one below as well.
set(path: string, value: unknown): void;Why is this needed?
this.set('showNewBadge_', isNew);nit: can't you just do `this.showNewBadge_ = isNew`?
LOG(ERROR) << "BSS Migration Debug - profile is null!";Think you left the log statement here by accident.
LOG(ERROR) << "BSS Migration Debug - starting checks."Same as above^
Related details
- Zack Han
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. |
Zack Han (Gerrit)
Zack Han added 8 comments![]( "Open in Gerrit")
nit: screencast of general behavior would be nice since this CL makes substantial UI changes.
Done
<div style="display: flex; align-items: center; gap: 8px;">nit: instead of inlining the styles I think it'll be better to give the div an id selector and style the selector instead.
Done
Is there a way to use these styles without `!important`?. Here and the one below as well.
Done
set(path: string, value: unknown): void;Zack HanWhy is this needed?
Done
nit: can't you just do `this.showNewBadge_ = isNew`?
Done
Think you left the log statement here by accident.
Done
LOG(ERROR) << "BSS Migration Debug - starting checks."Zack HanSame as above^
Done
Related details
- Awad Osman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zack Han (Gerrit)
Zack Han added 1 comment![]( "Open in Gerrit")
Hi Chris, can you take a look at the c/b/s/https_first_mode... related changes? Thanks!
Related details
- Awad Osman
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Awad Osman (Gerrit)
Awad Osman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Awad Osman
| Code-Review | +1 |
2 comments
LGTM % pending failing tests get resolved
securityStandardBundleSafeBrowsingDefault: SafeBrowsingSetting.STANDARD,What are these variables used for? Here and in the HATS suite below.
Related details
- Chris Thompson
- Zack Han
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. |
Chris Thompson (Gerrit)
Chris Thompson added 1 comment![]( "Open in Gerrit")
// Migrates existing Enhanced bundle users to Balanced HFM and schedules the
// upgrade Toast if they haven't manually customized HFM.
void MigrateEnhancedBundleUsersAndMaybeShowToast();Unintended removal of the logic added in the parent CL?
Related details
- Zack Han
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. |
Zack Han (Gerrit)
Zack Han added 2 comments![]( "Open in Gerrit")
// Migrates existing Enhanced bundle users to Balanced HFM and schedules the
// upgrade Toast if they haven't manually customized HFM.
void MigrateEnhancedBundleUsersAndMaybeShowToast();Unintended removal of the logic added in the parent CL?
ooops introduced from rebasing, fixed. thanks!
securityStandardBundleSafeBrowsingDefault: SafeBrowsingSetting.STANDARD,What are these variables used for? Here and in the HATS suite below.
Basically, the browser UI needs to know what the default settings should be when a user clicks on the "Standard" or "Enhanced" card. These values are originally passed from the C++ backend down to the JavaScript frontend through loadTimeData.
Since we're running isolated frontend WebUI unit tests here, there is no C++ backend running to give us those default values. So, we have to explicitly override those mock values in our suiteSetup so that the JS page behaves exactly like it would in the real browser. Otherwise, the mock page gets initialized with empty defaults, which breaks our test assertions when we click around.
Note we didn't need these overrides before because HFM used to be completely independent of the security cards clicking Standard or Enhanced didn't change HFM states at all. Now that we are bundling HFM directly into the Standard/Enhanced cards, clicking on a card dynamically queries loadTimeData to find out what default HFM values to set for that card. Since the mock test environment doesn't run the C++ backend to populate those defaults automatically, the card interactions crashed with undefined reads. Overriding them here in suiteSetup ensures the mock page behaves predictably when simulating card clicks. I have added a comment here. Please let me know if this makes sense.
Related details
- Awad Osman
- Chris Thompson
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. |
Chris Thompson (Gerrit)
Chris Thompson added 2 comments![]( "Open in Gerrit")
// If HFM Balanced Mode features are enabled and available, migrate
// uncustomized HFM settings to Balanced Mode.
if (base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) &&
!prefs->IsManagedPreference(prefs::kHttpsOnlyModeEnabled) &&
!prefs->IsManagedPreference(prefs::kHttpsFirstBalancedMode)) {
if (!prefs->HasPrefPath(prefs::kHttpsOnlyModeEnabled) &&
!prefs->HasPrefPath(prefs::kHttpsFirstBalancedMode)) {
prefs->SetBoolean(prefs::kHttpsFirstBalancedMode, true);
}
}Can we put this through the same `MigrateEnhancedBundleUsersAndMaybeShowToast()` path we added in the parent CL? Alternatively, we could add a check here for APP mode users as well.
#include "components/safe_browsing/core/common/features.h"Unused?
Related details
- Awad Osman
- Zack Han
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. |
Zack Han (Gerrit)
Zack Han added 3 comments![]( "Open in Gerrit")
Hi Thomas, can you take a look at the file: chrome/browser/ui/webui/settings/ settings_localized_strings_provider.cc Thanks!
// If HFM Balanced Mode features are enabled and available, migrate
// uncustomized HFM settings to Balanced Mode.
if (base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) &&
!prefs->IsManagedPreference(prefs::kHttpsOnlyModeEnabled) &&
!prefs->IsManagedPreference(prefs::kHttpsFirstBalancedMode)) {
if (!prefs->HasPrefPath(prefs::kHttpsOnlyModeEnabled) &&
!prefs->HasPrefPath(prefs::kHttpsFirstBalancedMode)) {
prefs->SetBoolean(prefs::kHttpsFirstBalancedMode, true);
}
}Can we put this through the same `MigrateEnhancedBundleUsersAndMaybeShowToast()` path we added in the parent CL? Alternatively, we could add a check here for APP mode users as well.
Done. Good catch! I removed the duplicate HFM migration block from safe_browsing_service.cc. Since setting the bundle to ENHANCED triggers the pref observer in HttpsFirstModeService, it will naturally run MigrateEnhancedBundleUsersAndMaybeShowToast() which already safely handles the APP check.
#include "components/safe_browsing/core/common/features.h"Zack HanUnused?
Done
Related details
- Awad Osman
- Chris Thompson
- Thomas Lukaszewicz
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. |
Awad Osman (Gerrit)
Awad Osman added 1 comment![]( "Open in Gerrit")
securityStandardBundleSafeBrowsingDefault: SafeBrowsingSetting.STANDARD,Zack HanWhat are these variables used for? Here and in the HATS suite below.
Basically, the browser UI needs to know what the default settings should be when a user clicks on the "Standard" or "Enhanced" card. These values are originally passed from the C++ backend down to the JavaScript frontend through loadTimeData.
Since we're running isolated frontend WebUI unit tests here, there is no C++ backend running to give us those default values. So, we have to explicitly override those mock values in our suiteSetup so that the JS page behaves exactly like it would in the real browser. Otherwise, the mock page gets initialized with empty defaults, which breaks our test assertions when we click around.
Note we didn't need these overrides before because HFM used to be completely independent of the security cards clicking Standard or Enhanced didn't change HFM states at all. Now that we are bundling HFM directly into the Standard/Enhanced cards, clicking on a card dynamically queries loadTimeData to find out what default HFM values to set for that card. Since the mock test environment doesn't run the C++ backend to populate those defaults automatically, the card interactions crashed with undefined reads. Overriding them here in suiteSetup ensures the mock page behaves predictably when simulating card clicks. I have added a comment here. Please let me know if this makes sense.
- Chris Thompson
- Thomas Lukaszewicz
- Zack Han
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. |
Chris Thompson (Gerrit)
Chris Thompson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Thompson
| Code-Review | +1 |
1 comment
// If HFM Balanced Mode features are enabled and available, migrate
// uncustomized HFM settings to Balanced Mode.
if (base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) &&
!prefs->IsManagedPreference(prefs::kHttpsOnlyModeEnabled) &&
!prefs->IsManagedPreference(prefs::kHttpsFirstBalancedMode)) {
if (!prefs->HasPrefPath(prefs::kHttpsOnlyModeEnabled) &&
!prefs->HasPrefPath(prefs::kHttpsFirstBalancedMode)) {
prefs->SetBoolean(prefs::kHttpsFirstBalancedMode, true);
}
}Zack HanCan we put this through the same `MigrateEnhancedBundleUsersAndMaybeShowToast()` path we added in the parent CL? Alternatively, we could add a check here for APP mode users as well.
Done. Good catch! I removed the duplicate HFM migration block from safe_browsing_service.cc. Since setting the bundle to ENHANCED triggers the pref observer in HttpsFirstModeService, it will naturally run MigrateEnhancedBundleUsersAndMaybeShowToast() which already safely handles the APP check.
- Thomas Lukaszewicz
- Zack Han
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz added 1 comment![]( "Open in Gerrit")
if (toast_state !=
static_cast<int>(SecuritySettingsBundleToastState::kPending) ||
toast_state ==
static_cast<int>(SecuritySettingsBundleToastState::kShown)) {Is this right? If
```
toast_state != SecuritySettingsBundleToastState::kPending
```
then I believe the second condition is irrelevant
Related details
- Chris Thompson
- Zack Han
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. |
Zack Han (Gerrit)
Zack Han added 2 comments![]( "Open in Gerrit")
if (toast_state !=
static_cast<int>(SecuritySettingsBundleToastState::kPending) ||
toast_state ==
static_cast<int>(SecuritySettingsBundleToastState::kShown)) {Is this right? If
```
toast_state != SecuritySettingsBundleToastState::kPending
```
then I believe the second condition is irrelevant
Yeah, the second condition was redundant! I have removed it. Thanks for catching it.
Related details
- Chris Thompson
- Thomas Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Thomas Lukaszewicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Lukaszewicz
| Code-Review | +1 |
1 comment
- Chris Thompson
- Zack Han
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zack Han (Gerrit)
Zack Han voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Chris Thompson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |