Closed
Bug 1212171
Opened 9 years ago
Closed 9 years ago
Cu is not defined in controller.js
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: euthanasia_waltz, Assigned: kapeels42, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 2 obsolete files)
4.28 KB,
patch
|
Details | Diff | Splinter Review |
When added New Separator in Bookmarks sidebar, browser console shows following messages.
>ReferenceError: Cu is not defined controller.js:273:7
>NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "Cu is not defined" {file: "chrome://browser/content/places/controller.js" line: 273}]'[JavaScript Error: "Cu is not defined" {file: "chrome://browser/content/places/controller.js" line: 273}]' when calling method: [nsIController::doCommand] controller.js:1724:0
Comment 1•9 years ago
|
||
yep, this file should use components.utils, not Cu.
Mentor: mak77
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=js]
Comment 2•9 years ago
|
||
Components.utils.
Assignee | ||
Comment 3•9 years ago
|
||
Hey! This is my first Firefox bug fix. Hopefully I did everything correct.
Attachment #8670787 -
Flags: review?(mak77)
Comment 4•9 years ago
|
||
Comment on attachment 8670787 [details] [diff] [review] bug1212171.diff Review of attachment 8670787 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/controller.js @@ -269,5 @@ > case "placesCmd_new:bookmark": > this.newItem("bookmark"); > break; > case "placesCmd_new:separator": > - this.newSeparator().catch(Cu.reportError); This is fine, though there is another Cu. usage at http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#1312
Attachment #8670787 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 5•9 years ago
|
||
Thanks for the feedback. Updated the patch.
Attachment #8670855 -
Flags: review?(mak77)
Assignee | ||
Comment 6•9 years ago
|
||
Wondering if I need to follow the style guide about the line length limit being 80. I see it violated here for similar code: http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#1628
Comment 7•9 years ago
|
||
Comment on attachment 8670855 [details] [diff] [review] bug1212171.diff Review of attachment 8670855 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following small indentation fix. ::: browser/components/places/content/controller.js @@ +1307,1 @@ > "reverting to a copy operation."); please align the strings like Components.utils.reportError("first" + "second"); The 80 chars limit should usually be respected, but exceptions are accepted when respecting it would make the code ugly or harder to read.
Attachment #8670855 -
Flags: review?(mak77) → review+
Updated•9 years ago
|
Assignee: nobody → kapeels42
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•9 years ago
|
||
Aligned the strings to not exceed 80 char limit.
Attachment #8670787 -
Attachment is obsolete: true
Attachment #8670855 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
thank you. Just a reminder, next time please add the reviewer at the end of the commit message, like ". r=mak" in this case.
Keywords: checkin-needed
Comment 10•9 years ago
|
||
note for the sheriffs, not wasting Try server time, cause this is a simple and safe change.
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/c175bf2dfde6
Keywords: checkin-needed
Assignee | ||
Comment 12•9 years ago
|
||
Ah, okay. Will do. Thanks!
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c175bf2dfde6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in
before you can comment on or make changes to this bug.
Description
•