Opened 15 years ago

Closed 14 years ago

Last modified 14 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 14 years ago.
2514_2.patch (1.2 KB) - added by Artur Formella 14 years ago.
2514_3.patch (1.3 KB) - added by Artur Formella 14 years ago.
2514_4.patch (1.3 KB) - added by Artur Formella 14 years ago.
2514_5.patch (1.3 KB) - added by Artur Formella 14 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 15 years ago by Artur Formella

Keywords: Confirmed added

Changed 14 years ago by Artur Formella

Attachment: 2514.patch added

comment:2 Changed 14 years ago by Artur Formella

Keywords: Review? added

comment:3 Changed 14 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 14 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 14 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 14 years ago by Artur Formella

Attachment: 2514_2.patch added

comment:6 Changed 14 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 14 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 14 years ago by Artur Formella

Attachment: 2514_3.patch added

comment:8 Changed 14 years ago by Artur Formella

Keywords: Review? added; Review- removed

comment:9 Changed 14 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 14 years ago by Artur Formella

Attachment: 2514_4.patch added

comment:10 Changed 14 years ago by Artur Formella

Keywords: Review? added; Review- removed

Oh. yes. Now it should be ok.

comment:11 Changed 14 years ago by Garry Yao

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

Changed 14 years ago by Artur Formella

Attachment: 2514_5.patch added

comment:12 in reply to:  11 Changed 14 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 14 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:14 Changed 14 years ago by Artur Formella

Resolution: fixed
Status: newclosed

Fixed with [3088]

comment:15 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.xCKEditor 3.0
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy