Bug 147622 – Refactor and improve TextIndicator to prepare for tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tim Horton]

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.

[Tim Horton]

Created attachment 258167 [details]
preliminary patch for ews

[Tim Horton]

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]

Created attachment 258351 [details]
preliminary patch for ews

[Tim Horton]

Created attachment 258936 [details]
Patch

[Tim Horton]

Created attachment 258939 [details]
Patch

[Tim Horton]

Created attachment 258945 [details]
Patch

[Simon Fraser (smfr)]

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]

(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]

http://trac.webkit.org/changeset/188420