New issue
Advanced search Search tips

Issue 866444 link

Starred by 1 user
Status: Assigned
Owner:
dvadym@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocking:
issue 889920
issue 902700
issue 908268
issue 911193



Sign in to add a comment

Password generation refactoring with using new Saving/Filling arcthitecture

Project Member Reported by dvadym@chromium.org, Jul 23 
The big refactoring of filling/saving passwords infrastructure is going on (issue 831123). Password generation might also benefit from it (using unique ids, skip passing server-side data to the renderer etc). Also it's good opportunity to make clean-ups and to redesign some base functionality with using new infrastructure. 

This is umbrella bug for this refactoring.

The main changes will be in PasswordGenerationAgent class which is responsible for processing generation in the renderer process. It's going to be significantly simplified.

As result of this refactoring, some feature will be received very cheap, eg. supporting multiple forms for generation.

Comment 1 by nepper@chromium.org, Jul 23 

Hey Vadym,

Dominic let me know that this change will fix the issues we have with supporting password generation on two forms on the same page.

Are we going to track the impact from this change in some way?

Are we going to send more or different data to Google servers than before?

Thanks

Patrick

Comment 2 by dvadym@chromium.org, Jul 23 

1.We can add metric UMA or UKM metric for assessing impact.
2.No, we're not going to send more data.
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52160654f82efb9d8b914f50d03e684f2a86d62c

commit 52160654f82efb9d8b914f50d03e684f2a86d62c
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Mon Jul 30 16:58:40 2018

Fix flakiness in PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected

The reason of flakiness is the following:
1.SendKeyToPopup(ui::VKEY_RETURN) initiates sending MOJO message with a
generated password to the renderer.
2.And then immediate GetFieldValue("password_field") runs JavaScript for
extracting of value of password field, which might be not filled yet, depending
on timing of MOJO from 1 and executing JavaScript.

This CL fixes this with the same approach as in
PasswordManagerBrowserTestBase::WaitForElementValue namely waiting for filling
password field value.

The flakiness of this test becomes especially bad after CL
https://chromium-review.googlesource.com/c/chromium/src/+/1146724 . Because
that CL makes renderer part significantly faster, as result the test fails in
about 50% of cases.

Bug: 866444


Change-Id: Ifbe521bf824a1055d9955a7764e5d1cae11307aa
Reviewed-on: https://chromium-review.googlesource.com/1154914
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579073}
[modify] https://crrev.com/52160654f82efb9d8b914f50d03e684f2a86d62c/chrome/browser/password_manager/password_generation_interactive_uitest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9451b87d7edf04f0e830eede5126f754e123e9c1

commit 9451b87d7edf04f0e830eede5126f754e123e9c1
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Mon Jul 30 19:03:55 2018

Remove client-side password generation form classifier.

This classifier is superseded by server-side classifiers. Since
the generation logic in the renderer process is going to be refactored
and simplified, that the good time to remove it.

Bug: 866444,  621442 
Change-Id: I813d53fda594956813fc8cd83567712e34331cfb
Reviewed-on: https://chromium-review.googlesource.com/1146724
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579109}
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/browser/password_manager/chrome_password_manager_client.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/browser/password_manager/chrome_password_manager_client.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/renderer/autofill/fake_mojo_password_manager_driver.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/renderer/autofill/fake_mojo_password_manager_driver.h
[delete] https://crrev.com/7732d4d0afcd71f550e351bbc368d49f12aa1f72/chrome/renderer/autofill/form_classifier_browsertest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/renderer/autofill/password_generation_agent_browsertest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/chrome/test/BUILD.gn
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/common/autofill_agent.mojom
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/common/autofill_driver.mojom
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/renderer/BUILD.gn
[delete] https://crrev.com/7732d4d0afcd71f550e351bbc368d49f12aa1f72/components/autofill/content/renderer/form_classifier.cc
[delete] https://crrev.com/7732d4d0afcd71f550e351bbc368d49f12aa1f72/components/autofill/content/renderer/form_classifier.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/renderer/password_generation_agent.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/core/browser/autofill_field.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/core/browser/autofill_field.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/core/browser/form_structure.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/core/browser/form_structure_unittest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/autofill/core/browser/proto/server.proto
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/browser_save_password_progress_logger.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/browser_save_password_progress_logger_unittest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_form_manager.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_form_manager.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_form_manager_unittest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_generation_manager.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_generation_manager.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_generation_manager_unittest.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_manager.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/password_manager_driver.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/vote_uploads_test_matchers.h
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/votes_uploader.cc
[modify] https://crrev.com/9451b87d7edf04f0e830eede5126f754e123e9c1/components/password_manager/core/browser/votes_uploader.h

