Opened 16 years ago

Closed 15 years ago

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

Download all attachments as: .zip

Change History (20)

comment:1 Changed 16 years ago by Artur Formella

Keywords: Confirmed added

Changed 15 years ago by Artur Formella

Attachment: 2514.patch added

comment:2 Changed 15 years ago by Artur Formella

Keywords: Review? added

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

Attachment: 2514_2.patch added

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

Attachment: 2514_3.patch added

comment:8 Changed 15 years ago by Artur Formella

Keywords: Review? added; Review- removed

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

Attachment: 2514_4.patch added

comment:10 Changed 15 years ago by Artur Formella

Keywords: Review? added; Review- removed

Oh. yes. Now it should be ok.

comment:11 Changed 15 years ago by Garry Yao

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

Changed 15 years ago by Artur Formella

Attachment: 2514_5.patch added

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

Keywords: Review+ added; Review? removed

comment:14 Changed 15 years ago by Artur Formella

Resolution: fixed
Status: newclosed

Fixed with [3088]

comment:15 Changed 15 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