Replace $('selector', domelement) with $(domelement).find('selector')

For issue purpose, most of this patch looks good to me. (I've not do real test)

+++ b/core/misc/tabledrag.jsundefined
@@ -385,9 +393,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr');
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr');
+          nextRow = $nextRow.get(0);

not same ??

while (nextRow && $nextRow.is(':hidden')) {

should be

while (nextRow && nextRow.is(':hidden')) {

+++ b/core/misc/tabledrag.jsundefined
@@ -631,8 +643,9 @@ Drupal.tableDrag.prototype.findDropTargetRow = function (x, y) {
-      while ($(row).is(':hidden') && $(row).prev('tr').is(':hidden')) {
-        row = $(row).prev('tr').get(0);
+      while ($row.is(':hidden') && $row.prev('tr').is(':hidden')) {
+        $row = $row.prev('tr');
+        row = $row.get(0);

why copy again? it returned "return row;" and not use $row anymore.

-28 days to next Drupal core point release.

The base/reasoning for this issue seems to be frontend performance.

However, looking at the referenced jQuery core lines, that code almost looks like a no-op to me.

As in PHP, the cost of a mere function call is almost unmeasurable in JS either.

So before we're going to do this (which in itself would imply kind of a new JS coding standard), and break many patches in the queue, I'd like to see that there is actually a measurable performance difference.

It's pattern is widely use in jQuery world:
http://addyosmani.com/jqprovenperformance/

$('selector').find() always better than context

It's the worst case:
http://jsperf.com/drupal-dom-selector/6

It looks like this can be backported to D7?

This needs manual testing, though not really sure how to approach that.

@nod_,

both you & me are wrong. I made a wrong suggestion on #1

+++ b/core/misc/tabledrag.jsundefined
@@ -385,9 +393,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr');
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr');
+          nextRow = $nextRow.get(0);

If we simplify the changes, it will looks like this:
http://jsfiddle.net/wk4Qe/

Orig: check on first matched element (FALSE)
Patched: check on all matched element (TRUE)

Reviewed code, and randomly do tests. It's worked.

http://jsperf.com/drupal-dom-selector/6 looks good to me there's a regression in opera but a decent improvement in every other browser.

However I agree this needs at least some manual testing, and there's no sign of that here.

Tagging as novice for the tabledrag testing.

Looks find to me.

Following can be a separated issue (remark)

+++ b/core/misc/tabledrag.jsundefined
@@ -345,9 +352,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
+        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
+        var previousRow = $previousRow.get(0);
+        while (previousRow && $previousRow.is(':hidden')) {

Few places have above pattern.

$previousRow.get(0);

equivalent to

$previousRow[0];

http://jsfiddle.net/kyJzh/
http://jsperf.com/get-0-vs-0

12 days to next Drupal core point release.

can I make it as RTBC ?

found some more instances of this issue

in ajax.js line 377,
was: $('.throbber', this.progress.element).after('<div class="message">' + this.progress.message + '</div>');
new: this.progress.element.find('.throbber').after('<div class="message">' + this.progress.message + '</div>');

in states.js line 530,
was: $('> legend a', e.target).click();
new: $(e.target).find$('> legend a').click();

in tabledrag.js, line 242,
was: var field = $('.' + group, row);
new: var field = $(row).find('.' + group);

in block.js, line 95,
was: if ($('option[value=' + regionName + ']', regionField).length == 0) {
new: if ($(regionField).find('option[value=' + regionName + ']').length == 0) {

and line 149,
was: $('tr.region-message', table).each(function () {
new: $(table).find('tr.region-message').each(function () {

in comment.js, line 12:
was: return Drupal.checkPlain($('.form-item-comment input:checked', context).next('label').text());
new: return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());

in field_ui.js, many changes:
(see patch)

in menu.js, line 5:
was: $('fieldset.menu-link-form', context).drupalSetSummary(function (context) {
new: $(context).find('fieldset.menu-link-form').drupalSetSummary(function (context) {

line 6:
was: if ($('.form-item-menu-enabled input', context).is(':checked')) {
new: if ($(context).find('.form-item-menu-enabled input').is(':checked')) {

line 7:
was: return Drupal.checkPlain($('.form-item-menu-link-title input', context).val());
new: return Drupal.checkPlain($(context).find('.form-item-menu-link-title input').val());

in user.permissions.js, line 33
was: $('input[type=checkbox]', this).not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {
new: $(this).find('input[type=checkbox]').not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {

line 38:
was: $('input[type=checkbox].rid-2', this)
new: $(this).find('input[type=checkbox].rid-2')

#28 updates reviewed. Looks good.

@cosmicdreams,
do you review other parts ? we hope one more more people review this thread :)

@droplet, I do as much as I can.

I'll manually test this patch today if I have time. I'll go change-by-change this evening.

#31: core-js-select-with-find-1419968-30.patch queued for re-testing.

git apply core-js-select-with-find-1419968-37.patch reports:

core-js-select-with-find-1419968-37.patch:146: trailing whitespace.
(function (source_id, options) {
core-js-select-with-find-1419968-37.patch:1516: trailing whitespace.

warning: 2 lines add whitespace errors.

This patch touches a lot of code (by design). I'm unsure how to test it all in the browser. Is eyeball review of the js the only possible method?

+++ b/core/misc/machine-name.js
@@ -28,10 +28,11 @@ Drupal.behaviors.machineName = {
+        (function (source_id, options) {      ¶

trailing whitespace

+++ b/core/misc/tabledrag.js
@@ -704,13 +722,16 @@ Drupal.tableDrag.prototype.updateField = function (changedRow, group) {
+    // TODO clean that up, It wasn't coherent compared to the rest of the function.

Should be:

// @todo Explain what is actually wrong.

The current todo doesn't actually explain what is wrong. Also make sure that the comment wraps at 80 chars. If the todo exceeds 80 chars, indent subsequent lines by two spaces (after the leading //).

+++ b/core/modules/file/file.js
@@ -75,7 +79,7 @@ Drupal.file = Drupal.file || {
-
+    ¶

Trailing white-space.

---
The manual testing that has been performed already is sufficient for D8, so this patch can land whenever it is ready.

The backport for D7 will likely require more manual testing. However, once it is in D8, we'll get some additional manual testing for free.

From the patch:

--- a/core/misc/machine-name.js
+++ b/core/misc/machine-name.js
@@ -29,9 +29,10 @@ Drupal.behaviors.machineName = {
     for (var i in settings.machineName) {
       if (settings.machineName.hasOwnProperty(i)) {
         (function (source_id, options) {
-          var $source = $(source_id, context).addClass('machine-name-source');
-          var $target = $(options.target, context).addClass('machine-name-target');
-          var $suffix = $(options.suffix, context);
+          var $context = $(context);
+          var $source = $context.find(source_id).addClass('machine-name-source');

Which occurs in context here:

Drupal.behaviors.machineName = {
  attach: function (context, settings) {
    var self = this;
    for (var i in settings.machineName) {
      if (settings.machineName.hasOwnProperty(i)) {
        (function (source_id, options) {
          var $context = $(context);
          var $source = $context.find(source_id).addClass('machine-name-source');

Shouldn't you create the var $context = $(context); variable outside the for loop? It looks like we're re-creating the same jQuery object on each loop pass.

There are other places where we could optimize the code by creating a temporary variable to hold a jQuery object. Should I mention all of them? or just leave it for later optimizations since its out of scope?

I'm only about 1/4 of the way through this review (I'm looking at tabledrag at the moment). I've applied the patch and I'm reviewing the changes, line by line, in context of the actual file. I'll post back when I'm all the way though my review. (Dinner time!)

@@ -349,9 +356,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
         break;
       case 38: // Up arrow.
       case 63232: // Safari up arrow.
-        var previousRow = $(self.rowObject.element).prev('tr').get(0);
-        while (previousRow && $(previousRow).is(':hidden')) {
-          previousRow = $(previousRow).prev('tr').get(0);
+        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
+        var previousRow = $previousRow.get(0);
+        while (previousRow && $previousRow.is(':hidden')) {
+          $previousRow = $(previousRow).prev('tr').eq(0);
+          previousRow = $previousRow.get(0);

and

@@ -361,9 +370,10 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
           if ($(item).is('.tabledrag-root')) {
             // Swap with the previous top-level row.
             var groupHeight = 0;
-            while (previousRow && $('.indentation', previousRow).length) {
-              previousRow = $(previousRow).prev('tr').get(0);
-              groupHeight += $(previousRow).is(':hidden') ? 0 : previousRow.offsetHeight;
+            while (previousRow && $previousRow.find('.indentation').length) {
+              $previousRow = $(previousRow).prev('tr').eq(0);
+              previousRow = $previousRow.get(0);
+              groupHeight += $previousRow.is(':hidden') ? 0 : previousRow.offsetHeight;

Couldn't $previousRow = $(previousRow).prev('tr').eq(0); be re-written as $previousRow = $previousRow.prev('tr').eq(0);?

@@ -389,9 +399,11 @@ Drupal.tableDrag.prototype.makeDraggable = function (item) {
         break;
       case 40: // Down arrow.
       case 63233: // Safari down arrow.
-        var nextRow = $(self.rowObject.group).filter(':last').next('tr').get(0);
-        while (nextRow && $(nextRow).is(':hidden')) {
-          nextRow = $(nextRow).next('tr').get(0);
+        var $nextRow = $(self.rowObject.group).filter(':last').next('tr').eq(0);
+        var nextRow = $nextRow.get(0);
+        while (nextRow && $nextRow.is(':hidden')) {
+          $nextRow = $(nextRow).next('tr').eq(0);
+          nextRow = $nextRow.get(0);

Same issue with $nextRow = $(nextRow).next('tr').eq(0);.

@@ -704,13 +722,15 @@ Drupal.tableDrag.prototype.updateField = function (changedRow, group) {
   // Parents, look up the tree until we find a field not in this group.
   // Go up as many parents as indentations in the changed row.
   else if (rowSettings.relationship == 'parent') {
-    var previousRow = $(changedRow).prev('tr');
-    while (previousRow.length && $('.indentation', previousRow).length >= this.rowObject.indents) {
-      previousRow = previousRow.prev('tr');
+    var $previousRow = $changedRow.prev('tr');
+    var previousRow = $previousRow;
+    while ($previousRow.length && $previousRow.find('.indentation').length >= this.rowObject.indents) {
+      $previousRow = $previousRow.prev('tr');
+      previousRow = $previousRow;

We don't need to set previousRow = $previousRow; in this bit of code because previousRow isn't used anywhere in or after this bit of code in the Drupal.tableDrag.prototype.updateField function.

+++ b/core/modules/color/preview.js
@@ -7,17 +7,17 @@
   Drupal.color = {
     callback: function(context, settings, form, farb, height, width) {
       // Solid background.
-      $('#preview', form).css('backgroundColor', $('#palette input[name="palette[base]"]', form).val());
+      form.find('#preview').css('backgroundColor', form.find('#palette input[name="palette[base]"]').val());

Is form passed as a jQuery object? I can't tell from the code in preview.js. Ditto with form in bartik/color/preview.js.

@@ -140,8 +144,8 @@ Drupal.fieldUIOverview = {
    */
   onChange: function () {
     var $trigger = $(this);
-    var row = $trigger.closest('tr').get(0);
-    var rowHandler = $(row).data('fieldUIRowHandler');
+    var $row = $trigger.closest('tr');
+    var rowHandler = $row.data('fieldUIRowHandler');

Shouldn't $row be defined like this? var $row = $trigger.closest('tr').eq(0); That would match the pattern you set earlier.

If you address my comments in #42 and #44 and roll a new patch (if needed), I can do an interdiff and mark this RTBC. This is a BIG patch and I went through it with a fine grained comb.

If i'm not wrong...

Couldn't $previousRow = $(previousRow).prev('tr').eq(0); be re-written as $previousRow = $previousRow.prev('tr').eq(0);?

@John suggest =

$(self.rowObject.element).prev('tr').eq(0)  // filter out other element, return single element
.prev('tr').eq(0);  // catch nothing.

Patch #41 is right. That restart to get the object again.

      case 63232: // Safari up arrow.
        var $previousRow = $(self.rowObject.element).prev('tr').eq(0);
        var previousRow = $previousRow.get(0);
        while (previousRow && $previousRow.is(':hidden')) {
          $previousRow = $(previousRow).prev('tr').eq(0);
          previousRow = $previousRow.get(0);
        }

also affect this condition, seems not correct??

  else if (self.table.tBodies[0].rows[0] != previousRow || $previousRow.is('.draggable')) {

ok. I reviewed the changes in #47 vs. my previous line-by-line review of #41, and all my comments in #42 and #44 have been addressed.

Nice work!

Anyway, why it needs eq(0) ??

My suggestion to tableDrag part.

Nod_ and droplet, yep, can one of you create a follow-up issue to work on the tabledrag inefficiencies? We can move the patch from #49 to that issue.

Unless others see problems in patch #47 that could break things, it remains RTBC.

Note to committers: Ignore the patch in #49 (it will be a follow-up). The patch in #47 is RTBC. I reviewed it in comments #42, #44 and #48.

OK I went to commit this but it no longer applies:

patching file core/modules/user/user.js
Hunk #3 FAILED at 47.
Hunk #4 succeeded at 186 (offset 5 lines).

Not sure which patch broke it, but could we get a quick re-roll?

Patch failure due to #331893: Add colouring (and description) to password checker.

Here is the new changes:

@@ -47,11 +47,12 @@ Drupal.behaviors.password = {
         }
 
         // Adjust the length of the strength indicator.
-        $(innerWrapper).find('.indicator').css('width', result.strength + '%');
-        $(innerWrapper).find('.indicator').css('background-color', result.indicatorColor);
+        innerWrapper.find('.indicator')
+          .css('width', result.strength + '%')
+          .css('background-color', result.indicatorColor);

Remark for follow up issue:
user.js

     var innerWrapper = $(this).parent();
      var outerWrapper = $(this).parent().parent();

Needs a clean-up

Thanks folks. Committed and pushed to 8.x, moving back to 7.x for backport. We should also add a note in coding standards if there's not one already.

Follow up created here #1524414: Rewrite tabledrag.js to use jQuery UI

Do you mean IE is slower with this patch ?

It's a big patch. Now is the good time to backport it.

maybe this issue: #1294584: jQuery performance fix (IE7/8)

+++ b/core/modules/contextual/contextual.jsundefined
@@ -12,7 +12,7 @@ Drupal.contextualLinks = Drupal.contextualLinks || {};
-    $('div.contextual', context).once('contextual-links', function () {
+    $(context).find('div.contextual-links-wrapper').once('contextual-links', function () {

This was totally busted. Not sure how we/I didn't catch this.

s/contextual-links-wrapper/contextual

.contextual-links-wrapper used to be the class name, until #1216978: Clean up the CSS for Contextual module. And it IS still the class name in D7.
So that might explain why the wires got crossed here.

Reupping for the bot.

Patch in #61 applies cleanly and works for me.

No, the patch committed to D8 was wrong, and #61/63 (same patch just for the testbot) fix it.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

There is no div.contextual-links-wrapper in D8.

TestSwarm just complained, reason is "find$" instead of "find" in core/misc/states.js 530

$(document).bind('state:collapsed', function(e) {
  if (e.trigger) {
    if ($(e.target).is('.collapsed') !== e.value) {
      $(e.target).find$('> legend a').click();
    }
  }
});

so.....
Which patch needs to be fixed (please reference comment number)

#53 contains the error

Tested #71. Looks good.

Thanks for the follow-up. Committed/push to 8.x, moving to 7.x again.

I actually think that this change, while being a minor performance improvement, is not really worth to backport.

There's simply too much functionality that would have to be tested in-depth, and we'd have to be 100% sure that every single detail works like before, in order to not break hundreds of thousands of Drupal 7 production sites.

Consequently, I think we can spend our time much better with cleaning up JS in D8 and making that some true awesomesauce. Hence, moving back to D8 and fixed.