Bug 224820

Summary: Rename WebCore::SelectionRect to WebCore::SelectionGeometry
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, changseok, darin, dmazzoni, esprehn+autocc, ews-watchlist, glenn, hi, jcraig, jdiggs, kondapallykalyan, megan_gardner, mmaxfield, pdr, samuel_white, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
none
For landing none

Description Wenson Hsieh 2021-04-20 10:07:18 PDT
.
Comment 1 Wenson Hsieh 2021-04-20 10:32:37 PDT
Created attachment 426567 [details]
For EWS
Comment 2 Darin Adler 2021-04-20 11:35:53 PDT
Comment on attachment 426567 [details]
For EWS

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

> Source/WebCore/platform/ios/SelectionGeometry.cpp:88
> +SelectionGeometry::SelectionGeometry()
> +    : m_direction(TextDirection::LTR)
> +    , m_minX(0)
> +    , m_maxX(0)
> +    , m_maxY(0)
> +    , m_lineNumber(0)
> +    , m_isLineBreak(false)
> +    , m_isFirstOnLine(false)
> +    , m_isLastOnLine(false)
> +    , m_containsStart(false)
> +    , m_containsEnd(false)
> +    , m_isHorizontal(true)
> +    , m_isInFixedPosition(false)
> +    , m_isRubyText(false)
> +    , m_pageNumber(0)
> +{
> +}

Apparently this was just moved code, but I suggest we initialize all of these in the class definition instead, and just have the default constructor be "= default" or something like that.

Relying on this should cut down the length of at least one of the other constructors, too, and make it harder to accidentally leave anything uninitialized.

> Source/WebCore/platform/ios/SelectionGeometry.h:98
> +    void setLogicalLeft(int left)
> +    {
> +        if (m_isHorizontal)
> +            m_rect.setX(left);
> +        else
> +            m_rect.setY(left);
> +    }
> +
> +    void setLogicalWidth(int width)
> +    {
> +        if (m_isHorizontal)
> +            m_rect.setWidth(width);
> +        else
> +            m_rect.setHeight(width);
> +    }
> +
> +    void setLogicalTop(int top)
> +    {
> +        if (m_isHorizontal)
> +            m_rect.setY(top);
> +        else
> +            m_rect.setX(top);
> +    }
> +
> +    void setLogicalHeight(int height)
> +    {
> +        if (m_isHorizontal)
> +            m_rect.setHeight(height);
> +        else
> +            m_rect.setWidth(height);
> +    }

Also not new: I suggest moving these inline function bodies out of the class definition, to make it easier to read it and get an overview.
Comment 3 Wenson Hsieh 2021-04-20 12:14:53 PDT
Comment on attachment 426567 [details]
For EWS

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

>> Source/WebCore/platform/ios/SelectionGeometry.cpp:88
>> +}
> 
> Apparently this was just moved code, but I suggest we initialize all of these in the class definition instead, and just have the default constructor be "= default" or something like that.
> 
> Relying on this should cut down the length of at least one of the other constructors, too, and make it harder to accidentally leave anything uninitialized.

Ah, so my next patch coming up does exactly this; I'll move that change into part of this patch instead.

>> Source/WebCore/platform/ios/SelectionGeometry.h:98
>> +    }
> 
> Also not new: I suggest moving these inline function bodies out of the class definition, to make it easier to read it and get an overview.

Sounds good! Will move these implementations into the cpp.
Comment 4 Wenson Hsieh 2021-04-20 13:02:03 PDT
Created attachment 426583 [details]
For landing
Comment 5 EWS 2021-04-20 17:04:03 PDT
Committed r276331 (236811@main): <https://commits.webkit.org/236811@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426583 [details].
Comment 6 Radar WebKit Bug Importer 2021-04-23 01:59:41 PDT
<rdar://problem/77062455>