New issue
Advanced search Search tips

Issue 905669 link

Starred by 1 user
Status: Assigned
Owner:
ftirelo@chromium.org
Cc:
fhorschig@google.com
ioanap@chromium.org
vasi...@chromium.org
ftirelo@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Generalize password accessory backend code

Project Member Reported by ftirelo@chromium.org, Nov 15 
Tracking bug for password accessory backend code refactoring to allow sending Autofill data to the frontend.

This comprises the following tasks, that should translate into separate CLs:

1. Spawn ManualFillingController from PasswordAccessoryController. This will allow us to later plug an AutofillAccessoryController to serve addresses and credit cards to the keyboard accessory.

2. Rename PasswordAccessoryViewAndroid/PasswordAccessoryBridge to ManualFillingViewAndroid/ManualFillingBridge.

3. Generalize password generation in ManualFillingController to forward a generic action from a given tab type to the appropriate controller.

4. Generalize favicon provider in ManualFillingController to forward a generic request from a given tab type to the appropriate controller.

5. Move all the logic for handling showing/hiding keyboard accessory to ManualFillingController.

Comment 1 by ftirelo@chromium.org, Nov 15

Description: Show this description

Comment 2 by fhorschig@chromium.org, Nov 19

Cc: ioanap@chromium.org
+ioanap@ who probably wants to track the generalization of password generation. Will the upcoming manual password generation conflict with this?

Comment 3 by ioanap@chromium.org, Nov 19 

Thank you for looping me in! From what I remember of my design of the manual password generation on Android, this should not conflict with it.

For manual password generation, the signals that should go through these controllers are: 
    * From backend to java: a signal that manual generation is available which should result in the action being displayed in the accessory sheet.
    * From java to backend: a signal that the user tapped on that action.

The rest of the logic will be extracted out of the PasswordAccessoryController.

Let me know if more details are needed!

Comment 4 by ftirelo@chromium.org, Nov 19 

Thanks for the details, Ioana!

We may have a few merge conflicts between our CLs, but I don't see major issues other than that. Cc'ing each other on CLs as FYI seems to be a good idea.

At some point, I will add a parameter to the ManualFillingController to indicate which controller should handle a view request (password or autofill).

I'm trying to land some unblockers now, so I have many things that could be done in parallel. The idea is that I can work on other parts of the project when you or fhorschig@ have tasks that would conflict a lot with the my refactoring tasks. In those cases, please let me know.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 20 

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

commit bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8
Author: Fabio Tirelo <ftirelo@chromium.org>
Date: Tue Nov 20 21:02:03 2018

[Mfill Android] Introduces ManualFillingController

This is the first CL to generalized the keyboard accessory handlers
on the Chrome backend, so they can be also used to serve Autofill data.

This CL spawns ManualFillingController from PasswordAccessoryController,
both controllers are attached to a WebContents object.

The new controller which is responsible for:
 - Coordinating requests from type-specific accessory controllers
   (Autofill and Password) and forwarding them to the native UI;
 - Forwarding events from the native UI to the type-specific
   controllers.

To allow mocking of these classes in unit tests, this CL defines an
interface and an implementation for each controller.

Code review order suggestion:
 - password_accessory_controller.h: split original class into
   interface + implementation, this is the interface;
 - password_accessory_controller_impl.h: class implementing
   the interface;
 - manual_filling_controller.h: very similar to
   password_accessory_controller, but git doesn't understand
   copies; same idea: interface + implementation;
 - manual_filling_controller_impl.cc: class implementing the
   interface;
 - password_accessory_controller_impl*.cc;
 - manual_filling_controller_impl*.cc;
 - everything else, basically minor changes.

In order to keep this CL more focused on the split, some steps of the
generalization will be done in follow-ups (check TODOs pointing to
https://crbug.com/896690), and the ManualFillingCoordinator simply
forwards events to PasswordAccessoryController. One example is
password generation: in this CL, MFC contains methods to forward
password generation events to PAC; in the final state, MFC will
define a general function that handles "actions" and will forward
events to type-specific coordinators (passwords or autofill) based
on action type.

Please refer to the linked bug for details on next steps of this
refactoring. Once it's done, we will be able to plug in Autofill
requests.

Bug: 905669
Change-Id: I4fb2fd1b4a729114d3c7e4dd59ec7a582ba8aea0
Reviewed-on: https://chromium-review.googlesource.com/c/1336557
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Fabio Tirelo <ftirelo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609797}
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/android/BUILD.gn
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/BUILD.gn
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/android/password_manager/password_accessory_view_android.cc
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/android/password_manager/password_accessory_view_android.h
[add] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/autofill/manual_filling_controller.h
[add] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/autofill/manual_filling_controller_impl.cc
[add] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/autofill/manual_filling_controller_impl.h
[add] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/autofill/manual_filling_controller_impl_unittest.cc
[rename] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/autofill/manual_filling_view_interface.h
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/chrome_password_manager_client.cc
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/password_accessory_controller.h
[rename] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/password_accessory_controller_impl.cc
[add] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/password_accessory_controller_impl.h
[rename] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/password_accessory_controller_impl_unittest.cc
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/browser/password_manager/password_generation_dialog_view_interface.h
[modify] https://crrev.com/bfbbb3e993f21eb144d73b88ea2d94a5161e6bb8/chrome/test/BUILD.gn

Comment 7 by benhenry@google.com, Jan 11

Status: Assigned (was: Untriaged)
This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this.
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 17 (5 days ago) 

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

commit 839d9d5f637eaf689442799db81935bb079b5970
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Thu Jan 17 14:34:31 2019

[Mfill-Android] Move autofill suggestions into keyboard accessory

With this change, all entries from the autofill popup move into the
keyboard accessory. The accessory will show up instead of the popup but
contain the exact same contents as the popup would have.
Only additions: fallbacks and (if available) password generation.

That means, the keyboard accessory is shown whenever at least one of
two sources provides suggestions (Passwords or Autofill).

Example for a password field on a site with one saved credential pair:
Before this CL:
   we would show a popup with the credential and
   additionally, the accessory with only the key icon.
With this CL:
   we show the accessory with the key icon and the credential
   (previously in the popup, now it's a suggestion). While tapping the
   credential fills the form, tapping the password chip in the
   accessory sheet fills only the password field.

Changes for consistency:
 - Usernames can now be filled into non-password fields.
 - Passwords can still only be filled into password fields.
 - For password fields, only passwords/autofill suggestions are enabled.
 - The "No username" username can never be filled.

Bug: 894793, 905669
Change-Id: I09f9e1910492840ed9e3071f0e44343bb5c01756
Reviewed-on: https://chromium-review.googlesource.com/c/1405310
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623680}
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/autofill/manual_filling_controller.h
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/autofill/manual_filling_controller_impl.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/autofill/manual_filling_controller_impl.h
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/autofill/manual_filling_controller_impl_unittest.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/password_manager/chrome_password_manager_client.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/password_manager/password_accessory_controller.h
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/password_manager/password_accessory_controller_impl.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/password_manager/password_accessory_controller_impl.h
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/password_manager/password_accessory_controller_impl_unittest.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
[modify] https://crrev.com/839d9d5f637eaf689442799db81935bb079b5970/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
►

Sign in to add a comment