Closed Bug 1415951 Opened 2 years ago Closed 2 years ago

Extend TPS to support Addresses and Credit Cards

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: tcsc, Assigned: eoger)

Details

(Whiteboard: [form autofill])

Attachments

(1 file)

TPS currently can't test either engine from the formautofill extension, but it would be nice if it could! 

This isn't a trivial amount of work, but it also shouldn't be *that* bad.
Priority: -- → P2
Whiteboard: [form autofill]
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8948583 [details]
Bug 1415951 - Extend TPS to support Addresses and Credit Cards.

https://reviewboard.mozilla.org/r/218002/#review224598

Thanks for this!

::: services/sync/tests/tps/test_creditcards.js:54
(Diff revision 1)
> +  [Sync]
> +]);
> +
> +Phase("phase3", [
> +  [Sync],
> +  [CreditCards.verify, cc1_after],

Do we pass `[CreditCards.verifyNot, cc1]` if you add it as well? Just curious, since IME I have a lot of duplicates.

::: services/sync/tests/unit/sync_ping_schema.json:81
(Diff revision 1)
>      "engine": {
>        "required": ["name"],
>        "additionalProperties": false,
>        "properties": {
>          "failureReason": { "$ref": "#/definitions/error" },
> -        "name": { "enum": ["addons", "bookmarks", "clients", "forms", "history", "passwords", "prefs", "tabs", "extension-storage"] },
> +        "name": { "enum": ["addons", "addresses", "bookmarks", "clients", "creditcards", "forms", "history", "passwords", "prefs", "tabs", "extension-storage"] },

TBH `"name": { "type": "string" }` would probably be fine. Not sure this validation gives us anything, especially since android blatantly disregards this rule and submits a bunch of engines that represent different parts of the sync that are clearly not engines.

::: services/sync/tps/extensions/tps/resource/modules/formautofill.jsm:18
(Diff revision 1)
> +
> +ChromeUtils.import("resource://tps/logger.jsm");
> +ChromeUtils.import("resource://formautofill/FormAutofillStorage.jsm");
> +ChromeUtils.import("resource://formautofill/MasterPassword.jsm");
> +
> +function FormAutofillBase(props) {

Nit: Use ES6 class for FormAutofillBase and subclasses? (unless you can think of a good reason not to -- AFAICT you no longer have to do the `var Blah = this.Blah = class Blah {}` nonsense to export without eslint complaining)
Attachment #8948583 - Flags: review?(tchiovoloni) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e651c7ceea32
Extend TPS to support Addresses and Credit Cards. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/e651c7ceea32
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.