WebView autofill implementation

WebView autofill implementation

WebView autofill implementation doesn't use chrome's autofill service
and UI, instead, it use autofill service in Android platform to get
form autofilled.

BUG=693204
Review-Url: https://codereview.chromium.org/2839023003
Cr-Commit-Position: refs/heads/master@{#476389}
Committed: https://chromium.googlesource.com/chromium/src/+/927a09ab04f76ed68c8a1940992381fe8cc33def

[michaelbai, 2017-04-26 01:24:37]

Description was changed from

==========
WebView autofill implementation

WebView autofill implementation doesn't use chrome's autofill service
and UI, instead, it use autofill service in Android platform to get
form autofilled.

BUG=693204
==========

to

==========
WebView autofill implementation

WebView autofill implementation doesn't use chrome's autofill service
and UI, instead, it use autofill service in Android platform to get
form autofilled.

BUG=693204
==========

[michaelbai, 2017-04-26 01:24:37]

michaelbai@chromium.org changed reviewers:
+ rogerm@chromium.org, sgurun@chromium.org

[michaelbai, 2017-04-26 01:24:56]

PTAL

[sgurun-gerrit only, 2017-04-27 21:51:44]

I just started looking, but one important comment below:

https://codereview.chromium.org/2839023003/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/2839023003/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:111:
private boolean mAutoCompleteEnabled = true;
do not enable autocomplete for O, it will be disabled.

[sgurun-gerrit only, 2017-04-28 01:46:40]

some more comments below.

https://codereview.chromium.org/2839023003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents.cc:292: void
AwContents::InitAutofillIfNecessary(bool enabled) {
In O and above initautofillifnecessary should always be called. AwAutofillClient
has a autocompleteenabled method. I think autocomplete can be turned off there.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:18:
* AutofillProvider handles one autofill session at time, each call of
is this limitation per WebContents? what if there are multiple webviews in the
app.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:55: // The id
isn't passed to Java side because Android API guaranttes the
nit: s/guaranttes/guarantees/

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:73:
form_.reset(new FormDataAndroid(form));
MakeUnique?

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:76: if
(!form_->GetFieldIndex(field, &index))
I don't understand why we need to do the similarity check here. Don't we already
do form_->SimilarFormAs(form) above? 

extra comments would be useful here.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:81:
Java_AutofillProvider_queryFormFieldAutofill(
keep the same name, i.e. onQueryFormFieldAutofill

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:83:
bounding_box.width(), bounding_box.height());
you don't seem to be converting x,y coordinates to screen bound, see
AwAutofillclient::Showautofillpopup? is this because Android API expects
boundaries relative to mContainerView?

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/form_field_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:54: void
FormFieldDataAndroid::PullAutofillValue() {
make names match with java, i.e. getValue?

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:71:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
nit: move dcheck ahead

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:77:
Java_FormFieldData_updataValue(env, obj,
nit: s/updata/update/

[Roger McFarlane (Chromium), 2017-04-28 20:28:27]

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:18:
* AutofillProvider handles one autofill session at time, each call of
What is the scope of a session?

How does this work for multi-form pages? Do you handle segmented forms?

On a given page, you might have:

  - a sign-in form at the top
  - a separate guest checkout form asking for contact and/or credit card
    information
      -> these might be seperate forms or they might be the same form
      -> you might have both billing and shipping addresses
  - the form might change dynamically on the page as the user (and
    autofill) are interacting with it.

IIUC, this is driven by the user attempting to autofill in a form (whose
fields are determined by the webview) and represents one fill attempt?
I.e., is this created in response to the user initiating some fill event
for a given set of fields that constitutes a form? Or, is this tied to a
web-contents?

[michaelbai, 2017-04-28 20:52:01]

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:18:
* AutofillProvider handles one autofill session at time, each call of
On 2017/04/28 20:28:27, Roger McFarlane (Chromium) wrote:
> What is the scope of a session?
> 
> How does this work for multi-form pages? Do you handle segmented forms?
> 
> On a given page, you might have:
> 
>   - a sign-in form at the top
>   - a separate guest checkout form asking for contact and/or credit card
>     information
>       -> these might be seperate forms or they might be the same form
>       -> you might have both billing and shipping addresses
>   - the form might change dynamically on the page as the user (and
>     autofill) are interacting with it.
> 
> IIUC, this is driven by the user attempting to autofill in a form (whose
> fields are determined by the webview) and represents one fill attempt?
> I.e., is this created in response to the user initiating some fill event
> for a given set of fields that constitutes a form? Or, is this tied to a
> web-contents?

Yes, It is driven by user, more specifically, A new session will started if the
form is different  among the calling of
AutofillManager::OnQueryFormFieldAutofill() ,

https://codereview.chromium.org/2839023003/diff/1/components/autofill/android...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:18:
* AutofillProvider handles one autofill session at time, each call of
On 2017/04/28 01:46:39, sgurun wrote:
> is this limitation per WebContents? what if there are multiple webviews in the
> app.

Each WebView has its own AutofillProvider, since each WebView is different
custom view, framework should have different session for them.

[michaelbai, 2017-05-03 04:43:59]

PTAL

https://codereview.chromium.org/2839023003/diff/1/android_webview/java/src/or...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/2839023003/diff/1/android_webview/java/src/or...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:111:
private boolean mAutoCompleteEnabled = true;
On 2017/04/27 21:51:44, sgurun wrote:
> do not enable autocomplete for O, it will be disabled. 

Done.

https://codereview.chromium.org/2839023003/diff/1/android_webview/native/aw_c...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/1/android_webview/native/aw_c...
android_webview/native/aw_contents.cc:292: void
AwContents::InitAutofillIfNecessary(bool enabled) {
On 2017/04/28 01:46:39, sgurun wrote:
> In O and above initautofillifnecessary should always be called.
AwAutofillClient
> has a autocompleteenabled method. I think autocomplete can be turned off
there.

Used different way, see new patch of this method.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:55: // The id
isn't passed to Java side because Android API guaranttes the
On 2017/04/28 01:46:39, sgurun wrote:
> nit: s/guaranttes/guarantees/

Done.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:73:
form_.reset(new FormDataAndroid(form));
On 2017/04/28 01:46:39, sgurun wrote:
> MakeUnique?

Done.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:76: if
(!form_->GetFieldIndex(field, &index))
On 2017/04/28 01:46:40, sgurun wrote:
> I don't understand why we need to do the similarity check here. Don't we
already
> do form_->SimilarFormAs(form) above? 
> 
> extra comments would be useful here.

This uses to get index of field.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:81:
Java_AutofillProvider_queryFormFieldAutofill(
On 2017/04/28 01:46:40, sgurun wrote:
> keep the same name, i.e. onQueryFormFieldAutofill

I intentionally used different name, OnQueryFormFieldAutofill() only calls
Java_AutofillProvider_queryFormFieldAutofill() when new session is needed, I
changed it to startAutofillSession to make it more clear.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_provider_android.cc:83:
bounding_box.width(), bounding_box.height());
On 2017/04/28 01:46:40, sgurun wrote:
> you don't seem to be converting x,y coordinates to screen bound, see
> AwAutofillclient::Showautofillpopup? is this because Android API expects
> boundaries relative to mContainerView?
> 

The bound still wasn't correct, after use the code in ShowAutofillPopup, I will
figure out reason, meanwhile, please just review other code.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/form_field_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:54: void
FormFieldDataAndroid::PullAutofillValue() {
On 2017/04/28 01:46:40, sgurun wrote:
> make names match with java, i.e. getValue?

Done.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:71:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2017/04/28 01:46:40, sgurun wrote:
> nit: move dcheck ahead 

Done.

https://codereview.chromium.org/2839023003/diff/1/components/autofill/core/br...
components/autofill/core/browser/form_field_data_android.cc:77:
Java_FormFieldData_updataValue(env, obj,
On 2017/04/28 01:46:40, sgurun wrote:
> nit: s/updata/update/

Done.

[michaelbai, 2017-05-04 19:47:27]

PTAL, Here is summary of patch 2 and 3's changes, hopefully could help for the
review. 

- Fixed coordinates 

- Changed the AutofillProviderAndroid's owner, now  it is owned by AwContent.
and  doesn't have 1:1 mapping to AwAutofillProvider.java.

- Moved autofill_provider_android to content/browser directory because it
accesses WebContents now, also removed core/browser/DEPS change.

- Used new SDK methods.

- Reset autofill session when webview navigate to other page.

[Roger McFarlane (Chromium), 2017-05-05 21:19:43]

This looks OK to me.

Adding sebsg@ for a second set of Autofill eyes.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:37:
* form by provided values.
nit: s/by/with the/

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:50:
* @param flags
description of flags?

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:57:
* service for the value to fill the form.
nit: s/value/values with which/

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:115:
* Invoked when AutofillManager reset cache.
nit: s/reset/resets its/

or "resets the"

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
File components/autofill/content/browser/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
components/autofill/content/browser/autofill_provider_android.cc:44:
Java_AutofillProvider_reset(env, obj, reinterpret_cast<long>(this));
are sizeof(long) and sizeof(ptr_t) always the same for Android?

[Roger McFarlane (Chromium), 2017-05-05 21:20:01]

rogerm@chromium.org changed reviewers:
+ sebsg@chromium.org

[sebsg, 2017-05-08 15:38:17]

Interesting! some comments:

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
File components/autofill/content/browser/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
components/autofill/content/browser/autofill_provider_android.h:5: #ifndef
COMPONENTS_AUTOFILL_CONTENT_BROWSER_AUTOFILL_PROVIDER_ANDROID_H_
Could you please move this file and the other _android files to
components/autofill/android?

Thanks!

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:530:
provider_->OnTextFieldDidChange(this, form, field, timestamp);
I notice there are a lot of if statements like this. It's difficult for me to
check how much of the original AutofillManager you are using, but you seem to
want to change/stop the behavior in a lot of places. 

Do you think subclassing the AutofillManager would be feasible?

I just feels weird to have all these if and return statements.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
components/autofill/core/common/form_data.h:27: // comparing fields, so we know
forms are similar enough to be considered as
comparing -> compare. Also I feel like what's after the coma doesn't add much to
understand what the function does, I would remove it.

[michaelbai, 2017-05-09 00:27:04]

PTAL

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:37:
* form by provided values.
On 2017/05/05 21:19:43, Roger McFarlane (Chromium) wrote:
> nit: s/by/with the/

Done.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:50:
* @param flags
On 2017/05/05 21:19:43, Roger McFarlane (Chromium) wrote:
> description of flags?

This is coming from Android API, added see...

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:57:
* service for the value to fill the form.
On 2017/05/05 21:19:43, Roger McFarlane (Chromium) wrote:
> nit: s/value/values with which/

Done.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:115:
* Invoked when AutofillManager reset cache.
On 2017/05/05 21:19:43, Roger McFarlane (Chromium) wrote:
> nit: s/reset/resets its/
> 
> or "resets the"

Done.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
File components/autofill/content/browser/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
components/autofill/content/browser/autofill_provider_android.cc:44:
Java_AutofillProvider_reset(env, obj, reinterpret_cast<long>(this));
On 2017/05/05 21:19:43, Roger McFarlane (Chromium) wrote:
> are sizeof(long) and sizeof(ptr_t) always the same for Android?

Thanks, to make it more clear, I changed it to jlong, to wrap the difference
between 32-bit and 64-bit native library, we always use jlong to pass the
pointer around.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
File components/autofill/content/browser/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/con...
components/autofill/content/browser/autofill_provider_android.h:5: #ifndef
COMPONENTS_AUTOFILL_CONTENT_BROWSER_AUTOFILL_PROVIDER_ANDROID_H_
On 2017/05/08 15:38:17, sebsg wrote:
> Could you please move this file and the other _android files to
> components/autofill/android?
> 
> Thanks!

Done.

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:530:
provider_->OnTextFieldDidChange(this, form, field, timestamp);
On 2017/05/08 15:38:17, sebsg wrote:
> I notice there are a lot of if statements like this. It's difficult for me to
> check how much of the original AutofillManager you are using, but you seem to
> want to change/stop the behavior in a lot of places. 
> 
> Do you think subclassing the AutofillManager would be feasible?
> 
> I just feels weird to have all these if and return statements.


We were planing to cherry-pick this patch to M59, also I have been told your
guys will refactorying those code (well, maybe password manager related code),
so I didn't want to change the current code too much.

I think I can do something like below, but in a following up patch if you want,

Basically, Autofill provider is only care about the interface between
AutofillManager and ContentAutofillDriver, and takes advantage of validation
code, so we can

- extract this interface and common code into an abstract class AutofillManager
- and rename current AutofillManager to AutofillManagerImpl
- Also create a new class like AutofillManagerProxy which subclass
AutofillManager also forward call to AutofillProvider,
- we create different implementation of AutofillManager in ContentAutofillDriver
based on if AutofillProvider is available.

Thought?

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
File components/autofill/core/common/form_data.h (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
components/autofill/core/common/form_data.h:27: // comparing fields, so we know
forms are similar enough to be considered as
On 2017/05/08 15:38:17, sebsg wrote:
> comparing -> compare. Also I feel like what's after the coma doesn't add much
to
> understand what the function does, I would remove it.

Done.

[michaelbai, 2017-05-09 23:54:35]

Roger, sebsg@ would you like review this again?

[Roger McFarlane (Chromium), 2017-05-10 02:31:54]

AFAICT, the diffs from the previous CL are just:

- moving some files under android/ subdir
- the nits raised in the previous review
- long -> jlong for pointer

?

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2839023003/diff/40001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:530:
provider_->OnTextFieldDidChange(this, form, field, timestamp);
On 2017/05/09 00:27:03, michaelbai wrote:
> On 2017/05/08 15:38:17, sebsg wrote:
> > I notice there are a lot of if statements like this. It's difficult for me
to
> > check how much of the original AutofillManager you are using, but you seem
to
> > want to change/stop the behavior in a lot of places. 
> > 
> > Do you think subclassing the AutofillManager would be feasible?
> > 
> > I just feels weird to have all these if and return statements.
> 
> 
> We were planing to cherry-pick this patch to M59, also I have been told your
> guys will refactorying those code (well, maybe password manager related code),
> so I didn't want to change the current code too much.
> 
> I think I can do something like below, but in a following up patch if you
want,
> 
> Basically, Autofill provider is only care about the interface between
> AutofillManager and ContentAutofillDriver, and takes advantage of validation
> code, so we can
> 
> - extract this interface and common code into an abstract class
AutofillManager
> - and rename current AutofillManager to AutofillManagerImpl
> - Also create a new class like AutofillManagerProxy which subclass
> AutofillManager also forward call to AutofillProvider,
> - we create different implementation of AutofillManager in
ContentAutofillDriver
> based on if AutofillProvider is available.
> 
> Thought?

That generally seems like a good approach.

But, you're right, that would likely be too large to merge into M59... so this
hooked approach makes sense in the interim. It's just tricky to make sure we
understand which functionality you meant to override vs reuse (i.e., if you
forgot a hook location, it's hard for us as reviewers to notice).

[michaelbai, 2017-05-10 02:37:10]

Yes, all diffs are for addressing the comments from sebsg@ and you

[Roger McFarlane (Chromium), 2017-05-10 03:10:35]

LGTM

Please wait for sebsg@ to concur (or not).

[sebsg, 2017-05-10 14:33:26]

Thanks for moving the files, and yeah sure, let's make it cleaner in a future CL
if you have to merge.

LGTM

[sgurun-gerrit only, 2017-05-11 00:13:29]

thanks for the hard work.

https://codereview.chromium.org/2839023003/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2839023003/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2918:
private AutofillProvider getAutofillProvider() {
let's use -the well established- nativeSetJavaPeers to set AutofillProvider,
unless there is some reason we cannot do it. 

simply in native code make sure not to create a autofill_provider_ if java
autofillprovider is null. the native peer can then set the pointer to it by
making a call to Java peer. 

this is the design pattern that we have used previously (see
AwContentsClientBridge for example). better to keep the same mechanism.

Then you can drop if (nativeAutofillProvider != mNativeAutofillProvider) calls
from your AwAutofillProvider and drop natigegetAutofillProvider thingy.

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:292: void
AwContents::InitAutofillIfNecessary(bool enabled) {
rename enabled to autocomplete_enabled

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:305:
base::android::ScopedJavaLocalRef<_jobject*> jAutofillProvider =
you can drop this once you use nativeSetJavaPeers.

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:308: // Just return, if the app neither
targets for O nor enables autocomplete.
s/app targets for O/app runs on O SDK/

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
File components/autofill/android/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
components/autofill/android/autofill_provider_android.cc:53: // The id isn't
passed to Java side because Android API guarantees the
by guarantee, if you mean that android won't make a call for autofill, we know
that this is not true anymore.

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
components/autofill/android/autofill_provider_android.cc:203: }  // namespace
autofil
nit: s/autofil/autofill

[michaelbai, 2017-05-12 00:37:23]

PTAL

https://codereview.chromium.org/2839023003/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/2839023003/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:2918:
private AutofillProvider getAutofillProvider() {
On 2017/05/11 00:13:29, sgurun wrote:
> let's use -the well established- nativeSetJavaPeers to set AutofillProvider,
> unless there is some reason we cannot do it. 
> 
> simply in native code make sure not to create a autofill_provider_ if java
> autofillprovider is null. the native peer can then set the pointer to it by
> making a call to Java peer. 
> 
> this is the design pattern that we have used previously (see
> AwContentsClientBridge for example). better to keep the same mechanism.
> 
> Then you can drop if (nativeAutofillProvider != mNativeAutofillProvider) calls
> from your AwAutofillProvider and drop natigegetAutofillProvider thingy.

Done.

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:305:
base::android::ScopedJavaLocalRef<_jobject*> jAutofillProvider =
On 2017/05/11 00:13:29, sgurun wrote:
> you can drop this once you use nativeSetJavaPeers.

Done.

https://codereview.chromium.org/2839023003/diff/60001/android_webview/native/...
android_webview/native/aw_contents.cc:308: // Just return, if the app neither
targets for O nor enables autocomplete.
On 2017/05/11 00:13:29, sgurun wrote:
> s/app targets for O/app runs on O SDK/

Done.

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
File components/autofill/android/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
components/autofill/android/autofill_provider_android.cc:53: // The id isn't
passed to Java side because Android API guarantees the
On 2017/05/11 00:13:29, sgurun wrote:
> by guarantee, if you mean that android won't make a call for autofill, we know
> that this is not true anymore. 

As we discussed, this statement is still true.

https://codereview.chromium.org/2839023003/diff/60001/components/autofill/and...
components/autofill/android/autofill_provider_android.cc:203: }  // namespace
autofil
On 2017/05/11 00:13:29, sgurun wrote:
> nit: s/autofil/autofill

Done.

[sgurun-gerrit only, 2017-05-12 22:47:37]

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
protected abstract void startAutofillSession(Object webContents,
sorry, I missed this in my first code review. 

Autofill component can depend on Content, right. Or are there any special
requirements here?

[sgurun-gerrit only, 2017-05-12 22:49:41]

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
protected abstract void startAutofillSession(Object webContents,
On 2017/05/12 22:47:37, sgurun wrote:
> sorry, I missed this in my first code review. 
> 
> Autofill component can depend on Content, right. Or are there any special
> requirements here?

i.e. I am asking because I saw Object webContents

[michaelbai, 2017-05-12 23:51:18]

PTAL

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
protected abstract void startAutofillSession(Object webContents,
On 2017/05/12 22:49:41, sgurun wrote:
> On 2017/05/12 22:47:37, sgurun wrote:
> > sorry, I missed this in my first code review. 
> > 
> > Autofill component can depend on Content, right. Or are there any special
> > requirements here?
> 
> i.e. I am asking because I saw Object webContents

I don't think autofill can't depend on content,  changed to use existing
pattern.

[sgurun-gerrit only, 2017-05-12 23:59:27]

On 2017/05/12 23:51:18, michaelbai wrote:
> PTAL
> 
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> File
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
> (right):
> 
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
> protected abstract void startAutofillSession(Object webContents,
> On 2017/05/12 22:49:41, sgurun wrote:
> > On 2017/05/12 22:47:37, sgurun wrote:
> > > sorry, I missed this in my first code review. 
> > > 
> > > Autofill component can depend on Content, right. Or are there any special
> > > requirements here?
> > 
> > i.e. I am asking because I saw Object webContents
> 
> I don't think autofill can't depend on content,  changed to use existing
> pattern.

rogerm@, can autofill depend on content?

It is not a super important issue, but passing a variable as Object is generally
not the best way as it removes type checking (well in this case it comes from
native, so less important)

[sgurun-gerrit only, 2017-05-15 19:51:53]

On 2017/05/12 23:59:27, sgurun wrote:
> On 2017/05/12 23:51:18, michaelbai wrote:
> > PTAL
> > 
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > File
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
> > protected abstract void startAutofillSession(Object webContents,
> > On 2017/05/12 22:49:41, sgurun wrote:
> > > On 2017/05/12 22:47:37, sgurun wrote:
> > > > sorry, I missed this in my first code review. 
> > > > 
> > > > Autofill component can depend on Content, right. Or are there any
special
> > > > requirements here?
> > > 
> > > i.e. I am asking because I saw Object webContents
> > 
> > I don't think autofill can't depend on content,  changed to use existing
> > pattern.
> 
> rogerm@, can autofill depend on content?
> 
> It is not a super important issue, but passing a variable as Object is
generally
> not the best way as it removes type checking (well in this case it comes from
> native, so less important)

Michael and I looked at it, and android can already depend Content, so that
change can be done from Object to WebContents.

lgtm once Michael does the change.

[Mathieu, 2017-05-15 19:56:34]

On 2017/05/15 19:51:53, sgurun wrote:
> On 2017/05/12 23:59:27, sgurun wrote:
> > On 2017/05/12 23:51:18, michaelbai wrote:
> > > PTAL
> > > 
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > > File
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
> > > protected abstract void startAutofillSession(Object webContents,
> > > On 2017/05/12 22:49:41, sgurun wrote:
> > > > On 2017/05/12 22:47:37, sgurun wrote:
> > > > > sorry, I missed this in my first code review. 
> > > > > 
> > > > > Autofill component can depend on Content, right. Or are there any
> special
> > > > > requirements here?
> > > > 
> > > > i.e. I am asking because I saw Object webContents
> > > 
> > > I don't think autofill can't depend on content,  changed to use existing
> > > pattern.
> > 
> > rogerm@, can autofill depend on content?
> > 
> > It is not a super important issue, but passing a variable as Object is
> generally
> > not the best way as it removes type checking (well in this case it comes
from
> > native, so less important)
> 
> Michael and I looked at it, and android can already depend Content, so that
> change can be done from Object to WebContents.
> 
> lgtm once Michael does the change.

Hi, I think the merge window is pretty much closing for M59, and this is a
non-trivial change. Please reconsider the need to merge?

[Mathieu, 2017-05-15 19:57:24]

On 2017/05/15 19:56:34, Mathieu wrote:
> On 2017/05/15 19:51:53, sgurun wrote:
> > On 2017/05/12 23:59:27, sgurun wrote:
> > > On 2017/05/12 23:51:18, michaelbai wrote:
> > > > PTAL
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > > > File
> > > >
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > > >
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
> > > > protected abstract void startAutofillSession(Object webContents,
> > > > On 2017/05/12 22:49:41, sgurun wrote:
> > > > > On 2017/05/12 22:47:37, sgurun wrote:
> > > > > > sorry, I missed this in my first code review. 
> > > > > > 
> > > > > > Autofill component can depend on Content, right. Or are there any
> > special
> > > > > > requirements here?
> > > > > 
> > > > > i.e. I am asking because I saw Object webContents
> > > > 
> > > > I don't think autofill can't depend on content,  changed to use existing
> > > > pattern.
> > > 
> > > rogerm@, can autofill depend on content?
> > > 
> > > It is not a super important issue, but passing a variable as Object is
> > generally
> > > not the best way as it removes type checking (well in this case it comes
> from
> > > native, so less important)
> > 
> > Michael and I looked at it, and android can already depend Content, so that
> > change can be done from Object to WebContents.
> > 
> > lgtm once Michael does the change.
> 
> Hi, I think the merge window is pretty much closing for M59, and this is a
> non-trivial change. Please reconsider the need to merge?

Also since this is only used on Android the code should be behind if
defined(OS_ANDROID) (see the autofill_assistant example)

[sgurun-gerrit only, 2017-05-15 20:29:25]

On 2017/05/15 19:57:24, Mathieu wrote:
> On 2017/05/15 19:56:34, Mathieu wrote:
> > On 2017/05/15 19:51:53, sgurun wrote:
> > > On 2017/05/12 23:59:27, sgurun wrote:
> > > > On 2017/05/12 23:51:18, michaelbai wrote:
> > > > > PTAL
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > > > > File
> > > > >
> > > >
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2839023003/diff/80001/components/autofill/and...
> > > > >
> > > >
> > >
> >
>
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:64:
> > > > > protected abstract void startAutofillSession(Object webContents,
> > > > > On 2017/05/12 22:49:41, sgurun wrote:
> > > > > > On 2017/05/12 22:47:37, sgurun wrote:
> > > > > > > sorry, I missed this in my first code review. 
> > > > > > > 
> > > > > > > Autofill component can depend on Content, right. Or are there any
> > > special
> > > > > > > requirements here?
> > > > > > 
> > > > > > i.e. I am asking because I saw Object webContents
> > > > > 
> > > > > I don't think autofill can't depend on content,  changed to use
existing
> > > > > pattern.
> > > > 
> > > > rogerm@, can autofill depend on content?
> > > > 
> > > > It is not a super important issue, but passing a variable as Object is
> > > generally
> > > > not the best way as it removes type checking (well in this case it comes
> > from
> > > > native, so less important)
> > > 
> > > Michael and I looked at it, and android can already depend Content, so
that
> > > change can be done from Object to WebContents.
> > > 
> > > lgtm once Michael does the change.
> > 
> > Hi, I think the merge window is pretty much closing for M59, and this is a
> > non-trivial change. Please reconsider the need to merge?

We will not merge.

> 
> Also since this is only used on Android the code should be behind if
> defined(OS_ANDROID) (see the autofill_assistant example)

[michaelbai, 2017-05-16 00:12:22]

michaelbai@chromium.org changed reviewers:
+ avi@chromium.org

[michaelbai, 2017-05-16 00:13:01]

avi@ could you help review the dep change of components/autofill/android/DEPS

[michaelbai, 2017-05-16 00:13:11]

The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 00:14:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 00:27:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 00:27:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Avi (use Gerrit), 2017-05-16 15:17:59]

On 2017/05/16 00:13:01, michaelbai wrote:
> avi@ could you help review the dep change of components/autofill/android/DEPS

LGTM

[sebsg, 2017-05-16 16:58:57]

If you don't need to merge, could you do the refactoring in this CL?

[Roger McFarlane (Chromium), 2017-05-16 17:13:24]

If this is no longer targeted to be merged into M59, then the rush factor goes
down and I'd strongly prefer not to hook in autofill_provider* in this way.

michaelbai@ had a good proposal for refactoring the manager/provider interface.
I'd much rather see us go straight to that instead of taking on the techical
debt to do it later.

re: depending on content

components/autofill/content and it's dependents can depend on content. Avoid
adding content dependencies to components/autofill/core

[michaelbai, 2017-05-16 18:47:42]

On 2017/05/16 17:13:24, Roger McFarlane (Chromium) wrote:
> If this is no longer targeted to be merged into M59, then the rush factor goes
> down and I'd strongly prefer not to hook in autofill_provider* in this way.
> 
> michaelbai@ had a good proposal for refactoring the manager/provider
interface.
> I'd much rather see us go straight to that instead of taking on the techical
> debt to do it later.
> 
> re: depending on content
> 
> components/autofill/content and it's dependents can depend on content. Avoid
> adding content dependencies to components/autofill/core

sure, will do it.

[michaelbai, 2017-05-19 03:28:23]

Roger & sebsg@ PTAL

This patch refactoring the AutofillManager, the main changes are

- added AutofillHandler for the interface between ContentAutofillDriver and
AutofillManager(AutofillProvider)
- added AutofillHandlerProxy to forward calls to AutofillProvider
- moved some common code between AutofillManager and AutofillProvider to
AutofillHandler.
- fixed unit test, deps issues.

[michaelbai, 2017-05-19 03:29:20]

The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 03:30:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 03:40:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 03:40:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[michaelbai, 2017-05-23 01:44:22]

ping, Roger & sebsg@ could you help to review this patch again.

[Roger McFarlane (Chromium), 2017-05-24 04:18:22]

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:158: if
(autofill_manager_ && !autofill_manager_->IsShowingUnmaskPrompt())
maybe add IsShowingUnmaskPrompt as a, default false, method on autofill handler?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:27: class AutofillHandler {
would the hierarchy be better named:

  AutofillManager
    ^  ^
    |  |
    |  +-- AutofillManagerProxy
    |
    +-- DefaultAutofillManager

I recall you wanter to avoid Impl in the class name.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.cc:21:
provider_->OnWillSubmitForm(this, form, timestamp);
should the provider return a bool?

[michaelbai, 2017-05-24 05:14:09]

Thanks Roger for working late, before starting my next patch, I have some
questions about your comments, please see inline comments.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:158: if
(autofill_manager_ && !autofill_manager_->IsShowingUnmaskPrompt())
On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> maybe add IsShowingUnmaskPrompt as a, default false, method on autofill
handler?

I didn't add this method to the interface because it is not directly called from
ContentAutofillDrive, it seems that AutofillProvider probably will never use it.
Do you think this method is meaningful for AutofiillProvider? I can add it to
interface if you like.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:27: class AutofillHandler {
On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> would the hierarchy be better named:
> 
>   AutofillManager
>     ^  ^
>     |  |
>     |  +-- AutofillManagerProxy
>     |
>     +-- DefaultAutofillManager
> 
> I recall you wanter to avoid Impl in the class name.

I will rename those classes, please be noticed renaming AutofillManager will
cause a lot of name changes both in components/autofill and chrome directory.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.cc:21:
provider_->OnWillSubmitForm(this, form, timestamp);
On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> should the provider return a bool?

this return value seemed no meaning for AutofillProvider, and also
ContentAutofillDriver doesn't handle the return value. shall we just change
return type to void?

What's meaning of return value of OnWillSubmitForm?

[sebsg, 2017-05-24 13:32:43]

Thanks for the refactoring, some comments.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:26: // Override
AutofillProvider methods
The usual syntax is

// AutofillProvider:


and then put all of those back to back without lines in between.

Thanks!

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:17: // avaliable in Java.
typo: available

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver_factory.h
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver_factory.h:78:
AutofillProvider* provider_;
Since this is only used on Android, we could put all of this in an ifdef? That
would lead to duplicate constructor in some classes but I think the result might
be more clear. 

What do you think?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:59: "autofill_provider.cc",
With the ifdef for android, these would only need to be added if the target is
Android. Another benefit to the ifdef

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:26: // This interface is
used by AutofillDriver to drive autofill.
This is not super clear to me, could you explain a bit more? Thanks!

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.h:16: class
AutofillHandlerProxy : public AutofillHandler {
This could be moved to an android location with the ifdef.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:123: void
OnFocusNoLongerOnForm() override;
Could you put all the overrides together with a comment telling from what class
they are from? Thanks!

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_provider.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_provider.h:5: #ifndef
COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_PROVIDER_H_
This should be in the android folder.

[michaelbai, 2017-05-24 16:45:28]

sebsg@ thanks for the comments, I have different thought about ifdef, though I
don't have strong feeling about it, still think we shouldn't add it. please see
my inline comments.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver_factory.h
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver_factory.h:78:
AutofillProvider* provider_;
On 2017/05/24 13:32:42, sebsg wrote:
> Since this is only used on Android, we could put all of this in an ifdef? That
> would lead to duplicate constructor in some classes but I think the result
might
> be more clear. 
> 
> What do you think?

I have different opinion for this.
- We should avoid os specific ifdef, shouldn't we? 
- The code actually become more complicated once we write two constructors
because they always need to share some code,  ContentAutofillDriver is good
example, we need to add InitAutofillManager() method for common code, the code
will become more complete from my aspect; further more, in the future, we would
have to test the patch in different platforms.
- AutofillProvider is almost an interface, it has little impact on binary size.
- I noticed we ifdef AutofillAssistant, it tells AutofillAssistant is only used
in Android and iOS, we could add comment in AutofillContentDriver to get same
effect.

[Mathieu, 2017-05-24 18:01:59]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org

[Mathieu, 2017-05-24 18:02:02]

Some comments, not done with the review, this is a big change!

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:45: 
// Remove the reference to this object on the Java side.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:58:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
From their sample code, is it typical to query Android Autofill APIs from the UI
thread? Will it cause jank?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:76:
ScopedJavaLocalRef<jobject> formObj = form_->GetJavaPeer();
hacker case: form_obj

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:100: if
(!ValidateHandler(handler))
This function would benefit from high-level comments, because there are quite a
few reasons for the early return

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:103: if
(!form_->SimilarFormAs(form))
can you combine those in a big if statement for the early return

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:168: for (auto field :
form.fields) {
const FormFieldData& ?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
Is this only used for WebView? if so, perhaps if should be
WebViewAutofillProvider

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
Can you add a class comment, detailing who owns this and what its lifetime is
attached to (one per tab, singleton, etc.)

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:32: 
nit: no need for extra lines in this "override" section

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:51: void
OnAutofillAvailable(JNIEnv* env, jobject jcaller, jobject formData);
hacker case everywhere: form_data

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:16: using
base::android::JavaRef;
not used, please clean up

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:54: for (auto& field : fields_)
let's be explicit in the type

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:103: }  // namespace autofil
autofill

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:16: // This class is native peer
of FormData.java, make autofill::FormData
*to make

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:25: // Pull autofill values from
Java side and return FormData.
"Pull" seems odd because your are simply returning data in |fields_|. How about
ToFormData?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:33: bool GetFieldIndex(const
FormFieldData& field, size_t* index);
Why do both GetFieldIndex and GetSimilarFieldIndex need to exist? You should
comment that somewhere so the future reader/user knows which to use

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:45: FormData form_;
should you subclass FormData instead?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:48: // index to popup fields to
Java.
this is unclear..

// Used to keep track of <blah> when doing <blah>

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_field_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.cc:44: env, jname, jlabel,
jvalue, jautocompleteAttr,
hacker_case everywhere

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.cc:76: }  // namespace
autofil
autofill

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_field_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.h:15: //
autofill::FormFieldData avaliable in Java.
available

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.h:18:
FormFieldDataAndroid(FormFieldData* field);
Can you mention the expected lifetime of |field| when used here. Naked pointers
are not so safe

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:110:
* Invoked when AutofillManager resets its cache.
can you mention in comments when this happens? The reader may not know about
AutofillManager

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java:13:
* The wrap class of native autofill::FormData.
*FormDataAndroid?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java:38:
FormFieldData formFieldData = nativeGetNextFormFieldData(mNativeObj);
should you check for !mNativeObj here?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java:11:
* The wrap class of native autofill::FormFieldData.
FormFieldDataAndroid?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java:38:
public String getValue() {
public methods should have javadoc

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:42: if (provider)
{
Please comment the behavior here. What is a provider, why is it making things
different

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.h:37:
ContentAutofillDriver(
Document what an AutofillProvider is

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:56: virtual void
OnFocusNoLongerOnForm() = 0;
Add comments to your virtual methods. This is the interface after all

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:243: // Override
AutofillManager
you mean override AutofillHandler

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_provider.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_provider.h:53: }
// namespace autofill

[Roger McFarlane (Chromium), 2017-05-24 20:03:31]

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:27: class AutofillHandler {
On 2017/05/24 05:14:09, michaelbai wrote:
> On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> > would the hierarchy be better named:
> > 
> >   AutofillManager
> >     ^  ^
> >     |  |
> >     |  +-- AutofillManagerProxy
> >     |
> >     +-- DefaultAutofillManager
> > 
> > I recall you wanter to avoid Impl in the class name.
> 
> I will rename those classes, please be noticed renaming AutofillManager will
> cause a lot of name changes both in components/autofill and chrome directory.

indeed. Maybe address any name changes in another CL that ONLY does name
changes.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.cc:21:
provider_->OnWillSubmitForm(this, form, timestamp);
On 2017/05/24 05:14:09, michaelbai wrote:
> On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> > should the provider return a bool?
> 
> this return value seemed no meaning for AutofillProvider, and also
> ContentAutofillDriver doesn't handle the return value. shall we just change
> return type to void?
> 
> What's meaning of return value of OnWillSubmitForm?

It's supposed to mean "I took a look at the submit and it is or isn't
interesting"... but as it's being ignored, and there's not other logic around
the call, maybe just change the signature to void.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.h:16: class
AutofillHandlerProxy : public AutofillHandler {
Why have AutofillHandlerProxy be a wrapper for Autofill Provider? As opposed to
having AutofillProvider directly implement AutofillHandler?

[michaelbai, 2017-05-24 20:43:24]

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:27: class AutofillHandler {
On 2017/05/24 20:03:30, Roger McFarlane (Chromium) wrote:
> On 2017/05/24 05:14:09, michaelbai wrote:
> > On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> > > would the hierarchy be better named:
> > > 
> > >   AutofillManager
> > >     ^  ^
> > >     |  |
> > >     |  +-- AutofillManagerProxy
> > >     |
> > >     +-- DefaultAutofillManager
> > > 
> > > I recall you wanter to avoid Impl in the class name.
> > 
> > I will rename those classes, please be noticed renaming AutofillManager will
> > cause a lot of name changes both in components/autofill and chrome
directory.
> 
> indeed. Maybe address any name changes in another CL that ONLY does name
> changes.

Thanks I will address name changes in following up patch.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.h:16: class
AutofillHandlerProxy : public AutofillHandler {
On 2017/05/24 20:03:31, Roger McFarlane (Chromium) wrote:
> Why have AutofillHandlerProxy be a wrapper for Autofill Provider? As opposed
to
> having AutofillProvider directly implement AutofillHandler?

It because that a WebContents has one AutofillProvider, but AutofillHandler
relates to frame.

[michaelbai, 2017-05-24 20:57:54]

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:58:
DCHECK_CURRENTLY_ON(BrowserThread::UI);
On 2017/05/24 18:02:01, Mathieu wrote:
> From their sample code, is it typical to query Android Autofill APIs from the
UI
> thread? Will it cause jank?

This isn't synchronous query and pretty safe to call in UI thread, also we are
not waiting for response by blocking UI thread.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:168: for (auto field :
form.fields) {
On 2017/05/24 18:02:01, Mathieu wrote:
> const FormFieldData& ?

Interesting, do we have any guideline about when auto should be used or not?

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
On 2017/05/24 18:02:01, Mathieu wrote:
> Is this only used for WebView? if so, perhaps if should be
> WebViewAutofillProvider

No, Chrome will also use this in future.

[Mathieu, 2017-05-25 18:18:37]

https://codereview.chromium.org/2839023003/diff/140001/android_webview/browse...
File android_webview/browser/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/android_webview/browse...
android_webview/browser/aw_contents.cc:314: // Just return, if the app neither
runs on O sdk nor enables autocomplete.
would make this a little more generic "if the Android Autofill provider is not
supported or autocomplete is not enabled".

[sebsg, 2017-05-25 20:57:36]

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver_factory.h
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver_factory.h:78:
AutofillProvider* provider_;
On 2017/05/24 16:45:28, michaelbai wrote:
> On 2017/05/24 13:32:42, sebsg wrote:
> > Since this is only used on Android, we could put all of this in an ifdef?
That
> > would lead to duplicate constructor in some classes but I think the result
> might
> > be more clear. 
> > 
> > What do you think?
> 
> I have different opinion for this.
> - We should avoid os specific ifdef, shouldn't we? 
> - The code actually become more complicated once we write two constructors
> because they always need to share some code,  ContentAutofillDriver is good
> example, we need to add InitAutofillManager() method for common code, the code
> will become more complete from my aspect; further more, in the future, we
would
> have to test the patch in different platforms.
> - AutofillProvider is almost an interface, it has little impact on binary
size.
> - I noticed we ifdef AutofillAssistant, it tells AutofillAssistant is only
used
> in Android and iOS, we could add comment in AutofillContentDriver to get same
> effect.
>   

sure, that's fair.

[michaelbai, 2017-05-26 23:12:08]

Thanks for review, I were a busy bug cop,  just got time to work on comments, I
should address all comments. PTAL!

https://codereview.chromium.org/2839023003/diff/140001/android_webview/browse...
File android_webview/browser/aw_contents.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/android_webview/browse...
android_webview/browser/aw_contents.cc:314: // Just return, if the app neither
runs on O sdk nor enables autocomplete.
On 2017/05/25 18:18:37, Mathieu wrote:
> would make this a little more generic "if the Android Autofill provider is not
> supported or autocomplete is not enabled".

We actually want to emphasize app should run on O sdk.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:45: 
On 2017/05/24 18:02:01, Mathieu wrote:
> // Remove the reference to this object on the Java side.

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:76:
ScopedJavaLocalRef<jobject> formObj = form_->GetJavaPeer();
On 2017/05/24 18:02:01, Mathieu wrote:
> hacker case: form_obj

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:100: if
(!ValidateHandler(handler))
On 2017/05/24 18:02:01, Mathieu wrote:
> This function would benefit from high-level comments, because there are quite
a
> few reasons for the early return

Could you suggest the comments? from my aspect, the code is clear, we don't need
to repeat what the code does.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:103: if
(!form_->SimilarFormAs(form))
On 2017/05/24 18:02:01, Mathieu wrote:
> can you combine those in a big if statement for the early return

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.cc:168: for (auto field :
form.fields) {
On 2017/05/24 20:57:54, michaelbai wrote:
> On 2017/05/24 18:02:01, Mathieu wrote:
> > const FormFieldData& ?
> 
> Interesting, do we have any guideline about when auto should be used or not?

OK, I found something about auto here
https://google.github.io/styleguide/cppguide.html#auto ,  from my reading, 
const auto& is encouraged in this case, you might thought the context isn't
clear, I changed to const FormFieldData

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
On 2017/05/24 18:02:01, Mathieu wrote:
> Can you add a class comment, detailing who owns this and what its lifetime is
> attached to (one per tab, singleton, etc.)

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:26: // Override
AutofillProvider methods
On 2017/05/24 13:32:42, sebsg wrote:
> The usual syntax is
> 
> // AutofillProvider:
> 
> 
> and then put all of those back to back without lines in between.
> 
> Thanks!

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:32: 
On 2017/05/24 18:02:01, Mathieu wrote:
> nit: no need for extra lines in this "override" section

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:51: void
OnAutofillAvailable(JNIEnv* env, jobject jcaller, jobject formData);
On 2017/05/24 18:02:01, Mathieu wrote:
> hacker case everywhere: form_data

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:16: using
base::android::JavaRef;
On 2017/05/24 18:02:01, Mathieu wrote:
> not used, please clean up

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:54: for (auto& field : fields_)
On 2017/05/24 18:02:01, Mathieu wrote:
> let's be explicit in the type

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.cc:103: }  // namespace autofil
On 2017/05/24 18:02:01, Mathieu wrote:
> autofill

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:16: // This class is native peer
of FormData.java, make autofill::FormData
On 2017/05/24 18:02:02, Mathieu wrote:
> *to make

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:17: // avaliable in Java.
On 2017/05/24 13:32:42, sebsg wrote:
> typo: available

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:25: // Pull autofill values from
Java side and return FormData.
On 2017/05/24 18:02:01, Mathieu wrote:
> "Pull" seems odd because your are simply returning data in |fields_|. How
about
> ToFormData?

It actually get data from Java side, changed to GetAutofillValues()

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:33: bool GetFieldIndex(const
FormFieldData& field, size_t* index);
On 2017/05/24 18:02:02, Mathieu wrote:
> Why do both GetFieldIndex and GetSimilarFieldIndex need to exist? You should
> comment that somewhere so the future reader/user knows which to use

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:45: FormData form_;
On 2017/05/24 18:02:02, Mathieu wrote:
> should you subclass FormData instead?

No, I don't think so, FormData is structure, but this is class, and this class
has more complicated logic than structure.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_data_android.h:48: // index to popup fields to
Java.
On 2017/05/24 18:02:02, Mathieu wrote:
> this is unclear..
> 
> // Used to keep track of <blah> when doing <blah>

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_field_data_android.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.cc:44: env, jname, jlabel,
jvalue, jautocompleteAttr,
On 2017/05/24 18:02:02, Mathieu wrote:
> hacker_case everywhere

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.cc:76: }  // namespace
autofil
On 2017/05/24 18:02:02, Mathieu wrote:
> autofill

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/form_field_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.h:15: //
autofill::FormFieldData avaliable in Java.
On 2017/05/24 18:02:02, Mathieu wrote:
> available

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/form_field_data_android.h:18:
FormFieldDataAndroid(FormFieldData* field);
On 2017/05/24 18:02:02, Mathieu wrote:
> Can you mention the expected lifetime of |field| when used here. Naked
pointers
> are not so safe

Done. see field_ptr_ declaration.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillProvider.java:110:
* Invoked when AutofillManager resets its cache.
On 2017/05/24 18:02:02, Mathieu wrote:
> can you mention in comments when this happens? The reader may not know about
> AutofillManager

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java:13:
* The wrap class of native autofill::FormData.
On 2017/05/24 18:02:02, Mathieu wrote:
> *FormDataAndroid?

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormData.java:38:
FormFieldData formFieldData = nativeGetNextFormFieldData(mNativeObj);
On 2017/05/24 18:02:02, Mathieu wrote:
> should you check for !mNativeObj here?

No, mNativeObj shouldn't be NULL in any case, if it is, we should crash
immediately.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java:11:
* The wrap class of native autofill::FormFieldData.
On 2017/05/24 18:02:02, Mathieu wrote:
> FormFieldDataAndroid?

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/java/src/org/chromium/components/autofill/FormFieldData.java:38:
public String getValue() {
On 2017/05/24 18:02:02, Mathieu wrote:
> public methods should have javadoc

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:42: if (provider)
{
On 2017/05/24 18:02:02, Mathieu wrote:
> Please comment the behavior here. What is a provider, why is it making things
> different

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver_factory.h
(right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver_factory.h:78:
AutofillProvider* provider_;
On 2017/05/25 20:57:36, sebsg wrote:
> On 2017/05/24 16:45:28, michaelbai wrote:
> > On 2017/05/24 13:32:42, sebsg wrote:
> > > Since this is only used on Android, we could put all of this in an ifdef?
> That
> > > would lead to duplicate constructor in some classes but I think the result
> > might
> > > be more clear. 
> > > 
> > > What do you think?
> > 
> > I have different opinion for this.
> > - We should avoid os specific ifdef, shouldn't we? 
> > - The code actually become more complicated once we write two constructors
> > because they always need to share some code,  ContentAutofillDriver is good
> > example, we need to add InitAutofillManager() method for common code, the
code
> > will become more complete from my aspect; further more, in the future, we
> would
> > have to test the patch in different platforms.
> > - AutofillProvider is almost an interface, it has little impact on binary
> size.
> > - I noticed we ifdef AutofillAssistant, it tells AutofillAssistant is only
> used
> > in Android and iOS, we could add comment in AutofillContentDriver to get
same
> > effect.
> >   
> 
> sure, that's fair.

Thanks

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/BUILD.gn (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/BUILD.gn:59: "autofill_provider.cc",
On 2017/05/24 13:32:42, sebsg wrote:
> With the ifdef for android, these would only need to be added if the target is
> Android. Another benefit to the ifdef

As we discussed, we will not use ifdef

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:26: // This interface is
used by AutofillDriver to drive autofill.
On 2017/05/24 13:32:42, sebsg wrote:
> This is not super clear to me, could you explain a bit more? Thanks!

Revised

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:56: virtual void
OnFocusNoLongerOnForm() = 0;
On 2017/05/24 18:02:02, Mathieu wrote:
> Add comments to your virtual methods. This is the interface after all

Acknowledged.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.cc (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.cc:21:
provider_->OnWillSubmitForm(this, form, timestamp);
On 2017/05/24 20:03:31, Roger McFarlane (Chromium) wrote:
> On 2017/05/24 05:14:09, michaelbai wrote:
> > On 2017/05/24 04:18:22, Roger McFarlane (Chromium) wrote:
> > > should the provider return a bool?
> > 
> > this return value seemed no meaning for AutofillProvider, and also
> > ContentAutofillDriver doesn't handle the return value. shall we just change
> > return type to void?
> > 
> > What's meaning of return value of OnWillSubmitForm?
> 
> It's supposed to mean "I took a look at the submit and it is or isn't
> interesting"... but as it's being ignored, and there's not other logic around
> the call, maybe just change the signature to void.
> 

I tried to change it  to void, but it seemed that unittest intentionally test
this case in both AutofillManagerTest.FormSubmittedAutocompleteEnabled and
AutofillMetricsTest.StoredProfileCountNonAutofillableFormSubmission, it is
important feature of autofill,  I think it is not proper to just drop it in this
patch. changed to bool

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_handler_proxy.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_handler_proxy.h:16: class
AutofillHandlerProxy : public AutofillHandler {
On 2017/05/24 13:32:42, sebsg wrote:
> This could be moved to an android location with the ifdef.

As we discussed, we will not use ifdef.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:123: void
OnFocusNoLongerOnForm() override;
On 2017/05/24 13:32:42, sebsg wrote:
> Could you put all the overrides together with a comment telling from what
class
> they are from? Thanks!

I intended to avoid unnecessary changes, since all these methods have been
changed, I moved them together, thanks.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_manager.h:243: // Override
AutofillManager
On 2017/05/24 18:02:02, Mathieu wrote:
> you mean override AutofillHandler

Done.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
File components/autofill/core/browser/autofill_provider.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/co...
components/autofill/core/browser/autofill_provider.h:5: #ifndef
COMPONENTS_AUTOFILL_CORE_BROWSER_AUTOFILL_PROVIDER_H_
On 2017/05/24 13:32:42, sebsg wrote:
> This should be in the android folder.

As we discussed, we will not use ifdef

[michaelbai, 2017-05-26 23:12:19]

The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 23:12:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 00:46:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 00:46:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sebsg, 2017-05-29 15:36:23]

Small question, thanks!

https://codereview.chromium.org/2839023003/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_provider.h (right):

https://codereview.chromium.org/2839023003/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_provider.h:19: // This class is native
peer of AutofillProvider, see AutofillProvider.java
Will this only be used on Android?

[michaelbai, 2017-05-30 16:02:28]

https://codereview.chromium.org/2839023003/diff/160001/components/autofill/co...
File components/autofill/core/browser/autofill_provider.h (right):

https://codereview.chromium.org/2839023003/diff/160001/components/autofill/co...
components/autofill/core/browser/autofill_provider.h:19: // This class is native
peer of AutofillProvider, see AutofillProvider.java
On 2017/05/29 15:36:23, sebsg wrote:
> Will this only be used on Android?

This comment should be moved to AutofillProviderAndroid, but AutofillProvider is
compiled to all platform, and will have instance in Android only.

[michaelbai, 2017-05-30 18:46:46]

Update the comment of AutofillProvider, PTAL

[Roger McFarlane (Chromium), 2017-05-31 15:08:32]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 15:08:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelbai, 2017-05-31 15:25:45]

Ping

[commit-bot: I haz the power, 2017-05-31 16:42:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 16:42:32]

Dry run: This issue passed the CQ dry run.

[Roger McFarlane (Chromium), 2017-05-31 18:33:24]

A few nits and a couple of longer term details to flesh out, but this mostly
LGTM.

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
On 2017/05/24 20:57:54, michaelbai wrote:
> On 2017/05/24 18:02:01, Mathieu wrote:
> > Is this only used for WebView? if so, perhaps if should be
> > WebViewAutofillProvider
> 
> No, Chrome will also use this in future.

More details, plz. In what capacity?

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/an...
File components/autofill/android/form_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/an...
components/autofill/android/form_data_android.h:45: void
OnTextFieldDidChange(size_t index, const base::string16& value);
nit: comment

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.h:133:
AutofillManager* autofill_manager_;
add a TODO to unify these (autofill_handler_ and autofill_manager_) to a single
pointer to a common root.

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:40: const base::TimeTicks&
timestamp);
nit: prefer to pass base::TimeTicks by value throughout.

See:
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=66cc96f0414e2c1b91d...

[michaelbai, 2017-05-31 21:05:55]

Thanks Roger, sebsg@ & mathieu PTAL

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
File components/autofill/android/autofill_provider_android.h (right):

https://codereview.chromium.org/2839023003/diff/140001/components/autofill/an...
components/autofill/android/autofill_provider_android.h:20: class
AutofillProviderAndroid : public AutofillProvider {
On 2017/05/31 18:33:23, Roger McFarlane (Chromium) wrote:
> On 2017/05/24 20:57:54, michaelbai wrote:
> > On 2017/05/24 18:02:01, Mathieu wrote:
> > > Is this only used for WebView? if so, perhaps if should be
> > > WebViewAutofillProvider
> > 
> > No, Chrome will also use this in future.
> 
> More details, plz. In what capacity?

I don't have detail to put, but will definitely involve your guys once we start
to work on it. If it turns out WebView only, I will move these files to
android_webview directory.

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/an...
File components/autofill/android/form_data_android.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/an...
components/autofill/android/form_data_android.h:45: void
OnTextFieldDidChange(size_t index, const base::string16& value);
On 2017/05/31 18:33:23, Roger McFarlane (Chromium) wrote:
> nit: comment

Done.

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.h:133:
AutofillManager* autofill_manager_;
On 2017/05/31 18:33:23, Roger McFarlane (Chromium) wrote:
> add a TODO to unify these (autofill_handler_ and autofill_manager_) to a
single
> pointer to a common root.

Done.

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
File components/autofill/core/browser/autofill_handler.h (right):

https://codereview.chromium.org/2839023003/diff/180001/components/autofill/co...
components/autofill/core/browser/autofill_handler.h:40: const base::TimeTicks&
timestamp);
On 2017/05/31 18:33:23, Roger McFarlane (Chromium) wrote:
> nit: prefer to pass base::TimeTicks by value throughout.
> 
> See:
>
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=66cc96f0414e2c1b91d...

Done.

[sebsg, 2017-05-31 21:07:48]

LGTM too

[michaelbai, 2017-06-01 15:21:32]

Thanks sebsg@!

mathp@ Would you like review the patch again? I want to get this landed this
week if possible, Thanks.

[Mathieu, 2017-06-01 16:13:02]

lgtm

[michaelbai, 2017-06-01 18:01:47]

Thanks all for your time to review this.

[michaelbai, 2017-06-01 18:02:02]

The CQ bit was checked by michaelbai@chromium.org

[michaelbai, 2017-06-01 18:02:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
avi@chromium.org, rogerm@chromium.org, mathp@chromium.org, sebsg@chromium.org
Link to the patchset: https://codereview.chromium.org/2839023003/#ps220001
(title: "sync")

[commit-bot: I haz the power, 2017-06-01 18:02:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 19:37:00]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496340122703520,
"parent_rev": "ff118bbd9f5f5781d02af29ff83eccdc5a5d1c0d", "commit_rev":
"927a09ab04f76ed68c8a1940992381fe8cc33def"}

[commit-bot: I haz the power, 2017-06-01 19:37:11]

Description was changed from

==========
WebView autofill implementation

WebView autofill implementation doesn't use chrome's autofill service
and UI, instead, it use autofill service in Android platform to get
form autofilled.

BUG=693204
==========

to

==========
WebView autofill implementation

WebView autofill implementation doesn't use chrome's autofill service
and UI, instead, it use autofill service in Android platform to get
form autofilled.

BUG=693204

Review-Url: https://codereview.chromium.org/2839023003
Cr-Commit-Position: refs/heads/master@{#476389}
Committed:
https://chromium.googlesource.com/chromium/src/+/927a09ab04f76ed68c8a19409923...
==========

[commit-bot: I haz the power, 2017-06-01 19:37:12]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/927a09ab04f76ed68c8a19409923...