Allow passing custom evaluator to node.getAscendant

[AlexW]

getAscendant is a bit limited, would be handy if it could filter by classes too.

// <div class="outer"><div class="inner"><p><b>Some text</b></p></div></div>
// If node == <b>
ascendant = node.getAscendant('div.outer'); // ascendant == <div class="outer">

[Piotrek Koszuliński]

I was also surprised that this function can do so little - I needed to look for an attribute and had to write it manually. So I'm also for extending it, though it must be a callback, not a CSS selector, because this will be faster and simpler to code (shorter too).

[Artur Delura]

Do we want indroduce function as a second/third argument (second one is optional) or allow place function as a first argument instead of string (node name)? I preffer second option because it's more flexible - we are not forced to specify node name. Both options of course with backward compability.

[Piotrek Koszuliński]

2nd option

[Artur Delura]

Solution and tests in ​branch:t/12279. See documentation as well. We could also implement ​http://en.wikipedia.org/wiki/Visitor_pattern in the future.

[Piotrek Koszuliński]

We could also implement ​​http://en.wikipedia.org/wiki/Visitor_pattern in the future.

I don't think it's possible or necessary:

The visitor pattern requires a programming language that supports single dispatch and method overloading

[Artur Delura]

I already implemented this pattern in JavaScript, maybe because I didn't know that such stuff is required ;). Function as a first class object is enought i think. It could bring us a lot of flexibility and less code.

[Piotrek Koszuliński]

Well, it's a variation about visitor pattern (because in your callback you have to handle type discovery - in visitor pattern it should happen automatically). But that's just naming - not important here, since callback is a standard (and much simpler) solution in JS world.

I found few issues after first glance:

1. Callback should be executed with CKEDITOR.dom.element instance. But avoid creating instances when filtering by name (performance).
2. Run linter.
3. You can compare CKEDITOR.dom.nodes using assert.areSame without accessing $ property.
4. You should split test cases into separate function and name them properly. It's hard to guess what you're testing. Don't use assertion message for that.
5. 'func_check' is not the right id. Neither name itself, nor its format. Use something that will tell that it's for getAscendant test - the same as other tests do.
6. We use el rather than elem.

[Artur Delura]

2. Run linter.

Found unused variable.

5. 'func_check' is not the right id. Neither name itself, nor its format. Use something that will tell that it's for getAscendant test - the same as other tests do.

As always I write tests in same style as closest environment is written.

Changes in the same branch.

[Piotrek Koszuliński]

As always I write tests in same style as closest environment is written.

Well, from the same file:

<div id="insertAfter"><!--abc-->def</div>
<div id="insertBefore"><!--abc-->def</div>
<div><br><br><!--a--><br><div>text<!--b-->text<p>text<br><i id="getAddress1">text</i></p></div></div>
<div><span><span><span><span><i id="getAddress2"></i></span></span></span></span></div>
<iframe id="getDocument"></iframe>
<div id="getNSN">
<i id="getNSN1">1</i>
<i id="getNSN2">2</i>
<i id="getNSN3">3</i>

So func_check definitely does not fit here. All ids are camel cased and are references to tested methods names.

---

var el = ( conditionChecker === reference ? new CKEDITOR.dom.node( $ ) : $ );

This may be optimized. You can avoid two things:

1. Unnecessary assignment if callback was passed - don't create unnecessary el variable in this case.
2. Checking whether callback was passed in every loop step.

[Artur Delura]

---

var el = ( conditionChecker === reference ? new CKEDITOR.dom.node( $ ) : $ );

This may be optimized. You can avoid two things:

1. Unnecessary assignment if callback was passed - don't create unnecessary el variable in this case.

I make such variables to avoid very long lines, which make code hard to read. When I will join this two lines into one it will have over 100 characters length. Common standard is less than 80. Maybe you don't see problem but I do, because in my IDE I have font size set to 18.

2. Checking whether callback was passed in every loop step.

So you want to move that check out of while loop? Just like:

var customChecker = conditionChecker === reference;
while() {

If yes, it does not make sense. We shouldn't optimize everything, but just parts which are slow. I made a quick performance tests and found out that such checking need to be called 23585 times to take 1ms. It's nothing.

[Piotrek Koszuliński]

First of all - do not create microbenchmarks. Never do that. You just tested empty loop or no loop at all.

Second - the reason for this changes is sanity - it simply does not make sense to test the value every time or make variable assignments when they are totally unnecessary there.

Also, think about readability - if you make a variable check every time, does this mean that this variable may change during the iteration? Code which does not do unnecessary things will be more clear for the reader.

Finally, note that I didn't mention performance in comment:9 ;).

[Artur Delura]

Second - the reason for this changes is sanity - it simply does not make sense to test the value every time or make variable assignments when they are totally unnecessary there.

Also, think about readability - if you make a variable check every time, does this mean that this variable may change during the iteration? Code which does not do unnecessary things will be more clear for the reader.

Finally, note that I didn't mention performance in comment:9 ;).

That make sense, thought you want this improvements because of performance. +1 for readability :) Changes in same ​branch:t/12279.

[Piotrek Koszuliński]

There were still some issues which I mentioned earlier and which hasn't been resolved, so I pushed additional commit.

Merged to master with ​git:8749148.