Closed
Bug 1304322
Opened 8 years ago
Closed 8 years ago
Refactor LoginStore.jsm to make it reusable for Form Autofill
Categories
(Toolkit :: Form Manager, defect, P3)
Toolkit
Form Manager
Tracking
()
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: lchang, Assigned: lchang)
References
(Depends on 1 open bug)
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Running tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=974dd89e1bbf
Comment 3•8 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 5•8 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Whiteboard: [form autofill:M1] → [form autofill:M1][ETA=10/14]
Iteration: --- → 52.2 - Oct 17
Comment 7•8 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 8•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
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 12•8 years ago
|
||
mozreview-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.
Comment 13•8 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
Hi Matt, No problem, I'll send the mail. Thanks a lot for your review.
Comment 16•8 years ago
|
||
Try looked good so I auto-landed this
Comment 17•8 years ago
|
||
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
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd5b18585fd8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1309500
Depends on: 1309501
Updated•7 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•