Bug 36741 - Duplicate, slightly divergent implementation of Position[Iterator]::isCandidate()
Summary: Duplicate, slightly divergent implementation of Position[Iterator]::isCandida...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-29 00:07 PDT by Roland Steiner
Modified: 2017-07-18 08:29 PDT (History)
7 users (show)

See Also:


Attachments
patch - consolidate implementations into PositionIterator::isCandidate() (3.36 KB, patch)
2010-03-29 00:23 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2010-03-29 00:07:08 PDT
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.
Comment 1 Roland Steiner 2010-03-29 00:23:14 PDT
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...
Comment 2 Eric Seidel (no email) 2010-04-01 17:04:32 PDT
Comment on attachment 51879 [details]
patch - consolidate implementations into PositionIterator::isCandidate()

I like the idea.  Concerned about possible performace troubles, but OK.
Comment 3 WebKit Commit Bot 2010-04-02 01:31:23 PDT
Comment on attachment 51879 [details]
patch - consolidate implementations into PositionIterator::isCandidate()

Clearing flags on attachment: 51879

Committed r56989: <http://trac.webkit.org/changeset/56989>
Comment 4 WebKit Commit Bot 2010-04-02 01:31:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 mitz 2010-04-02 09:04:40 PDT
(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?
Comment 6 Mark Rowe (bdash) 2010-04-05 14:33:45 PDT
This introduced bug 37115.
Comment 7 Adele Peterson 2010-04-05 15:57:30 PDT
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?
Comment 8 Adele Peterson 2010-04-05 16:18:25 PDT
wow.  I meant to say - why weren't the performance concerns addressed?
Comment 9 Eric Seidel (no email) 2010-04-05 16:26:01 PDT
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.
Comment 10 Mark Rowe (bdash) 2010-04-05 18:46:40 PDT
I rolled this out in r57110 to address bug 37115.  I added a test case that covers that crash in r57111.
Comment 11 Roland Steiner 2010-04-05 19:12:12 PDT
[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.
Comment 12 Mark Rowe (bdash) 2010-04-05 22:43:26 PDT
(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.
Comment 13 Enrica Casucci 2010-04-06 08:51:10 PDT
(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.