1434483 - Rename profileStorage singleton to formAutofillStorage to make it more clear

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Matthew N. [:MattN]]

+++ 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

Comment 1

[Daniel Marshall (:starsandspirals)]

I'm submitting a patch that changes the name in all the relevant files!

Comment 2

[Daniel Marshall (:starsandspirals)]

Attachment: Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review commit: https://reviewboard.mozilla.org/r/219294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219294/

Comment 3

[Matthew N. [:MattN]]

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.

Comment 4

[Daniel Marshall (:starsandspirals)]

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.

Comment 5

[Matthew N. [:MattN]]

(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

Comment 6

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219294/diff/1-2/

Comment 7

[Daniel Marshall (:starsandspirals)]

Thanks for the workaround; I've got all the tests passing now!

Comment 8

[Matthew N. [:MattN]]

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.

Comment 9

[Matthew N. [:MattN]]

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

Comment 10

[Matthew N. [:MattN]]

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 11

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219294/diff/2-3/

Comment 12

[Matthew N. [:MattN]]

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 13

[Matthew N. [:MattN]]

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

Comment 14

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219294/diff/3-4/

Comment 15

[Matthew N. [:MattN]]

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!

Comment 16

[Mozilla Autoland]

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)

Comment 17

[Matthew N. [:MattN]]

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.

Comment 18

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219294/diff/4-5/

Comment 19

[Daniel Marshall (:starsandspirals)]

Sorry about that, should be sorted out now!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8130e46c530
Renamed profileStorage singleton to formAutofillStorage to make it more clear; r=MattN

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4b0b23059ba
Backed out changeset a8130e46c530 for ESlint failures on browser_change_shipping.js. CLOSED TREE

Comment 22

[Matthew N. [:MattN]]

Sorry Daniel, looks like there were more changes to be made since the rebase: https://treeherder.mozilla.org/logviewer.html#?job_id=166067049&repo=autoland&lineNumber=248

Comment 23

[Daniel Marshall (:starsandspirals)]

Comment on attachment 8950015 [details]
Bug 1434483 - Renamed profileStorage singleton to formAutofillStorage to make it more clear;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219294/diff/5-6/

Comment 24

[Daniel Marshall (:starsandspirals)]

Rebased again and got all the tests passing again, so fingers crossed everything is fine this time!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca36a410083e
Renamed profileStorage singleton to formAutofillStorage to make it more clear; r=MattN

Comment 26

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ca36a410083e