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.
Created attachment 258167 [details] preliminary patch for ews
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.
Created attachment 258351 [details] preliminary patch for ews
Created attachment 258936 [details] Patch
Created attachment 258939 [details] Patch
Created attachment 258945 [details] Patch
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.
(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.
http://trac.webkit.org/changeset/188420