946022 - (rAc-android) Implement base requestAutocomplete UI for Android

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Brian Nicholson (:bnicholson)]

Implementation specific to Firefox for Android; see bug 939351 for the core bug.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: requestAutocomplete API changes

Not much here yet, but it'd be good to take an early look since I haven't done a lot of core development.

I don't know who the best reviewer is, so I'm just requesting review from smaug since we had a brief discussion about this feature in #content.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: WIP requestAutocomplete implementation

A rough, very much WIP patch.

I'd like to split this so that a generic implementation can be used across products, and I'm not sure of the best way forward. Ideas I've considered so far:
* Have a product-generic AutofillController along with product-specific AutofillPopup implementations. AutofillController calls AutofillPopup.show(), where (on Android) AutofillPopup will message the Java UI, somehow refer back to the AutofillController (or another component) for validation, then asynchronously call back to AutofillController after the form is submitted.
* Alternatively, have a product-generic AutofillUtils that contains many util functions for iterating input fields on the page, validation, possibly storage, etc. We then have multiple product-specific AutofillControllers that use AutofullUtils to implement the correct behavior.

Other questions:
* Do we want validation to be implemented as a shared core component, or should it be product-specific? Currently, I'm leaning toward shared since validation might get pretty complicated, and we may not want to reimplement this logic for every product. The disadvantage of this would be that we'll have to do validation in Gecko, then update the UI asynchronously on Android.
* Same question for storage -- again, I'm leaning toward a shared component. I'm not sure why the SQL databases would differ on any products; they'll all need to hold the billing/shipping/payment data, along with any sync data. Again, the has the disadvantage that data will need to be sent asynchronously between Gecko and the UI.

Comment 3

[Brian Nicholson (:bnicholson)]

Sample build for testing requestAutocomplete: https://dl.dropboxusercontent.com/u/35559547/fennec-autofill.apk

This (mostly) works with the requestAutocomplete demo linked to by the docs: https://googledrive.com/host/0B28BnxIvH5DuWGc3Mm5sZE9DekE/

To make things easier to test, I've included some dummy data in the build to prepopulate the form.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8342063 [details] [diff] [review]
Part 1: requestAutocomplete API changes

There shouldn't be reason to add anything to nsIDOMHTMLFormElement.idl
New stuff should go to .webidl.

If we're planning to implement this first only on Android, we sure
should have a pref for void requestAutocomplete();
So you should add
[Pref="your.pref.name"]

See
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#.5BPref.3Dprefname.5D

In the DOM C++ code, use {} always with if

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8342063 [details] [diff] [review]
Part 1: requestAutocomplete API changes


>+[scriptable, uuid(cbf47f9d-a13d-4fad-8221-8964086b1b8a)]
>+interface nsIAutofillController : nsISupports
>+{
>+  void autocomplete(in nsIDOMHTMLFormElement form);
>+};
I think we should reuse requestAutocomplete() for the method name, since this is actually a request.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8342070 [details] [diff] [review]
Part 2: WIP requestAutocomplete implementation


>+  observe: function observe(aSubject, aTopic, aData) {
>+    // Null data means the user cancelled; abort and send autocomplete error event.
>+    if (!aData) {
>+      Services.obs.removeObserver(this, "Autofill:Submit");
Is it guaranteed that we end up removing the observer always? Even in the strangest cases, since if not, 
we leak.


>+  autocomplete: function autocomplete(aForm) {
>+    this._win = aForm.ownerDocument.defaultView;
>+    this._form = aForm;
>+
>+    if (this._isActive || aForm.autocomplete === "off") {
>+      let evt = new this._win.AutocompleteErrorEvent("autocompleteerror", { bubbles: true, reason: "disabled" });
this._win can be null here. Should requestAutocomplete() work with elements which aren't in document or not in a document
with defaultView?


+  // TODO: Pull user and payment info from database
+  _getStoredData: function _getStoredData() {
What will that look like? I hope it doesn't block the caller. Can we read the data off-the-Gecko-and-main-thread?

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8342070 [details] [diff] [review]
> Part 2: WIP requestAutocomplete implementation
> 
> 
> >+  observe: function observe(aSubject, aTopic, aData) {
> >+    // Null data means the user cancelled; abort and send autocomplete error event.
> >+    if (!aData) {
> >+      Services.obs.removeObserver(this, "Autofill:Submit");
> Is it guaranteed that we end up removing the observer always? Even in the
> strangest cases, since if not, 
> we leak.

Yes, we always need to remove this observer, so we need to be careful that we go through the completion path in all circumstances. Aside from the leak you pointed out, not reaching this code would also prevent this._isActive from getting reset, which would prevent any future requestAutocomplete requests from working. (The _isActive flag, in case you were curious, is used as specified by the algorithm in http://wiki.whatwg.org/wiki/RequestAutocomplete.)

> >+  autocomplete: function autocomplete(aForm) {
> >+    this._win = aForm.ownerDocument.defaultView;
> >+    this._form = aForm;
> >+
> >+    if (this._isActive || aForm.autocomplete === "off") {
> >+      let evt = new this._win.AutocompleteErrorEvent("autocompleteerror", { bubbles: true, reason: "disabled" });
> this._win can be null here. Should requestAutocomplete() work with elements
> which aren't in document or not in a document
> with defaultView?

Good question; the spec doesn't mention anything about forms not yet added to the document. I'll add yet another TODO comment to investigate.
 
> +  // TODO: Pull user and payment info from database
> +  _getStoredData: function _getStoredData() {
> What will that look like? I hope it doesn't block the caller. Can we read
> the data off-the-Gecko-and-main-thread?

Yes, this should be async and off the Gecko thread. This will need to use some kind of protected database since it will be storing credit card information, though I'm not really familiar with our current options. I imagine this will overlap with the new Firefox accounts work -- Richard, any thoughts on how/where this data should be stored?

Olli, thanks for the feedback so far! Do you have recommendations for how to break this up between generic and Android components? Do either of the ideas in comment 2 make sense to you?

Comment 8

[Richard Newman [:rnewman]]

I have only a tenuous grasp of what this is for, so here's my small amount of feedback:

* Why not store this in the form history DB? If not there, store it in (encrypted) JSON, and manage the values in-memory.

* I suspect that different platforms will have very different modes of interaction between this data and the page -- for example, on most phones it won't be possible to even see all of the fields on-screen. I think exposing an API from the page to access the appropriate form fields, and from storage to access saved values, and then utils as appropriate (which will span two programming languages!), is likely to be least constraining.

* Some validation and transformation will need to happen on the Java side (e.g., fixed-width credit card presentation fields, date pickers, etc.). By all means have validation on the JS side, too.

* See Bug 939351 for how I really feel.

Hope that's of help...

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #4)
> If we're planning to implement this first only on Android, we sure
> should have a pref for void requestAutocomplete();
> So you should add
> [Pref="your.pref.name"]

I don't see a good reason to make this android-only; seems like this patch should live in bug 939351.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > If we're planning to implement this first only on Android, we sure
> > should have a pref for void requestAutocomplete();
> > So you should add
> > [Pref="your.pref.name"]
> 
> I don't see a good reason to make this android-only; seems like this patch
> should live in bug 939351.

I agree that part of part 1 should live in bug 939351. The part that impls the nsIAutofillController as a JS component is Android specific, and should live here.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8342070 [details] [diff] [review]
Part 2: WIP requestAutocomplete implementation

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java
> import org.mozilla.gecko.widget.GeckoActionProvider;
> import org.mozilla.gecko.widget.ButtonToast;
>-
> import org.json.JSONArray;

Keep 'em separated

> import org.json.JSONException;
> import org.json.JSONObject;

>+    private AutofillHandler mAutofillHandler;

Putting this in BrowserApp means Web Apps won't be able to use it. Consider the implications and maybe move to GeckoApp.

>+        registerEventListener("Autofill:Prompt");
>+        registerEventListener("Autofill:Response");

Why are these here? Aren't they in AutofillHandler.java?

>         unregisterEventListener("Settings:Show");
>         unregisterEventListener("Updater:Launch");

No unregister ? Not a concern if the registers should be removed anyway.


I think you have a good start on the approach. Looks like you could keep moving it forward and see what other issues pop up.

Comment 12

[Brian Nicholson (:bnicholson)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > If we're planning to implement this first only on Android, we sure
> > should have a pref for void requestAutocomplete();
> > So you should add
> > [Pref="your.pref.name"]
> 
> I don't see a good reason to make this android-only; seems like this patch
> should live in bug 939351.

Agreed -- I was going to do that after figuring out how the rest of the IDL changes would look (i.e., figuring out comment 2), but I guess I can just move patch 1 to bug 939351 now and do the remainder separately as the implementation gets sorted out.

(In reply to Mark Finkle (:mfinkle) from comment #11)
> Keep 'em separated

Eclipse does that automatically, need to figure out how to turn that off...

> > import org.json.JSONException;
> > import org.json.JSONObject;
> 
> >+    private AutofillHandler mAutofillHandler;
> 
> Putting this in BrowserApp means Web Apps won't be able to use it. Consider
> the implications and maybe move to GeckoApp.

Yeah, I didn't think WebApps would be making use of this feature, but I guess it can't hurt to move it just in case.

> >+        registerEventListener("Autofill:Prompt");
> >+        registerEventListener("Autofill:Response");
> 
> Why are these here? Aren't they in AutofillHandler.java?

Oops, I used to have these here and left some turds behind.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #9)
> (In reply to Olli Pettay [:smaug] from comment #4)
> > If we're planning to implement this first only on Android, we sure
> > should have a pref for void requestAutocomplete();
> > So you should add
> > [Pref="your.pref.name"]
> 
> I don't see a good reason to make this android-only; seems like this patch
> should live in bug 939351.
I said "first". So if we don't have implementation on all the platforms, we should pref it, 
and once we have implementation everywhere, remove the pref.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: (prototype) requestAutocomplete dialog

Here are screenshots of the UI so far. Still working on cleaning up the patches for feedback -- some of them should be ready by tomorrow.

Build here: http://people.mozilla.org/~bnicholson/fennec_builds/autocomplete-1.apk
requestAutocomplete demo site here: https://googledrive.com/host/0B28BnxIvH5DuWGc3Mm5sZE9DekE/

Comment 15

[Brian Nicholson (:bnicholson)]

Attachment: (prototype) Autofill preferences screen

Comment 16

[Brian Nicholson (:bnicholson)]

Attachment: (prototype) Credit card edit dialog

Comment 17

[Brian Nicholson (:bnicholson)]

Attachment: (prototype) Address edit dialog

Comment 18

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Android requestAutocomplete implementation

Updated patch for Android. Still lots of dirty parts (several of them commented), so I'm mostly just looking for higher-level design comments for now. But also happy to hear any other comments if you want to scrutinize it more closely.

As I mentioned in bug 939351, I created an etherpad at https://etherpad.mozilla.org/requestautocomplete that I'll start filling in soon.

Comment 19

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Support editing entries

Same story as above.

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8401722 [details] [diff] [review]
Part 1: Android requestAutocomplete implementation

I need to read up on the requestAutocomplete spec, but I assume it specifies all of the fields you are using in the patch. We aren't making them up.


You are adding a handful of Java data classes. Just wondering if we need all of them or not. It's not too bad, but Java tends to breed classes, so I want to be mindful.

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+Cu.import("resource://gre/modules/JavaRequest.jsm");

I'd like to think about needing to force importing code.

>+let Autofill = {
>+  init: function () {
>+    JavaRequest.addListener(this, "Autofill:Get");
>+    JavaRequest.addListener(this, "Autofill:Validate");
>+    Services.obs.addObserver(this, "Autofill:Edit", false);
>+  },
>+
>+  uninit: function () {
>+    JavaRequest.removeListener(this, "Autofill:Get");
>+    JavaRequest.removeListener(this, "Autofill:Validate");
>+    Services.obs.removeObserver(this, "Autofill:Edit");
>+  },

Also wondering if we could move this to an external JS file. Maybe we can't. It looks like you were being thoughtful and only including the minimal amount of code needed to register. We do have a helper for observer service, I wonder if we could make a helper for JavaRequest. Just thinking out loud more than anything here.

f+, looks good overall.

Comment 21

[Brian Nicholson (:bnicholson)]

Attachment: Implement rAc Android base UI

This gives us parity with desktop for a minimal rAc dialog. It's not very useful since the user can't actually change the values that get submitted, but this sets up the framework by implementing some essential files:
  * RequestAutocompleteUI.jsm - Hooks into the existing rAc platform framework and triggers the Java-side rAc handling
  * AutofillGeckoClient.java - The bridge between Gecko and Java rAc code, responsible for serializing/deserializing data and dispatching autofill events
  * AutofillDialog.java - Displays the rAc dialog

This feature is preffed off by default, so with this patch applied, the "dom.forms.autocomplete.experimental" and "dom.forms.requestAutocomplete" prefs need to be enabled (the build linked below already has them enabled).

Build here: https://people.mozilla.org/~bnicholson/fennec_builds/autocomplete-2.apk

To test this out, you can go to https://people.mozilla.org/~bnicholson/test/rac-simple.html and click the requestAutocomplete button. For now, the dialog simply shows OK and cancel; if you click OK, hardcoded dummy values will be sent to the page. If you enable these prefs on a desktop build and follow the same steps, you should see the same behavior.

Comment 22

[Brian Nicholson (:bnicholson)]

Attachment: Implement rAc Android base UI, v2

Forgot to refresh.

Comment 23

[Brian Nicholson (:bnicholson)]

Renaming bug since this is just a base implementation and far from a working rAc.

Also marking bug 1043633 as a dependency since this uses the namespaced, promise-based Messaging.jsm.

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 8475342 [details] [diff] [review]
Implement rAc Android base UI, v2

Review of attachment 8475342 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good as a base implementation. I assume that additional non-address sections will be added to AutofillData? It could be nice to add a note in the comments that further fields/sections can be added there.

Could you point me to the bug that handles the messages back to Gecko? I don't see a callback in the RequestAutocompleteUI, or what would be calling it with the autofill data.

A handful of nits and maybe some naming - all small things.

::: mobile/android/base/autofill/AutofillData.java
@@ +8,5 @@
> +/**
> + * Data requested by a requestAutocomplete invocation.
> + *
> + * When a request is handled, the sections, address sections, and fields are populated to match
> + * the data being requested. The UI is reponsible for populating the data in each field. When the

spelling: responsible

@@ +26,5 @@
> + *         contactType: ""
> + *
> + */
> +public class AutofillData implements Iterable<AutofillData.Section> {
> +    private final ArrayList<Section> sections = new ArrayList<Section>();

Nit: Use List in the type declaration here.

@@ +52,5 @@
> +                section.addAddressSection(addressSection);
> +            }
> +
> +            sections.add(section);
> +        }

Will there be handling for individual fields that are not part of an implemented section?

@@ +58,5 @@
> +
> +    /**
> +     * Data for a section, corresponding to the section-* autocomplete attribute.
> +     */
> +    public static class Section implements Iterable<AddressSection> {

How will this change when there are more section types than just AddressSection?

::: mobile/android/base/autofill/AutofillDialog.java
@@ +19,5 @@
> +    private final Context context;
> +    private AlertDialog dialog;
> +    private SubmitListener submitListener;
> +
> +    private static final HashMap<String, String> DUMMY_DB = new HashMap<String, String>();

Nit: Use Map in declaration.

@@ +49,5 @@
> +
> +    private void showDialog(final AutofillData requestData) {
> +        ThreadUtils.assertOnUiThread();
> +
> +        // This view is being inflated for a dialog, so no root exists yet.

I think it might be more clear to say that this view is being inflated manually to be passed to a Dialog builder, which handles setting the root - up to you.

::: mobile/android/base/autofill/AutofillException.java
@@ +1,3 @@
> +package org.mozilla.gecko.autofill;
> +
> +@SuppressWarnings("serial")

I looked at some of our other exceptions, and it looks we just include some serializationUID. I don't think this is an exception that would be serialized or anything, but maybe it's worth doing anyways?

::: mobile/android/base/autofill/AutofillGeckoClient.java
@@ +16,5 @@
> +/**
> + * Bridge between Gecko and the Java front-end for accessing autofill data.
> + */
> +public class AutofillGeckoClient {
> +    private static AutofillListener autofillListener;

Maybe this should be AutofillCallback or AutofillHandler (or AutofillDelegate), because it doesn't do any actual listening, but gets called when the event listener gets a message.

@@ +18,5 @@
> + */
> +public class AutofillGeckoClient {
> +    private static AutofillListener autofillListener;
> +
> +    public interface SubmitListener {

Similar, maybe a Handler or Callback? (I don't feel very strongly about this one, though.)

@@ +30,5 @@
> +    public interface AutofillListener {
> +        /**
> +         * Called when the page has fired requestAutocomplete().
> +         *
> +         * The listener *must* respond to the request by calling either

Clarify that the *submit* listener must call... because this interface is also called a Listener.

@@ +105,5 @@
> +     * @param callback Callback from the NativeEventListener used to send a response to Gecko
> +     */
> +    private static void submitAutofill(AutofillData data, EventCallback callback) {
> +        try {
> +            JSONArray jsonFields = new JSONArray();

final?

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8475342 [details] [diff] [review]
Implement rAc Android base UI, v2


>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

Call me crazy, but I don't like using the "dialog" as the main driver. I'd rather use the "Gecko Client" as the driver. I'd be happier never seeing the dialog instantiated unless we really get a callback.

Thoughts?

>diff --git a/mobile/android/base/resources/layout/autofill_dummy_dialog.xml b/mobile/android/base/resources/layout/autofill_dummy_dialog.xml

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+              android:layout_width="fill_parent"
>+              android:layout_height="wrap_content"
>+              android:orientation="vertical">
>+
>+        <TextView android:layout_width="fill_parent"
>+                  android:layout_height="wrap_content"
>+                  android:text="requestAutocomplete demo dialog" />

What's the next step to be able to get a real dialog with some level of user value? Doesn't need to be a full down rAC with credit cards and payments.

>diff --git a/mobile/android/modules/RequestAutocompleteUI.jsm b/mobile/android/modules/RequestAutocompleteUI.jsm

>+this.RequestAutocompleteUI = function (aAutofillData) {

nit: function(

>+  show: Task.async(function* () {

I hate you

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Let's try to get back to this bug after sprinting. Product and UX have been asking about "Form Fill Improvements" and this is the heart of it.

Comment 27

[Matthew N. [:MattN]]

requestAutocomplete was removed from the HTML specification.