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.

Files: 
CommentFileSizeAuthor
#23 extlink-1329786-23.patch8.93 KBelachlan
PASSED: [[SimpleTest]]: [MySQL] 29 pass(es).
[ View ]
#21 extlink-1329786-21.patch8.45 KBelachlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 extlink-1329786-18.patch7.78 KBelachlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 extlink-1329786-15.patch7.31 KBelachlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 extlink-1329786-13.patch7.52 KBelachlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 1329786-7.extlink-js.patch7.48 KBmstef
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-7.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#6 1329786-6.extlink-js.patch7.43 KBmstef
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-6.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1329786-1.extlink-js.patch7.42 KBksenzee
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-1.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new7.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-1.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

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.

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) {
});

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.

StatusFileSize
new7.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-6.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Edit: ignore this patch

StatusFileSize
new7.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1329786-7.extlink-js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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.

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?

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.

#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.

Assigned:Unassigned» elachlan
Priority:Normal» Major

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

Version:7.x-1.12» 7.x-1.13
Status:Needs work» Needs review
StatusFileSize
new7.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new7.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Sorry, bad patch format.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

StatusFileSize
new7.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

White space issues.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new8.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch extlink-1329786-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new8.93 KB
PASSED: [[SimpleTest]]: [MySQL] 29 pass(es).
[ View ]

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.

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.

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.

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

Status:Needs review» Patch (to be ported)

Commited to Git. Needs port to 8.x

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?