RESOLVED FIXED 147622
Refactor and improve TextIndicator to prepare for tests
https://bugs.webkit.org/show_bug.cgi?id=147622
Summary Refactor and improve TextIndicator to prepare for tests
Tim Horton
Reported 2015-08-04 02:45:38 PDT
Refactor TextIndicator to have behavior controlled by flags instead of platform #ifdefs so that we can write some generic tests for its behavior. Also, improve the behavior slightly where one platform or the other was doing things at all callsites, by bringing that logic into TextIndicator itself and sharing it. Also, move more presentation-specific nonsense into TextIndicatorWindow.
Attachments
preliminary patch for ews (66.15 KB, patch)
2015-08-04 02:46 PDT, Tim Horton
no flags
preliminary patch for ews (66.15 KB, patch)
2015-08-05 23:06 PDT, Tim Horton
no flags
Patch (94.40 KB, patch)
2015-08-13 14:35 PDT, Tim Horton
no flags
Patch (95.17 KB, patch)
2015-08-13 15:33 PDT, Tim Horton
no flags
Patch (99.46 KB, patch)
2015-08-13 15:52 PDT, Tim Horton
simon.fraser: review+
Tim Horton
Comment 1 2015-08-04 02:46:31 PDT
Created attachment 258167 [details] preliminary patch for ews
Tim Horton
Comment 2 2015-08-04 02:49:28 PDT
Comment on attachment 258167 [details] preliminary patch for ews View in context: https://bugs.webkit.org/attachment.cgi?id=258167&action=review > Source/WebCore/page/TextIndicator.cpp:127 > + if ((!renderer->isInline() || renderer->isReplaced()) && range.intersectsNode(node, ASSERT_NO_EXCEPTION)) Counting inline-but-replaced as "complex" is one of the big behavior changes. > Source/WebCore/page/TextIndicator.cpp:188 > + if (textRects.isEmpty()) { This block (falling back to the bounding rect if we are asked to or if we end up with no text rects, and clipping said bounding rect to the visible rect) is one of the big behavior changes. Humorously, this also lets us use TextIndicator for images on Mac, and it looks pretty reasonable.
Tim Horton
Comment 3 2015-08-05 23:06:04 PDT
Created attachment 258351 [details] preliminary patch for ews
Tim Horton
Comment 4 2015-08-13 14:35:50 PDT
Tim Horton
Comment 5 2015-08-13 15:33:56 PDT
Tim Horton
Comment 6 2015-08-13 15:52:51 PDT
Simon Fraser (smfr)
Comment 7 2015-08-13 16:06:57 PDT
Comment on attachment 258945 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258945&action=review > Source/WebCore/dom/Range.h:177 > + enum class QuadsCoordinateSpace { Absolute, Client }; I don't think the "Quads" part of this name adds anything, esp. since you pass in to boundingRectInternal. > Source/WebCore/page/TextIndicator.cpp:65 > +RefPtr<TextIndicator> TextIndicator::createWithRange(const Range& range, TextIndicatorOptions options, TextIndicatorPresentationTransition presentationTransition, unsigned margin) What units is 'margin' in. Don't like unsigned for dimensions. > Source/WebCore/page/TextIndicator.cpp:84 > + data.presentationTransition = presentationTransition; > + data.indicatesCurrentSelection = !areRangesEqual(&range, oldSelection.toNormalizedRange().get()); > + data.options = options; Can't TextIndicatorData have a constructor? > Source/WebCore/page/TextIndicator.cpp:170 > + float asIsSnapshotScaleFactor; "asIs"? > Source/WebCore/page/mac/TextIndicatorWindow.mm:127 > +static bool indicatorWantsBounce(TextIndicator& indicator) const TextIndicator& here and below.
Tim Horton
Comment 8 2015-08-13 16:12:28 PDT
(In reply to comment #7) > Comment on attachment 258945 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=258945&action=review > > > Source/WebCore/dom/Range.h:177 > > + enum class QuadsCoordinateSpace { Absolute, Client }; > > I don't think the "Quads" part of this name adds anything, esp. since you > pass in to boundingRectInternal. Quite right. > > Source/WebCore/page/TextIndicator.cpp:65 > > +RefPtr<TextIndicator> TextIndicator::createWithRange(const Range& range, TextIndicatorOptions options, TextIndicatorPresentationTransition presentationTransition, unsigned margin) > > What units is 'margin' in. Don't like unsigned for dimensions. Fair enough. Will change that separately, though, since it's existing code. > > Source/WebCore/page/TextIndicator.cpp:84 > > + data.presentationTransition = presentationTransition; > > + data.indicatesCurrentSelection = !areRangesEqual(&range, oldSelection.toNormalizedRange().get()); > > + data.options = options; > > Can't TextIndicatorData have a constructor? I suppose it could. > > Source/WebCore/page/TextIndicator.cpp:170 > > + float asIsSnapshotScaleFactor; > > "asIs"? Heh. I'll find a better name. It's the "don't change anything, paint the whole thing, even paint the selection highlight" snapshot. As-is on-screen (except for compositing). > > Source/WebCore/page/mac/TextIndicatorWindow.mm:127 > > +static bool indicatorWantsBounce(TextIndicator& indicator) > > const TextIndicator& here and below. Makes sense.
Tim Horton
Comment 9 2015-08-13 17:07:14 PDT
Note You need to log in before you can comment on or make changes to this bug.