Comment 5 by dvadym@chromium.org, Nov 7

Blocking: 889920

Comment 6 by dvadym@chromium.org, Nov 7

Blocking: 902700

Comment 7 by dvadym@chromium.org, Nov 26

Blocking: 908268
Project Member

Comment 8 by bugdroid1@chromium.org, Dec 4 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3935716d3ee73d51013fe374de1e261f0a9a84d

commit f3935716d3ee73d51013fe374de1e261f0a9a84d
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Tue Dec 04 09:12:57 2018

Sending FoundFormEligibleForGeneration based on new parsing.

This is the first CL for implementing password generation with new
saving/filling architecture. It implements a full path of propagating
information that the form is eligible for generation from the form parser
to the renderer (namely to PasswordGenerationAgent). In the subsequent
CLs processing of this message in the renderer will be implemented.

In more details this CL contains:
1.Extending PasswordForm with new fields that are needed from parsing.
2.NewPasswordFormManager checks whether new-password field is found by
reliable sources and if yes it calls corresponding function in the driver.
3.The driver checks whether generation is enabled and if yes, it sends
MOJO message
4.The empty function for receving MOJO message is implemented in
PasswordGenerationAgent.

Bug: 866444
Change-Id: I8ea33430bc8ddd9f8ee0589a567391253ec5264e
Reviewed-on: https://chromium-review.googlesource.com/c/1346401
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613478}
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_agent.mojom
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_types.mojom
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_types.typemap
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_types_struct_traits.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_types_struct_traits.h
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/autofill_types_struct_traits_unittest.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/common/test_autofill_types.mojom
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/content/renderer/password_generation_agent.h
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/core/common/password_form.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/core/common/password_form.h
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/autofill/core/common/password_form_generation_data.h
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/core/browser/form_parsing/form_parser.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/core/browser/form_parsing/form_parser_unittest.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/core/browser/new_password_form_manager.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/core/browser/new_password_form_manager_unittest.cc
[modify] https://crrev.com/f3935716d3ee73d51013fe374de1e261f0a9a84d/components/password_manager/core/browser/password_manager_driver.h

Comment 9 by dvadym@chromium.org, Dec 4

Blocking: 911193
Project Member

Comment 10 by bugdroid1@chromium.org, Dec 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2287ff1e1dd6ec043a6e7f2550495d086faa6706

commit 2287ff1e1dd6ec043a6e7f2550495d086faa6706
Author: Vadym Doroshenko <dvadym@chromium.org>
Date: Thu Dec 27 11:13:49 2018

Automatic password generation in new architecture.

This CL contains:
1.Implementing automatic password generation with data from
NewPasswordFormGenerationData structure, that's sent from the browser
process in MOJO message FoundFormEligibleForGeneration (it was implemented
on CL https://chromium-review.googlesource.com/c/chromium/src/+/1346401).
2.New helper look-up function in DOM by unique_renderer_id
3.Stoping sending NonBlacklisted when new parser is enabled. We don't
need it anymore because  FoundFormEligibleForGeneration is sent only
in case when no blacklisted form.

Bug: 866444
Change-Id: I00f0bd844413710a0952c52e2a1d390ff3a4eb59
Reviewed-on: https://chromium-review.googlesource.com/c/1384258
Commit-Queue: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619024}
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/chrome/renderer/autofill/password_generation_agent_browsertest.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/content/renderer/form_autofill_util.h
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/content/renderer/form_autofill_util_browsertest.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/content/renderer/password_generation_agent.h
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/autofill/core/common/password_form_generation_data.h
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/password_manager/core/browser/new_password_form_manager.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/password_manager/core/browser/password_form_filling.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/password_manager/core/browser/password_form_filling_unittest.cc
[modify] https://crrev.com/2287ff1e1dd6ec043a6e7f2550495d086faa6706/components/password_manager/core/browser/password_generation_manager.cc
►

Sign in to add a comment