Implement frontend changes for the password leak toggle move [chromium/src : main]

1 view
Skip to first unread message

Demetrios Papadopoulos (Gerrit)

unread,
Oct 4, 2024, 1:26:40 PM (3 days ago) Oct 4
to Awad Osman, Rainhard Findling, Side YILMAZ, Chromium LUCI CQ, [email protected]
Attention needed from Awad Osman and Rainhard Findling

Demetrios Papadopoulos added 4 comments

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Demetrios Papadopoulos . resolved

Adding rainhrad@ as well, as a more specific OWNER.

File chrome/browser/resources/settings/privacy_page/security_page.html
Line 59, Patchset 2 (Latest): #httpsOnlyModeToggle, #passwordsLeakToggleMove {
Demetrios Papadopoulos . unresolved
Each selector should be on its own line per styleguide.
```suggestion
#httpsOnlyModeToggle,
#passwordsLeakToggleMove {
```
Line 289, Patchset 2 (Latest): <settings-toggle-button id="passwordsLeakToggleMove"
Demetrios Papadopoulos . unresolved

Is there a benefit of using a different ID here? The two elements are mutuallly exclusive, no? Using `passwordsLeakToggle` would result in less necessary chanegs in other parts of the code I think.

File chrome/test/data/webui/settings/security_page_test.ts
Line 643, Patchset 2 (Latest):
Demetrios Papadopoulos . unresolved

Need to update the tests that exercise this toggle to also be triggered with `passwordLeakToggleMove: true`. Otherwise, the newly added toggle is not tested anywhere and what will happen with these tests once the feature launches? Will whoever cleans up the flag just delete these tests, or port them to the new toggle at that time?

Both of the above sound suboptimal to me.

Open in Gerrit

Related details

Attention is currently required from:
  • Awad Osman
  • Rainhard Findling
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iacd6f6f8b8ad38a97d950d375592969e71ca1631
Gerrit-Change-Number: 5899277
Gerrit-PatchSet: 2
Gerrit-Owner: Awad Osman <[email protected]>
Gerrit-Reviewer: Awad Osman <[email protected]>
Gerrit-Reviewer: Demetrios Papadopoulos <[email protected]>
Gerrit-Reviewer: Rainhard Findling <[email protected]>
Gerrit-CC: Chromium LUCI CQ <[email protected]>
Gerrit-CC: Side YILMAZ <[email protected]>
Gerrit-CC: [email protected]
Gerrit-Attention: Rainhard Findling <[email protected]>
Gerrit-Attention: Awad Osman <[email protected]>
Gerrit-Comment-Date: Fri, 04 Oct 2024 20:26:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Rainhard Findling (Gerrit)

unread,
4:20 AM (2 hours ago) 4:20 AM
to Awad Osman, Side YILMAZ, Demetrios Papadopoulos, Chromium LUCI CQ, [email protected]
Attention needed from Awad Osman and Demetrios Papadopoulos

Rainhard Findling added 4 comments

File chrome/browser/resources/settings/privacy_page/security_page.html
Line 59, Patchset 2 (Latest): #httpsOnlyModeToggle, #passwordsLeakToggleMove {
Demetrios Papadopoulos . unresolved
Each selector should be on its own line per styleguide.
```suggestion
#httpsOnlyModeToggle,
#passwordsLeakToggleMove {
```
Rainhard Findling

+1, and I'd recommend to craft the CSS selector so that it doesn't the toggle in the old position, and matches the toggle in the new position.

Line 289, Patchset 2 (Latest): <settings-toggle-button id="passwordsLeakToggleMove"
Demetrios Papadopoulos . unresolved

Is there a benefit of using a different ID here? The two elements are mutuallly exclusive, no? Using `passwordsLeakToggle` would result in less necessary chanegs in other parts of the code I think.

Rainhard Findling

+1, you should be able to use the same ID, and craft the CSS selectors to match when the element is in the position here, and to not match when the element is in the position further up.

Also: I find "Move" in this ID confusing, because the toggle itself is not about "moving" a password leak or so. Hence "move" is not descriptive for what the toggle does or represents. (IIUC, you probably mean "move" to refer to the feature being enabled moving the toggle to a different position - which is _not_ an attribute of the toggle itself.)

File chrome/browser/resources/settings/privacy_page/security_page.ts
File chrome/test/data/webui/settings/security_page_test.ts
Demetrios Papadopoulos . unresolved

Need to update the tests that exercise this toggle to also be triggered with `passwordLeakToggleMove: true`. Otherwise, the newly added toggle is not tested anywhere and what will happen with these tests once the feature launches? Will whoever cleans up the flag just delete these tests, or port them to the new toggle at that time?

Both of the above sound suboptimal to me.

Rainhard Findling

+1, please follow the guidance in go/test-new-codepaths to enable the feature for the complete test file and to explicitly test both the new and old codepaths respectively.

Open in Gerrit

Related details

Attention is currently required from:
  • Awad Osman
  • Demetrios Papadopoulos
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iacd6f6f8b8ad38a97d950d375592969e71ca1631
Gerrit-Change-Number: 5899277
Gerrit-PatchSet: 2
Gerrit-Owner: Awad Osman <[email protected]>
Gerrit-Reviewer: Awad Osman <[email protected]>
Gerrit-Reviewer: Demetrios Papadopoulos <[email protected]>
Gerrit-Reviewer: Rainhard Findling <[email protected]>
Gerrit-CC: Chromium LUCI CQ <[email protected]>
Gerrit-CC: Side YILMAZ <[email protected]>
Gerrit-CC: [email protected]
Gerrit-Attention: Awad Osman <[email protected]>
Gerrit-Attention: Demetrios Papadopoulos <[email protected]>
Gerrit-Comment-Date: Mon, 07 Oct 2024 11:19:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Demetrios Papadopoulos <[email protected]>
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages