1020607 - Populate pending elements with values given by requestAutocomplete UI

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Brian Nicholson (:bnicholson)]

After the user has selected values in the requestAutocomplete dialog, we need to go through the list of pending elements in the form to populate them.

Since we need an existing list of pending elements to iterate after getting the response from the UI, this bug is blocked by bug 1020602.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Populate pending elements with values given by requestAutocomplete UI

Populates pending elements with dummy values given by the UI.

Works on this sample page: http://people.mozilla.org/~bnicholson/test/autocomplete.html

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8446261 [details] [diff] [review]
Populate pending elements with values given by requestAutocomplete UI

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

This is pretty good but I think we should use a weakmap for elements and avoid overwriting parts of the data passed to the UI.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +62,4 @@
>  
>    _getAutofillFields: function (aForm) {
>      let fields = {};
> +    let elements = [];

A weakmap would reduce chances of leaks in the event the tab closes during rAc.

@@ +80,5 @@
>        }
>  
> +      elements.push({
> +        el: el,
> +        attr: parsedAttr

We can afford to be more verbose e.g. element and tokens/autocompleteTokens but switching to a weakmap makes this a moot point.

@@ +109,5 @@
> +   * @param aAutofillData The set of values to perform the autofill.
> +   */
> +  _autofillElements: function (aElements, aAutofillData) {
> +    // TODO: Make sure the elements haven't changed (bug 1020466).
> +    for (let elData of aElements) {

With a weakmap, loop over form.elements here. We would have to skip over some of the same fields skipped in _getAutofillFields so we may eventually want to make our own iterator to avoid duplication.

::: toolkit/components/formautofill/desktopui/RequestAutocompleteDesktopUI.jsm
@@ +61,5 @@
> +          if (!(fieldName in this._dummyDB[addressType])) {
> +            continue;
> +          }
> +
> +          this._fields[section][addressType][fieldName] = this._dummyDB[addressType][fieldName];

Eventually this._fields[section][addressType][fieldName] would contain an object (or array of objects) with properties describing the field(s) whereas you're currently passing null. Regardless, this is overwriting those values which feels weird. I haven't thought too much about what data needs to go back to the page but I don't think overwriting the object is a good idea. If we think we need to pass some of the original info back to the content script, then I would prefer we just set some "value" property on the object for the field. Otherwise, I think it's less error prone to construct nested objects of the same structure and do as you're doing above by assigning the result string to the fieldName key. Since you're holding on to the field data you passed here, we probably don't need to send any of that back as long as you can identify fields to set the value. Thoughts?

Comment 3

[:Paolo Amadini]

Comment on attachment 8446261 [details] [diff] [review]
Populate pending elements with values given by requestAutocomplete UI

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

(In reply to Matthew N. [:MattN] from comment #2)
>  Thoughts?

I agree we should return a different object, we don't need to send back most of the information we received.

While I'm here, I'll mention that in case the dialog was canceled, I think the UI may return something like { canceled: true }, and this can be handled normally by the component. Promise rejections should be reserved for unexpected errors, like JavaScript exceptions in general, unless of course the code defines its own exception type for specific scenarios.

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +28,5 @@
>    classID: Components.ID("{ed9c2c3c-3f86-4ae5-8e31-10f71b0f19e6}"),
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIFormAutofillContentService]),
>  
>    // nsIFormAutofillContentService
> +  requestAutocomplete: Task.async(function* (aForm, aWindow) {

We should use Task.spawn(...).catch(Cu.reportError); since the requestAutocomplete function returns void, while Task.async return a promise. In any case I think I'll move this refactoring to the UI patch, it makes a lot of sense.

::: toolkit/components/formautofill/desktopui/RequestAutocompleteDesktopUI.jsm
@@ +25,5 @@
>  /**
>   * Handles the requestAutocomplete user interface on Desktop.
>   */
>  this.RequestAutocompleteDesktopUI = function (aProperties) {
> +  this._fields = JSON.parse(aProperties);

I think aProprties should be an object, and all the interprocess communication should be handled in a separate module before the call and UI object creation.

Comment 4

[:Paolo Amadini]

Attachment: Rebased patch

I rebased the current patch on top of the previous bugs, and did some minimal changes required for it to work with the existing tests.

I'll address the review comments and add some relevant tests next.

Comment 5

[Marco Mucci [:MarcoM]]

Added to Iteration 33.2

Comment 6

[:Paolo Amadini]

Attachment: Next version of the patch

This works with the updated format.

I believe we'll want to factor out the form handler to a separate object, instead of placing the global functions on the service. Note how we're passing around the parameters as redundant arguments on various functions, and are even using multiple return values.

Comment 7

[:Paolo Amadini]

Attachment: The patch

Still without tests, but in final form.

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8450320 [details] [diff] [review]
The patch

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

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +27,5 @@
>  function FormHandler(aForm, aWindow) {
>    this.form = aForm;
>    this.window = aWindow;
> +
> +  this.mapElementToLocalId = new WeakMap();

Probably don't need a WeakMap here (as discussed on IRC).

@@ +139,5 @@
>        };
>        addressSection.fields.push(field);
> +
> +      // Store the association between the element and the identifier we used.
> +      this.mapElementToLocalId.set(element, field.localId);

We'll need to keep the attribute tokens to make sure they didn't change while the dialog was showing (also discussed on IRC).

Comment 9

[:Paolo Amadini]

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> We'll need to keep the attribute tokens to make sure they didn't change
> while the dialog was showing (also discussed on IRC).

Note that we'll probably use them as keys so we'll keep them anyways, though the actual code and tests for this check will be in bug 1020466 or dependencies of it.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8450320 [details] [diff] [review]
The patch

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

I agree with the discussed changes.

Comment 11

[:Paolo Amadini]

Attachment: Updated patch

This patch has been updated with the changes we discussed.

Comment 12

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/c517a17ebe6b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c517a17ebe6b