Joshua Hood (Gerrit)

unread,

Nov 26, 2025, 12:05:38 AM11/26/25

to Awad Osman, Rainhard Findling, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Awad Osman, Nico Weber and Rainhard Findling

Joshua Hood voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Awad Osman
Nico Weber
Rainhard Findling

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rainhard Findling (Gerrit)

unread,

Nov 26, 2025, 7:07:55 AM11/26/25

to Joshua Hood, Awad Osman, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Awad Osman, Joshua Hood and Nico Weber

Rainhard Findling added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Rainhard Findling . resolved

Will TAL after awado@ reviewed for security_page :)

Open in Gerrit

Related details

Attention is currently required from:

Awad Osman
Joshua Hood
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Awad Osman (Gerrit)

unread,

Nov 26, 2025, 11:26:44 AM11/26/25

to Joshua Hood, Rainhard Findling, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Joshua Hood and Nico Weber

Awad Osman added 3 comments

File chrome/browser/resources/settings/privacy_page/security/security_page_v2.html

Line 196, Patchset 6 (Latest): disabled="[[!isHttpsFirstModeEnabled_(

Awad Osman . unresolved

Does the disabled attribute need to be repeated for each controlled radio button if it's on the settings-radio-group? I'm not sure so if you can check it out and keep or remove based on your analysis that would be sweet!

File chrome/test/data/webui/settings/security_page_v2_test.ts

Line 344, Patchset 6 (Latest): await microtasksFinished();

Awad Osman . unresolved

nit: I believe its preferred to use `await flushTasks()`

Line 382, Patchset 6 (Latest): await microtasksFinished();

Awad Osman . unresolved

nit: use `await flushTasks()`

Open in Gerrit

Related details

Attention is currently required from:

Joshua Hood
Nico Weber

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joshua Hood (Gerrit)

unread,

Nov 26, 2025, 4:15:56 PM11/26/25

to Awad Osman, Rainhard Findling, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Awad Osman and Nico Weber

Joshua Hood voted and added 3 comments

Votes added by Joshua Hood

Commit-Queue

+1

3 comments

File chrome/browser/resources/settings/privacy_page/security/security_page_v2.html

Line 196, Patchset 6: disabled="[[!isHttpsFirstModeEnabled_(

Awad Osman . resolved

Does the disabled attribute need to be repeated for each controlled radio button if it's on the settings-radio-group? I'm not sure so if you can check it out and keep or remove based on your analysis that would be sweet!

Joshua Hood

I tested some things and it looks like:

having the disabled attribute on the `settings-radio-group` makes it so that when the toggle is off the actual buttons/circles of both `controlled-radio-button` elements are empty.
having the disabled attribute set on the `controlled-radio-button` elements themselves does the same as having it on the `settings-radio-group` AND makes them greyed out when the toggle is off.

So I've removed the attribute from the `settings-radio-group` and updated the tests accordingly.

File chrome/test/data/webui/settings/security_page_v2_test.ts

Line 344, Patchset 6: await microtasksFinished();

Awad Osman . resolved

nit: I believe its preferred to use `await flushTasks()`

Joshua Hood

Done

Line 382, Patchset 6: await microtasksFinished();

Awad Osman . resolved

nit: use `await flushTasks()`

Joshua Hood

Done

Open in Gerrit

Related details

Attention is currently required from:

Awad Osman
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Awad Osman (Gerrit)

unread,

Nov 26, 2025, 5:52:50 PM11/26/25

to Joshua Hood, Rainhard Findling, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Joshua Hood and Nico Weber

Awad Osman voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Joshua Hood
Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rainhard Findling (Gerrit)

unread,

Nov 27, 2025, 5:04:42 AM11/27/25

to Joshua Hood, Awad Osman, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Joshua Hood and Nico Weber

Rainhard Findling voted and added 8 comments

Votes added by Rainhard Findling

Code-Review

+1

8 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Rainhard Findling . resolved

Thx! LGTM % comments and nits

File chrome/browser/resources/settings/privacy_page/security/security_page_v2.html

Line 58, Patchset 9 (Latest): #safeBrowsingRadioGroup, #httpsFirstModeRadioGroup {

Rainhard Findling . unresolved

Nit: why not instead use the common element type?

Line 62, Patchset 9 (Latest): #safeBrowsingRow, #passwordsLeakToggle {

Rainhard Findling . unresolved

Recommendation: I'd recommend avoiding implicitly hardcoding which rows you expect to be on top, in the middle, or in the bottom. This can lead to unnecessary complexity, e.g. when further rows get added in the future behind a feature. I'd instead recommend to let CSS selectors handle that where possible, e.g. via something like `:first-of-type` (and its inverse via `:not(...)`)

File chrome/test/data/webui/settings/security_page_v2_test.ts

