Closed Bug 1304322 Opened 3 years ago Closed 3 years ago

Refactor LoginStore.jsm to make it reusable for Form Autofill

Categories

(Toolkit :: Form Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Iteration:
52.2 - Oct 17
Tracking Status
firefox52 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Depends on 2 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [form autofill:M1][ETA=10/14])

Attachments

(1 file)

Since we decided to store Form Autofill profiles in a simple JSON file as Password Manager does, it would be better to share the same storage IO module between these two features.
Comment on attachment 8794063 [details]
Bug 1304322 - Refactor LoginStore.jsm to make it reusable for Form Autofill;

https://reviewboard.mozilla.org/r/80674/#review79346

::: toolkit/modules/JSONStore.jsm:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please make this an `hg move` from the original file to preserve blame and make it easier to review.

::: toolkit/modules/JSONStore.jsm:89
(Diff revision 1)
> + *                       default value will be applied if omitted.
> + */
> +function JSONStore(config) {
> +  this.path = config.path;
> +
> +  if (typeof config.dataPostProcess === 'function') {

Nit: use double quotes

::: toolkit/modules/tests/xpcshell/test_JSONStore.js:1
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Please make this an `hg move` from the original file to preserve blame and make it easier to review.
Attachment #8794063 - Flags: review?(MattN+bmo)
Comment on attachment 8794063 [details]
Bug 1304322 - Refactor LoginStore.jsm to make it reusable for Form Autofill;

https://reviewboard.mozilla.org/r/80674/#review79366

::: toolkit/components/passwordmgr/moz.build:60
(Diff revision 2)
>      EXTRA_JS_MODULES += [
> -        'LoginImport.jsm',
> +        'LoginImport.jsm'
> -        'LoginStore.jsm',
>      ]

Nit: leave a trailing comma so blame doesn't change on this line

::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:136
(Diff revision 2)
>   */
>  add_task(function* test_import()
>  {
> -  let store = new LoginStore(getTempFile("test-import.json").path);
> +  let store = new JSONStore({
> +    path: getTempFile("test-import.json").path,
> +    dataPostProcess: dataPostProcess

Nit: add the trailing comma

::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:203
(Diff revision 2)
> +    dataPostProcess: dataPostProcess
> +  });

Nit: add the trailing comma

::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:243
(Diff revision 2)
> +    dataPostProcess: dataPostProcess
> +  });

Ditto

::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:262
(Diff revision 2)
> +    dataPostProcess: dataPostProcess
> +  });

Ditto

::: toolkit/modules/tests/xpcshell/test_JSONStore.js:96
(Diff revision 2)
> +    dataPostProcess: dataPostProcess
> +  });

Nit: add the trailing comma here and the rest of the changes in the file.
Whiteboard: [form autofill:M1] → [form autofill:M1][ETA=10/14]
Iteration: --- → 52.2 - Oct 17
Comment on attachment 8794063 [details]
Bug 1304322 - Refactor LoginStore.jsm to make it reusable for Form Autofill;

https://reviewboard.mozilla.org/r/80674/#review79368

::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:113
(Diff revision 2)
> +function dataPostProcess(data)
> +{
> +  if (!data.logins) {
> +    data.logins = [];
> +  }
> +
> +  if (!data.disabledHosts) {
> +    data.disabledHosts = [];
> +  }
> +



::: toolkit/components/passwordmgr/test/unit/test_module_LoginImport.js:113
(Diff revision 2)
> +function dataPostProcess(data)
> +{
> +  if (!data.logins) {
> +    data.logins = [];
> +  }
> +
> +  if (!data.disabledHosts) {
> +    data.disabledHosts = [];
> +  }
> +
> +  return data;
> +}

I don't think duplicating the post processing function in each test is a good idea as it means we're not actually testing the production function. This is why I was suggesting we keep a LoginStore.jsm module that has the post process function and but uses JSONStore.jsm.
Attachment #8794063 - Flags: review?(MattN+bmo) → review-
Hi Matt,

Do you mean we leave only post process function in LoginStore.jsm module but delegate all storage handling to JSONStore.jsm, then we can test LoginStore.jsm as usual?
Flags: needinfo?(MattN+bmo)
Comment on attachment 8794063 [details]
Bug 1304322 - Refactor LoginStore.jsm to make it reusable for Form Autofill;

https://reviewboard.mozilla.org/r/80674/#review82700

Yep, this is what I had in mind :)

One suggestion would be to replace "dataPostProcess" with "dataPostProcessor" throughout the patch as it feels more natural to me when it's passed as an argument.
Attachment #8794063 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8794063 [details]
Bug 1304322 - Refactor LoginStore.jsm to make it reusable for Form Autofill;

https://reviewboard.mozilla.org/r/80674/#review82702

::: toolkit/modules/JSONStore.jsm:1
(Diff revision 5)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

I also think "JSONFile.jsm" is a more descriptive name.
Adding dev-doc-needed for the new JSM to help with serializing JSON to disk.

Luke, would you mind sending an email to firefox-dev[1] telling other devs about this new module when it lands?

Thanks

[1] https://mail.mozilla.org/listinfo/firefox-dev
Flags: needinfo?(MattN+bmo)
Keywords: dev-doc-needed
Hi Matt,

No problem, I'll send the mail. Thanks a lot for your review.
Try looked good so I auto-landed this
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/dd5b18585fd8
Refactor LoginStore.jsm to make it reusable for Form Autofill; r=MattN
https://hg.mozilla.org/mozilla-central/rev/dd5b18585fd8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1309481
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.