Bug 197177

Summary:

Return annotated text checking strings via UIWKDocumentContext

Product:

WebKit

Reporter:

Tim Horton <thorton>

Component:

New Bugs

Assignee:

Tim Horton <thorton>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, cdumez, commit-queue, rniwa, webkit-bug-importer, wenson_hsieh

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Description Tim Horton 2019-04-22 14:00:01 PDT

Return annotated text checking strings via UIWKDocumentContext

Comment 1 Tim Horton 2019-04-22 14:00:20 PDT

Created attachment 367971 [details]
Patch

Comment 2 Tim Horton 2019-04-22 14:00:22 PDT

<rdar://problem/49064839>

Comment 3 Ryosuke Niwa 2019-04-23 13:44:57 PDT

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)...

Comment 4 Chris Dumez 2019-04-23 13:46:40 PDT

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?

Comment 5 Tim Horton 2019-04-23 15:41:46 PDT

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

Comment 6 Tim Horton 2019-04-23 16:18:57 PDT

Created attachment 368079 [details]
Patch

Comment 7 WebKit Commit Bot 2019-04-23 16:46:51 PDT

Comment on attachment 368079 [details]
Patch

Clearing flags on attachment: 368079

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

Comment 8 WebKit Commit Bot 2019-04-23 16:46:53 PDT

All reviewed patches have been landed.  Closing bug.

Comment 9 Ryosuke Niwa 2019-04-23 21:02:53 PDT

(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());

Comment 10 Chris Dumez 2019-04-24 08:48:40 PDT

(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 :)