Closed Bug 1417803 Opened 7 years ago Closed 6 years ago

[Form Autofill] Implement `activeSection` attribute in FormAutofillHandler or FormAutofillContent to simplify the architecture.

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(3 files)

Bug 1417803 - Part 1: Use activeSection to record the current focused field or section.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details
Bug 1417803 - Part 2: Add a underscore to the private functions.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details 
bug 1339731 is going to provide the section concept and hide the section concept in FormAutofillHandler.
`activeSection` property in FormAutofillHandler or FormAutofillContent should provide a way to access the current focused section (or even fieldDetail), and the module can access the section directly without the duplicated queries, e.g. getFieldDetailsByElement, getSectionByElement, etc.
In the part 1 patch, FormAutofillContent is able to access `activeSection`'s method to manipulate the following methods:
allFieldNames
getFilledRecordGUID()
getAdaptedProfiles(records)
clearPreviewedFormFields()
previewFormFields(profile)
clearPopulatedForm()

`activeSection` is determined by the setter of `focusedInput` in FormAutofillHandler, and `formHandler.focusedInput` will be updated in `identifyAutofillFields` function.

`formFillController.focusedInput` was used in many places in `FormAutofillContent`, and I will fix this in the following patch.
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment on attachment 8934896 [details]
Bug 1417803 - Part 1: Use activeSection to record the current focused field or section.

https://reviewboard.mozilla.org/r/205824/#review212256

Overall it looks good. Only a few things that we can discuss face to face.

