[rAc Android dialog] Stubs for Autofill dialog integration.

[rAc Android dialog] Stubs for Autofill dialog integration.

This includes JNI bindings, Java interfaces and necessary glue for the requestAutocomplete dialog.

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

[aruslan, 2013-08-09 21:54:58]

Hi Ted, PTAL, thanks!

[Ted C, 2013-08-10 01:33:35]

lgtm -- I would look at trimming some of the fat of the data object class, but
looks good overall

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java
(right):

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:11:

i'd drop one of these blank lines

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:18:
private static final String TAG = "AutofillDialogControllerAndroid";
not used

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:41:
* @param guidLastUsedCard GUI of the last selected Autofill credit card, or null
missing D at the end of GUI?

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:44:
AutofillDialogResult.ResultWallet fullWallet,
+4 indent

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:50:
* An interface to the dialog.
I would say something "Exposes the necessary functionality for an Autofill
dialog".

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:69:
* @param delegate Continuations for the dialog
I would put a blank line above this to split it up a bit.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:74:
* @param incognitoMode True if the dialog starts from the incognito tab
started from an

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:84:
final AutofillDialogDelegate delegate,
+4 indent

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:121:
new AutofillDialogDelegate() {
extract this as a param for readability

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java
(right):

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:10:

remove one here too

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:21:
private final int mExpirationMonth;
if you want, as long as these are all final, you can make them public and drop
all the getters.  The names should all change as well to drop the leading "m"
and make it camel cased

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:234:
private static String getWalletEmail(Object fullWallet) {
I think these can be ResultWallet instead Object, worth checking and dropping
all the casts.

Maybe look at making the accessors above CalledByNative and drop the static
functions entirely.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:235:
return((ResultWallet) fullWallet).getEmail();
space after return

https://codereview.chromium.org/22566004/diff/10001/chrome/browser/ui/android...
File chrome/browser/ui/android/autofill/autofill_dialog_result.cc (right):

https://codereview.chromium.org/22566004/diff/10001/chrome/browser/ui/android...
chrome/browser/ui/android/autofill/autofill_dialog_result.cc:37: #define
FETCH_JSTRING(utf, env, jobj, getter) \
I don't see a lot of macros in Chromium code, but maybe I don't look hard
enough.  That's all I have to say about that.

[aruslan, 2013-08-10 03:00:07]

Thanks, Ted!

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java
(right):

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:11:

On 2013/08/10 01:33:35, Ted C wrote:
> i'd drop one of these blank lines

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:18:
private static final String TAG = "AutofillDialogControllerAndroid";
On 2013/08/10 01:33:35, Ted C wrote:
> not used

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:41:
* @param guidLastUsedCard GUI of the last selected Autofill credit card, or null
On 2013/08/10 01:33:35, Ted C wrote:
> missing D at the end of GUI?

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:44:
AutofillDialogResult.ResultWallet fullWallet,
On 2013/08/10 01:33:35, Ted C wrote:
> +4 indent

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:50:
* An interface to the dialog.
On 2013/08/10 01:33:35, Ted C wrote:
> I would say something "Exposes the necessary functionality for an Autofill
> dialog".

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:69:
* @param delegate Continuations for the dialog
On 2013/08/10 01:33:35, Ted C wrote:
> I would put a blank line above this to split it up a bit.

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:74:
* @param incognitoMode True if the dialog starts from the incognito tab
On 2013/08/10 01:33:35, Ted C wrote:
> started from an

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:84:
final AutofillDialogDelegate delegate,
On 2013/08/10 01:33:35, Ted C wrote:
> +4 indent

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogControllerAndroid.java:121:
new AutofillDialogDelegate() {
On 2013/08/10 01:33:35, Ted C wrote:
> extract this as a param for readability

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java
(right):

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:10:

On 2013/08/10 01:33:35, Ted C wrote:
> remove one here too

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:21:
private final int mExpirationMonth;
On 2013/08/10 01:33:35, Ted C wrote:
> if you want, as long as these are all final, you can make them public and drop
> all the getters.  The names should all change as well to drop the leading "m"
> and make it camel cased

As discussed, kept these private, but made the getters @CalledByNative.
This allowed C++ to directly JNI into the nested classes, and all statics below
got dropped.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:234:
private static String getWalletEmail(Object fullWallet) {
On 2013/08/10 01:33:35, Ted C wrote:
> I think these can be ResultWallet instead Object, worth checking and dropping
> all the casts.
> 
> Maybe look at making the accessors above CalledByNative and drop the static
> functions entirely.

Dropped the statics entirely.

https://codereview.chromium.org/22566004/diff/10001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillDialogResult.java:235:
return((ResultWallet) fullWallet).getEmail();
On 2013/08/10 01:33:35, Ted C wrote:
> space after return

Done.

https://codereview.chromium.org/22566004/diff/10001/chrome/browser/ui/android...
File chrome/browser/ui/android/autofill/autofill_dialog_result.cc (right):

https://codereview.chromium.org/22566004/diff/10001/chrome/browser/ui/android...
chrome/browser/ui/android/autofill/autofill_dialog_result.cc:37: #define
FETCH_JSTRING(utf, env, jobj, getter) \
On 2013/08/10 01:33:35, Ted C wrote:
> I don't see a lot of macros in Chromium code, but maybe I don't look hard
> enough.  That's all I have to say about that.

Agreed, but it made my life much simpler.
git grep -E '#define(.+)\\' | grep \.cc | wc -l prints 911.

[commit-bot: I haz the power, 2013-08-10 12:40:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22566004/13001

[commit-bot: I haz the power, 2013-08-10 16:31:01]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 16:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aruslan@chromium.org/22566004/13001

[commit-bot: I haz the power, 2013-08-10 18:27:08]

Change committed as 216883