Bug 210369

Summary: Move more from live range to SimpleRange: callers of absoluteTextRects
Product: WebKit Reporter: Darin Adler <darin>
Component: WebCore Misc.Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, kangil.han, kondapallykalyan, mifenton, pdr, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch andersca: review+

Darin Adler
Reported 2020-04-10 19:43:08 PDT
Move more from live range to SimpleRange: callers of absoluteTextRects
Attachments
Patch (36.58 KB, patch)
2020-04-11 08:39 PDT, Darin Adler
no flags
Patch (36.81 KB, patch)
2020-04-11 10:44 PDT, Darin Adler
andersca: review+
Darin Adler
Comment 1 2020-04-11 08:39:35 PDT
Anders Carlsson
Comment 2 2020-04-11 08:48:52 PDT
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?
Darin Adler
Comment 3 2020-04-11 10:01:26 PDT
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.
Darin Adler
Comment 4 2020-04-11 10:44:38 PDT
Darin Adler
Comment 5 2020-04-11 10:44:54 PDT
Anders, you didn’t set review+. Did you mean to?
Anders Carlsson
Comment 6 2020-04-11 12:12:23 PDT
(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!
Darin Adler
Comment 7 2020-04-11 12:36:50 PDT
Radar WebKit Bug Importer
Comment 8 2020-04-11 12:37:16 PDT
Note You need to log in before you can comment on or make changes to this bug.