I wanted to override just a part of extlink.js, and found I had to override the whole thing. So I figured some refactoring was in order. Patch forthcoming.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

Status: Active » Needs review
FileSize
7.42 KB

This patch should apply against the master branch. I haven't changed any actual functionality - just moved things around. Here's what I did:

- Replaced Drupal.settings with the settings parameter from Drupal.attachBehaviors. This is technically more correct for D7, although I doubt it makes any functional difference. However, it does have the advantage of being faster; accessing a.b.c is slower than accessing b.c.

- Replaced jQuery .each() with for loops. It turns out that function-based iteration is a lot slower than traditional loop iteration. (If we wanted to get even faster, we could use a do-while and decrement a counter, but I find that harder to read.)

- Factored out the one-liners that add the extlink and mailto classes into a separate function. There was a lot going on in that one line, and a lot of it was duplicate code that it made sense to consolidate.

- Moved the extlinkAttach function into the Drupal.extlink namespace, so people who want to override the whole thing don't have to add to the global namespace. I did leave in a check for whether the extlinkAttach function exists, for the benefit of people who were already overriding it.

quicksketch’s picture

This patch looks like a good improvement. I don't think the backwards compatible function is really needed though, because extlinkAttach() is already locally scoped inside of the (function ($) { wrapper at the top of the file.

ksenzee’s picture

Right, the original function is file scoped, but anyone who needs to override it has to put *their* extlinkAttach function in the global scope in order for Drupal.behaviors.extlink.attach to be able to see it. That's what I was thinking it would be nice to avoid.

RobLoach’s picture

Status: Needs review » Needs work

This is a huge improvement! Looking at the current extlink.js makes me vomit a bit.

+++ b/extlink.jsundefined
@@ -22,73 +25,64 @@ function extlinkAttach(context) {
   // In jQuery 1.1 and higher, we'd use a filter method here, but it is not
   // available in jQuery 1.0 (Drupal 5 default).
   var external_links = new Array();
   var mailto_links = new Array();

We're on Drupal 7, let's get rid of all the old jQuery version checks.

+++ b/extlink.jsundefined
@@ -22,73 +25,64 @@ function extlinkAttach(context) {
     try {
-      var url = this.href.toLowerCase();
+      var url = link.href.toLowerCase();
       if (url.indexOf('http') == 0 && (!url.match(internal_link) || (extInclude && url.match(extInclude))) && !(extExclude && url.match(extExclude))) {
-        external_links.push(this);
+        external_links.push(link);
       }
       // Do not include area tags with begin with mailto: (this prohibits
       // icons from being added to image-maps).
       else if (this.tagName != 'AREA' && url.indexOf('mailto:') == 0) {
-        mailto_links.push(this);
+        mailto_links.push(link);
       }
     }
     // IE7 throws errors often when dealing with irregular links, such as:
     // <a href="node/10"></a> Empty tags.
     // <a href="http://user:pass@example.com">example</a> User:pass syntax.
-    catch(error) {
+    catch (error) {
       return false;

Would be nice to not need that try/catch. Is it just the url.match calls that throw it on IE7?

+++ b/extlink.jsundefined
@@ -96,12 +90,46 @@ function extlinkAttach(context) {
+  if (parseFloat($().jquery) < 1.2) {
+    $links_to_process = $(links).not('[img]');

Probably don't need that version check.

+++ b/extlink.jsundefined
@@ -96,12 +90,46 @@ function extlinkAttach(context) {
+  var length = $links_to_process.length;
+  for (i = 0; i < length; i++) {
+    var $link = $($links_to_process[i]);
+    if ($link.css('display') == 'inline') {
+      $link.after('<span class=' + class_name + '></span>');

$.each($links_to_process, function(index, link) {
});

ksenzee’s picture

I'm going to disagree with you about jQuery.each. See my second point in #1. We ought to be avoiding it anywhere we can in performance-sensitive code.

mstef’s picture

FileSize
7.43 KB

Edit: ignore this patch

mstef’s picture

FileSize
7.48 KB

Slight reroll of #1 to properly apply with drush make.

elachlan’s picture

This is a real worthwhile change and should really be done before #2038235: Add minified version of extlink.js.

I will work on it soon, unless someone wants to re-roll a patch.

elachlan’s picture

Status: Needs work » Postponed (maintainer needs more info)

Can I get some input on whether we want to go with the for loop instead of each?

#5 - Do you know how much time it will save doing it this way?

ksenzee’s picture

Status: Postponed (maintainer needs more info) » Needs work

I happen to hate jQuery.each with a hot hot hate, and I have never understood the widespread reluctance to use a simple venerable for loop, but as it happens modern browsers have built in each handling so jQuery.each isn't nearly as slow for them. I wouldn't hold up the patch for it either way. The refactoring is more important.

elachlan’s picture

#10 - if you re-roll it without the for loop I will include it. There have been a few changes to the javascript in the master branch, so be sure to check that first.

We can then add it later pending input from other maintainers.

elachlan’s picture

Assigned: Unassigned » elachlan
Priority: Normal » Major

Setting the priority to Major. I will re-roll this patch soon, unless someone does it first.

elachlan’s picture

Version: 7.x-1.12 » 7.x-1.13
Status: Needs work » Needs review
FileSize
7.52 KB

Re-worked the patch. Uses jQuery.each

I am happy to move away from it if quicksketch agrees to it. He knows more than me about it.

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-13.patch, failed testing.

elachlan’s picture

FileSize
7.31 KB

Sorry, bad patch format.

elachlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-15.patch, failed testing.

elachlan’s picture

FileSize
7.78 KB

White space issues.

elachlan’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-18.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
8.45 KB

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-21.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review
FileSize
8.93 KB

I think it is failing because it's applying to 7.x-1.13 and not the master branch.

Trying again anyway.

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-23.patch, failed testing.

Dave Reid’s picture

You should create a 7.x-1.x-dev release for this project so that you can properly assign versions to things in development here in the issue queue.

elachlan’s picture

Version: 7.x-1.13 » 7.x-1.x-dev
Status: Needs work » Needs review

Added the new branch for 7.x-1.x and dev release.

elachlan’s picture

#23: extlink-1329786-23.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, extlink-1329786-23.patch, failed testing.

elachlan’s picture

Status: Needs work » Needs review

#23: extlink-1329786-23.patch queued for re-testing.

elachlan’s picture

Status: Needs review » Patch (to be ported)

Commited to Git. Needs port to 8.x

elachlan’s picture

By having the backwards compatibility section we add 0.05% to the page execution time.

This doesn't sound like much, but it is 45% of the execution time for external links. Is there a better way we can do it?

DamienMcKenna’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes

Moving to the correct branch.

elachlan’s picture

It looks like this was already done as apart of the conversion.

acrosman’s picture

Agreed, it looks like this patch is already in the 8.x branch.

rootwork’s picture

Status: Patch (to be ported) » Closed (outdated)