Bug 147622 - Refactor and improve TextIndicator to prepare for tests
Summary: Refactor and improve TextIndicator to prepare for tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-04 02:45 PDT by Tim Horton
Modified: 2015-08-13 17:07 PDT (History)
5 users (show)

See Also:


Attachments
preliminary patch for ews (66.15 KB, patch)
2015-08-04 02:46 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
preliminary patch for ews (66.15 KB, patch)
2015-08-05 23:06 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (94.40 KB, patch)
2015-08-13 14:35 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (95.17 KB, patch)
2015-08-13 15:33 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (99.46 KB, patch)
2015-08-13 15:52 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 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.
Comment 1 Tim Horton 2015-08-04 02:46:31 PDT
Created attachment 258167 [details]
preliminary patch for ews
Comment 2 Tim Horton 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.
Comment 3 Tim Horton 2015-08-05 23:06:04 PDT
Created attachment 258351 [details]
preliminary patch for ews
Comment 4 Tim Horton 2015-08-13 14:35:50 PDT
Created attachment 258936 [details]
Patch
Comment 5 Tim Horton 2015-08-13 15:33:56 PDT
Created attachment 258939 [details]
Patch
Comment 6 Tim Horton 2015-08-13 15:52:51 PDT
Created attachment 258945 [details]
Patch
Comment 7 Simon Fraser (smfr) 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.
Comment 8 Tim Horton 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.
Comment 9 Tim Horton 2015-08-13 17:07:14 PDT
http://trac.webkit.org/changeset/188420