Line 194, Patchset 9 (Latest): (row as any).expanded, 'Initially, the row should be collapsed');

Rainhard Findling . unresolved

Nit: Is this needed? I wouldn't expect it to

Line 199, Patchset 9 (Latest):

        const expandButton =
            page.$.httpsFirstModeRow.shadowRoot!
                .querySelector<CrExpandButtonElement>('#expandButton');

Rainhard Findling . unresolved

I think it would make sense to expose those buttons as part of the row's interface, so that tests don't consistently need to query them, and so that local variables like those one aren't needed. (Same below)

Line 204, Patchset 9 (Latest): // Click on the expand button, expands content and we can see the radio

Rainhard Findling . unresolved

Nit: generally avoid "we". Where the entity is necessary, instead state the entity (e.g. company, product). In cases like this leave it out.

(Also, general tip for comments: the code should be written so that it explains the "what". Comments should focus on explaining only the "why". Sticking with that paradigm you might be able to remove some comments throughout those tests, and tweak some others to focus only on the "why".)

Line 208, Patchset 9 (Latest): await microtasksFinished();

Rainhard Findling . unresolved

Nit: Not needed? (Same below)

Line 238, Patchset 9 (Latest): // Set to DISABLED initially

Rainhard Findling . unresolved

Nit: comments need period at the end

Open in Gerrit

Related details

Attention is currently required from:

Joshua Hood
Nico Weber

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joshua Hood (Gerrit)

unread,

Dec 1, 2025, 4:31:32 PM12/1/25

to Rainhard Findling, Awad Osman, Nico Weber, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Nico Weber

Joshua Hood voted and added 7 comments

Votes added by Joshua Hood

Commit-Queue

+1

7 comments

File chrome/browser/resources/settings/privacy_page/security/security_page_v2.html

Line 58, Patchset 9: #safeBrowsingRadioGroup, #httpsFirstModeRadioGroup {

Rainhard Findling . resolved

Nit: why not instead use the common element type?

Joshua Hood

Done.

I thought that would end up affecting the styling of `#bundlesRadioGroup`, but I tried it out and it does not. Thanks!

Line 62, Patchset 9: #safeBrowsingRow, #passwordsLeakToggle {

Rainhard Findling . resolved

Recommendation: I'd recommend avoiding implicitly hardcoding which rows you expect to be on top, in the middle, or in the bottom. This can lead to unnecessary complexity, e.g. when further rows get added in the future behind a feature. I'd instead recommend to let CSS selectors handle that where possible, e.g. via something like `:first-of-type` (and its inverse via `:not(...)`)

Joshua Hood

Done.

I thought about that, but it couldn't see a way to make it work with the page's current structure. I've wrapped the sections in divs now so that I can apply it in that way though.

I also have a follow-up [CL](crrev.com/c/7201464) in progress that will use some of the other shared settings styling and gets rid of the need for this.

File chrome/test/data/webui/settings/security_page_v2_test.ts

Line 194, Patchset 9: (row as any).expanded, 'Initially, the row should be collapsed');

Rainhard Findling . resolved

Nit: Is this needed? I wouldn't expect it to

Joshua Hood

Actually looking deeper into this, chrome/test/data/webui/settings/security_page_feature_row_test.ts has tests that cover all of this behavior for the component already. This test is redundant and removing it should be fine.

Line 199, Patchset 9: const expandButton =


            page.$.httpsFirstModeRow.shadowRoot!
                .querySelector<CrExpandButtonElement>('#expandButton');

Rainhard Findling . resolved

I think it would make sense to expose those buttons as part of the row's interface, so that tests don't consistently need to query them, and so that local variables like those one aren't needed. (Same below)

Joshua Hood

Based on our offline conversation, right now this might not work well in practice since the expand button is switched when the row is expanded or collapsed.

This test case is redundant, so I'm removing it from this CL, but will expose the button as part of the row's interface in the same follow-up that makes sure there is only one expand button for each row.

Line 204, Patchset 9: // Click on the expand button, expands content and we can see the radio

Rainhard Findling . resolved

Nit: generally avoid "we". Where the entity is necessary, instead state the entity (e.g. company, product). In cases like this leave it out.
(Also, general tip for comments: the code should be written so that it explains the "what". Comments should focus on explaining only the "why". Sticking with that paradigm you might be able to remove some comments throughout those tests, and tweak some others to focus only on the "why".)

Joshua Hood

Acknowledged

Line 208, Patchset 9: await microtasksFinished();

Rainhard Findling . resolved

Nit: Not needed? (Same below)

Joshua Hood

Acknowledged

Line 238, Patchset 9: // Set to DISABLED initially

Rainhard Findling . resolved

Nit: comments need period at the end

Joshua Hood

Done

