| Summary: | Align implementation of PositionIterator::isCandidate() on Position::isCandidate() | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Frédéric Wang (:fredw) <fred.wang> | ||||||
| Component: | HTML Editing | Assignee: | Frédéric Wang (:fredw) <fred.wang> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, darin, esprehn+autocc, ews-watchlist, fred.wang, kangil.han, rniwa, sam, webkit-bug-importer, wenson_hsieh | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Frédéric Wang (:fredw)
2021-07-30 05:47:50 PDT
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]. (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. |