#2514 closed Bug (fixed)
the code should try to use Array.indexOf if it does exist
Reported by: | Alfonso Martínez de Lizarrondo | Owned by: | Artur Formella |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.0 |
Component: | General | Version: | SVN (FCKeditor) - Retired |
Keywords: | Confirmed Review+ | Cc: |
Description
see http://developer.mozilla.org/En/Core_JavaScript_1.5_Reference:Objects:Array:indexOf and http://www.prototypejs.org/api/array/indexof
The function in CKEDITOR.tools should be adjusted to call the native implementation if it is detected.
Attachments (5)
Change History (20)
comment:1 Changed 16 years ago by
Keywords: | Confirmed added |
---|
Changed 16 years ago by
Attachment: | 2514.patch added |
---|
comment:2 Changed 16 years ago by
Keywords: | Review? added |
---|
comment:3 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
comment:4 follow-up: 5 Changed 16 years ago by
Also, my idea was that instead of checking how to execute it everytime, at initialization time the detection was executed on Array only once, similar to other methods in the tools.js
comment:5 Changed 16 years ago by
Replying to alfonsoml:
Also, my idea was that instead of checking how to execute it everytime, at initialization time the detection was executed on Array only once, similar to other methods in the tools.js
Good point... that would be even better.
Changed 16 years ago by
Attachment: | 2514_2.patch added |
---|
comment:6 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
In Google Chrome there is no Array.indexOf() but [].indexOf() works. What do you think about this code?
if ( Array.indexOf || [].indexOf ) {
comment:7 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|---|
Owner: | set to Artur Formella |
Being this executed once only, when loading the code, it would be acceptable to have the ( [].indexOf )
check only (please add a comment about it, so we can understand the reason why). We don't need the Array.indexOf check then.
You can also use a conditional operator, instead of a private function to simplify the code. Something like this:
indexOf : [].indexOf ? function( array, entry ) { ... } : function( array, entry ) { ... }
Check CKEDITOR.dom.element.getComputedStyle for an example.
Changed 16 years ago by
Attachment: | 2514_3.patch added |
---|
comment:8 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
comment:9 Changed 16 years ago by
Keywords: | Review- added; Review? removed |
---|
We are almost there...
return index = array.indexOf( entry );
I think that's "index" var has nothing to do here. It should be simply:
return array.indexOf( entry );
Changed 16 years ago by
Attachment: | 2514_4.patch added |
---|
comment:10 Changed 16 years ago by
Keywords: | Review? added; Review- removed |
---|
Oh. yes. Now it should be ok.
comment:11 follow-up: 12 Changed 16 years ago by
My idea is if we can use Array.prototype.indexOf for feature detection.
Changed 16 years ago by
Attachment: | 2514_5.patch added |
---|
comment:12 Changed 16 years ago by
Replying to garry.yao:
My idea is if we can use Array.prototype.indexOf for feature detection.
It works in Opera, FF2, FF3, IE7, Chrome and Safari.
comment:13 Changed 16 years ago by
Keywords: | Review+ added; Review? removed |
---|
comment:15 Changed 16 years ago by
Milestone: | CKEditor 3.x → CKEditor 3.0 |
---|
if( array.indexOf )
. Less code and better performance (no string comparison).