PDA

View Full Version : Ext.menu.Manager - dependencies



LesJ
13 Dec 2011, 12:19 PM
I think the Ext.menu.Manager code can be simplified a little...

"requires" can be removed (because there's no constructor) and I don't see that the "Ext.util.KeyMap" is used, so it can be removed as well.

I'm not trying to be a smart guy. This feedback is just a learning experience for me. :)


// Ext.define('Ext.menu.Manager', {
// singleton: true,
// requires: [
// 'Ext.util.MixedCollection',
// 'Ext.util.KeyMap'
// ],
// alternateClassName: 'Ext.menu.MenuMgr',

// uses: ['Ext.menu.Menu'],

Ext.define('Ext.menu.Manager', {
singleton: true,
uses: [
'Ext.util.MixedCollection',
'Ext.menu.Menu'
],
alternateClassName: 'Ext.menu.MenuMgr',

mitchellsimoens
13 Dec 2011, 8:12 PM
Ext.util.KeyMap is required as in the init method it does this:


Ext.getDoc().addKeyListener(27, function() {
if (me.active.length > 0) {
me.hideAll();
}
}, me);

Ext.menu.Manager also needs Ext.util.MixedCollection as it will build a mixed collection instance for the menus. All seems to be ok.

LesJ
15 Dec 2011, 5:49 AM
Thanks, but it's still not clear to me why there's 'requires' in this singleton.

'requires' lists classes that have to be loaded before instantiating a class, but Ext.menu.Manager has no constructor.

I think the 'requires' dependencies are loaded too early(?). These dependencies could have been loaded using 'uses'.

Nickname
15 Dec 2011, 10:04 AM
I think LesJ is right.

Ext.menu.Manager is a singleton, which calls init only if a Ext.menu.Menu gets registered. It is not needed to instantiate the singleton.

Of course there could be some some piece of code somewhere, which instantiates a "template|phantom" Ext.menu.Menu object directly document "onBeforeExtIsready" and then the MenuManager would "require" the MixedCollection.
I'm pretty sure, that AbstractContainer/AbstractPanel or even Ext.Application is called before Ext.menu.Menu Manager and MixedCollection should be available in MenuManager.

And the KeyMap Class is not used in the MenuManager.
Ext.util.KeyMap is not a singleton, rather then a Class to instantiate, which I cannot see here. The "addKeyListener" is a Ext.Element method and "27" is just a Number, which is defined/mapped to "ESC" in core/EventObject, but not used here.

KeyMap is used by Ext.Element.key.js, which is Core and is definitely loaded before MenuManager.

The easiest thing would be to test the code from LesJ, but I really don't think that this will improve the performance in any way. Maybe the source is 20bytes shorter, but nothing more!

Nickname
16 Dec 2011, 5:58 AM
A short finding on performance "optimizations" in Ext4:

Ext.element#hasCls() and Ext.element#toggleCls() uses browser native DOMClassList, because Ext.supports.ClassList is set to true on my tested browser (FF8, Cromium14 on Ubuntu 10.10) [1].
jsperf.com has a testsuite for this [2] and the "old" regex/indexOf tested way (used by browsers which doesn't support classList) is 2-3 times faster [3] than using classList#contains()

Only Safari 5.x and Android have different results. There is no performance constraint on IE[6-10] because the Trident platform does not support DOMClassList.

This single issue will not decrease the performance over the complete framework, but if there are more "optimizations" like this one it sums up to a problem.

[1] http://caniuse.com/classlist
[2] http://jsperf.com/element-classlist-vs-element-classname/4
[3] http://www.browserscope.org/user/tests/table/agt1YS1wcm9maWxlcnINCxIEVGVzdBiF68YKDA

Why I'm posting this? I still was thinking about the load time or optimization with the uses/requires and now I'm pretty sure, that there are better places to optimize than the order of loading classes.

Sorry for double post, if anyone feels bothered ;)

Animal
22 Dec 2011, 11:31 AM
I think the OP is absolutely correct.

"uses" should be used wherever possible over "requires" because "uses" does not block the class creation. It only flags that the "used" class is going to be necessary for the operation of the class being defined, so there does not need to be a block.

This is something we need to address.