Bug 209462 – Simplify characterRectsForRange() in WebPage::requestDocumentEditingContext()

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

[Daniel Bates]

Simplify characterRectsForRange() before I make changes to it.

[Daniel Bates]

Created attachment 394350 [details]
Patch

[Daniel Bates]

Comment on attachment 394350 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4246
> +                rects.append({ createLiveRange(iterator.range())->absoluteBoundingBox(Range::BoundingRectBehavior::IgnoreEmptyTextSelections), { ++offsetSoFar, stride } });

r-, should be offsetSoFar++

[Daniel Bates]

Created attachment 394353 [details]
Patch

[Wenson Hsieh]

Comment on attachment 394353 [details]
Patch

r=mews

[Daniel Bates]

Thanks Wenson!

[Daniel Bates]

Comment on attachment 394353 [details]
Patch

Clearing flags on attachment: 394353

Committed r258920: <https://trac.webkit.org/changeset/258920>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60832084>

[Darin Adler]

Comment on attachment 394350 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4239
> +    auto characterRectsForRange = [](const Range& range, unsigned startOffset) {

No reason for this to take const Range& instead of const SimpleRange&; all this function does with the range is create a CharacterIterator, which takes a SimpleRange.

[Daniel Bates]

(In reply to Darin Adler from comment #9)
> Comment on attachment 394350 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394350&action=review
> 
> > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4239
> > +    auto characterRectsForRange = [](const Range& range, unsigned startOffset) {
> 
> No reason for this to take const Range& instead of const SimpleRange&; all
> this function does with the range is create a CharacterIterator, which takes
> a SimpleRange.

That's too simple to do.

[Daniel Bates]

(In reply to Daniel Bates from comment #10)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 394350 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=394350&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:4239
> > > +    auto characterRectsForRange = [](const Range& range, unsigned startOffset) {
> > 
> > No reason for this to take const Range& instead of const SimpleRange&; all
> > this function does with the range is create a CharacterIterator, which takes
> > a SimpleRange.
> 
> That's too simple to do.

I was joking by the way. I can change it, but the caller currently passes a Range. I haven't put thought into whether the caller can be a SimpleRange.

[Darin Adler]

(In reply to Daniel Bates from comment #11)
> I was joking by the way. I can change it, but the caller currently passes a
> Range. I haven't put thought into whether the caller can be a SimpleRange.

You can pass a Range to a function that takes a const SimpleRange&. Caller won’t have to change at all.

[Daniel Bates]

(In reply to Darin Adler from comment #12)
> (In reply to Daniel Bates from comment #11)
> > I was joking by the way. I can change it, but the caller currently passes a
> > Range. I haven't put thought into whether the caller can be a SimpleRange.
> 
> You can pass a Range to a function that takes a const SimpleRange&. Caller
> won’t have to change at all.

OK. Patch up at bug #209495.