Closed Bug 1212171 Opened 9 years ago Closed 9 years ago

Cu is not defined in controller.js

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
minor

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)

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
yep, this file should use components.utils, not Cu.
Mentor: mak77
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=js]
Components.utils.
Attached patch bug1212171.diff (obsolete) — Splinter Review
Hey! This is my first Firefox bug fix. Hopefully I did everything correct.
Attachment #8670787 - Flags: review?(mak77)
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+
Attached patch bug1212171.diff (obsolete) — Splinter Review
Thanks for the feedback. Updated the patch.
Attachment #8670855 - Flags: review?(mak77)
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 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+
Assignee: nobody → kapeels42
Status: NEW → ASSIGNED
Attached patch bug1212171.diffSplinter Review
Aligned the strings to not exceed 80 char limit.
Attachment #8670787 - Attachment is obsolete: true
Attachment #8670855 - Attachment is obsolete: true
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
note for the sheriffs, not wasting Try server time, cause this is a simple and safe change.
Ah, okay. Will do. Thanks!
https://hg.mozilla.org/mozilla-central/rev/c175bf2dfde6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
You need to log in before you can comment on or make changes to this bug.