::: browser/extensions/formautofill/FormAutofillContent.jsm:509
(Diff revision 2)
>      let formHandler = this.getFormHandler(element);
>      if (!formHandler) {
>        let formLike = FormLikeFactory.createFromField(element);
>        formHandler = new FormAutofillHandler(formLike);
>      } else if (!formHandler.updateFormIfNeeded(element)) {
> +      formHandler.focusedInput = element;

It seems a little bit obscure to set `focusedInput` in `identifyAutofillFields` method. How about exposing another method (e.g. `FormAutofillContent.setFocusedInput()`) which can be called from `FormAutofillFrameScript`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:155
(Diff revision 2)
> -  getFieldDetailsByElement(element) {
> -    let targetSet = this._getTargetSet(element);
> +  getFieldDetailsByFocusedInput() {
> +    let targetSet = this._getTargetSet();
>      return targetSet ? targetSet.fieldDetails : [];
>    }

Can we just expose `_getTargetSet`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:881
(Diff revision 2)
>      let sections = FormAutofillHeuristics.getFormInfo(this.form, allowDuplicates);
>      let allValidDetails = [];
>      for (let fieldDetails of sections) {
>        let section = new FormAutofillSection(fieldDetails, this.winUtils);
>        this.sections.push(section);
> -      allValidDetails.push(...section.validDetails);
> +      allValidDetails.push(...section._validDetails);

It's a bit weird to access a private variable of `section` here. Do you have any further plan to change this part?
Comment on attachment 8934896 [details]
Bug 1417803 - Part 1: Use activeSection to record the current focused field or section.

https://reviewboard.mozilla.org/r/205824/#review212770

::: browser/extensions/formautofill/FormAutofillContent.jsm:509
(Diff revision 2)
>      let formHandler = this.getFormHandler(element);
>      if (!formHandler) {
>        let formLike = FormLikeFactory.createFromField(element);
>        formHandler = new FormAutofillHandler(formLike);
>      } else if (!formHandler.updateFormIfNeeded(element)) {
> +      formHandler.focusedInput = element;

Agree, and it will be refactored in another part for FormAutofillContent.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:155
(Diff revision 2)
> -  getFieldDetailsByElement(element) {
> -    let targetSet = this._getTargetSet(element);
> +  getFieldDetailsByFocusedInput() {
> +    let targetSet = this._getTargetSet();
>      return targetSet ? targetSet.fieldDetails : [];
>    }

Just like `getFilledRecordGUID` but it's for `fieldDetails`. So my suggestion is to remain this.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:881
(Diff revision 2)
>      let sections = FormAutofillHeuristics.getFormInfo(this.form, allowDuplicates);
>      let allValidDetails = [];
>      for (let fieldDetails of sections) {
>        let section = new FormAutofillSection(fieldDetails, this.winUtils);
>        this.sections.push(section);
> -      allValidDetails.push(...section.validDetails);
> +      allValidDetails.push(...section._validDetails);

Yes, it should not be a private member since `FormAutofillHandler` uses it.
Comment on attachment 8934896 [details]
Bug 1417803 - Part 1: Use activeSection to record the current focused field or section.

https://reviewboard.mozilla.org/r/205824/#review213190

LGTM, thanks.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:102
(Diff revision 4)
> +  set focusedInput(element) {
> +    this._focusedDetail = this.getFieldDetailByElement(element);
> +  }

In section, it seems we don't actually set the focused input but its detail, and we still have to get the element again from .elementWeakRef in the latter part. Would change the setter to setFocusedFieldDetail(element) be more clear?
Attachment #8934896 - Flags: review?(ralin) → review+
Comment on attachment 8936215 [details]
Bug 1417803 - Part 2: Add a underscore to the private functions.

https://reviewboard.mozilla.org/r/206988/#review213192
Attachment #8936215 - Flags: review?(ralin) → review+
Comment on attachment 8934896 [details]
Bug 1417803 - Part 1: Use activeSection to record the current focused field or section.

https://reviewboard.mozilla.org/r/205824/#review213232
Attachment #8934896 - Flags: review?(lchang) → review+
Comment on attachment 8936215 [details]
Bug 1417803 - Part 2: Add a underscore to the private functions.

https://reviewboard.mozilla.org/r/206988/#review213234
Attachment #8936215 - Flags: review?(lchang) → review+
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213284

Looks good, just a concern about the consistency between out activeInput and formController's focused input. Should we reset/clear the _activeDetail when focus on ineligible input?

::: browser/extensions/formautofill/FormAutofillContent.jsm:102
(Diff revision 1)
> -
>      this.forceStop = false;
>  
>      let savedFieldNames = FormAutofillContent.savedFieldNames;
>  
> -    let focusedInput = formFillController.focusedInput;
> +    let focusedInput = FormAutofillContent.activeInput;

Would destructing assignment be more concise? Like:
```
let {activeInput, activeSection, activeFieldDetail} = FormAutofillContent;
```

::: browser/extensions/formautofill/FormAutofillContent.jsm:469
(Diff revision 1)
> +    handler.focusedInput = element;
> +    this._activeDetail = {
> +      handler,
> +      element,
> +      section: handler.activeSection,
> +      fieldDetail: null,

Didn't observe in previous patches. But with these line of codes, it looks implict where the .activeSection is from. No solid idea comes to mind, but I wonder if we can have more explanatory way here?

::: browser/extensions/formautofill/FormAutofillContent.jsm:604
(Diff revision 1)
>                .getInterface(Ci.nsIContentFrameMessageManager);
>    },
>  
>    _onKeyDown(e) {
>      let lastAutoCompleteResult = ProfileAutocomplete.lastProfileAutoCompleteResult;
> -    let focusedInput = formFillController.focusedInput;
> +    let focusedInput = FormAutofillContent.activeInput;

_onKeyDown listen to all the keydown event, so here I guess the activeInput doesn't 100% equal to formFillController.focusedInput in all cases since we skip updateActiveInput when input is ineligible.
Attachment #8936333 - Flags: review?(ralin)
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213736

Looks good! Thanks!
Attachment #8936333 - Flags: review?(ralin) → review+
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213284

> _onKeyDown listen to all the keydown event, so here I guess the activeInput doesn't 100% equal to formFillController.focusedInput in all cases since we skip updateActiveInput when input is ineligible.

After the offline discussion, `updateActiveInput` will be applied to the focus event handler in frame script.
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213748

::: browser/extensions/formautofill/FormAutofillContent.jsm:304
(Diff revision 4)
>        autocompleteController.searchString = profile[fieldName];
>      });
>    },
>  
>    _clearProfilePreview() {
> -    let focusedInput = formFillController.focusedInput || this.lastProfileAutoCompleteFocusedInput;
> +    if (!this.lastProfileAutoCompleteFocusedInput || !FormAutofillContent.activeSection) {

Check `this.lastProfileAutoCompleteFocusedInput` is enough so bug 1423181 can be fixed as well.
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213750

::: browser/extensions/formautofill/FormAutofillContent.jsm:346
(Diff revision 3)
>    /**
>     * @type {Set} Set of the fields with usable values in any saved profile.
>     */
>    savedFieldNames: null,
>  
> +  _activeDetail: {},

Need a commet describing its properties.

BTW, people may confuse the naming with `activeFormDetails`. How about something like `activeItems` or `activeReferences`?

::: browser/extensions/formautofill/FormAutofillContent.jsm:465
(Diff revision 3)
> -  getFormDetails(element) {
> -    let formHandler = this.getFormHandler(element);
> +  get activeFormDetails() {
> +    let formHandler = this.activeHandler;
>      return formHandler ? formHandler.fieldDetails : null;
>    },
>  
> -  getAllFieldNames(element) {
> +  updateActiveInput(element) {

Need a comment, too.

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:55
(Diff revision 3)
>      addMessageListener("FormAutoComplete:PopupClosed", this);
>      addMessageListener("FormAutoComplete:PopupOpened", this);
>    },
>  
>    handleEvent(evt) {
> +    FormAutofillContent.updateActiveInput();

We probably shouldn't update the active input if the event isn't trusted or Autofill isn't enabled. Can we put this line below those checks?
Attachment #8936333 - Flags: review?(lchang) → review+
Comment on attachment 8936333 [details]
Bug 1417803 - Part 3: Use _activeItems to record the information of the current focused input.

https://reviewboard.mozilla.org/r/207064/#review213750

> Need a commet describing its properties.
> 
> BTW, people may confuse the naming with `activeFormDetails`. How about something like `activeItems` or `activeReferences`?

`_activeItems` is taken as the variable name.

> We probably shouldn't update the active input if the event isn't trusted or Autofill isn't enabled. Can we put this line below those checks?

It's moved to the end of `if (!evt.isTrusted || !FormAutofillUtils.isAutofillEnabled)` block.
The try result looks good, so it's ready to be landed. Thanks for the review.
Keywords: checkin-needed
Remove checkin-needed since the patch needs to be rebased.
Keywords: checkin-needed
Rebased for renaming `_getFieldDetailByName` in bug 1422404.
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fe9d556bdef4
Part 1: Use activeSection to record the current focused field or section. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/cd5e96cab7c5
Part 2: Add a underscore to the private functions. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/af5b96ec9db0
Part 3: Use activeField to record the current focused input. r=lchang,ralin
Keywords: checkin-needed
Backed out for failing browser/browser_device_width.js on Linuxx64 Stylo Disabled Debug platform

backout: https://hg.mozilla.org/integration/autoland/rev/7521d3eb77f9a54c9b84f37e1042804ce707dc15

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=af5b96ec9db0bcb2f56e93a6f681112f12a5f1ff&selectedJob=152154096

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=152154096&repo=autoland&lineNumber=14103

   INFO - TEST-INFO | Main app process: exit 0
14103
[task 2017-12-18T18:50:42.214Z] 18:50:42    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_device_width.js | leaked 2 window(s) until shutdown [url = chrome://devtools/content/responsive.html/index.xhtml]
14104
[task 2017-12-18T18:50:42.215Z] 18:50:42     INFO - TEST-INFO | devtools/client/responsive.html/test/browser/browser_device_width.js | windows(s) leaked: [pid = 2291] [serial = 79], [pid = 2291] [serial = 77]
Flags: needinfo?(selee)
I think this is caused by keeping element reference in _activeItems. I am going to push a new patch to fix this. Thanks.
Flags: needinfo?(selee)
The dt* issue are resolved in the latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83eeeecb9cd2972b2ff8e76f514a93d28cc8c5fb

I think it's ready to be landed again. Thanks.
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/636aaabed9ff
Part 1: Use activeSection to record the current focused field or section. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/9e6df87f7599
Part 2: Add a underscore to the private functions. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/18c3ccc73536
Part 3: Use _activeItems to record the information of the current focused input. r=lchang,ralin
Keywords: checkin-needed
Depends on: 1426549
Depends on: 1426561
Depends on: 1427219
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: