Opened 10 years ago

Closed 10 years ago

#3791 closed Bug (fixed)

JSLint errors in the SCAYT plugin

Reported by: Frederico Caldeira Knabben Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.0
Component: UI : Spell Checker Version: SVN (CKEditor) - OLD
Keywords: Confirmed Review+ Cc: WebSpellChecker.net

Description

When running our jslint tool (at _dev/jslint), we can note that the scayt plugin contains several errors.

We have fixed most of the errors, but there are still several warnings there. It happens in the plugin.js file, as well as the dialog related files.

Attachments (4)

3791.patch (6.1 KB) - added by Garry Yao 10 years ago.
3791-2.patch (789 bytes) - added by WebSpellChecker.net 10 years ago.
3791_2.patch (8.0 KB) - added by Garry Yao 10 years ago.
3791_3.patch (6.9 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by Frederico Caldeira Knabben

The main problem here is that the plugin creates and make references to global variables directly. For example, the global "djScaytConfig" is created inside the loadEngine function.

These globals should not be created in this way. Preferably, we should not created global scope variables, using the CKEDITOR._ property for them. As a last the "window" object should be used to make references to them, like "window.djScaytConfig".

comment:2 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: newclosed

Fixed by [3857].

comment:3 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

The latest SCAYT changes didn't fixed this ticket yet. There are several lint errors still being thrown:

...\_source\plugins\scayt\dialogs\options.js(197): lint warning: undeclared identifier: dic
...\_source\plugins\scayt\dialogs\options.js(306): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\dialogs\options.js(332): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\dialogs\options.js(357): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\dialogs\options.js(382): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\dialogs\options.js(438): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\plugin.js(43): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\plugin.js(123): lint warning: undeclared identifier: djConfig
...\_source\plugins\scayt\plugin.js(126): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\plugin.js(166): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\plugin.js(395): lint warning: undeclared identifier: scayt
...\_source\plugins\scayt\plugin.js(474): lint warning: undeclared identifier: scayt
...\_source\plugins\uicolor\dialogs\uicolor.js(16): lint warning: undeclared identifier: YAHOO
...\_source\plugins\uicolor\dialogs\uicolor.js(45): lint warning: undeclared identifier: YAHOO
...\_source\plugins\wsc\dialogs\wsc.js(58): lint warning: undeclared identifier: _cancelOnError
...\_source\plugins\wsc\dialogs\wsc.js(81): lint warning: undeclared identifier: gFCKPluginName
...\_source\plugins\wsc\dialogs\tmpFrameset.html(27): lint warning: undeclared identifier: opener

As we can see we have several "undeclared identifier" warnings. We should have those variables even declared with "var", or, if really related to global objects, preceded by "window.".

comment:4 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: reopenednew

Changed 10 years ago by Garry Yao

Attachment: 3791.patch added

comment:5 Changed 10 years ago by Garry Yao

Keywords: Review? added
Status: newassigned

Actually it's not a global variable writing/initialization violation, it's just global variable reading and not harmful, we may consider turn off this option from our lint tools.

comment:6 in reply to:  5 Changed 10 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

Actually it's not a global variable writing/initialization violation, it's just global variable reading and not harmful

Definitely there is no violation and it's not harmful. But this is bad coding practice.

we may consider turn off this option from our lint tools.

No way! Proper variable declaration is one of the most important checks. Undeclared vars have less performance, are not compressed and may collide easily.

comment:7 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • We still have the "YAHOO" warnings. We should have "window.YAHOO" there as well.
  • The "opener" variable assignment (tmpFrameset.html line 27) is still throwing a warning. Is it supposed to be a local variable or we're assigning the window.opener value? This is a question to SpellChecker.net.

comment:8 Changed 10 years ago by WebSpellChecker.net

The "opener" variable is a global and could be defined throw "window" global object, the same as "parent" variable in this file.

Please note, that "...\_source\plugins\wsc\dialogs\tmpFrameset.html", "...\_source\plugins\wsc\dialogs\wsc.js" files are affiliates to wsc plugin, but not the scayt.

Changed 10 years ago by WebSpellChecker.net

Attachment: 3791-2.patch added

comment:9 Changed 10 years ago by WebSpellChecker.net

The last jslint warning was fixed

Changed 10 years ago by Garry Yao

Attachment: 3791_2.patch added

comment:10 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Providing a new patch based no SpellChecker.net's explaination.

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • The WSC stopped working after the patch (js error).
  • There is an extra comma being left at line 34 of _source/plugins/wsc/dialogs/tmpFrameset.html, introducing a new jslint warning.

comment:12 Changed 10 years ago by Artur Formella

Line 27 in _source/plugins/wsc/dialogs/tmpFrameset.html cannot be changed. There is no other way.

opener = window.parent;

Changed 10 years ago by Garry Yao

Attachment: 3791_3.patch added

comment:13 in reply to:  12 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

Replying to arczi:

Line 27 in _source/plugins/wsc/dialogs/tmpFrameset.html cannot be changed. There is no other way.

opener = window.parent;

You are right Artur, window.opener is unfortunately a DOM property, thanks for the tip.

comment:14 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:15 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3971].

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy