[PATCH 2 of 2] hgweb: optimize process_dates function

[Alexander Plavin]

10.08.2013, 16:57, "Laurens Holst" <laurens.nospam at grauw.nl>:
> Op 08-08-13 15:16, Alexander Plavin schreef:
>
>>  # HG changeset patch
>>  # User Alexander Plavin <alexander at plav.in>
>>  # Date 1374682432 -14400
>>  #      Wed Jul 24 20:13:52 2013 +0400
>>  # Node ID 2e3064b1a8b3719bc896e61a5804fd515a0bc9cb
>>  # Parent  d2e91a2d576deff3bb97fefc1fe71c2a64d64bb6
>>  hgweb: optimize process_dates function
>>
>>  This function looped over every node due to getElementsByTagName('*'), instead
>>  of using selectors. In this patch we use querySelectorAll('.age') and process
>>  only these nodes, which is much faster and also doesn't require extra condition.
>>
>>  Browser compatibility isn't sacrificed: IE 8+, FF 3.5+, Opera 10+.
>
> How much do we care about compatibility with IE8?

May be not much, but it's not bad anyway :)

>
>>  diff -r d2e91a2d576d -r 2e3064b1a8b3 mercurial/templates/static/mercurial.js
>>  --- a/mercurial/templates/static/mercurial.js Wed Jul 24 18:53:55 2013 +0400
>>  +++ b/mercurial/templates/static/mercurial.js Wed Jul 24 20:13:52 2013 +0400
>>  @@ -245,21 +245,18 @@
>>                    }
>>            }
>>
>>  -    var nodes = document.getElementsByTagName('*');
>>  -    var ageclass = new RegExp('\\bage\\b');
>>  +    var nodes = document.querySelectorAll('.age');
>
> Possibly getElementsByClassName() is more efficient.

Yes, it's more efficient of course, but:
- querying by a selector gives a very easy way to add some scope limit, see http://mercurial.markmail.org/message/kwcml3c6ewwonssr?q=process_dates
- this part (I mean getting list of nodes) is anyway much faster than actual processing of the nodes below

>
> Looks like it’s only supported in IE9 though. (Other browsers actually
> had it *before* querySelectorAll().)

As for me, querySelectorAll is supported for so long that we definitely shouldn't care about this, and this shouldn't be an argument against it.

>
>>        var dateclass = new RegExp('\\bdate\\b');
>
> I would use a literal /\bdate\b/ instead of new RegExp().

I would too, but it's not that good to add (I think) to add this change to this patch.

>
>>        for (var i=0; i<nodes.length; ++i){
>>            var node = nodes[i];
>>            var classes = node.className;
>>  -        if (ageclass.test(classes)){
>>  -            var agevalue = age(node.textContent);
>>  -            if (dateclass.test(classes)){
>>  -                // We want both: date + (age)
>>  -                node.textContent += ' ('+agevalue+')';
>>  -            } else {
>>  -                node.title = node.textContent;
>>  -                node.textContent = agevalue;
>>  -            }
>>  +        var agevalue = age(node.textContent);
>>  +        if (dateclass.test(classes)){
>
> As you’re using classList elsewhere, I think you could use
> classList.contains() instead of the regexp match?

It's already used (by me :) ) in line wrapping switch, and in my opinion getting readable dates (this place, process_dates) is more important than having the possibility to switch line wrapping dynamically. Also, this part of code existed before this patch, so why change it, what's the reason?

>
> Though MDN tells me it is only supported in IE10 onwards, but if that’s
> an argument then we shouldn’t be using it in the other place either.
>
>>  +            // We want both: date + (age)
>>  +            node.textContent += ' ('+agevalue+')';
>>  +        } else {
>>  +            node.title = node.textContent;
>>  +            node.textContent = agevalue;
>>            }
>>        }
>>    }
>>  _______________________________________________
>>  Mercurial-devel mailing list
>>  Mercurial-devel at selenic.com
>>  http://selenic.com/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel