Implement the autofill UI for chromium powered android webview.

Implement the autofill UI for chromium powered android webview.

This CL depends on https://codereview.chromium.org/15097004/.

Bug: b/5013406

TBR=battre@chromium.org, palmer@chromium.org
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208986

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209182

[sgurun-gerrit only, 2013-05-31 23:47:45]

PTAL, thanks

[Ilya Sherman, 2013-06-03 23:41:22]

What parts of this CL did you want me to look at?

[joth, 2013-06-04 01:14:46]

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:90: const std::string&)>&
callback) {
feels like these empty methods should have a NOTIMPLEMENTED() in them at least.

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:114:
aw_contents->ShowAutofillPopup(element_bounds_in_screen_space,
If moving AwAutofillManagerDelegate into aw/native, you might as well have this
class have its own Java peer object and do its own JNI. This helps avoid
pressure on AwContents turning into a God class.

If there's another reason that it must do JNI via AwContents, then the preferred
pattern is to leave this class in aw/browser and have it define an abstraction
it talks to AwContents via --
as AwContents calls AwAutofillManagerDelegate::CreateForWebContents() it could
totally pass itself in as, er, a AwAutofillManagerDelegateDelegate or something.
(yeah... we have a delegate with a delegate. This is  a good sign this class
should just be doing its own JNI)

For object lifetime, AwContents.java can hold a strong ref to
AwAutofillManagerDelegate.java so the native side can just hold a weak ref to
it.

[benm (inactive), 2013-06-04 13:35:18]

https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1264: *
@see android.webkit.WebView#clearFormData()
might be worth a note to say that this is poorly named for legacy reasons, the
intent is to simply hide the popup

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:124:
aw_contents->HideAutofillPopup();
reset delegate_?

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:131:
delegate_->DidAcceptSuggestion(values_[position], identifiers_[position]);
check delegate_ is set and still valid (as it's weak)?

is it possible for this to race, i.e. get a second call to ShowAutofillPopup so
that new values_ are set whilst a call to SuggestionSelected is still in flight?

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:182:
Java_AwContents_createAutofillSuggestionArray(env, count);
Do we really need a JNI hop for this?

env->NewObjectArray(count, GetClass(...), NULL);

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_contents.cc:190:
Java_AwContents_addToAutofillSuggestionArray(
ditto, can we just populate the array using native calls? Let's not bloat up the
java class even more with trivial methods like this.

[sgurun-gerrit only, 2013-06-04 15:30:28]

On 2013/06/03 23:41:22, Ilya Sherman wrote:
> What parts of this CL did you want me to look at?

@isherman:
sorry for not being clear. This was a FYI as there is a refactoring effort going
on right now in components/autofill. (especially for the
aw_autofill_manager_delegate and aw_autofill_external_delegate). Thanks!

[joth, 2013-06-04 19:09:05]

On 4 June 2013 06:35, <benm@chromium.org> wrote:

>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/java/src/org/**chromium/android_webview/**AwContents.java<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java>
> File
> android_webview/java/src/org/**chromium/android_webview/**AwContents.java
> (right):
>
> https://codereview.chromium.**org/16212007/diff/4001/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwContents.java#newcode1264<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode1264>
> android_webview/java/src/org/**chromium/android_webview/**
> AwContents.java:1264:
> * @see android.webkit.WebView#**clearFormData()
> might be worth a note to say that this is poorly named for legacy
> reasons, the intent is to simply hide the popup
>
>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**autofill_manager_delegate.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc>
> File android_webview/native/aw_**autofill_manager_delegate.cc (right):
>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode124<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode124>
> android_webview/native/aw_**autofill_manager_delegate.cc:**124:
> aw_contents->**HideAutofillPopup();
> reset delegate_?
>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode131<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode131>
> android_webview/native/aw_**autofill_manager_delegate.cc:**131:
> delegate_->**DidAcceptSuggestion(values_[**position],
> identifiers_[position]);
> check delegate_ is set and still valid (as it's weak)?
>
> is it possible for this to race, i.e. get a second call to
> ShowAutofillPopup so that new values_ are set whilst a call to
> SuggestionSelected is still in flight?
>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**contents.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc>
> File android_webview/native/aw_**contents.cc (right):
>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**contents.cc#newcode182<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode182>
> android_webview/native/aw_**contents.cc:182:
> Java_AwContents_**createAutofillSuggestionArray(**env, count);
> Do we really need a JNI hop for this?
>
> env->NewObjectArray(count, GetClass(...), NULL);
>


The JNI Generator docs suggest never calling GetClass or call constructor
methods, but to wrap in a factory method.



>
> https://codereview.chromium.**org/16212007/diff/4001/**
>
android_webview/native/aw_**contents.cc#newcode190<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode190>
> android_webview/native/aw_**contents.cc:190:
> Java_AwContents_**addToAutofillSuggestionArray(
> ditto, can we just populate the array using native calls? Let's not
> bloat up the java class even more with trivial methods like this.
>
>
https://codereview.chromium.**org/16212007/<https://codereview.chromium.org/1...
>

[benm (inactive), 2013-06-05 11:04:19]

Fair enough, never seen those docs. Out of interest, where are they? I'd
like to see why they recommend against this.

FWIW, the array helper methods inside jni_array.cc do this, but then as
they are using simple types maybe it's OK.


On 4 June 2013 20:08, Jonathan Dixon <joth@chromium.org> wrote:

>
>
>
> On 4 June 2013 06:35, <benm@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/java/src/org/**chromium/android_webview/**AwContents.java<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java>
>> File
>> android_webview/java/src/org/**chromium/android_webview/**AwContents.java
>> (right):
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>> android_webview/java/src/org/**chromium/android_webview/**
>>
AwContents.java#newcode1264<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode1264>
>> android_webview/java/src/org/**chromium/android_webview/**
>> AwContents.java:1264:
>> * @see android.webkit.WebView#**clearFormData()
>> might be worth a note to say that this is poorly named for legacy
>> reasons, the intent is to simply hide the popup
>>
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**autofill_manager_delegate.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc>
>> File android_webview/native/aw_**autofill_manager_delegate.cc (right):
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode124<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode124>
>> android_webview/native/aw_**autofill_manager_delegate.cc:**124:
>> aw_contents->**HideAutofillPopup();
>> reset delegate_?
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode131<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode131>
>> android_webview/native/aw_**autofill_manager_delegate.cc:**131:
>> delegate_->**DidAcceptSuggestion(values_[**position],
>> identifiers_[position]);
>> check delegate_ is set and still valid (as it's weak)?
>>
>> is it possible for this to race, i.e. get a second call to
>> ShowAutofillPopup so that new values_ are set whilst a call to
>> SuggestionSelected is still in flight?
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**contents.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc>
>> File android_webview/native/aw_**contents.cc (right):
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**contents.cc#newcode182<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode182>
>> android_webview/native/aw_**contents.cc:182:
>> Java_AwContents_**createAutofillSuggestionArray(**env, count);
>> Do we really need a JNI hop for this?
>>
>> env->NewObjectArray(count, GetClass(...), NULL);
>>
>
>
> The JNI Generator docs suggest never calling GetClass or call constructor
> methods, but to wrap in a factory method.
>
>
>
>>
>> https://codereview.chromium.**org/16212007/diff/4001/**
>>
android_webview/native/aw_**contents.cc#newcode190<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode190>
>> android_webview/native/aw_**contents.cc:190:
>> Java_AwContents_**addToAutofillSuggestionArray(
>> ditto, can we just populate the array using native calls? Let's not
>> bloat up the java class even more with trivial methods like this.
>>
>>
https://codereview.chromium.**org/16212007/<https://codereview.chromium.org/1...
>>
>
>

[joth, 2013-06-05 19:59:26]

yeah maybe this applies more to calling "<init>" on a class than
constructing instances of built-ins like object arrays. Chat with Marcus :)


http://src.chromium.org/viewvc/chrome/trunk/src/base/android/jni_generator/ja...

69 // - Use static factory functions annotated with @CalledByNative rather
than calling the 70// constructors directly.


On 5 June 2013 04:04, Ben Murdoch <benm@chromium.org> wrote:

> Fair enough, never seen those docs. Out of interest, where are they? I'd
> like to see why they recommend against this.
>
> FWIW, the array helper methods inside jni_array.cc do this, but then as
> they are using simple types maybe it's OK.
>
>
> On 4 June 2013 20:08, Jonathan Dixon <joth@chromium.org> wrote:
>
>>
>>
>>
>> On 4 June 2013 06:35, <benm@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>> android_webview/java/src/org/**chromium/android_webview/**
>>>
AwContents.java<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java>
>>> File
>>> android_webview/java/src/org/**chromium/android_webview/**
>>> AwContents.java
>>> (right):
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>> android_webview/java/src/org/**chromium/android_webview/**
>>>
AwContents.java#newcode1264<https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/org/chromium/android_webview/AwContents.java#newcode1264>
>>> android_webview/java/src/org/**chromium/android_webview/**
>>> AwContents.java:1264:
>>> * @see android.webkit.WebView#**clearFormData()
>>> might be worth a note to say that this is poorly named for legacy
>>> reasons, the intent is to simply hide the popup
>>>
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**autofill_manager_delegate.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc>
>>> File android_webview/native/aw_**autofill_manager_delegate.cc (right):
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode124<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode124>
>>> android_webview/native/aw_**autofill_manager_delegate.cc:**124:
>>> aw_contents->**HideAutofillPopup();
>>> reset delegate_?
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**autofill_manager_delegate.cc#**newcode131<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_autofill_manager_delegate.cc#newcode131>
>>> android_webview/native/aw_**autofill_manager_delegate.cc:**131:
>>> delegate_->**DidAcceptSuggestion(values_[**position],
>>> identifiers_[position]);
>>> check delegate_ is set and still valid (as it's weak)?
>>>
>>> is it possible for this to race, i.e. get a second call to
>>> ShowAutofillPopup so that new values_ are set whilst a call to
>>> SuggestionSelected is still in flight?
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**contents.cc<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc>
>>> File android_webview/native/aw_**contents.cc (right):
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**contents.cc#newcode182<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode182>
>>> android_webview/native/aw_**contents.cc:182:
>>> Java_AwContents_**createAutofillSuggestionArray(**env, count);
>>> Do we really need a JNI hop for this?
>>>
>>> env->NewObjectArray(count, GetClass(...), NULL);
>>>
>>
>>
>> The JNI Generator docs suggest never calling GetClass or call constructor
>> methods, but to wrap in a factory method.
>>
>>
>>
>>>
>>> https://codereview.chromium.**org/16212007/diff/4001/**
>>>
android_webview/native/aw_**contents.cc#newcode190<https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_contents.cc#newcode190>
>>> android_webview/native/aw_**contents.cc:190:
>>> Java_AwContents_**addToAutofillSuggestionArray(
>>> ditto, can we just populate the array using native calls? Let's not
>>> bloat up the java class even more with trivial methods like this.
>>>
>>>
https://codereview.chromium.**org/16212007/<https://codereview.chromium.org/1...
>>>
>>
>>
>

[benm (inactive), 2013-06-06 10:54:56]

On 2013/06/05 19:59:26, joth wrote:
> yeah maybe this applies more to calling "<init>" on a class than
> constructing instances of built-ins like object arrays. Chat with Marcus :)
> 
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/base/android/jni_generator/ja...
> 
> 69 // - Use static factory functions annotated with @CalledByNative rather
> than calling the 70// constructors directly.
> 

The advice is to use helper methods like you've done for non-POD types, so
please disregard my last comments :)

Could we move them out of AwContents though as they are just static methods to
avoid polluting it's namespace? Maybe create AwAutofillManagerDelegate.java to
hold them?

[sgurun-gerrit only, 2013-06-15 01:16:51]

joth/ben, PTAL, thanks.

https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/o...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/java/src/o...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1264: *
@see android.webkit.WebView#clearFormData()
On 2013/06/04 13:35:18, benm wrote:
> might be worth a note to say that this is poorly named for legacy reasons, the
> intent is to simply hide the popup

Done.

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:90: const std::string&)>&
callback) {
I am not sure if all the methods qualify for a NOTIMPLEMENTED (ben has raised
this issue before, in a prev. CL). Some of them should simply stay as nop I
think. We can search the code and try to identify the "NOTIMPLEMENTED" ones if
you insist.

On 2013/06/04 01:14:46, joth wrote:
> feels like these empty methods should have a NOTIMPLEMENTED() in them at
least.

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:114:
aw_contents->ShowAutofillPopup(element_bounds_in_screen_space,
On 2013/06/04 01:14:46, joth wrote:
> If moving AwAutofillManagerDelegate into aw/native, you might as well have
this
> class have its own Java peer object and do its own JNI. This helps avoid
> pressure on AwContents turning into a God class.
> 
> If there's another reason that it must do JNI via AwContents, then the
preferred
> pattern is to leave this class in aw/browser and have it define an abstraction
> it talks to AwContents via --
> as AwContents calls AwAutofillManagerDelegate::CreateForWebContents() it could
> totally pass itself in as, er, a AwAutofillManagerDelegateDelegate or
something.
> (yeah... we have a delegate with a delegate. This is  a good sign this class
> should just be doing its own JNI)
> 
> For object lifetime, AwContents.java can hold a strong ref to
> AwAutofillManagerDelegate.java so the native side can just hold a weak ref to
> it.

Done. I have created the peer from the native side and routed through AwContents
as we spoke.

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:124:
aw_contents->HideAutofillPopup();
On 2013/06/04 13:35:18, benm wrote:
> reset delegate_?

Done.

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:131:
delegate_->DidAcceptSuggestion(values_[position], identifiers_[position]);
On 2013/06/04 13:35:18, benm wrote:
> check delegate_ is set and still valid (as it's weak)?
> 

done

> is it possible for this to race, i.e. get a second call to ShowAutofillPopup
so
> that new values_ are set whilst a call to SuggestionSelected is still in
flight?
showautofillpopup is called from the ui delegate, i.e. the external_delegate. It
hides the popup when visibility changes, or there are no suggestion values, etc.
I think in this case we will get hideautofillpopup first and then we will reset
the delegate. But I will think on it a little more.

[benm (inactive), 2013-06-17 09:48:51]

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:90: const std::string&)>&
callback) {
I think I may have talked about adding NOTREACHED() to these methods before,
NOTIMPLEMENTED()'s more assertive cousin.

NOTIMPLEMETED will just log at runtime, so we should notice if something is
called when we might want it to be. Think it makes sense for these methods.

[joth, 2013-06-17 17:00:54]

lgtm for JNI & object ownership changes since my last review.
Over to ben for autofill correctness review :)

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/4001/android_webview/native/aw_...
android_webview/native/aw_autofill_manager_delegate.cc:90: const std::string&)>&
callback) {
On 2013/06/17 09:48:52, benm wrote:
> I think I may have talked about adding NOTREACHED() to these methods before,
> NOTIMPLEMENTED()'s more assertive cousin.
> 
> NOTIMPLEMETED will just log at runtime, so we should notice if something is
> called when we might want it to be. Think it makes sense for these methods.

0/ empty method with a comment makes sense for something we expect to be called
and we want to be a no-op.
1/ NOTREACHED makes sense we we positively think a method must never be called.
2/ NOTIMPLEMETED is really for something we intend to implement but haven't
gotten around to, but also serves as a middle ground between #0 and #1 when
we're not really sure what the situation is

[benm (inactive), 2013-06-17 17:23:17]

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1252:
public void clearFormData() {
Going back on my previous comment: perhaps we can name this method properly and
wire it up the poorly named one in the glue layer.

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1386:
mAwAutofillManagerDelegate  = delegate;
nit extra space before =

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1502: 
remove?

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.cc:50: return;
this seems a bit dodgy... we'll return a half constructed object at this point.
DCHECK better?

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.cc:125: values_ = values;
I think it'd be clearer to move the methods we implement to the top, and then
the ones we don't under a 
// NOT IMPLEMENTED header or something like that.

https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:109: virtual bool
ShouldSaveFormData(content::WebContents* web_contents);
I don't see caller in this patch?

[sgurun-gerrit only, 2013-06-25 02:44:31]

ben, PTAL, thanks.

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1252:
public void clearFormData() {
On 2013/06/17 17:23:17, benm wrote:
> Going back on my previous comment: perhaps we can name this method properly
and
> wire it up the poorly named one in the glue layer.

Done.

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1386:
mAwAutofillManagerDelegate  = delegate;
On 2013/06/17 17:23:17, benm wrote:
> nit extra space before =

Done.

https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1502: 
On 2013/06/17 17:23:17, benm wrote:
> remove?

Done.

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
File android_webview/native/aw_autofill_manager_delegate.cc (right):

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.cc:50: return;
On 2013/06/17 17:23:17, benm wrote:
> this seems a bit dodgy... we'll return a half constructed object at this
point.
> DCHECK better?

If it is null it will crash anyway. Maybe do no checks at all?

https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.cc:125: values_ = values;
On 2013/06/17 17:23:17, benm wrote:
> I think it'd be clearer to move the methods we implement to the top, and then
> the ones we don't under a 
> // NOT IMPLEMENTED header or something like that.

Done.

https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
File components/autofill/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
components/autofill/browser/autofill_external_delegate.h:109: virtual bool
ShouldSaveFormData(content::WebContents* web_contents);
On 2013/06/17 17:23:17, benm wrote:
> I don't see caller in this patch?

Done.

[sgurun-gerrit only, 2013-06-25 15:31:36]

On 2013/06/25 02:44:31, sgurun wrote:
> ben, PTAL, thanks.
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:1252:
> public void clearFormData() {
> On 2013/06/17 17:23:17, benm wrote:
> > Going back on my previous comment: perhaps we can name this method properly
> and
> > wire it up the poorly named one in the glue layer.
> 
> Done.
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:1386:
> mAwAutofillManagerDelegate  = delegate;
> On 2013/06/17 17:23:17, benm wrote:
> > nit extra space before =
> 
> Done.
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:1502: 
> On 2013/06/17 17:23:17, benm wrote:
> > remove?
> 
> Done.
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> File android_webview/native/aw_autofill_manager_delegate.cc (right):
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> android_webview/native/aw_autofill_manager_delegate.cc:50: return;
> On 2013/06/17 17:23:17, benm wrote:
> > this seems a bit dodgy... we'll return a half constructed object at this
> point.
> > DCHECK better?
> 
> If it is null it will crash anyway. Maybe do no checks at all?
> 
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> android_webview/native/aw_autofill_manager_delegate.cc:125: values_ = values;
> On 2013/06/17 17:23:17, benm wrote:
> > I think it'd be clearer to move the methods we implement to the top, and
then
> > the ones we don't under a 
> > // NOT IMPLEMENTED header or something like that.
> 
> Done.
> 
>
https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
> File components/autofill/browser/autofill_external_delegate.h (right):
> 
>
https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
> components/autofill/browser/autofill_external_delegate.h:109: virtual bool
> ShouldSaveFormData(content::WebContents* web_contents);
> On 2013/06/17 17:23:17, benm wrote:
> > I don't see caller in this patch?
> 
> Done.

Peh, this has merge/rebase errors. will fix.

[sgurun-gerrit only, 2013-06-25 16:29:43]

On 2013/06/25 15:31:36, sgurun wrote:
> On 2013/06/25 02:44:31, sgurun wrote:
> > ben, PTAL, thanks.
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> > File android_webview/java/src/org/chromium/android_webview/AwContents.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:1252:
> > public void clearFormData() {
> > On 2013/06/17 17:23:17, benm wrote:
> > > Going back on my previous comment: perhaps we can name this method
properly
> > and
> > > wire it up the poorly named one in the glue layer.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:1386:
> > mAwAutofillManagerDelegate  = delegate;
> > On 2013/06/17 17:23:17, benm wrote:
> > > nit extra space before =
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/java/src/...
> > android_webview/java/src/org/chromium/android_webview/AwContents.java:1502: 
> > On 2013/06/17 17:23:17, benm wrote:
> > > remove?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> > File android_webview/native/aw_autofill_manager_delegate.cc (right):
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> > android_webview/native/aw_autofill_manager_delegate.cc:50: return;
> > On 2013/06/17 17:23:17, benm wrote:
> > > this seems a bit dodgy... we'll return a half constructed object at this
> > point.
> > > DCHECK better?
> > 
> > If it is null it will crash anyway. Maybe do no checks at all?
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/android_webview/native/aw...
> > android_webview/native/aw_autofill_manager_delegate.cc:125: values_ =
values;
> > On 2013/06/17 17:23:17, benm wrote:
> > > I think it'd be clearer to move the methods we implement to the top, and
> then
> > > the ones we don't under a 
> > > // NOT IMPLEMENTED header or something like that.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
> > File components/autofill/browser/autofill_external_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/16212007/diff/22001/components/autofill/brows...
> > components/autofill/browser/autofill_external_delegate.h:109: virtual bool
> > ShouldSaveFormData(content::WebContents* web_contents);
> > On 2013/06/17 17:23:17, benm wrote:
> > > I don't see caller in this patch?
> > 
> > Done.
> 
> Peh, this has merge/rebase errors. will fix.

Fixed. Ben PTAL, thanks.

[sgurun-gerrit only, 2013-06-25 16:32:09]

@bulach, please review ui/android and chrome/android. Thanks,

[benm (inactive), 2013-06-25 17:19:07]

lgtm

[bulach, 2013-06-26 17:35:18]

lgtm, thanks!

https://codereview.chromium.org/16212007/diff/46001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
(right):

https://codereview.chromium.org/16212007/diff/46001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java:24:
private int mNativeAwAutofillManagerDelegate;
nit: final ?

https://codereview.chromium.org/16212007/diff/46001/android_webview/native/aw...
File android_webview/native/aw_autofill_manager_delegate.h (right):

https://codereview.chromium.org/16212007/diff/46001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.h:127: bool
RegisterAwAutofillManagerDelegate(JNIEnv* env);
nit: in some places we're using as static "Register" in the class rather than as
a global.

[sgurun-gerrit only, 2013-06-26 23:48:46]

@battre need LGTM as I am adding components/user_prefs to android webview DEPS

@palmer need LGTM as I am adding components/autofill/core/common to android
webview DEPS

https://codereview.chromium.org/16212007/diff/46001/android_webview/java/src/...
File
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java
(right):

https://codereview.chromium.org/16212007/diff/46001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwAutofillManagerDelegate.java:24:
private int mNativeAwAutofillManagerDelegate;
On 2013/06/26 17:35:19, bulach wrote:
> nit: final ?

Done.

https://codereview.chromium.org/16212007/diff/46001/android_webview/native/aw...
File android_webview/native/aw_autofill_manager_delegate.h (right):

https://codereview.chromium.org/16212007/diff/46001/android_webview/native/aw...
android_webview/native/aw_autofill_manager_delegate.h:127: bool
RegisterAwAutofillManagerDelegate(JNIEnv* env);

On 2013/06/26 17:35:19, bulach wrote:
> nit: in some places we're using as static "Register" in the class rather than
as
> a global.

I guess in webview, we generally use it this way (see AwContents, and
AwSettings. maybe I can address these in a further CL.

[commit-bot: I haz the power, 2013-06-27 15:48:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/16212007/70001

[commit-bot: I haz the power, 2013-06-27 15:48:05]

Can't process patch for file
ui/android/java/res/drawable-hdpi/autofill_popup_background_down.9.png.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[battre, 2013-06-27 16:25:57]

LGTM @ adding components/user_prefs to android webview DEPS

[palmer, 2013-06-27 19:22:07]

lgtm

[commit-bot: I haz the power, 2013-06-27 20:14:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/16212007/79001

[commit-bot: I haz the power, 2013-06-27 20:19:28]

Change committed as 208986

[commit-bot: I haz the power, 2013-06-27 23:37:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/16212007/83001

[commit-bot: I haz the power, 2013-06-28 00:33:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/16212007/83001

[commit-bot: I haz the power, 2013-06-28 17:48:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/16212007/83001

[commit-bot: I haz the power, 2013-06-28 17:56:58]

Change committed as 209182