Bug 228635 – Align implementation of PositionIterator::isCandidate() on Position::isCandidate()

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Frédéric Wang (:fredw)]

Follow-up of https://trac.webkit.org/changeset/280381/webkit

This can cause subtle bug, so we should avoid that they go out of sync.

[Frédéric Wang (:fredw)]

Created attachment 434616 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 434616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434616&action=review

> Source/WebCore/dom/Position.cpp:970
> +// This function should be keep in sync with PositionIterator::isCandidate().

kept*

> Source/WebCore/dom/PositionIterator.cpp:148
> +// This function should be keep in sync with Position::isCandidate().

kept*

[Darin Adler]

Comment on attachment 434616 [details]
Patch

Seems like a good idea, and having no effect on any test is a mixed bag (is it really an improvement if it’s not detectable?).

Can we do a further improvement and make most of this be shared code, too?

[Frédéric Wang (:fredw)]

Created attachment 434819 [details]
Patch for landing

[Frédéric Wang (:fredw)]

(In reply to Darin Adler from comment #3)
> Seems like a good idea, and having no effect on any test is a mixed bag (is
> it really an improvement if it’s not detectable?).

Yeah, I considered writing tests but I'm not sure how difficult it is. For example in r280381 there was some subtle things happening and the crash test did not work without using a <button>. It seems more urgent to make the two functions in sync than having to spend time on testing that can be covered by fuzzing instead.

> Can we do a further improvement and make most of this be shared code, too?

OK, I opened bug 228729 for that. But again there are some subtle diffs when  m_anchorType is used, so I preferred not to touch that for now.

[EWS]

Committed r280585 (240207@main): <https://commits.webkit.org/240207@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434819 [details].

[Radar WebKit Bug Importer]

<rdar://problem/81447460>

[Darin Adler]

(In reply to Frédéric Wang (:fredw) from comment #5)
> (In reply to Darin Adler from comment #3)
> > Seems like a good idea, and having no effect on any test is a mixed bag (is
> > it really an improvement if it’s not detectable?).
> 
> Yeah, I considered writing tests but I'm not sure how difficult it is. For
> example in r280381 there was some subtle things happening and the crash test
> did not work without using a <button>. It seems more urgent to make the two
> functions in sync than having to spend time on testing that can be covered
> by fuzzing instead.

Not sure that I agree. Where is the evidence that this is covered by fuzzing?

I think the tests may be *more* valuable than code changes that by making things more consistent *might* make things better.