281445 - In <editor.js>, "Warning: redeclaration of var editor"

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b) Gecko/20050207] (nightly) (W98SE)

{{
Warning: redeclaration of var editor
Source File: chrome://editor/content/editor.js
Line: 199, Column: 10
Source Code:
      var editor = GetCurrentEditor();
}}

{{
 164 jst        1.233     
 165                      if (prefName.substr(0, kUseCssPref.length) == kUseCssPref)
 166                      {
 167                        var cmd = document.getElementById("cmd_highlight");
 168                        if (cmd) {
 169                          var prefs = GetPrefs();
 170                          var useCSS = prefs.getBoolPref(prefName);
 171                          var editor = GetCurrentEditor();

 199 cmanske    1.193       var editor = GetCurrentEditor();
}}

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-Bw) <editor.js>

Possible additional cleanup:
I wonder if "all" |GetPrefs()| and |var prefs| could be replaced by |gPrefs| !?

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 173689 [details] [diff] [review]
(Av1-Bw) <editor.js>

Neil:
Could you have a look at my previous comment(s), and possibly take r? and sr? ?
(This patch still applies cleanly))

Comment 3

[[e-mail]]

Comment on attachment 173689 [details] [diff] [review]
(Av1-Bw) <editor.js>

>+        var useCSS = GetPrefs().getBoolPref(prefName);
Yes, this should probably use gPrefs. (I managed to use both in one patch, and you've now made me feel really stupid, but fortunately my reviewer didn't spot it either, so I'm not the only one!)

>+    else if (editor && (prefName.substr(0, kCRInParagraphsPref.length) == kCRInParagraphsPref))
This doesn't need to be substr; that was only necessary for the button preferences because editor watches all the buttons at once. Here we're only watching one pref.

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2-Bw) <editor.js>

Av1-Bw, with comment 3 suggestion(s).

I kept the 3 |substr()| related to buttons, and another one...

(untested and not for checkin)

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <editor.js> [Checked in: Comment 6]

Same as Av2-Bw, with full diff, for checkin.

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 216779 [details] [diff] [review]
(Av2) <editor.js>
[Checked in: Comment 6]


Checkin: { 2006-04-04 00:59	neil%parkwaycc.co.uk 	mozilla/editor/ui/composer/content/editor.js 	1.238 }

'approval-branch-1.8.1=?':
Trivial JS code cleanup, no risk.

Comment 7

[Mike Schroepfer]

Not clear why this is needed on the 1.8 branch since it is just code cleanup.  Let us know if there is a functional reason you want it there.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Not clear why this is needed on the 1.8 branch since it is just code cleanup. 
> Let us know if there is a functional reason you want it there.

To make it short, it's more a matter of synchronization/optimization than of need/functionality.
Yet, what would be the reason not to have it on 1.8.1 !? (1.8.0 being left alone)