Summary: | Use Node::length to replace Node::maxCharacterOffset and lastOffsetInNode; switch more offsets from int to unsigned | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
Component: | DOM | Assignee: | Darin Adler <darin> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | aboxhall, andersca, apinheiro, cdumez, cfleizach, dmazzoni, esprehn+autocc, ews-watchlist, gyuyoung.kim, jcraig, jdiggs, kangil.han, koivisto, kondapallykalyan, mifenton, samuel_white, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 212093 | ||||||||||||||
Attachments: |
|
Description
Darin Adler
2020-04-08 21:20:01 PDT
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> 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. |