Use change password form filler in `PasswordChangeFromCheckupDelegate` [chromium/src : main]
Fiorella Barrientos Villalta (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Viktor Semeniuk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov added 8 comments![]( "Open in Gerrit")
sources += [Why are you moving the files to the bigger polluted target ?
ChangePasswordFormFillingSubmissionHelper::SubmissionResult result) {You have a browser test but this code isn't executed there for some reason?
#include "base/run_loop.h"Don't add includes that are not used.
void SetUpOnMainThread() override {Any need to declare it?
password_manager::CredentialUIEntry CreateEntry(It can be implemented outside of the class
form.signon_realm = url_string;Realm should be typically without the path.
EXPECT_FALSE(delegate->HasActorTaskSubscriptionForTesting());Is it useful after you have the same condition in the `RunUntil`?
FlowDoesntStartWhenThereIsActiveTask) {What exactly does this test check?
That if Bluedog is running, we don't start the flow? Why is it desired?
Related details
- Fiorella Barrientos Villalta
- Viktor Semeniuk
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. |
Fiorella Barrientos Villalta (Gerrit)
Fiorella Barrientos Villalta voted and added 8 comments![]( "Open in Gerrit")
Votes added by Fiorella Barrientos Villalta
| Commit-Queue | +1 |
8 comments
sources += [Why are you moving the files to the bigger polluted target ?
Need chrome client and it creates a circular dependency to have it in here. The traditional APC also is in the bigger target and depends on the client so I am following that pattern. I can also change this in a separate CL and have it in a separate target that depends on traditional APC as a refactor. But kept it this way for now.
ChangePasswordFormFillingSubmissionHelper::SubmissionResult result) {You have a browser test but this code isn't executed there for some reason?
Updated, did not add it before because there is no state change. I plan to add this in a next CL, but temporarily checking that this is reset in the browser test.
Don't add includes that are not used.
Done
Any need to declare it?
No need before, but with the new changes it is.
It can be implemented outside of the class
True, moved to namespace.
Realm should be typically without the path.
Thanks, updated. Also just removed the param, we do not need it for now.
EXPECT_FALSE(delegate->HasActorTaskSubscriptionForTesting());Is it useful after you have the same condition in the `RunUntil`?
Removed.
What exactly does this test check?
That if Bluedog is running, we don't start the flow? Why is it desired?
Yes I added this to prevent the flow from starting if another BD task was running to simplify auto-login handling. Because my first idea for handling AL was to 'allow always' for a credential when there is an APC flow running, but this would be undesired if there is another BD task in the same site (unrelated to APC) running at the same time...or it would require more thought. But after talking with Ioana, I think that it is better to remove this check and just figure out how to handle these cases.
So, I updated the delegate logic to retrieve the task ID directly from the tab instead of subscribing to a single existing task like in previous patch (under the assumption that only an APC task is running).
Related details
- Fiorella Barrientos Villalta
- Vasilii Sukhanov
- Viktor Semeniuk
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Viktor Semeniuk (Gerrit)
Viktor Semeniuk added 3 comments![]( "Open in Gerrit")
sources += [Fiorella Barrientos VillaltaWhy are you moving the files to the bigger polluted target ?
Need chrome client and it creates a circular dependency to have it in here. The traditional APC also is in the bigger target and depends on the client so I am following that pattern. I can also change this in a separate CL and have it in a separate target that depends on traditional APC as a refactor. But kept it this way for now.
ChangePasswordFormWaiter only needs `password_manager::PasswordFormManager` which you can provide in a constructor to avoid depending on ChromePasswordFormManager directly. Let's try to avoid extending `chrome/browser/BUILD.gn` if possible
std::unique_ptr<ModelQualityLogsUploader> logs_uploader_;Why are we using this? I don't think we want to emit MQLS for regular APC. Let's not mix them
std::optional<actor::TaskId> actor_task_id;actor_task_id_
Related details
- Fiorella Barrientos Villalta
- Vasilii Sukhanov
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. |
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasilii Sukhanov
| Code-Review | +1 |
2 comments
form.signon_realm = url.DeprecatedGetOriginAsURL().spec();Don't use deprecated functions.
create_services_subscription_ =
BrowserContextDependencyManager::GetInstance()
->RegisterCreateServicesCallbackForTesting(
base::BindRepeating([](content::BrowserContext* context) {
OptimizationGuideKeyedServiceFactory::GetInstance()
->SetTestingFactory(
context,
base::BindRepeating(
[](content::BrowserContext* context)
-> std::unique_ptr<KeyedService> {
return std::make_unique<testing::NiceMock<
MockOptimizationGuideKeyedService>>();
}));
}));Why isn't simple `OptimizationGuideKeyedServiceFactory::SetTestingFactory` enough?
Related details
- Fiorella Barrientos Villalta
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. |