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.
Created attachment 434616 [details] Patch
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*
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?
Created attachment 434819 [details] Patch for landing
(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.
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].
<rdar://problem/81447460>
(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.