Inefficient selectors in common.dev.js

[GamajoTech]

There are some repeated jQuery selector lookups that could be cached, and incorrect attempts at adding contexts in common.dev.js.

Some minor optimisations that sacrifices a little bit of code readability, for a double speed improvement of caching expensive jQuery selector lookups, and reducing the size of common.js by ~300 bytes (5%) when minifying.

[GamajoTech]

Minor performance optimisations

[scribu]

We should make a JavaScript guideline page in the codex with the rule that jQuery collection variables start with a '$'. This is common practice.

In your patch, this would mean $body instead of body etc.

[azaozz]

These two do the same thing:

$('.wp-menu-toggle', menu)...

menu.find('.wp-menu-toggle')...

They limit the search for the .wp-menu-toggle class only to children of menu. Did some testing few months ago and the first one was slightly faster.

It can also be written as:

$('li.wp-has-submenu', '#adminmenu')...

i.e. find li nodes that have wp-has-submenu class but search only in the node with id = adminmenu.

[nacin]

A few of these make sense. But please see azaozz's comments.

Also, we use tabs, not spaces. If you can correct the patch, it would be more readable.

Optimizations like wpHasSubmenu aren't necessary, as it's only used once (referenced twice, but there's an if/else there).

[GamajoTech]

Replying to scribu:

We should make a JavaScript guideline page in the codex with the rule that jQuery collection variables start with a '$'. This is common practice.

In your patch, this would mean $body instead of body etc.

I completely agree, but I couldn't find any agreed guidelines about which preference WP code should use.

[GamajoTech]

Replying to azaozz:

These two do the same thing:

$('.wp-menu-toggle', menu)...

menu.find('.wp-menu-toggle')...

They limit the search for the .wp-menu-toggle class only to children of menu.

Agreed.

Did some testing few months ago and the first one was slightly faster.

As the first is converted internally to use the second, I find your results interesting. Could you share your test code?

From ​http://api.jquery.com/jQuery/

Internally, selector context is implemented with the .find() method, so $('span', this) is equivalent to $(this).find('span').

I see no reason not to jump straight to using the find() method for the optimizations I've listed.

It can also be written as:

$('li.wp-has-submenu', '#adminmenu')...

i.e. find li nodes that have wp-has-submenu class but search only in the node with id = adminmenu.

Actually, no it can't. Again from ​http://api.jquery.com/jQuery/ the context is:

A DOM Element, Document, or jQuery to use as context

A string doesn't cut it; the context would still be "document", and not "#adminmenu".

Further information at ​http://brandonaaron.net/blog/2009/06/24/understanding-the-context-in-jquery

[GamajoTech]

Replying to nacin:

A few of these make sense. But please see azaozz's comments.

Also, we use tabs, not spaces. If you can correct the patch, it would be more readable.

Optimizations like wpHasSubmenu aren't necessary, as it's only used once (referenced twice, but there's an if/else there).

All noted. I'll re-do the patch.

[nacin]

We don't enforce or typically use the $body style.

[Denis-de-Bernardy]

@GamajoTech: see #10021

[GamajoTech]

Switched from spaces to tabs. Also expanded on abbreviated variables (no real reason to keep abbreviations in a .dev.js as the variables get renamed upon minification anyway). Sorted a few spacing issues, but there are still plenty of inconsistencies (waiting on core contributor handbook).

[ocean90]

Most things from common.dev.js.2.patch​ are implemented. Please open new tickets for further improvements.