Ticket #2514 (closed Bug: fixed)

Opened 6 years ago

Last modified 5 years ago

the code should try to use Array.indexOf if it does exist

Reported by: alfonsoml Owned by: arczi
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

2514.patch (435 bytes) - added by arczi 5 years ago.
2514_2.patch (1.2 KB) - added by arczi 5 years ago.
2514_3.patch (1.3 KB) - added by arczi 5 years ago.
2514_4.patch (1.3 KB) - added by arczi 5 years ago.
2514_5.patch (1.3 KB) - added by arczi 5 years ago.

Change History

comment:1 Changed 6 years ago by arczi

  • Keywords Confirmed added

Changed 5 years ago by arczi

comment:2 Changed 5 years ago by arczi

  • Keywords Review? added

comment:3 Changed 5 years ago by fredck

  • 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 follow-up: ↓ 5 Changed 5 years ago by 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

comment:5 in reply to: ↑ 4 Changed 5 years ago by fredck

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 5 years ago by arczi

comment:6 Changed 5 years ago by arczi

  • 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 5 years ago by fredck

  • Owner set to arczi
  • Keywords Review- added; Review? removed

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 5 years ago by arczi

comment:8 Changed 5 years ago by arczi

  • Keywords Review? added; Review- removed

comment:9 Changed 5 years ago by fredck

  • 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 5 years ago by arczi

comment:10 Changed 5 years ago by arczi

  • Keywords Review? added; Review- removed

Oh. yes. Now it should be ok.

comment:11 follow-up: ↓ 12 Changed 5 years ago by garry.yao

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

Changed 5 years ago by arczi

comment:12 in reply to: ↑ 11 Changed 5 years ago by arczi

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 5 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:14 Changed 5 years ago by arczi

  • Status changed from new to closed
  • Resolution set to fixed

Fixed with [3088]

comment:15 Changed 5 years ago by fredck

  • Milestone changed from CKEditor 3.x to CKEditor 3.0
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy