Opened 10 years ago

Closed 10 years ago

#2876 closed Bug (fixed)

V3: Simplify domWalker

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.0
Component: General Version:
Keywords: Confirmed Review+ Cc: Garry Yao

Description

The current domWalker implementation is much more complex than it needs to be. It's using our event system to control its execution even internally, with no benefit from it. It should instead use direct function calls, like onDown, onUp and onSibling.

The event system is to be used if we know we may have more than one, or an unknonwn number of listeners to the events. It's there to give flexibility, not just because it's nice to code with it.

If instead we know we'll just have a single listener, we can safely do direct function calls. This eliminates the overhead coming from the even system, which definitely impacts on performance.

Attachments (2)

2876.patch (5.2 KB) - added by Garry Yao 10 years ago.
2876_2.patch (9.7 KB) - added by Frederico Caldeira Knabben 10 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 years ago by Garry Yao

Cc: Garry Yao added

comment:2 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 10 years ago by Garry Yao

Attachment: 2876.patch added

comment:3 Changed 10 years ago by Garry Yao

Keywords: Review? added

The patch remaining all existing interfaces while use direct function calls for guard control.

comment:4 Changed 10 years ago by Frederico Caldeira Knabben

Priority: HighNormal

comment:5 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: assignednew

While working on #3299, I've faced a limitation in the domWalker system. As said in this ticket the domWalker is far too complex for what we really need. It's also in part, a duplication of node::getNextSourceNode and node::getPreviousSourceNode.

I've analyzed the parts of the code that use the domWalker, and I've found out that it's event system is mostly not needed. It's useful for its own code only.

So, let's KISS... I've written a new walker class from scratch.

Changed 10 years ago by Frederico Caldeira Knabben

Attachment: 2876_2.patch added

comment:6 Changed 10 years ago by Frederico Caldeira Knabben

Keywords: Review? added; Review- removed
Status: newassigned

To make things less risk, I'm not touching the original domWalker for now. I'm instead introducing a new class: CKEDITOR.dom.walker. In this way we can fist introduce this new class, and them replace the code using domWalker on dedicated tickets.

This new walker class, other than simplifying the code, adds a few features to the original implementation. It also enhances the node::getNextSourceNode and node::getPreviousSourceNode functions by adding the guard feature to them.

This class will be used by #3299, so the review can happen simultaneously.

comment:7 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:8 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: assignedclosed

Fixed with [3351].

#3304 has been opened to deal with the remaining domWalker stuff.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy