[cr93 followup] Removed disablement of kEnablePasswordsAccountStorage. #9807
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This feature flag no longer works as all the code it wasn't guarding has been removed and password account storage is the only option now. This also means that we will regress on brave/brave-browser#16926 again, but this can be addressed in a different fix. Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1108738 https://chromium.googlesource.com/chromium/src.git/+/90ac58cb2c56df119ca3d5631cbbc3feabb8a82b commit 90ac58cb2c56df119ca3d5631cbbc3feabb8a82b Author: Marc Treib <treib@chromium.org> Date: Fri Jul 9 10:43:20 2021 +0000 B4P cleanup, part 2: Delete old Save/Update bubble PasswordSaveUpdateView has been replaced by PasswordSaveUpdateWithAccountStoreView, similar for the corresponding controller. This CL deletes the old, non-AccountStore versions. This is a no-op on Win/Mac/Linux/ChromeOS (which already always used the new versions) and on Android/iOS (which don't use this UI at all). (Note that ChromeOS doesn't actually use/support the account-scoped storage, since the signed-in non-syncing state doesn't exist. But the new UI code is a strict superset of the old, so using it on ChromeOS as well is just simpler and more consistent - and this was already the case.) This CL also ports some recent unit test improvements (see crrev.com/c/3006215) from the old to the new controller. Bug: 1108738
Goes around upstream check for gaia credential page and empty primary account in the browser. Fixes brave/brave-browser#17595 Fixes brave/brave-browser#16926
mkarolin
force-pushed
the
maxk-cr93fu-enable-kEnablePasswordsAccountStorage
branch
from
e233a56
to
d454b1a
Compare
fmarier
approved these changes
Aug 25, 2021
|
|
||
| bool SyncCredentialsFilter::ShouldSave(const PasswordForm& form) const { | ||
| bool should_save = ShouldSave_ChromiumImpl(form); | ||
| if (!should_save && sync_util::IsGaiaCredentialPage(form.signon_realm)) { |
There was a problem hiding this comment.
Note: this does both the login page and the change password page. That looks okay to me.
bsclifton
deleted the
maxk-cr93fu-enable-kEnablePasswordsAccountStorage
branch
mkarolin
pushed a commit
that referenced
this pull request
Aug 25, 2021
…dsAccountStorage [cr93 followup] Removed disablement of kEnablePasswordsAccountStorage.
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes brave/brave-browser#17595
This feature flag no longer works as all the code it wasn't guarding has
been removed and password account storage is the only option now.
This also means that we will regress on
brave/brave-browser#16926 again, and needs a different fix added here in a separate commit.
Chromium issue:
https://bugs.chromium.org/p/chromium/issues/detail?id=1108738
https://chromium.googlesource.com/chromium/src.git/+/90ac58cb2c56df119ca3d5631cbbc3feabb8a82b
commit 90ac58cb2c56df119ca3d5631cbbc3feabb8a82b
Author: Marc Treib treib@chromium.org
Date: Fri Jul 9 10:43:20 2021 +0000
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: