1212171 - Cu is not defined in controller.js

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[atlanto]

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

[Marco Bonardo [:mak]]

yep, this file should use components.utils, not Cu.

Comment 2

[Marco Bonardo [:mak]]

Components.utils.

Comment 3

[Kapeel Sable]

Attachment: bug1212171.diff

Hey! This is my first Firefox bug fix. Hopefully I did everything correct.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[Kapeel Sable]

Attachment: bug1212171.diff

Thanks for the feedback. Updated the patch.

Comment 6

[Kapeel Sable]

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

[Marco Bonardo [:mak]]

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.

Comment 8

[Kapeel Sable]

Attachment: bug1212171.diff

Aligned the strings to not exceed 80 char limit.

Comment 9

[Marco Bonardo [:mak]]

thank you.
Just a reminder, next time please add the reviewer at the end of the commit message, like ". r=mak" in this case.

Comment 10

[Marco Bonardo [:mak]]

note for the sheriffs, not wasting Try server time, cause this is a simple and safe change.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c175bf2dfde6

Comment 12

[Kapeel Sable]

Ah, okay. Will do. Thanks!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c175bf2dfde6