Populate pending elements with values given by requestAutocomplete UI

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: Paolo)

Tracking

(Blocks 1 bug)

unspecified
mozilla33
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Flags: firefox-backlog+
Whiteboard: p=3 → p=3 [qa-]
Populates pending elements with dummy values given by the UI.

Works on this sample page: http://people.mozilla.org/~bnicholson/test/autocomplete.html
Attachment #8446261 - Flags: review?(paolo.mozmail)
Attachment #8446261 - Flags: review?(MattN+bmo)
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?
Attachment #8446261 - Flags: review?(MattN+bmo) → review-
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.
Attachment #8446261 - Flags: review?(paolo.mozmail)
Posted patch Rebased patch (obsolete) — Splinter Review
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.
Assignee: bnicholson → paolo.mozmail
Attachment #8446261 - Attachment is obsolete: true
Iteration: --- → 33.2
Points: --- → 3
QA Whiteboard: [qa-]
Whiteboard: p=3 [qa-]
Added to Iteration 33.2
Posted patch Next version of the patch (obsolete) — Splinter Review
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.
Attachment #8450199 - Flags: feedback?(MattN+bmo)
Attachment #8448722 - Attachment is obsolete: true
Posted patch The patch (obsolete) — Splinter Review
Still without tests, but in final form.
Attachment #8450199 - Attachment is obsolete: true
Attachment #8450199 - Flags: feedback?(MattN+bmo)
Attachment #8450320 - Flags: review?(MattN+bmo)
Iteration: 33.2 → 33.3
Blocks: 1036481
Blocks: 1023862
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).
Attachment #8450320 - Flags: feedback+
(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 on attachment 8450320 [details] [diff] [review]
The patch

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

I agree with the discussed changes.
Attachment #8450320 - Flags: review?(MattN+bmo) → feedback+
Posted patch Updated patchSplinter Review
This patch has been updated with the changes we discussed.
Attachment #8450320 - Attachment is obsolete: true
Attachment #8454561 - Flags: review?(MattN+bmo)
Attachment #8454561 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/c517a17ebe6b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.