Summary: | computeSelectionStart and computeSelectionEnd shouldn't trigger synchronous layout | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||
Component: | Forms | Assignee: | Ryosuke Niwa <rniwa> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | barraclough, benjamin, darin, enrica, kling, koivisto | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=207724 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 127832 | ||||||
Attachments: |
|
Description
Ryosuke Niwa
2014-02-14 02:28:53 PST
Created attachment 224295 [details]
Fixes the bug
Comment on attachment 224295 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review > Source/WebCore/html/HTMLTextFormControlElement.cpp:333 > + int index = indexForPosition(position.deepEquivalent()); Why int rather than unsigned? > Source/WebCore/html/HTMLTextFormControlElement.cpp:601 > + index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode())); Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>. > Source/WebCore/html/HTMLTextFormControlElement.cpp:616 > +#ifndef NDEBUG > + VisiblePosition visiblePosition = passedPosition; > + unsigned indexComputedByVisiblePosition = 0; > + if (visiblePosition.isNotNull()) > + indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */); > + ASSERT(index == indexComputedByVisiblePosition); > +#endif I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG. (In reply to comment #2) > (From update of attachment 224295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224295&action=review > > > Source/WebCore/html/HTMLTextFormControlElement.cpp:333 > > + int index = indexForPosition(position.deepEquivalent()); > > Why int rather than unsigned? Will fix. > > Source/WebCore/html/HTMLTextFormControlElement.cpp:601 > > + index += std::min(length, static_cast<unsigned>(passedPosition.offsetInContainerNode())); > > Instead of static_cast<unsigned>, I suggest writing std::min<unsigned>. Will fix. > > Source/WebCore/html/HTMLTextFormControlElement.cpp:616 > > +#ifndef NDEBUG > > + VisiblePosition visiblePosition = passedPosition; > > + unsigned indexComputedByVisiblePosition = 0; > > + if (visiblePosition.isNotNull()) > > + indexComputedByVisiblePosition = WebCore::indexForVisiblePosition(innerText, visiblePosition, false /* forSelectionPreservation */); > > + ASSERT(index == indexComputedByVisiblePosition); > > +#endif > > I think an assertion like this should be written with a helper function so it doesn’t need to be inside an #if. Also can consider ASSERT_DISABLED instead of NDEBUG. I'd like to be able to see indexComputedByVisiblePosition and visiblePosition for debugging purposes when the assertion is hit. If I put the computation inside a helper function, it would be tricky to get those objects/values in the debugger. But I'd use ASSERT_DISABLED. Committed r164180: <http://trac.webkit.org/changeset/164180> |