Closed Bug 1242988 Opened 4 years ago Closed 4 years ago

rename "_" function in style editor


(DevTools :: Style Editor, defect, P3)



(firefox48 fixed)

Firefox 48
Tracking Status
firefox48 --- fixed


(Reporter: tromey, Assigned: esteban.escandell)



(Whiteboard: [btpp-backlog][good first bug])


(1 file, 2 obsolete files)

In bug 1240183, Patrick suggested renaming the "_" function from the style
editor to something a bit less obscure.
Priority: -- → P3
Whiteboard: [btpp-backlog][good-first-bug]
Modified Whiteboard 'good-first-bug' to 'good first bug'.
Whiteboard: [btpp-backlog][good-first-bug] → [btpp-backlog][good first bug]

I would like to work on this issue if it's possible.
This would be my first contribution here.
Sure, that'd be great. Thanks for wanting to fix this.

So the _ function is in /devtools/client/styleeditor/StyleEditorUtil.jsm (right towards the top, you should find it easily). You can use DXR to browse code, so for example:

So, the fix itself should be easy, just finding a name that makes sense and renaming all place that use it this function.

Since this is your first bug, what will take more time is to get your dev environment set up.
Please go through this guide first:
There are a lot of resources linked from this page too that you might want to check out if you're interested.

Also, don't hesitate to get in touch with us:
Assignee: nobody → esteban.escandell
Attached patch bug1242988.patch (obsolete) — Splinter Review
Thank you for your help, I hope I forgot nothing.
Attachment #8731792 - Flags: review?(pbrosset)
Comment on attachment 8731792 [details] [diff] [review]

Review of attachment 8731792 [details] [diff] [review]:

This looks good to me.
I've done a quick search in the code and I don't think you missed any spot, so this should work fine.
I just have some doubts about the function name. Since it returns a string, maybe the name should start with 'get'. So maybe 'getLocalizedString', or simpler, 'getString'? What do you think? I don't have strong feelings about this, so I'll let you make the last call.

One thing though, I noticed there's no commit message on this patch. Please add one and format it this way:
Bug <bugnumber> - <summary of what you changed>; r=<reviewer>
So, in your case:
Bug 1242988 - Replaced styleeditor's _ l10n function with getString; r=pbro

Also, we need to do a TRY build, to make sure all tests still pass and avoid regressions due to something we might have missed. Since you appear to be new to bugzilla, I'm guessing you don't have the right to push yet, so I've done it for you:

Let's wait until all test jobs are done and green.
Can you re-upload a patch with the function name changed and the commit message added in the meantime?
Attachment #8731792 - Flags: review?(pbrosset) → review+
Attached patch bug1242988.patch (obsolete) — Splinter Review
The name 'getString' sounds good to me.

This is the updated patch with the new function name and the commit message as requested.
Attachment #8731792 - Attachment is obsolete: true
Attachment #8732947 - Flags: review?(pbrosset)
Comment on attachment 8732947 [details] [diff] [review]

Review of attachment 8732947 [details] [diff] [review]:

Thanks for updating this. Looks good.
The TRY build I started yesterday has ended. Looks like all tests still pass, so no problems there. However, the ESLint job failed with the following errors:

TEST-UNEXPECTED-ERROR | devtools/client/styleeditor/StyleEditorUI.jsm:838:1 | Line 838 exceeds the maximum line length of 80. (max-len)
TEST-UNEXPECTED-ERROR | devtools/client/styleeditor/StyleSheetEditor.jsm:751:1 | Line 751 exceeds the maximum line length of 80. (max-len)
TEST-UNEXPECTED-ERROR | devtools/client/styleeditor/StyleSheetEditor.jsm:755:1 | Line 755 exceeds the maximum line length of 80. (max-len)

This means that your changes have introduced 3 new ESLint rule violations that need to be fixed before we can land this patch.
You can run ESLint locally to check this and fix it.
Find more information about it here:
Attachment #8732947 - Flags: review?(pbrosset)
Attached patch bug1242988.patchSplinter Review
My bad, I should have run these tests before sending the patch.
Now everything seems to work without warnings.
Attachment #8732947 - Attachment is obsolete: true
Attachment #8733294 - Flags: review?(pbrosset)
Comment on attachment 8733294 [details] [diff] [review]

Review of attachment 8733294 [details] [diff] [review]:

There was no code changes in the last 2 patches, so the green TRY build I ran before is still relevant, which means we can land this now!
Please add the "checkin-needed" keyword inside the keywords field at the top of this page so that someone comes in and lands this patch for you!
Attachment #8733294 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.