Bug 197177 – Return annotated text checking strings via UIWKDocumentContext

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Return annotated text checking strings via UIWKDocumentContext

[Tim Horton]

Created attachment 367971 [details]
Patch

[Tim Horton]

<rdar://problem/49064839>

[Ryosuke Niwa]

Comment on attachment 367971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367971&action=review

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.h:54
> +    AttributedString annotatedSubstringBetweenPositions(WebCore::VisiblePosition, WebCore::VisiblePosition);

Use const &?

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:171
> +    RefPtr<Document> document = start.deepEquivalent().document();

makeRefPtr?

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:185
> +        RefPtr<Range> currentTextRange = it.range();

It's expensive to create range. Do this after the continue below.

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:190
> +        [string replaceCharactersInRange:NSMakeRange(stringLength, 0) withString:it.text().createNSStringWithoutCopying().get()];

Why not just appendString?

> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:202
> +            auto subrange = TextIterator::subrange(*currentTextRange, marker->startOffset(), marker->endOffset() - marker->startOffset());
> +
> +            size_t subrangeLocation;
> +            size_t subrangeLength;
> +            TextIterator::getLocationAndLengthFromRange(commonAncestor.get(), &subrange.get(), subrangeLocation, subrangeLength);

This is a pretty bad o(n^2)...

[Chris Dumez]

Comment on attachment 367971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=367971&action=review

>> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:171
>> +    RefPtr<Document> document = start.deepEquivalent().document();
> 
> makeRefPtr?

What's the benefit?

[Tim Horton]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 367971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367971&action=review
>
> > Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:202
> > +            auto subrange = TextIterator::subrange(*currentTextRange, marker->startOffset(), marker->endOffset() - marker->startOffset());
> > +
> > +            size_t subrangeLocation;
> > +            size_t subrangeLength;
> > +            TextIterator::getLocationAndLengthFromRange(commonAncestor.get(), &subrange.get(), subrangeLocation, subrangeLength);
> 
> This is a pretty bad o(n^2)...

Yeah this whole thing is awful. Is there a straightforward better way, though?

[Tim Horton]

Created attachment 368079 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368079 [details]
Patch

Clearing flags on attachment: 368079

Committed r244570: <https://trac.webkit.org/changeset/244570>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 367971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367971&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:171
> >> +    RefPtr<Document> document = start.deepEquivalent().document();
> > 
> > makeRefPtr?
> 
> What's the benefit?

The benefit is that we can use auto as in:
auto document = makeRefPtr(start.deepEquivalent().document());

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #9)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 367971 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=367971&action=review
> > 
> > >> Source/WebKit/WebProcess/WebPage/Cocoa/TextCheckingControllerProxy.mm:171
> > >> +    RefPtr<Document> document = start.deepEquivalent().document();
> > > 
> > > makeRefPtr?
> > 
> > What's the benefit?
> 
> The benefit is that we can use auto as in:
> auto document = makeRefPtr(start.deepEquivalent().document());

ah, with "auto". I get it now :)