Closed Bug 1480925 Opened 6 years ago Closed 4 years ago

remove const Ci.nsIWhatever pattern from Javascript modules in PSM

Categories

(Core :: Security: PSM, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dipen, Assigned: carolina.jimenez.g)

Details

(Whiteboard: [psm-cleanup])

Attachments

(12 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704003137

Steps to reproduce:

Patters such as these are prevalent in Javascript code in PSM.

const nsIWhatever = Ci.nsIWhatever
const nsWhatever = "mozilla.org/whatever;1" 

This is actually an anti-pattern and should just use Ci.nsIWhatever and "mozilla.org/whatever;1" directly.
Priority: -- → P3
Whiteboard: [psm-cleanup]
Assignee: nobody → carolina.jimenez.g
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

I would also like to know if I can run something to check anything is broken

Hello Dana, On Tuesday I wrote some emails to the persons listed here https://wiki.mozilla.org/Modules/All and only 2 of them answered (for the moment)... I need someone to review the changes on the browser, accessible, extensions, mobile, testing and toolkit modules... I don't know where else can I reach them.

Also, the changes on Phabricator are only like the half of what I wanted to fix, but I didn't push more of them because the revision is already too large. Would you like me to fix the rest and put it on the same revision link? Should I do it in another revision? Or shouldn't I finish the fix?

Thank you

Flags: needinfo?(dkeeler)

I would give people a little bit more time to respond, and then we'll see what we can do. Also, for changes that affect many parts of the tree, sometimes it can be easier to either break up the patch into multiple patches that each address one module (so there's only one reviewer per patch) or to even have one bug per module. The nice thing about these changes is they don't have to land all at once - you can fix one module more or less independently of the others (in fact, when I asked Dipen to file this bug, I was really only intending to fix this for files in security/ ). Long story short, I think it would be fine to amend the current patch to only include changes that have been reviewed as of now and file follow-up bugs to address the rest of the changes to be made (each in the appropriate component) (if you decide to go this route, we should also file a meta-bug that tracks each of the other bugs).

Flags: needinfo?(dkeeler)

Hello Danna, I split some of my commits, did rebase, and now I left in the revission D25686 only the files related with security, layout and devtools. I think I didn't break anything.

I will upload the rest I had for each module in differents revissions, all of them "tagged" with this bug.

Bug 1480925 - Removes more anti-patterns related with toolkit/

Bug 1480925 - Removes some eslint problems in toolkit/

Bug 1480925 - Removes more anti-patterns related with browser/

Bug 1480925 - Removes some eslint problems in browser/

Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fefc86a2f630
Removes anti-patterns related with Ci.nsIWhatever. r=keeler,yzen
Assignee: carolina.jimenez.g → nobody
Status: ASSIGNED → NEW

The leave-open keyword is there and there is no activity for 6 months.
:keeler, maybe it's time to close this bug?

Flags: needinfo?(dkeeler)

Let's limit the scope of this back to PSM and call it fixed.

Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(dkeeler)
Resolution: --- → FIXED
Summary: remove const Ci.nsIWhatever pattern from Javascript modules → remove const Ci.nsIWhatever pattern from Javascript modules in PSM
Assignee: nobody → carolina.jimenez.g
You need to log in before you can comment on or make changes to this bug.