Closed
Bug 1242988
Opened 8 years ago
Closed 8 years ago
rename "_" function in style editor
Categories
(DevTools :: Style Editor, defect, P3)
DevTools
Style Editor
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: tromey, Assigned: esteban.escandell)
References
Details
(Whiteboard: [btpp-backlog][good first bug])
Attachments
(1 file, 2 obsolete files)
5.40 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
In bug 1240183, Patrick suggested renaming the "_" function from the style editor to something a bit less obscure.
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: [btpp-backlog][good-first-bug]
Comment 1•8 years ago
|
||
Modified Whiteboard 'good-first-bug' to 'good first bug'. See https://wiki.mozilla.org/Good_first_bug
Whiteboard: [btpp-backlog][good-first-bug] → [btpp-backlog][good first bug]
Assignee | ||
Comment 2•8 years ago
|
||
Hello, I would like to work on this issue if it's possible. This would be my first contribution here.
Comment 3•8 years ago
|
||
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: https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUtil.jsm#39 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: https://wiki.mozilla.org/DevTools/Hacking 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: https://wiki.mozilla.org/DevTools/GetInvolved#Communication
Assignee: nobody → esteban.escandell
Assignee | ||
Comment 4•8 years ago
|
||
Thank you for your help, I hope I forgot nothing.
Attachment #8731792 -
Flags: review?(pbrosset)
Comment 5•8 years ago
|
||
Comment on attachment 8731792 [details] [diff] [review] bug1242988.patch 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: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ad1d9497623 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+
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
Comment on attachment 8732947 [details] [diff] [review] bug1242988.patch 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: https://wiki.mozilla.org/DevTools/CodingStandards
Attachment #8732947 -
Flags: review?(pbrosset)
Assignee | ||
Comment 8•8 years ago
|
||
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 9•8 years ago
|
||
Comment on attachment 8733294 [details] [diff] [review] bug1242988.patch Review of attachment 8733294 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! 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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6bb46bf9e115
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6bb46bf9e115
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•