Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#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)

2514.patch (435 bytes) - added by Artur Formella 9 years ago.
2514_2.patch (1.2 KB) - added by Artur Formella 9 years ago.
2514_3.patch (1.3 KB) - added by Artur Formella 9 years ago.
2514_4.patch (1.3 KB) - added by Artur Formella 9 years ago.
2514_5.patch (1.3 KB) - added by Artur Formella 9 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by Artur Formella

Keywords: Confirmed added

Changed 9 years ago by Artur Formella

Attachment: 2514.patch added

comment:2 Changed 9 years ago by Artur Formella

Keywords: Review? added

comment:3 Changed 9 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  1. There is no need to do a typeof check. You can do if( array.indexOf ). Less code and better performance (no string comparison).
  1. If we'll be using the browser implementation, we need to make our custom implementation work in the same way as the specifications. It means that strict equality (===) checks must be used.

comment:4 Changed 9 years ago by Alfonso Martínez de Lizarrondo

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 in reply to:  4 Changed 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Artur Formella

Attachment: 2514_2.patch added

comment:6 Changed 9 years ago by Artur Formella

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 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Artur Formella

Attachment: 2514_3.patch added

comment:8 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:9 Changed 9 years ago by Frederico Caldeira Knabben

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 9 years ago by Artur Formella

Attachment: 2514_4.patch added

comment:10 Changed 9 years ago by Artur Formella

Keywords: Review? added; Review- removed

Oh. yes. Now it should be ok.

comment:11 Changed 9 years ago by Garry Yao

My idea is if we can use Array.prototype.indexOf for feature detection.

Changed 9 years ago by Artur Formella

Attachment: 2514_5.patch added

comment:12 in reply to:  11 Changed 9 years ago by Artur Formella

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 9 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:14 Changed 9 years ago by Artur Formella

Resolution: fixed
Status: newclosed

Fixed with [3088]

comment:15 Changed 9 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.xCKEditor 3.0
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy