Rename ProfileStorage.jsm to FormAutofillStorage.jsm to make it more clear

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: MattN, Assigned: ninjaprawn, Mentored)

Tracking

Trunk
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [form autofill:tech-debt], URL)

Attachments

(1 attachment, 4 obsolete attachments)

The "ProfileStorage" name is too generic. It would be much more clear as FormAutofillStorage.jsm to align with the other related JSM files.
I am submitting a Patch by renaming the files and the occurrences soon :)
Assignee: nobody → hossainalikram
Status: NEW → ASSIGNED
Posted patch Renamed-Files.patch (obsolete) — Splinter Review
Please review and let me know if I should modify something more :)
Attachment #8894105 - Flags: review?(MattN+bmo)
Comment on attachment 8894105 [details] [diff] [review]
Renamed-Files.patch

Review of attachment 8894105 [details] [diff] [review]:
-----------------------------------------------------------------

Hello,

This is on the right track but the patch is missing the actual move of the JSM file. You can run the following to do the move:
> hg mv browser/extensions/formautofill/ProfileStorage.jsm browser/extensions/formautofill/FormAutofillStorage.jsm

We should also rename the exported symbol from the module to be `formAutofillStorage` instead of `profileStorage` and rename the "ProfileStorage" object too.

Thanks
Attachment #8894105 - Flags: review?(MattN+bmo) → feedback+
Posted patch Updatedpatch.patch (obsolete) — Splinter Review
Patch Updated. Let me know, if this is okay or anything should be modified :)
Attachment #8894105 - Attachment is obsolete: true
Attachment #8895267 - Flags: review?(MattN+bmo)
Comment on attachment 8895267 [details] [diff] [review]
Updatedpatch.patch

Review of attachment 8895267 [details] [diff] [review]:
-----------------------------------------------------------------

Hello,

The patch still seems to be missing the JSM that you're renaming: ProfileStorage.jsm/FormAutofillStorage.jsm
Attachment #8895267 - Flags: review?(MattN+bmo) → feedback+
Posted patch new-patch.patch (obsolete) — Splinter Review
Not sure, why this is happening, Sorry for the troubles. Please check this one and let me know about this one.
Attachment #8895267 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8896026 - Flags: review?(MattN+bmo)
Comment on attachment 8896026 [details] [diff] [review]
new-patch.patch

Review of attachment 8896026 [details] [diff] [review]:
-----------------------------------------------------------------

> diff --git a/browser/extensions/formautofill/ProfileStorage.jsm b/browser/extensions/formautofill/FormAutofillStorage.jsm

I do see the rename this time but you didn't change the EXPORTED_SYMBOLS it seems. Are you using Git or Mercurial?
Attachment #8896026 - Flags: review?(MattN+bmo) → feedback+
Flags: needinfo?(MattN+bmo)
Hey Matthew,

I am using mercurial but I am not sure how do I change the EXPORTED_SYMBOLS?
Flags: needinfo?(MattN+bmo)
It's just a variable inside ProfileStorage.jsm
Flags: needinfo?(MattN+bmo)
Posted patch latest.patch (obsolete) — Splinter Review
Please, let me know, if this change is correct or not :)
Attachment #8896026 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8904703 - Flags: review?(MattN+bmo)
Comment on attachment 8904703 [details] [diff] [review]
latest.patch

Review of attachment 8904703 [details] [diff] [review]:
-----------------------------------------------------------------

Hello,

You don't need to needinfo and set a review flag since the review flag is sufficient to request review.

::: browser/extensions/formautofill/FormAutofillStorage.jsm
@@ +103,4 @@
>  
>  // We expose a singleton from this module. Some tests may import the
>  // constructor via a backstage pass.
> +this.EXPORTED_SYMBOLS = ["FormAutofillStorage"];

Hello, I guess you didn't run the tests[1] since this name needs to match the global to be exported from the file but you didn't rename the variable below.

[1] `mach test browser/extensions/formautofill/`
Attachment #8904703 - Flags: review?(MattN+bmo) → feedback+
Flags: needinfo?(MattN+bmo)
Component: Form Manager → Form Autofill
(Assignee)

Comment 12

a year ago
Posted patch 1387632.patchSplinter Review
Attachment #8944583 - Flags: review?(MattN+bmo)
Attachment #8904703 - Attachment is obsolete: true
Assignee: hossainalikram → ninjaprawn
Comment on attachment 8944583 [details] [diff] [review]
1387632.patch

Review of attachment 8944583 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I was originally thinking we would rename the exported symbol tool but I'll file a new bug for that.
Attachment #8944583 - Flags: review?(MattN+bmo) → review+

Comment 14

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26403995065c
Rename ProfileStorage module to FormAutofillStorage. r=MattN

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26403995065c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.