Bug 210369

Created attachment 396164 [details]
Patch

Comment on attachment 396164 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:-1183
> -void Range::absoluteTextRects(Vector<IntRect>& rects, bool useSelectionHeight, RangeInFixedPosition* inFixed, OptionSet<BoundingRectBehavior> rectOptions) const

I would change this to return the rects instead of using an out parameter.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:463
> +        return { { { *innerText, 0 }, { *innerText, 0 } } };

Maybe specify SimpleRange here to cut down on some of the curly braces.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:488
> +    return { { { *startNode, start }, { *endNode, end } } };

Maybe specify SimpleRange here to cut down on some of the curly braces.

> Source/WebCore/rendering/RenderObject.cpp:1904
> +        else if (is<RenderText>(renderer)) {

I assume is<RenderText> will return false here if renderer is null?

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1114
> +    return range ? [range textRects] : @[];

range.textRects?

Comment on attachment 396164 [details]
Patch

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

Looks like build failures are because WebFrame.mm needs to include DOM.h.

>> Source/WebCore/dom/Range.cpp:-1183
>> -void Range::absoluteTextRects(Vector<IntRect>& rects, bool useSelectionHeight, RangeInFixedPosition* inFixed, OptionSet<BoundingRectBehavior> rectOptions) const
> 
> I would change this to return the rects instead of using an out parameter.

Yes, agreed. I would do that if I was planning to keep this. But I am planning to delete it. I only removed the argument as a prelude to removing the entire function soon.

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:463
>> +        return { { { *innerText, 0 }, { *innerText, 0 } } };
> 
> Maybe specify SimpleRange here to cut down on some of the curly braces.

I’d be willing to do that. Not sure it’s better. If you are sure, I will do it. It allows me to remove the outer set.

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:488
>> +    return { { { *startNode, start }, { *endNode, end } } };
> 
> Maybe specify SimpleRange here to cut down on some of the curly braces.

Ditto.

>> Source/WebCore/rendering/RenderObject.cpp:1904
>> +        else if (is<RenderText>(renderer)) {
> 
> I assume is<RenderText> will return false here if renderer is null?

Yes.

>> Source/WebKitLegacy/mac/WebView/WebFrame.mm:1114
>> +    return range ? [range textRects] : @[];
> 
> range.textRects?

Will do.

Created attachment 396174 [details]
Patch

Anders, you didn’t set review+. Did you mean to?

(In reply to Darin Adler from comment #5)
> Anders, you didn’t set review+. Did you mean to?

I wanted to wait until everything was green!

Committed r259933: <https://trac.webkit.org/changeset/259933>

<rdar://problem/61644860>