Bug 215070 - Remove some member functions of Range and many calls to createLiveRange
Summary: Remove some member functions of Range and many calls to createLiveRange
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-01 19:14 PDT by Darin Adler
Modified: 2020-08-03 09:29 PDT (History)
22 users (show)

See Also:


Attachments
Patch (123.13 KB, patch)
2020-08-01 20:14 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (124.34 KB, patch)
2020-08-01 21:05 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (124.98 KB, patch)
2020-08-01 23:20 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-08-01 19:14:45 PDT
Remove some member functions of Range and many calls to createLiveRange
Comment 1 Darin Adler 2020-08-01 20:14:55 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-08-01 21:05:10 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-08-01 23:20:54 PDT
Created attachment 405809 [details]
Patch
Comment 4 Darin Adler 2020-08-02 13:31:31 PDT
Passing all tests now; given recent history I’m guessing review will be Sam?
Comment 5 Sam Weinig 2020-08-02 17:37:10 PDT
Comment on attachment 405809 [details]
Patch

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2342
> +// FIXME: No reason for this to be a method instead of a function.
> +- (NSRange)_convertToNSRange:(const SimpleRange&)range

Reminds me we should investigate using direct methods / members at some point (https://reviews.llvm.org/D69991 / https://nshipster.com/direct/).

> Source/WebCore/page/TextIndicator.cpp:306
> +        textRects = RenderObject::collectSelectionRects(range).map([&](const auto& rect) -> FloatRect {

Is the explicit return type needed here?

> Source/WebCore/rendering/RenderObject.h:551
> +    WEBCORE_EXPORT static Vector<SelectionRect> collectSelectionRects(const SimpleRange&);
> +    WEBCORE_EXPORT static Vector<SelectionRect> collectSelectionRectsWithoutUnionInteriorLines(const SimpleRange&);

I'm a little sad to be adding more to RenderObject, but given that there is already collectSelectionRects functions here, it seems ok. Do they use the fact that they are static member functions in anyway?
Comment 6 EWS 2020-08-02 17:47:57 PDT
Committed r265190: <https://trac.webkit.org/changeset/265190>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405809 [details].
Comment 7 Radar WebKit Bug Importer 2020-08-02 17:48:17 PDT
<rdar://problem/66454378>
Comment 8 Darin Adler 2020-08-03 09:29:46 PDT
Comment on attachment 405809 [details]
Patch

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

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:2342
>> +- (NSRange)_convertToNSRange:(const SimpleRange&)range
> 
> Reminds me we should investigate using direct methods / members at some point (https://reviews.llvm.org/D69991 / https://nshipster.com/direct/).

Sure, that would apply to even more methods, including ones that, unlike this one, do use self!

>> Source/WebCore/page/TextIndicator.cpp:306
>> +        textRects = RenderObject::collectSelectionRects(range).map([&](const auto& rect) -> FloatRect {
> 
> Is the explicit return type needed here?

Yes, because otherwise it’s an IntRect.

>> Source/WebCore/rendering/RenderObject.h:551
>> +    WEBCORE_EXPORT static Vector<SelectionRect> collectSelectionRectsWithoutUnionInteriorLines(const SimpleRange&);
> 
> I'm a little sad to be adding more to RenderObject, but given that there is already collectSelectionRects functions here, it seems ok. Do they use the fact that they are static member functions in anyway?

They do not. These functions can live anywhere appropriate. I am happy to move them out of RenderObject to a better home. When moving them out of Range I chose to put them next to the underlying RenderObject functions they are based on, but would be happy to put them somewhere else.

Clearly they are part of "rendering".

Really open to moving them again! It won’t be hard.