Open in Gerrit

Related details

Attention is currently required from:

Nico Weber

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Joshua Hood (Gerrit)

unread,

Dec 2, 2025, 12:23:41 PM12/2/25

to Avi Drissman, Rainhard Findling, Awad Osman, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Avi Drissman

Joshua Hood added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Joshua Hood . resolved

Hi Avi, could you PTAL at the lint changes in chrome/browser/ssl/https_first_mode_settings_tracker.h?

Open in Gerrit

Related details

Attention is currently required from:

Avi Drissman

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Avi Drissman (Gerrit)

unread,

Dec 2, 2025, 12:32:25 PM12/2/25

to Joshua Hood, Avi Drissman, Rainhard Findling, Awad Osman, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Attention needed from Joshua Hood

Avi Drissman voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Joshua Hood

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Joshua Hood (Gerrit)

unread,

Dec 2, 2025, 12:32:46 PM12/2/25

to Avi Drissman, Rainhard Findling, Awad Osman, Chromium LUCI CQ, chromium...@chromium.org, srahim...@chromium.org

Joshua Hood voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Dec 2, 2025, 12:35:52 PM12/2/25

to Joshua Hood, Avi Drissman, Rainhard Findling, Awad Osman, chromium...@chromium.org, srahim...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[Bundled Security Settings] Add "Secure connections" feature row

This CL adds a security settings feature row for "Secure connections"
(AKA HTTPS-First Mode) to the Bundled Security Settings version of the
chrome://settings/security page.

Currently, the toggle is only visible when the feature row's dropdown is
expanded. A subsequent CL will make the toggle always visible.

Screenshot: crbug.com/460195217#comment5

NO_IFTTT=changes to
`chrome/browser/ssl/https_first_mode_settings_tracker.h` are updates to
`LINT.ThenChange()` comment itself.

Bug: b:460195217

Change-Id: Ia73e0c0faa035c2a616acdbf44117030a3309b78

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7201463

Reviewed-by: Awad Osman <aw...@google.com>

Reviewed-by: Rainhard Findling <rain...@chromium.org>

Reviewed-by: Avi Drissman <a...@chromium.org>

Commit-Queue: Joshua Hood <j...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1552906}

Files:

M chrome/app/settings_strings.grdp
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_BALANCED_LABEL.png.sha1
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_BALANCED_SUBLABEL.png.sha1
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_STRICT_LABEL.png.sha1
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_STRICT_SUBLABEL.png.sha1
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_TOGGLE_LABEL.png.sha1
A chrome/app/settings_strings_grdp/IDS_SETTINGS_SECURITY_HTTPS_FIRST_MODE_TOGGLE_SUBLABEL.png.sha1
M chrome/browser/resources/settings/privacy_page/security/security_page_feature_row.html
M chrome/browser/resources/settings/privacy_page/security/security_page_feature_row.ts
M chrome/browser/resources/settings/privacy_page/security/security_page_v2.html
M chrome/browser/resources/settings/privacy_page/security/security_page_v2.ts
M chrome/browser/ssl/https_first_mode_settings_tracker.h
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/security_page_v2_test.ts

Change size: M

Delta: 14 files changed, 207 insertions(+), 10 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Rainhard Findling, +1 by Awad Osman, +1 by Avi Drissman

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Yi Gu (Gerrit)

unread,

Dec 2, 2025, 5:02:58 PM12/2/25

to Joshua Hood, Chromium LUCI CQ, Avi Drissman, Rainhard Findling, Awad Osman, chromium...@chromium.org, srahim...@chromium.org

Yi Gu has created a revert of this change

Open original change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[Bundled Security Settings] Add "Secure connections" feature row [chromium/src : main]

Joshua Hood (Gerrit)

Joshua Hood voted Commit-Queue+1

Related details

Rainhard Findling (Gerrit)

Rainhard Findling added 1 comment

Related details

Awad Osman (Gerrit)

Awad Osman added 3 comments

Related details

Joshua Hood (Gerrit)

Joshua Hood voted and added 3 comments

Votes added by Joshua Hood

3 comments

Related details

Awad Osman (Gerrit)

Awad Osman voted Code-Review+1

Related details

Rainhard Findling (Gerrit)

Rainhard Findling voted and added 8 comments

Votes added by Rainhard Findling

8 comments

Related details

Joshua Hood (Gerrit)

Joshua Hood voted and added 7 comments

Votes added by Joshua Hood

7 comments

Related details

Joshua Hood (Gerrit)

Joshua Hood added 1 comment

Related details

Avi Drissman (Gerrit)

Avi Drissman voted Code-Review+1

Related details

Joshua Hood (Gerrit)

Joshua Hood voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information

Yi Gu (Gerrit)

Yi Gu has created a revert of this change

Related details