Bug 36741 – Duplicate, slightly divergent implementation of Position[Iterator]::isCandidate()

• Status: REOPENED
• Component: HTML Editing

[Roland Steiner]

The member function isCandidate is implemented twice, once on the Position class, and once on the PositionIterator class. The latter also does not contain the latest modifications to that function.

There should be only one implementation.

[Roland Steiner]

Created attachment 51879 [details]
patch - consolidate implementations into PositionIterator::isCandidate()

As stated in the ChangeLog, the rationale for using PositionIterator::isCandidate() over Position::isCandidate() is that a PositionIterator is cheaper to construct from a Position than vice versa (no call to nodeIndex()). Also, PositionIterator::isCandidate is called in a tight loop within next/previousCandidate (editing/htmlediting.cpp).

The code as is doesn't look very nice though, and I believe that PositionIterator and Position should be merged at some point. But that is another story, for another 1001 commits...

[Eric Seidel (no email)]

Comment on attachment 51879 [details]
patch - consolidate implementations into PositionIterator::isCandidate()

I like the idea.  Concerned about possible performace troubles, but OK.

[WebKit Commit Bot]

Comment on attachment 51879 [details]
patch - consolidate implementations into PositionIterator::isCandidate()

Clearing flags on attachment: 51879

Committed r56989: <http://trac.webkit.org/changeset/56989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #1)
> a
> PositionIterator is cheaper to construct from a Position than vice versa (no
> call to nodeIndex()).

But there is a call to childNode(). Isn't is just as expensive? I am concerned about the performance impact of this patch. How did you measure it?

[Mark Rowe (bdash)]

This introduced bug 37115.

[Adele Peterson]

I'm considering rolling this out based on the introduced bug, and the performance concerns.  I guess it was after the commit, but why were's the performance concerns addressed?

[Adele Peterson]

wow.  I meant to say - why weren't the performance concerns addressed?

[Eric Seidel (no email)]

You should of course feel free to roll it out.

I'm not sure there is any performance data for or against this change.  It creates and extra object on the stack (in a relatively hot code path).  But that doesn't necessarily mean it's noticeably slower.

[Mark Rowe (bdash)]

I rolled this out in r57110 to address bug 37115.  I added a test case that covers that crash in r57111.

[Roland Steiner]

[sorry for the belated response]

Regarding the performance concerns: Note that even
PositionIterator::isCandidate() contains conversions back to Position, so the
implementation is sub-optimal in this regard anyway.

As mentioned in my initial comment, and also on webkit-dev@ I believe that
those classes should be refactored anyway, even more so now that consolidating
the divergent implementations apparently even leads to crashes.

[Mark Rowe (bdash)]

(In reply to comment #11)
> As mentioned in my initial comment, and also on webkit-dev@ I believe that
> those classes should be refactored anyway, even more so now that consolidating
> the divergent implementations apparently even leads to crashes.

What it says to me is that refactoring needs to be approached carefully as it is easy to unintentionally break something.

[Enrica Casucci]

(In reply to comment #11)
> [sorry for the belated response]
> 
> Regarding the performance concerns: Note that even
> PositionIterator::isCandidate() contains conversions back to Position, so the
> implementation is sub-optimal in this regard anyway.
> 
> As mentioned in my initial comment, and also on webkit-dev@ I believe that
> those classes should be refactored anyway, even more so now that consolidating
> the divergent implementations apparently even leads to crashes.

I've read you e-mail to webkit dev very carefully and, even though I like the idea of refactoring Position, PositionIterator and Visible position, I think it is a major undertaking.
There are too many places in the editing code where heavy assumptions are made on what VisiblePosition or Position will provide, that will force you to revisit many of the editing commands.
I considered something similar for Selection and VisibleSelection at some point, encouraged by the comment in the VisibleSelection class but I decided not to move forward because the chances of breaking many places were too high.
As far as this specific change is concerned, my advice is to run performance tests before making this change. As you know, the purpose of Position iterator is to allow you to move among positions fast and we want to make sure we are not slowing it down.
I like the idea of having VisiblePosition work on renderers.
isCandidate can be very expensive at times, especially since we introduced the ability to have a selection in an editable container that contains only non editable elements and in empty editable spans within non editable content.