Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned
Created attachment 395908 [details] Patch
While working on reducing the use of live ranges, discovered this improvement, so getting it reviewed and landed separately.
Created attachment 395910 [details] Patch
Looks like there is a behavior change here. Tests show failures but also new PASSes in WPT.
Looks like behavior change is in the text input selection part of this; I think it’s a change we will want, but I will separate that out into a much smaller patch.
Created attachment 395982 [details] Patch
This newer patch should leave out the behavior change, which we can consider and do later/separately.
Oops, looks like the behavior change is still there.
Some behavior change.
Created attachment 396008 [details] Patch
This last tiny layout change difference is a mystery.
Created attachment 396136 [details] Patch
Mystery solved. Finally had a chance to debug and it was a refactoring error; fixed now.
This patch is now both a simple refactoring, and all tests are passing, so should be a quick review for someone.
Committed r259930: <https://trac.webkit.org/changeset/259930>
<rdar://problem/61638646>
Comment on attachment 396136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396136&action=review > Source/WebCore/dom/DocumentMarkerController.cpp:600 > + if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) { This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()). Just for me: This iOS code was added to fix <rdar://problem/11478253>. > Source/WebCore/dom/DocumentMarkerController.cpp:615 > list->remove(i); Just for me: ^^^ line above (not part of this patch) is incorrect: performs **unsigned** comparison, but delta is signed so "+ delta" + implicit conversion to unsigned can blow up and make comparison meaningless. Also expression should reference targetEndOffset instead of re-computing <-- may have made the bug more obvious.
Comment on attachment 396136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396136&action=review >> Source/WebCore/dom/DocumentMarkerController.cpp:600 >> + if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) { > > This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()). > > Just for me: This iOS code was added to fix <rdar://problem/11478253>. Are you fixing it, or would you like me to? Do you have an idea how to add a test?
Comment on attachment 396136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396136&action=review >>> Source/WebCore/dom/DocumentMarkerController.cpp:600 >>> + if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) { >> >> This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()). >> >> Just for me: This iOS code was added to fix <rdar://problem/11478253>. > > Are you fixing it, or would you like me to? > > Do you have an idea how to add a test? You can fix it, but I will supply you with a bug + a test in a few minutes...
Comment on attachment 396136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396136&action=review >>>> Source/WebCore/dom/DocumentMarkerController.cpp:600 >>>> + if (static_cast<unsigned>(targetStartOffset) >= node.length() || targetEndOffset <= 0) { >>> >>> This is a change in behavior that causes a regression: marker at the start of the text will **now be removed** if trailing whitespace was subsequently inserted and that whitespace is collapsed. When collapse happens, marker.startOffset() == 0, **delta** == -1. So, targetStartOffset == -1 and this blows up when casted to unsigned (i.e. it ALWAYS is >= node.length()). >>> >>> Just for me: This iOS code was added to fix <rdar://problem/11478253>. >> >> Are you fixing it, or would you like me to? >> >> Do you have an idea how to add a test? > > You can fix it, but I will supply you with a bug + a test in a few minutes... Bug + test at posted at bug #212093.
Comment on attachment 396136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396136&action=review >> Source/WebCore/dom/DocumentMarkerController.cpp:615 >> list->remove(i); > > Just for me: ^^^ line above (not part of this patch) is incorrect: performs **unsigned** comparison, but delta is signed so "+ delta" + implicit conversion to unsigned can blow up and make comparison meaningless. Also expression should reference targetEndOffset instead of re-computing <-- may have made the bug more obvious. That’s incorrect. If targetEndOffset is <= 0 we won’t get here; checked for above.