Closed Bug 1434483 Opened 2 years ago Closed 2 years ago

Rename profileStorage singleton to formAutofillStorage to make it more clear

Categories

(Toolkit :: Form Autofill, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: MattN, Assigned: starsandspirals, Mentored)

References

()

Details

(Whiteboard: [form autofill:tech-debt][lang=js])

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #1387632 +++

The exported "profileStorage" name is too generic. It would be much more clear as FormAutofillStorage to align with the filename (after bug 1387632), prototype, and the other related JSM files.

Run tests with
> ./mach test browser/extensions/formautofill/
> ./mach test toolkit/components/payments/
> ./mach lint -w -o
I'm submitting a patch that changes the name in all the relevant files!
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Hello Daniel,

You're going in the right direction but you seemed to have missed references in toolkit/components/payments/. Did the test commands from comment 0 pass after a `./mach build`? They should have failed with the missing changes.
Attachment #8950015 - Flags: review?(MattN+bmo) → feedback+
I think I've found all the missing references and updated them.

However, whenever I run the first set of tests, an exception is raised saying that there is a missing head file in browser/extensions/formautofill/test/unit/heuristics/ when the test gets to the point of retrying tests that failed when run in parallel, so it's hard to tell if the tests are failing because of more missed references or not.

Do you know what could be causing this problem? Thanks for the help.
Flags: needinfo?(MattN+bmo)
(In reply to Daniel Marshall (:starsandspirals) from comment #4)
> I think I've found all the missing references and updated them.
> 
> However, whenever I run the first set of tests, an exception is raised
> saying that there is a missing head file in
> browser/extensions/formautofill/test/unit/heuristics/ when the test gets to
> the point of retrying tests that failed when run in parallel, so it's hard
> to tell if the tests are failing because of more missed references or not.
> 
> Do you know what could be causing this problem? Thanks for the help.

Oh, sorry, that is a known issue that I never tracked down. (Feel free to file it in the Testing::General component and CC me).

It it passes after running sequentially then that's fine.

To workaround that head file issue for autofill you can instead run these two commands:
> ./mach mochitest browser/extensions/formautofill/
> ./mach xpcshell-test browser/extensions/formautofill/

along with 

> ./mach test toolkit/components/payments/
> ./mach lint -w -o
Flags: needinfo?(MattN+bmo)
Thanks for the workaround; I've got all the tests passing now!
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review229690

Sorry for the delay. I was travelling last week. I will do a try push and mark it for checkin-needed after.
Attachment #8950015 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review229772

::: commit-message-a8e15:1
(Diff revision 2)
> +Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear; r?MattN

This is failing a test because it also requires a change:

> browser/components/migration/tests/marionette/test_refresh_firefox.py TestFirefoxRefresh.testReset
> JavascriptException: TypeError: global.profileStorage is undefined

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7589dbefd0f5ecf70b6622f6bc7a1b5919a2d224&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
Attachment #8950015 - Flags: review+ → review-
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review229776

::: commit-message-a8e15:1
(Diff revision 2)
> +Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear; r?MattN

Please also edit the entry in tools/lint/eslint/modules.json
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review230544

I did a new try push now to see if we got everything this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4691fe3fe806caf6e0308a7d1bb04b2ea66f4fa
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review230560

::: commit-message-a8e15:1
(Diff revision 3)
> +Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear; r?MattN

Sorry, looks like changes are also needed to services/sync/tps/extensions/tps/resource/modules/formautofill.jsm now
Attachment #8950015 - Flags: review?(MattN+bmo)
Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

https://reviewboard.mozilla.org/r/219294/#review230732

Sorry about all the churn and thanks for your persistence!
Attachment #8950015 - Flags: review?(MattN+bmo) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a874d1bd869a87631973bb0a564cb1a95f026b9f -d 8e02f0924908: rebasing 450336:a874d1bd869a "Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear; r=MattN" (tip)
merging browser/extensions/formautofill/FormAutofillParent.jsm
merging browser/extensions/formautofill/FormAutofillStorage.jsm
merging browser/extensions/formautofill/FormAutofillSync.jsm
merging toolkit/components/payments/content/paymentDialogWrapper.js
merging toolkit/components/payments/test/browser/browser_profile_storage.js
merging toolkit/components/payments/test/browser/browser_show_dialog.js
merging toolkit/components/payments/test/browser/head.js
merging tools/lint/eslint/modules.json
warning: conflicts while merging browser/extensions/formautofill/FormAutofillStorage.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Would you mind rebasing? If you have your commit applied you can `hg pull --rebase` to pull and rebase otherwise use `hg rebase` directly after pulling.
Assignee: nobody → daniel
Status: NEW → ASSIGNED
Flags: needinfo?(daniel)
Sorry about that, should be sorted out now!
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a8130e46c530
Renamed profileStorage singleton to formAutofillStorage to make it more clear; r=MattN
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a4b0b23059ba
Backed out changeset a8130e46c530 for ESlint failures on browser_change_shipping.js. CLOSED TREE
Rebased again and got all the tests passing again, so fingers crossed everything is fine this time!
Flags: needinfo?(daniel)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/ca36a410083e
Renamed profileStorage singleton to formAutofillStorage to make it more clear; r=MattN
https://hg.mozilla.org/mozilla-central/rev/ca36a410083e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.