WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 258936
[details]
Patch
Tim Horton
Comment 5
2015-08-13 15:33:56 PDT
Created
attachment 258939
[details]
Patch
Tim Horton
Comment 6
2015-08-13 15:52:51 PDT
Created
attachment 258945
[details]
Patch
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
http://trac.webkit.org/changeset/188420
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug